Re: [for-6.16-final PATCH 8/9] nfs/localio: avoid bouncing LOCALIO if nfs_client_is_local()

[Mike Snitzer <snitzer@kernel.org>]

On Mon, Jul 14, 2025 at 02:19:33PM +1000, NeilBrown wrote:
> On Mon, 14 Jul 2025, Mike Snitzer wrote:
> > From: Mike Snitzer <snitzer@hammerspace.com>
> > 
> > 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@hammerspace.com>
> 
> Reviewed-by: NeilBrown <neil@brown.name>
> 
> 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