Re: [PATCH v11 00/20] nfs/nfsd: add support for localio

[Mike Snitzer <snitzer@kernel.org>]

On Tue, Jul 02, 2024 at 06:06:09PM +0000, Chuck Lever III wrote:
> 
> 
> > On Jul 2, 2024, at 12:28 PM, Mike Snitzer <snitzer@kernel.org> wrote:
> > 
> > Hi,
> > 
> > There seems to be consensus that these changes worthwhile and
> > extensively iterated on.
> 
> I don't see a public consensus about "extensively iterated
> on". The folks you talk to privately might believe that,
> though.
> 
> 
> > I'd very much like these changes to land upstream as-is (unless review
> > teases out some show-stopper).  These changes have been tested fairly
> > extensively (via xfstests) at this point.
> > 
> > Can we now please provide formal review tags and merge these changes
> > through the NFS client tree for 6.11?
> 
> Contributors don't get to determine the kernel release where
> their code lands; maintainers make that decision. You've stated
> your preference, and we are trying to accommodate. But frankly,
> the (server) changes don't stand up to close inspection yet.
> 
> One of the client maintainers has had years to live with this
> work. But the server maintainers had their first look at this
> just a few weeks ago, and this is not the only thing any of us
> have on our plates at the moment. So you need to be patient.
> 
> 
> > FYI:
> > - I do not intend to rebase this series ontop of NeilBrown's partial
> >  exploration of simplifying away the need for a "fake" svc_rqst
> >  (noble goals and happy to help those changes land upstream as an
> >  incremental improvement):
> >  https://marc.info/?l=linux-nfs&m=171980269529965&w=2
> 
> Sorry, rebasing is going to be a requirement.

What?  You're imposing a rebase on completely unfinished and untested
code?  Any idea when Neil will post v2?  Or am I supposed to take his
partial first pass and fix it?

> Again, as with the dprintk stuff, this is code that would get
> reverted or replaced as soon as we merge. We don't knowingly
> merge that kind of code; we fix it first.

Nice rule, except there is merit in tested code landing without it
having to see last minute academic changes.  These aren't dprintk,
these are disruptive changes that aren't fully formed.  If they were
fully formed I wouldn't be resisting them.

> To make it official, for v11 of this series:
> 
>   Nacked-by: Chuck Lever <chuck.lever@oracle.com>

Thanks for that.

> I'll be much more ready to consider an Acked-by: once the
> "fake svc_rqst" code has been replaced.

If Neil completes his work I'll rebase.  But last time I rebased to
his simplification of the localio protocol (to use array and not
lists, nice changes, appreciated but it took serious work on my part
to fold them in): the code immediately BUG_ON()'d in sunrpc trivially.
So please be considerate of my time and the requirement for code to
actually work.

I'm fine with these changes not landing for 6.11 if warranted.  I just
seriously question the arbitrary nature of what constitutes necessary
change to allow inclusion.

> > - In addition, tweaks to use nfsd_file_acquire_gc() instead of
> >  nfsd_file_acquire() aren't a priority.
> 
> The discussion has moved well beyond that now... IIUC the
> preferred approach might be to hold the file open until the
> local app is done with it. However, I'm still not convinced
> there's a benefit to using the NFSD file cache vs. a plain
> dentry_open().

Saving an nfs_file to open_context, etc.  All incremental improvement
(that needs time to stick the landing).

Why do you think it appropriate to cause upheaval on code that has
clearly drawn a line in the sand in terms of established fitness?

Eliding allocation of things and micro-optimizing can come later.  But
I guess I'll just have to agree to disagree with this approach.
Really feels like I'll be forced to keep both pieces when it breaks in
the near-term.

By all means layer on new improvements.  But this fear to establish a
baseline out of fear that we _might_ change it: don't even know where
to begin with that.

> Neil's clean-up might not need add a new nfsd_file_acquire()
> API if we go with plain dentry_open().
> 
> There are still interesting choices to make here before it
> is merged, so IMO the choices around nfsd_file_acquire()
> remain a priority for merge-readiness.

Maybe Neil will post a fully working v12 rebased on his changes.

Mike