fix(mfs): fix fsync deadlock, set attrs, disable default caching#11255
fix(mfs): fix fsync deadlock, set attrs, disable default caching#11255wjmelements wants to merge 4 commits intoipfs:masterfrom
Conversation
| // no-op (actual data flushing happens in Flush) | ||
| func (file *File) Fsync(ctx context.Context, req *fuse.FsyncRequest) error { | ||
| return file.mfsFile.Sync() | ||
| return nil |
There was a problem hiding this comment.
I also had to make this change for macFUSE to work but I mainly tested this on linux.
Here is an extended explanation from Claude of why this caused a deadlock preventing VIM :w and then a catastrophic timeout:
mfsFile.Sync() acquires desclock.Lock(), which is already held for the
lifetime of any open write descriptor (acquired in File.Open, released in
fileDescriptor.Close). Calling Sync() while a write descriptor is open
therefore deadlocks — the write descriptor won't be closed until after the
fsync syscall returns, but the fsync goroutine is blocked waiting for the
lock. Actual data flushing happens in FileHandler.Flush, which FUSE calls
on the handle before Release, so a no-op here is safe.
|
|
||
| // File attributes. | ||
| func (file *File) Attr(ctx context.Context, attr *fuse.Attr) error { | ||
| attr.Valid = 0 |
There was a problem hiding this comment.
This fixed an issue where after writing a new file in VIM, ls -l would show size zero for a while. This was because it was cached. Claude explains:
attr.Valid is a time.Duration that tells the kernel how long to cache the inode's attributes. Setting it to 0 disables that cache entirely, so every stat() triggers a fresh Attr call into the FUSE handler.
| attr.Uid = uint32(os.Getuid()) | ||
| attr.Gid = uint32(os.Getgid()) |
There was a problem hiding this comment.
Without this change the files are owned by root. Perhaps we can make this configurable in the future, but since FUSE runs in userspace it's fairly intuitive for them to be owned by the user running ipfs.
There was a problem hiding this comment.
I can make the same change for /ipns and /ipfs
There was a problem hiding this comment.
It would makes sense to do this for all mountpoints.
|
Ok still, seeing some test failures on this branch. Demoting to draft. |
gammazero
left a comment
gammazero
left a comment
There was a problem hiding this comment.
Tests are all passing now. I think this looks good, but I need to check if any additional testing is needed.
| attr.Uid = uint32(os.Getuid()) | ||
| attr.Gid = uint32(os.Getgid()) |
There was a problem hiding this comment.
It would makes sense to do this for all mountpoints.
gammazero
added
the
need/maintainers-input
2 participants
Reviewer @lidel
These are some fixes that got MFS+fuse3 working for me on Ubuntu
I used VIM to edit files in MFS which triggered some new pathways.
Initially it would fail to write with a deadlock and eventually a fuse timeout.
After these fixes I am able to use MFS with pinmfs on Ubuntu.
Changes