feat(core): protect MFS root in node with lock
Depends on https://github.com/ipfs/go-ipfs/pull/8574. Please review and land that first to simplify this diff.
Replace the readily accessible MFS root in the node (FilesRoot) with a lock-protected version (LockedFilesRoot).
Testing: We currently only do a basic test manually (through the temporarily added script test-mfs-lock.bash), but we should look for a robust way to test the lock (preferably not through sharness/bash but through the internal Go API).
Note that this entire logic applies only when having a daemon running where the different GC and files commands contend for the same MFS root. If the commands are run in standalone mode then each one will contend for the entire repo's lock (and this mechanism isn't relevant there).
TODO:
- [x] Update this description according to the new changes.
- [x] Land https://github.com/ipfs/go-ipfs/pull/8574 on which this depends.
- [ ] Figure out how to reliably test this (that is, how to test that the race condition doesn't happen anymore). I haven't seen any sharness test that could be used as models.
- [ ] Resolve the blocking
FIXMEs. - [ ] Review the unlocks and be more fine-grained where possible instead of using
deferalways.
Closes https://github.com/ipfs/go-ipfs/issues/6113.
Assigned to @petar for review as this is a newer area for all the current go-ipfs stewards.
@aschmahmann From your previous comment I take it you approve the direction of this PR but could you make it explicit here just to make sure I'm on the right track before proceeding, please?
@schomatis yes, having a global write lock on MFS seems like the best thing to do in this PR.
For reads we might want to be more careful than a global lock though. For example, not locking ipfs files ls or ipfs files stat while GC is ongoing, but preventing corruption between MFS write and read operations (out of date data, is fine with concurrent operations, but corruption would be bad). It might be that MFS already handles some of this, but we should verify.
If we find ourselves stuck with locking on some of the read commands I'd really like ipfs files stat /ipfs/.... to not utilize the same locking mechanism since the data is immutable and shouldn't need to be locked. It's also important in order to make sure nobody has a reason to continue using ipfs object stat.
Continuing with this.
I'd really like ipfs files stat /ipfs/.... to not utilize the same locking mechanism since the data is immutable and shouldn't need to be locked.
Confirming this is already the case since the /ipfs/ prefix is handled through a different API and doesn't use the MFS root:
https://github.com/ipfs/go-ipfs/blob/c00065cc8412ec660037f3fed4df7d3fed467b17/core/commands/files.go#L424-L429
@aschmahmann Petar confirmed the current proposal but I'm still stuck on how to test it other than some manual confirmation than the lock is being taken. How would you like to test this to wrap it up?