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

Chuck Lever III
2024-07-02T18:06:21Z

> 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.

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.

To make it official, for v11 of this series:

Nacked-by: Chuck Lever <chuck.lever@oracle.com>

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

> - 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().

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.

--
Chuck Lever