From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756029AbaHYP62 (ORCPT ); Mon, 25 Aug 2014 11:58:28 -0400 Received: from relay.parallels.com ([195.214.232.42]:54124 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754715AbaHYP61 (ORCPT ); Mon, 25 Aug 2014 11:58:27 -0400 Message-ID: <53FB5D20.8040806@parallels.com> Date: Mon, 25 Aug 2014 19:58:24 +0400 From: Maxim Patlasov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Miklos Szeredi CC: fuse-devel , Anand Avati , Kernel Mailing List Subject: Re: [PATCH 5/6] fuse: fix synchronous case of fuse_file_put() References: <20140821160304.11005.15166.stgit@localhost.localdomain> <20140821160916.11005.37344.stgit@localhost.localdomain> In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.30.22.200] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/22/2014 06:08 PM, Miklos Szeredi wrote: > On Thu, Aug 21, 2014 at 6:09 PM, Maxim Patlasov 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 >> --- >> 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