Basepath-hash inode algorithm #1323

PhracturedBlue · 2024-03-24T21:39:54Z

This is an updated version of the patch from #1002

I ran into a similar issue recently where after replacing the SATA controller, the device-ids all changed, and my backup tool which relies on inodes being consistent got very confused.

This is just the patch from @thrnz cleaned up for 2.40.2, with the addition of README updates for the changes.

README.md

trapexit · 2024-04-01T01:21:36Z

src/fs_inode.hpp

@@ -37,18 +37,23 @@ namespace fs
                  const uint64_t  fusepath_len,
                  const mode_t    mode,
                  const dev_t     dev,
-                  const ino_t     ino);
+                  const ino_t     ino,
+                  const std::string &basepath = "");


I would prefer the basepath be the first argument.

trapexit · 2024-04-01T01:22:53Z

src/fs_inode.hpp

@@ -37,18 +37,23 @@ namespace fs
                  const uint64_t  fusepath_len,
                  const mode_t    mode,
                  const dev_t     dev,
-                  const ino_t     ino);
+                  const ino_t     ino,
+                  const std::string &basepath = "");


Is the default value really necessary? This would create a temp object if it was called with the default which isn't great.

src/fs_inode.cpp

trapexit · 2024-04-01T15:10:02Z

BTW... once we are happy with this please squash all these commits

PhracturedBlue · 2024-04-01T15:12:05Z

Sorry about so many typos in such a small amount of text. I even checked it with a spell-checker 1st.

As far as getting rid of the default, I started on it, but I ran into a couple of snags. To get the base branch-dir everywhere that inode::calc is called, requires access to the Branch.path variable. In fuse_readdir_cosr.cpp that requires passing it in the DirRV structure which can't be done as a pointer, so it grows the structure. I'm not sure if that is an issue. For fuse_fgetattr I'm not sure how to get the base branch-dir. For now I passed in "" which is what the default is anyway, but it doesn't seem right, and I'm not sure why the code works today (yet it does for practical puroposes, as I've been using it for some time). So I'm open to inputs on this, hough I'll continue working on it myself too.

Edit: Are the instructions on running the tests somewhere? I tried make tests, but they fail even on the main branch.

trapexit · 2024-04-01T15:49:20Z

but it doesn't seem right

It isn't right :)

Need to ensure that the data required for the computation is available anywhere it is used. Otherwise it will provide invalid / inconsistent results.

As for tests: they wouldn't test this. But you're right. I changed something and didn't fix the tests. Will add it to the todo list.

PhracturedBlue · 2024-04-01T17:03:12Z

To fix fgetattr I think I need to get the basepath somewhere, but all that we have is the file-descriptopr and the fusepath.
One option is to pass the basepath through the FileInfo object. That seems like it could be a lot of overhead for something not many users will need. Alternatively, I can use the Config policy to look it up from the fusepath. If I do it in fgetattr, then that has a performance cost that everyone would pay even when not using basepath-hash. An alternative may be to do the lookup in the basepath-hash function itself when an empty string is provided to calc(). There is obviously a performance impact to that too, but more localized. A third option would be to compute the inode when the file is opened and store that in the FileInfo (which would then just be a 64bit value stored for each FileInfo which may be palateable, though it comes at the cost of slightly slowing down 'open' to speed up fgetattr.

Do you think one of those options is better than another?

trapexit · 2024-04-02T03:43:12Z

Storing the basepath in FileInfo is likely the best general solution.

PhracturedBlue · 2024-04-02T03:46:16Z

After looking at the code, deferring the identification of the basepath to calc() time doesn't seem feasible, as the fusepath is not sufficient to determine the basepath.
So I decided to create the indoe during open() and create() which seemed overall like it might be a better tradeoff of cpu vs memory for each call than passing the basepath around with ever FileInfo object.
But now that I've done it, it seems you prefer the other one, so I'll try that instead :)

trapexit · 2024-04-02T11:52:45Z

I just figured the basepath is a reasonable value to include in the fileinfo and might be useful elsewhere.

PhracturedBlue · 2024-04-02T12:17:35Z

It turns out your choice is far superior. I didn't realize aI could pass it as a pointer (since the branch string is a const in Config) which means it takes up the same memory as the inode solution, but is far simpler and without the performance penalty.

I have made the change. I wil squash the commits once we're happy with the change. I just find it easier to keep the history during development.

I also tested on my system to confirm and the original pathc (with the default="") gives the same inode for stat() and fstat(). That was surprising to me, since I assumed fgetattr would be used to generate the fstat() results. I added some debug, and it seems fgetattr is not (always?) called for an fstat(). It seems fuse is doing some caching, and calls 'getattr' during open() and caches that result rather than calling fgetattr when fstat() is called.

I'm not usre if there is a better way to test ths code.

trapexit · 2024-04-02T12:32:06Z

I didn't realize aI could pass it as a pointer

Actually, that's a bad idea. The config, currently, can be changed at almost any time. That includes the branch list. So the branch could be removed while the file is open and then the memory would be freed and a use after free would then be possible. The performance and memory usage of a single string is minimal. In theory it could be interned but we've not gotten to a point where that should be needed. So just use a std::string.

I also tested on my system to confirm and the original pathc (with the default="") gives the same inode for stat() and fstat().

Thing is... fstat doesn't translate into fgetattr on the mergerfs side. There is some caching depending on the settings but they added fgetattr a while back but didn't really plumb the kernel side fully to use it in all the places it might be used. Honestly I'm not sure if it currently is used at all but I've kept it for that time when they do. So it really isn't a good test. Because a default of empty would naturally have to change the hash output. Otherwise it'd be a garbage hash :)

PhracturedBlue · 2024-04-02T12:47:30Z

I made the change to dereference the basepath as you suggested.
I found this thread:
libfuse/libfuse#62
which shows that you can trigger fgetattr if you call fstat on a newly created file. I confirmed this does actually trigger fgetattr to be called, though it doesn't reslt in the returned inode being different even in the default="" case, so clearly there are still fuse shenanigans going on.

But regardless, at least I can trigger the code and verify that fgetattr and getattr now produce the same result (though I had to do it via injecting log messages rather than being able to write a test app)

Edit: Also, I appreciate you taking your time to help me get this right. Thanks! I've been using mergerfs for many years, and really appreciate your dedication to it.

feltcat · 2024-04-04T18:04:44Z

README.md

+* basepath-hash: Hashes the branch base path along with
+  the inode of the underlying entry.  This has a similar purpose to
+  devino-hash, but by using the path instead of the device-id, the inodes
+  will be gauranteed to be stable across reboots.  Useful for backup or


Suggested change

will be gauranteed to be stable across reboots. Useful for backup or

will be guaranteed to be stable across reboots. Useful for backup or

A filesystem's device id (st_dev) may change on reboot (eg with zfs). Instead, we can use the files base path (+underlying inode) to generate an inode, which will remain constant across reboots. However, this may have unexpected effects if multiple unique devices appear under a base path. Like hybrid_hash, basehybrid_hash/32 hashes relative path for dirs and basepath_hash for files Original patch by thrnz@github

trapexit reviewed Mar 31, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

trapexit reviewed Mar 31, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

trapexit reviewed Apr 1, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

trapexit reviewed Apr 1, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

trapexit reviewed Apr 1, 2024

View reviewed changes

src/fs_inode.cpp Outdated Show resolved Hide resolved

PhracturedBlue force-pushed the basepath-hash branch from 623bc71 to 7ceac03 Compare April 1, 2024 14:47

feltcat reviewed Apr 4, 2024

View reviewed changes

PhracturedBlue force-pushed the basepath-hash branch from b33355d to d0d265a Compare April 5, 2024 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basepath-hash inode algorithm #1323

Basepath-hash inode algorithm #1323

PhracturedBlue commented Mar 24, 2024

trapexit Apr 1, 2024

trapexit Apr 1, 2024

trapexit commented Apr 1, 2024

PhracturedBlue commented Apr 1, 2024 •

edited

trapexit commented Apr 1, 2024

PhracturedBlue commented Apr 1, 2024 •

edited

trapexit commented Apr 2, 2024

PhracturedBlue commented Apr 2, 2024 •

edited

trapexit commented Apr 2, 2024

PhracturedBlue commented Apr 2, 2024 •

edited

trapexit commented Apr 2, 2024

PhracturedBlue commented Apr 2, 2024 •

edited

feltcat Apr 4, 2024

	will be gauranteed to be stable across reboots. Useful for backup or
	will be guaranteed to be stable across reboots. Useful for backup or

Basepath-hash inode algorithm #1323

Are you sure you want to change the base?

Basepath-hash inode algorithm #1323

Conversation

PhracturedBlue commented Mar 24, 2024

trapexit Apr 1, 2024

Choose a reason for hiding this comment

trapexit Apr 1, 2024

Choose a reason for hiding this comment

trapexit commented Apr 1, 2024

PhracturedBlue commented Apr 1, 2024 • edited

trapexit commented Apr 1, 2024

PhracturedBlue commented Apr 1, 2024 • edited

trapexit commented Apr 2, 2024

PhracturedBlue commented Apr 2, 2024 • edited

trapexit commented Apr 2, 2024

PhracturedBlue commented Apr 2, 2024 • edited

trapexit commented Apr 2, 2024

PhracturedBlue commented Apr 2, 2024 • edited

feltcat Apr 4, 2024

Choose a reason for hiding this comment

PhracturedBlue commented Apr 1, 2024 •

edited

PhracturedBlue commented Apr 1, 2024 •

edited

PhracturedBlue commented Apr 2, 2024 •

edited

PhracturedBlue commented Apr 2, 2024 •

edited

PhracturedBlue commented Apr 2, 2024 •

edited