Re: [PATCH 1/5] fuse: flush pending FUSE_RELEASE requests before sending FUSE_DESTROY

[Joanne Koong <joannelkoong@gmail.com>]

On Tue, Feb 24, 2026 at 11:57 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Feb 24, 2026 at 11:33:12AM -0800, Joanne Koong wrote:
> > On Mon, Feb 23, 2026 at 3:06 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.
> > >
> > > For upper-level fuse servers that don't use fuseblk mode this isn't a
> > > problem because libfuse responds to the connection going down by pruning
> > > its inode cache and calling the fuse server's ->release for any open
> > > files before calling the server's ->destroy function.
> > >
> > > For fuseblk servers this is a problem, however, because the kernel sends
> > > FUSE_DESTROY to the fuse server, and the fuse server has to write all of
> > > its pending changes to the block device before replying to the DESTROY
> > > request because the kernel releases its O_EXCL hold on the block device.
> > > This means that the kernel must flush all pending FUSE_RELEASE requests
> > > before issuing FUSE_DESTROY.
> > >
> > > For fuse-iomap servers this will also be a problem because iomap servers
> > > are expected to release all exclusively-held resources before unmount
> > > returns from the kernel.
> > >
> > > Create a function to push all the background requests to the queue
> > > before sending FUSE_DESTROY.  That way, all the pending file release
> > > events are processed by the fuse server before it tears itself down, and
> > > we don't end up with a corrupt filesystem.
> > >
> > > Note that multithreaded fuse servers will need to track the number of
> > > open files and defer a FUSE_DESTROY request until that number reaches
> > > zero.  An earlier version of this patch made the kernel wait for the
> > > RELEASE acknowledgements before sending DESTROY, but the kernel people
> > > weren't comfortable with adding blocking waits to unmount.
> > >
> > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> >
> > Overall LGTM, left a few comments below
> >
> > Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
>
> Thanks!
>
> > > ---
> > >  fs/fuse/fuse_i.h |    5 +++++
> > >  fs/fuse/dev.c    |   19 +++++++++++++++++++
> > >  fs/fuse/inode.c  |   12 +++++++++++-
> > >  3 files changed, 35 insertions(+), 1 deletion(-)
> > >
> > >
> > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > index 7f16049387d15e..1d4beca5c7018d 100644
> > > --- a/fs/fuse/fuse_i.h
> > > +++ b/fs/fuse/fuse_i.h
> > > @@ -1287,6 +1287,11 @@ void fuse_request_end(struct fuse_req *req);
> > >  void fuse_abort_conn(struct fuse_conn *fc);
> > >  void fuse_wait_aborted(struct fuse_conn *fc);
> > >
> > > +/**
> > > + * Flush all pending requests but do not wait for them.
> > > + */
> >
> > nit: /*  */ comment style
>
> I'm very confused by the comment style in this header file.  Some of
> them look like kerneldoc comments (albeit not documenting the sole
> parameter), but others are just regular C comments.

Oh I see your confusion now. Yeah the comment styles in this .h file
are kind of all over the place. Most of the functions don't even have
comments.

>
> <shrug> I sorta dislike kerneldoc's fussiness so I'll change it to a C
> comment so that I don't have to propagate this "@param fc fuse
> connection" verbosity.
>
> > > +void fuse_flush_requests(struct fuse_conn *fc);
> > > +
> > >  /* Check if any requests timed out */
> > >  void fuse_check_timeout(struct work_struct *work);
> > >
> > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > > index 0b0241f47170d4..ac9d7a7b3f5e68 100644
> > > --- a/fs/fuse/dev.c
> > > +++ b/fs/fuse/dev.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/splice.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/seq_file.h>
> > > +#include <linux/nmi.h>
> >
> > I don't think you meant to add this?
>
> Yep, that was added for a previous iteration and can go away now.
>
> > >
> > >  #include "fuse_trace.h"
> > >
> > > @@ -2430,6 +2431,24 @@ static void end_polls(struct fuse_conn *fc)
> > >         }
> > >  }
> > >
> > > +/*
> > > + * Flush all pending requests and wait for them.  Only call this function when
> >
> > I think you meant "don't wait" for them?
>
> Right.  Fixed.
>
> > > + * it is no longer possible for other threads to add requests.
> > > + */
> > > +void fuse_flush_requests(struct fuse_conn *fc)
> > > +{
> > > +       spin_lock(&fc->lock);
> > > +       spin_lock(&fc->bg_lock);
> > > +       if (fc->connected) {
> > > +               /* Push all the background requests to the queue. */
> > > +               fc->blocked = 0;
> > > +               fc->max_background = UINT_MAX;
> > > +               flush_bg_queue(fc);
> > > +       }
> > > +       spin_unlock(&fc->bg_lock);
> > > +       spin_unlock(&fc->lock);
> > > +}
> > > +
> > >  /*
> > >   * Abort all requests.
> > >   *
> > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > index e57b8af06be93e..58c3351b467221 100644
> > > --- a/fs/fuse/inode.c
> > > +++ b/fs/fuse/inode.c
> > > @@ -2086,8 +2086,18 @@ void fuse_conn_destroy(struct fuse_mount *fm)
> > >  {
> > >         struct fuse_conn *fc = fm->fc;
> > >
> > > -       if (fc->destroy)
> > > +       if (fc->destroy) {
> > > +               /*
> > > +                * Flush all pending requests (most of which will be
> > > +                * FUSE_RELEASE) before sending FUSE_DESTROY, because the fuse
> > > +                * server must close the filesystem before replying to the
> > > +                * destroy message, because unmount is about to release its
> > > +                * O_EXCL hold on the block device.  We don't wait, so libfuse
> > > +                * has to do that for us.
> >
> > nit: imo the "because the fuse server must close the filesystem before
> > replying to the destroy message, because..." part is confusing. Even
> > if that weren't true, the pending requests would still have to be sent
> > before the destroy, no? i think it would be less confusing if that
> > part of the paragraph was removed. I think it might be better to
> > remove the "we don't wait, so libfuse has to do that for us" part too
> > or rewording it to something like "flushed requests are sent before
> > the FUSE_DESTROY. Userspace is responsible for ensuring flushed
> > requests are handled before replying to the FUSE_DESTROY".
>
> How about I simplify it to:
>
>         /*
>          * Flush all pending requests before sending FUSE_DESTROY.  The
>          * fuse server must reply to the flushed requests before
>          * handling FUSE_DESTROY because unmount is about to release
>          * its O_EXCL hold on the block device.
>          */

This sounds a lot better to me.

Thanks,
Joanne
>
> --D
>
> >
> > Thanks,
> > Joanne
> >
> > > +                */
> > > +               fuse_flush_requests(fc);
> > >                 fuse_send_destroy(fm);
> > > +       }
> > >
> > >         fuse_abort_conn(fc);
> > >         fuse_wait_aborted(fc);
> > >
> >