Re: [RESEND, PATCH v2] fuse: Don't drop NOTIFY_REPLY if we promised it

[Kirill Smelkov <kirr@nexedi.com>]

On Wed, Feb 27, 2019 at 09:26:32PM +0100, Miklos Szeredi wrote:
> On Wed, Feb 27, 2019 at 9:02 PM Kirill Smelkov <kirr@nexedi.com> wrote:
> >
> > Miklos, first of all thanks for feedback.
> >
> > On Tue, Feb 26, 2019 at 04:14:22PM +0100, Miklos Szeredi wrote:
> > > On Tue, Feb 19, 2019 at 10:42 AM Kirill Smelkov <kirr@nexedi.com> wrote:
> > > >
> > > > A successful call to NOTIFY_RETRIEVE by filesystem carries promise from
> > > > the kernel to send back NOTIFY_REPLY message. However if the filesystem
> > > > is not reading requests with fuse_conn->max_pages capacity,
> > >
> > > That's a violation of the contract by the fuse server, not the kernel.
> >
> > Do you mean that even if filesystem server configures via
> > init_out.max_write that it is accepting e.g. only 32K max writes, it
> > still has to be issuing sys_read with buffer of 128K (= hardcoded
> > fuse_conn->max_pages before Linux 4.20, and default since Linux 4.20)?
> 
> Filesystem is asking for a specific number of bytes to retrieve.  It
> does not have to be less than max_writes, but it does have to fit into
> the request buffer it is using.  If the filesystem is asking to
> retrieve 64k and it is using a 32k request buffer, then that obviously
> won't work.   Kernel could limit the retrieve length to max_writes,
> that may make sense, but it doesn't fundamentally change the fact that
> if the filesystem is not properly sizing the request buffer, it may
> result in various forms of breakage.

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?

Thanks,
Kirill