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

[Maxim Patlasov <mpatlasov@parallels.com>]

On 08/22/2014 06:08 PM, Miklos Szeredi wrote:
> On Thu, Aug 21, 2014 at 6:09 PM, Maxim Patlasov <MPatlasov@parallels.com> 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@parallels.com>
>> ---
>>   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