Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <YvpRLJ79GRWYjLdf@tycho.pizza> <20220901140647.1125079-1-tycho@tycho.pizza>
 <CAJfpegswSAeUdxHR1Z8jC_nQtUm7_mD=ZZC_LyQczaoJWTPe3g@mail.gmail.com>
In-Reply-To: <CAJfpegswSAeUdxHR1Z8jC_nQtUm7_mD=ZZC_LyQczaoJWTPe3g@mail.gmail.com>
From:   Stef Bon <stefbon@gmail.com>
Date:   Thu, 29 Sep 2022 16:05:54 +0200
Message-ID: <CANXojcw6bi0TMu62wiXxkXg-osTELt2=5UcNdVeAHNCwd_xOhg@mail.gmail.com>
Subject: Re: [fuse-devel] [PATCH] fuse: In fuse_flush only wait if someone
 wants the return code
To:     Miklos Szeredi <miklos@szeredi.hu>
Cc:     Tycho Andersen <tycho@tycho.pizza>,
        fuse-devel@lists.sourceforge.net,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Eric Biederman <ebiederm@xmission.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

I've some idea;s about the cause of the error.
In the first message about this:

"However, there's a problem when the fuse daemon
itself spawns a thread that does a flush: since the thread has a copy of
the fd table with an fd pointing to the same fuse device, the reference
count isn't decremented to zero in fuse_dev_release(), and the task hangs
forever."

If the kernel starts to abort the filesystem (since the daemon in
userspace is terminated), and cannot do that since a file handle is
still open due to a flush, resulting in a hang, maybe the reason to
stop/abort the filesystem is wrong. The kernel should look at the fuse
device fd (which is duplicated after spawning), find there is still
one fd open, and should not go into aborting the fs.

I hope this helps,

Stef Bon
the Netherlands

Op di 27 sep. 2022 om 11:48 schreef Miklos Szeredi via fuse-devel
<fuse-devel@lists.sourceforge.net>:


>
> On Thu, 1 Sept 2022 at 16:07, Tycho Andersen <tycho@tycho.pizza> wrote:
> >
> > From: "Eric W. Biederman" <ebiederm@xmission.com>
> >
> > In my very light testing this resolves a hang where a thread of the
> > fuse server was accessing the fuse filesystem (the fuse server is
> > serving up), when the fuse server is killed.
> >
> > The practical problem is that the fuse server file descriptor was
> > being closed after the file descriptor into the fuse filesystem so
> > that the fuse filesystem operations were being blocked for instead of
> > being aborted.  Simply skipping the unnecessary wait resolves this
> > issue.
> >
> > This is just a proof of concept and someone should look to see if the
> > fuse max_background limit could cause a problem with this approach.
>
> Maybe you missed my comments here:
>
> https://lore.kernel.org/all/CAJfpegsTmiO-sKaBLgoVT4WxDXBkRES=HF1YmQN1ES7gfJEJ+w@mail.gmail.com/
>
> I'm generally okay with this, but please write a proper changelog for
> the patch, also mentioning the issues related to posix locks.
>
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -464,6 +464,67 @@ static void fuse_sync_writes(struct inode *inode)
> >         fuse_release_nowrite(inode);
> >  }
> >
> > +struct fuse_flush_args {
> > +       struct fuse_args args;
> > +       struct fuse_flush_in inarg;
> > +       struct inode *inode;
> > +       struct fuse_file *ff;
> > +};
> > +
> > +static void fuse_flush_end(struct fuse_mount *fm, struct fuse_args *args, int err)
> > +{
> > +       struct fuse_flush_args *fa = container_of(args, typeof(*fa), args);
> > +
> > +       if (err == -ENOSYS) {
> > +               fm->fc->no_flush = 1;
> > +               err = 0;
> > +       }
> > +
> > +       /*
> > +        * In memory i_blocks is not maintained by fuse, if writeback cache is
> > +        * enabled, i_blocks from cached attr may not be accurate.
> > +        */
> > +       if (!err && fm->fc->writeback_cache)
> > +               fuse_invalidate_attr_mask(fa->inode, STATX_BLOCKS);
> > +
> > +
> > +       iput(fa->inode);
> > +       fuse_file_put(fa->ff, false, false);
> > +       kfree(fa);
> > +}
> > +
> > +static int fuse_flush_async(struct file *file, fl_owner_t id)
> > +{
> > +       struct inode *inode = file_inode(file);
> > +       struct fuse_mount *fm = get_fuse_mount(inode);
> > +       struct fuse_file *ff = file->private_data;
> > +       struct fuse_flush_args *fa;
> > +       int err;
> > +
> > +       fa = kzalloc(sizeof(*fa), GFP_KERNEL);
> > +       if (!fa)
> > +               return -ENOMEM;
> > +
> > +       fa->inarg.fh = ff->fh;
> > +       fa->inarg.lock_owner = fuse_lock_owner_id(fm->fc, id);
> > +       fa->args.opcode = FUSE_FLUSH;
> > +       fa->args.nodeid = get_node_id(inode);
> > +       fa->args.in_numargs = 1;
> > +       fa->args.in_args[0].size = sizeof(fa->inarg);
> > +       fa->args.in_args[0].value = &fa->inarg;
> > +       fa->args.force = true;
> > +       fa->args.nocreds = true;
> > +       fa->args.end = fuse_flush_end;
> > +       fa->inode = igrab(inode);
>
> Grabbing the inode should already taken care of by fuse_file_release().
>
> Also please try to reduce duplication in both the above functions.
>
> Thanks,
> Miklos
>
>
> --
> fuse-devel mailing list
> To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel