Basepath-hash inode algorithm

[PhracturedBlue]

This is an updated version of the patch from https://github.com/trapexit/mergerfs/discussions/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.

[trapexit]

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

[PhracturedBlue]

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]

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]

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]

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

[PhracturedBlue]

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]

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

[PhracturedBlue]

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]

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]

I made the change to dereference the basepath as you suggested. I found this thread: https://github.com/libfuse/libfuse/issues/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.