Re: [PATCH 2/7] fuse: flush pending fuse events before aborting the connection

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2025-07-22 at 08:30 -0400, Jeff Layton wrote:
> On Fri, 2025-07-18 at 17:32 -0700, Darrick J. Wong wrote:
> > On Fri, Jul 18, 2025 at 03:23:30PM -0700, Joanne Koong wrote:
> > > On Thu, Jul 17, 2025 at 4:26 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > 
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > generic/488 fails with fuse2fs in the following fashion:
> > > > 
> > > > generic/488       _check_generic_filesystem: filesystem on /dev/sdf is inconsistent
> > > > (see /var/tmp/fstests/generic/488.full for details)
> > > > 
> > > > This test opens a large number of files, unlinks them (which really just
> > > > renames them to fuse hidden files), closes the program, unmounts the
> > > > filesystem, and runs fsck to check that there aren't any inconsistencies
> > > > in the filesystem.
> > > > 
> > > > Unfortunately, the 488.full file shows that there are a lot of hidden
> > > > files left over in the filesystem, with incorrect link counts.  Tracing
> > > > fuse_request_* shows that there are a large number of FUSE_RELEASE
> > > > commands that are queued up on behalf of the unlinked files at the time
> > > > that fuse_conn_destroy calls fuse_abort_conn.  Had the connection not
> > > > aborted, the fuse server would have responded to the RELEASE commands by
> > > > removing the hidden files; instead they stick around.
> > > 
> > > Tbh it's still weird to me that FUSE_RELEASE is asynchronous instead
> > > of synchronous. For example for fuse servers that cache their data and
> > > only write the buffer out to some remote filesystem when the file gets
> > > closed, it seems useful for them to (like nfs) be able to return an
> > > error to the client for close() if there's a failure committing that
> > 
> > I don't think supplying a return value for close() is as helpful as it
> > seems -- the manage says that there is no guarantee that data has been
> > flushed to disk; and if the file is removed from the process' fd table
> > then the operation succeeded no matter the return value. :P
> > 
> > (Also C programmers tend to be sloppy and not check the return value.)
> > 
> 
> The POSIX spec and manpage for close(2) make no mention of writeback
> errors, so it's not 100% clear that returning them there is at all OK.
> Everyone sort of assumes that it makes sense to do so, but it can be
> actively harmful.
> 

Actually, they do mention this, but I still argue that it's not a good
idea to do so. If you want writeback errors use fsync() (or maybe the
new ioctl() that someone was plumbing in that scrapes errors without
doing writeback).

> Suppose we do this:
> 
> open() = 1
> write(1)
> close(1) 
> open() = 2
> fsync(2) = ???
> 
> Now, assume there was a writeback error that happens either before or
> after the close.
> 
> With the way this works today, you will get back an error on that final
> fsync() even if fd 2 was opened _after_ the writeback error occurred,
> because nothing will have scraped it yet.
> 
> If you scrape the error to return it on the close though, then the
> result of that fsync() would be inconclusive. If the error happens
> before the close(), then fsync() will return 0. If it fails after the
> close(), then the fsync() will see an error.
> 
> > > data; that also has clearer API semantics imo, eg users are guaranteed
> > > that when close() returns, all the processing/cleanup for that file
> > > has been completed.  Async FUSE_RELEASE also seems kind of racy, eg if
> > > the server holds local locks that get released in FUSE_RELEASE, if a
> > 
> > Yes.  I think it's only useful for the case outined in that patch, which
> > is that a program started an asyncio operation and then closed the fd.
> > In that particular case the program unambiguously doesn't care about the
> > return value of close so it's ok to perform the release asynchronously.
> > 
> > > subsequent FUSE_OPEN happens before FUSE_RELEASE then depends on
> > > grabbing that lock, then we end up deadlocked if the server is
> > > single-threaded.
> > 
> > Hrm.  I suppose if you had a script that ran two programs one after the
> > other, each of which expected to be able to open and lock the same file,
> > then you could run into problems if the lock isn't released by the time
> > the second program is ready to open the file.
> > 
> > But having said that, some other program could very well open and lock
> > the file as soon as the lock drops.
> > 
> > > I saw in your first patch that sending FUSE_RELEASE synchronously
> > > leads to a deadlock under AIO but AFAICT, that happens because we
> > > execute req->args->end() in fuse_request_end() synchronously; I think
> > > if we execute that release asynchronously on a worker thread then that
> > > gets rid of the deadlock.
> > 
> > <nod> Last time I think someone replied that maybe they should all be
> > asynchronous.
> > 
> > > If FUSE_RELEASE must be asynchronous though, then your approach makes
> > > sense to me.
> > 
> > I think it only has to be asynchronous for the weird case outlined in
> > that patch (fuse server gets stuck closing its own client's fds).
> > Personally I think release ought to be synchronous at least as far as
> > the kernel doing all the stuff that close() says it has to do (removal
> > of record locks, deleting the fd table entry).
> > 
> > Note that doesn't necessarily mean that the kernel has to be completely
> > done with all the work that entails.  XFS defers freeing of unlinked
> > files until a background garbage collector gets around to doing that.
> > Other filesystems will actually make you wait while they free all the
> > data blocks and the inode.  But the kernel has no idea what the fuse
> > server actually does.
> > 
> > > > Create a function to push all the background requests to the queue and
> > > > then wait for the number of pending events to hit zero, and call this
> > > > before fuse_abort_conn.  That way, all the pending events are processed
> > > > by the fuse server and we don't end up with a corrupt filesystem.
> > > > 
> > > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > > ---
> > > >  fs/fuse/fuse_i.h |    6 ++++++
> > > >  fs/fuse/dev.c    |   38 ++++++++++++++++++++++++++++++++++++++
> > > >  fs/fuse/inode.c  |    1 +
> > > >  3 files changed, 45 insertions(+)
> > > > 
> > > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > > +/*
> > > > + * Flush all pending requests and wait for them.  Only call this function when
> > > > + * it is no longer possible for other threads to add requests.
> > > > + */
> > > > +void fuse_flush_requests(struct fuse_conn *fc, unsigned long timeout)
> > > 
> > > It might be worth renaming this to something like
> > > 'fuse_flush_bg_requests' to make it more clear that this only flushes
> > > background requests
> > 
> > Hum.  Did I not understand the code correctly?  I thought that
> > flush_bg_queue puts all the background requests onto the active queue
> > and issues them to the fuse server; and the wait_event_timeout sits
> > around waiting for all the requests to receive their replies?
> > 
> > I could be mistaken though.  This is my rough understanding of what
> > happens to background requests:
> > 
> > 1. Request created
> > 2. Put request on bg_queue
> > 3. <wait>
> > 4. Request removed from bg_queue
> > 5. Request sent
> > 6. <wait>
> > 7. Reply received
> > 8. Request ends and is _put.
> > 
> > Non-background (foreground?) requests skip steps 2-4.  Meanwhile,
> > fc->waiting tracks the number of requests that are anywhere between the
> > end of step 1 and the start of step 8.
> > 
> > In any case, I want to push all the bg requests and wait until there are
> > no more requests in the system.
> > 
> > --D

-- 
Jeff Layton <jlayton@kernel.org>