On Mon, Jul 14, 2025 at 02:19:33PM +1000, NeilBrown wrote:
> On Mon, 14 Jul 2025, Mike Snitzer wrote:
> > From: Mike Snitzer <snitzer@xxxxxxxxxxxxxxx>
> >
> > Previously nfs_local_probe() was made to disable and then attempt to
> > re-enable LOCALIO (via LOCALIO protocol handshake) if/when it was
> > called and LOCALIO already enabled.
> >
> > Vague memory for _why_ this was the case is that this was useful
> > if/when a local NFS server were to be restarted with a local NFS
> > client connected to it.
> >
> > But as it happens this causes an absurd amount of LOCALIO flapping
> > which has a side-effect of too much IO being needlessly sent to NFSD
> > (using RPC over the loopback network interface). This is the
> > definition of "serious performance loss" (that negates the point of
> > having LOCALIO).
> >
> > So remove this mis-optimization for re-enabling LOCALIO if/when an NFS
> > server is restarted (which is an extremely rare thing to do). Will
> > revisit testing that scenario again but in the meantime this patch
> > restores the full benefit of LOCALIO.
> >
> > Fixes: 56bcd0f07fdb ("nfs: implement client support for NFS_LOCALIO_PROGRAM")
> > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxxxxxxx>
>
> Reviewed-by: NeilBrown <neil@xxxxxxxxxx>
>
> I cannot see any justification for probing if localio is currently
> thought to be working. If for some reason it doesn't work, then when we
> notice that is a good time to disable - which it what this patch does.
Thanks for the review!
> However...
>
> > ---
> > fs/nfs/localio.c | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> > index a4bacd9a5052..e3ae003118cb 100644
> > --- a/fs/nfs/localio.c
> > +++ b/fs/nfs/localio.c
> > @@ -180,10 +180,8 @@ static void nfs_local_probe(struct nfs_client *clp)
> > return;
> > }
> >
> > - if (nfs_client_is_local(clp)) {
> > - /* If already enabled, disable and re-enable */
> > - nfs_localio_disable_client(clp);
> > - }
> > + if (nfs_client_is_local(clp))
> > + return;
> >
> > if (!nfs_uuid_begin(&clp->cl_uuid))
> > return;
> > @@ -241,7 +239,8 @@ __nfs_local_open_fh(struct nfs_client *clp, const struct cred *cred,
> > case -ENOMEM:
> > case -ENXIO:
> > case -ENOENT:
> > - /* Revalidate localio, will disable if unsupported */
> > + /* Revalidate localio */
> > + nfs_localio_disable_client(clp);
> > nfs_local_probe(clp);
>
> Shouldn't that be nfs_local_probe_async() ???? I wonder why that wasn't
> changed in
> Commit 1ff4716f420b ("NFS: always probe for LOCALIO support asynchronously")
Creates the potential for many files being opened and each triggering
their own async probe. I reasoned it best to make this error path
wait. In practice that has worked reasonably...
Mike
