From: Kirill Smelkov <kirr@nexedi.com> To: Miklos Szeredi <miklos@szeredi.hu> Cc: Miklos Szeredi <mszeredi@redhat.com>, <linux-fsdevel@vger.kernel.org>, fuse-devel <fuse-devel@lists.sourceforge.net>, Han-Wen Nienhuys <hanwen@google.com>, Jakob Unterwurzacher <jakobunt@gmail.com>, stable <stable@vger.kernel.org> Subject: Re: [RESEND, PATCH v2] fuse: Don't drop NOTIFY_REPLY if we promised it Date: Thu, 28 Feb 2019 11:48:03 +0000 [thread overview] Message-ID: <20190228114757.GA2796@deco.navytux.spb.ru> (raw) In-Reply-To: <CAJfpegvXK+AmpWFSF1qdVt+EFTBwU94AD_JZCEeFF1KEZCObwA@mail.gmail.com> On Thu, Feb 28, 2019 at 09:10:15AM +0100, Miklos Szeredi wrote: > On Wed, Feb 27, 2019 at 9:39 PM Kirill Smelkov <kirr@nexedi.com> wrote: > > > I more or less agree with this statement. However can we please make the > > breakage to be explicitly visible with an error instead of exhibiting it > > via harder to debug stucks/deadlocks? For example sys_read < max_write > > -> error instead of getting stuck. And if notify_retrieve requests > > buffer larger than max_write -> error or cut to max_write, but don't > > return OK when we know we will never send what was requested to > > filesystem even if it uses max_write sized reads. What is the point of > > breaking in hard to diagnose way when we can make the breakage showing > > itself explicitly? Would a patch for such behaviour accepted? > > Sure, if it's only adds a couple of lines. Adding more than say ten > lines for such a non-bug fix is definitely excessive. Ok, thanks. Please consider applying the following patch. (It's a bit pity to hear the problem is not considered to be a bug, but anyway). I will also send the second patch as another mail, since I could not made `git am --scissors` to apply several patched extracted from one mail successfully. Thanks, Kirill ---- 8< ---- From: Kirill Smelkov <kirr@nexedi.com> Date: Thu, 28 Feb 2019 13:06:18 +0300 Subject: [PATCH 1/2] fuse: retrieve: cap requested size to negotiated max_write MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit FUSE filesystem server and kernel client negotiate during initialization phase, what should be the maximum write size the client will ever issue. Correspondingly the filesystem server then queues sys_read calls to read requests with buffer capacity large enough to carry request header + that max_write bytes. A filesystem server is free to set its max_write in anywhere in the range between [1·page, fc->max_pages·page]. In particular go-fuse[2] sets max_write by default as 64K, wheres default fc->max_pages corresponds to 128K. Libfuse also allows users to configure max_write, but by default presets it to possible maximum. If max_write is < fc->max_pages·page, and in NOTIFY_RETRIEVE handler we allow to retrieve more than max_write bytes, corresponding prepared NOTIFY_REPLY will be thrown away by fuse_dev_do_read, because the filesystem server, in full correspondence with server/client contract, will be only queuing sys_read with ~max_write buffer capacity, and fuse_dev_do_read throws away requests that cannot fit into server request buffer. In turn the filesystem server could get stuck waiting indefinitely for NOTIFY_REPLY since NOTIFY_RETRIEVE handler returned OK which is understood by clients as that NOTIFY_REPLY was queued and will be sent back. -> Cap requested size to negotiate max_write to avoid the problem. This aligns with the way NOTIFY_RETRIEVE handler works, which already unconditionally caps requested retrieve size to fuse_conn->max_pages. This way it should not hurt NOTIFY_RETRIEVE semantic if we return less data than was originally requested. Please see [1] for context where the problem of stuck filesystem was hit for real, how the situation was traced and for more involving patch that did not make it into the tree. [1] https://marc.info/?l=linux-fsdevel&m=155057023600853&w=2 [2] https://github.com/hanwen/go-fuse Signed-off-by: Kirill Smelkov <kirr@nexedi.com> Cc: Han-Wen Nienhuys <hanwen@google.com> Cc: Jakob Unterwurzacher <jakobunt@gmail.com> Cc: <stable@vger.kernel.org> # v2.6.36+ --- fs/fuse/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 8a63e52785e9..38e94bc43053 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -1749,7 +1749,7 @@ static int fuse_retrieve(struct fuse_conn *fc, struct inode *inode, offset = outarg->offset & ~PAGE_MASK; file_size = i_size_read(inode); - num = outarg->size; + num = min(outarg->size, fc->max_write); if (outarg->offset > file_size) num = 0; else if (outarg->offset + num > file_size) -- 2.21.0.352.gf09ad66450
WARNING: multiple messages have this Message-ID (diff)
From: Kirill Smelkov <kirr@nexedi.com> To: Miklos Szeredi <miklos@szeredi.hu> Cc: Miklos Szeredi <mszeredi@redhat.com>, <linux-fsdevel@vger.kernel.org>, fuse-devel <fuse-devel@lists.sourceforge.net>, Han-Wen Nienhuys <hanwen@google.com>, Jakob Unterwurzacher <jakobunt@gmail.com>, stable <stable@vger.kernel.org> Subject: Re: [RESEND, PATCH v2] fuse: Don't drop NOTIFY_REPLY if we promised it Date: Thu, 28 Feb 2019 11:48:04 +0000 [thread overview] Message-ID: <20190228114757.GA2796@deco.navytux.spb.ru> (raw) In-Reply-To: <CAJfpegvXK+AmpWFSF1qdVt+EFTBwU94AD_JZCEeFF1KEZCObwA@mail.gmail.com> On Thu, Feb 28, 2019 at 09:10:15AM +0100, Miklos Szeredi wrote: > On Wed, Feb 27, 2019 at 9:39 PM Kirill Smelkov <kirr@nexedi.com> wrote: > > > I more or less agree with this statement. However can we please make the > > breakage to be explicitly visible with an error instead of exhibiting it > > via harder to debug stucks/deadlocks? For example sys_read < max_write > > -> error instead of getting stuck. And if notify_retrieve requests > > buffer larger than max_write -> error or cut to max_write, but don't > > return OK when we know we will never send what was requested to > > filesystem even if it uses max_write sized reads. What is the point of > > breaking in hard to diagnose way when we can make the breakage showing > > itself explicitly? Would a patch for such behaviour accepted? > > Sure, if it's only adds a couple of lines. Adding more than say ten > lines for such a non-bug fix is definitely excessive. Ok, thanks. Please consider applying the following patch. (It's a bit pity to hear the problem is not considered to be a bug, but anyway). I will also send the second patch as another mail, since I could not made `git am --scissors` to apply several patched extracted from one mail successfully. Thanks, Kirill ---- 8< ---- From: Kirill Smelkov <kirr@nexedi.com> Date: Thu, 28 Feb 2019 13:06:18 +0300 Subject: [PATCH 1/2] fuse: retrieve: cap requested size to negotiated max_write MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit FUSE filesystem server and kernel client negotiate during initialization phase, what should be the maximum write size the client will ever issue. Correspondingly the filesystem server then queues sys_read calls to read requests with buffer capacity large enough to carry request header + that max_write bytes. A filesystem server is free to set its max_write in anywhere in the range between [1·page, fc->max_pages·page]. In particular go-fuse[2] sets max_write by default as 64K, wheres default fc->max_pages corresponds to 128K. Libfuse also allows users to configure max_write, but by default presets it to possible maximum. If max_write is < fc->max_pages·page, and in NOTIFY_RETRIEVE handler we allow to retrieve more than max_write bytes, corresponding prepared NOTIFY_REPLY will be thrown away by fuse_dev_do_read, because the filesystem server, in full correspondence with server/client contract, will be only queuing sys_read with ~max_write buffer capacity, and fuse_dev_do_read throws away requests that cannot fit into server request buffer. In turn the filesystem server could get stuck waiting indefinitely for NOTIFY_REPLY since NOTIFY_RETRIEVE handler returned OK which is understood by clients as that NOTIFY_REPLY was queued and will be sent back. -> Cap requested size to negotiate max_write to avoid the problem. This aligns with the way NOTIFY_RETRIEVE handler works, which already unconditionally caps requested retrieve size to fuse_conn->max_pages. This way it should not hurt NOTIFY_RETRIEVE semantic if we return less data than was originally requested. Please see [1] for context where the problem of stuck filesystem was hit for real, how the situation was traced and for more involving patch that did not make it into the tree. [1] https://marc.info/?l=linux-fsdevel&m=155057023600853&w=2 [2] https://github.com/hanwen/go-fuse Signed-off-by: Kirill Smelkov <kirr@nexedi.com> Cc: Han-Wen Nienhuys <hanwen@google.com> Cc: Jakob Unterwurzacher <jakobunt@gmail.com> Cc: <stable@vger.kernel.org> # v2.6.36+ --- fs/fuse/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 8a63e52785e9..38e94bc43053 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -1749,7 +1749,7 @@ static int fuse_retrieve(struct fuse_conn *fc, struct inode *inode, offset = outarg->offset & ~PAGE_MASK; file_size = i_size_read(inode); - num = outarg->size; + num = min(outarg->size, fc->max_write); if (outarg->offset > file_size) num = 0; else if (outarg->offset + num > file_size) -- 2.21.0.352.gf09ad66450
next prev parent reply other threads:[~2019-02-28 12:03 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-19 9:42 [RESEND, PATCH v2] fuse: Don't drop NOTIFY_REPLY if we promised it Kirill Smelkov 2019-02-19 9:42 ` Kirill Smelkov 2019-02-26 15:14 ` Miklos Szeredi 2019-02-27 20:02 ` Kirill Smelkov 2019-02-27 20:02 ` Kirill Smelkov 2019-02-27 20:26 ` Miklos Szeredi 2019-02-27 20:39 ` Kirill Smelkov 2019-02-28 8:10 ` Miklos Szeredi 2019-02-28 11:48 ` Kirill Smelkov [this message] 2019-02-28 11:48 ` Kirill Smelkov 2019-02-28 11:50 ` [PATCH 2/2] fuse: require /dev/fuse reads to have enough buffer capacity as negotiated Kirill Smelkov 2019-03-07 9:34 ` [RESEND, PATCH v2] fuse: Don't drop NOTIFY_REPLY if we promised it Kirill Smelkov 2019-03-14 10:45 ` [RESEND3, PATCH 0/2] fuse: don't stuck clients on retrieve_notify with size > max_write Kirill Smelkov 2019-03-14 10:46 ` [PATCH 1/2] fuse: retrieve: cap requested size to negotiated max_write Kirill Smelkov 2019-03-14 10:46 ` [PATCH 2/2] fuse: require /dev/fuse reads to have enough buffer capacity as negotiated Kirill Smelkov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190228114757.GA2796@deco.navytux.spb.ru \ --to=kirr@nexedi.com \ --cc=fuse-devel@lists.sourceforge.net \ --cc=hanwen@google.com \ --cc=jakobunt@gmail.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=miklos@szeredi.hu \ --cc=mszeredi@redhat.com \ --cc=stable@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.