Re: [PATCH 5/6] fuse: fix synchronous case of fuse_file_put()

[Maxim Patlasov]

On 08/22/2014 06:08 PM, Miklos Szeredi wrote:
> On Thu, Aug 21, 2014 at 6:09 PM, Maxim Patlasov <MPatlasov@xxxxxxxxxxxxx> wrote:
> > If fuse_file_put() is called with sync==true, the user may be blocked for
> > a while, until userspace ACKs our FUSE_RELEASE request. This blocking must be
> > uninterruptible. Otherwise request could be interrupted, but file association
> > in user space remains.
> >
> > Signed-off-by: Maxim Patlasov <mpatlasov@xxxxxxxxxxxxx>
> > ---
> >   fs/fuse/file.c |    4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index cd55488..b92143a 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -136,6 +136,10 @@ static void fuse_file_put(struct fuse_file *ff, bool sync)
> >                          path_put(&req->misc.release.path);
> >                          fuse_put_request(ff->fc, req);
> >                  } else if (sync) {
> > +                       /* Must force. Otherwise request could be interrupted,
> > +                        * but file association in user space remains.
> > +                        */
> > +                       req->force = 1;
> >                          req->background = 0;
> >                          fuse_request_send(ff->fc, req);
> >                          path_put(&req->misc.release.path);
>
> Some thought needs to go into this:  if RELEASE is interrupted, then
> we should possibly allow that, effectively backgrounding the request.
>
> The synchronous nature is just an optimization and we really don't
> know where we are being interrupted, possibly in a place which very
> much *should* allow interruption.

A fuse daemon who explicitly enables the feature (synchronous release) 
would definitely want non-interruptible behaviour of last fput. 
Otherwise, it would face the same problem that the feature tries to 
resolve: an application was killed and exited, but there is no way to 
determine why actual processing of RELEASE will be completed.

As for fuseblk mounts, I'm not so sure. I believed the lack of force=1 
was a bug and my patch fixes it. If you think it's safer to preserve old 
behaviour, I could set "force" conditionally. May be you could explain 
in more details why you think we should allow interruption somewhere. 
Any examples or use cases? Btw, fuse_flush also uses force=1. Do you 
concerns deal with it as well?

> Also fuse really should distinguish fatal and non-fatal interruptions
> and handle them accordingly...

Do you think it's worthy to elaborate this in the scope of "synchronous 
release" feature?

Thanks,
Maxim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/