Re: [PATCH 1/6] nfs_localio: use cmpxchg() to install new nfs_file_localio

[Mike Snitzer <snitzer@kernel.org>]

On Wed, Apr 30, 2025 at 02:01:11PM +1000, NeilBrown wrote:
> Rather than using nfs_uuid.lock to protect installing
> a new ro_file or rw_file, change to use cmpxchg().
> Removing the file already uses xchg() so this improves symmetry
> and also makes the code a little simpler.
> 
> Also remove the optimisation of not taking the lock, and not removing
> the nfs_file_localio from the linked list, when both ->ro_file and
> ->rw_file are already NULL.  Given that ->nfs_uuid was not NULL, it is
> extremely unlikely that neither ->ro_file or ->rw_file is NULL so
> this optimisation can be of little value and it complicates
> understanding of the code - why can the list_del_init() be skipped?
> 
> Finally, move the assignment of NULL to ->nfs_uuid until after
> the last action on the nfs_file_localio (the list_del_init).  As soon as
> this is NULL a racing nfs_close_local_fh() can bypass all the locking
> and go on to free the nfs_file_localio, so we must be certain to be
> finished with it first.
> 
> Fixes: 86e00412254a ("nfs: cache all open LOCALIO nfsd_file(s) in client")
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfs/localio.c           | 11 +++--------
>  fs/nfs_common/nfslocalio.c | 36 ++++++++++++++++--------------------
>  2 files changed, 19 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> index 5c21caeae075..3674dd86f095 100644
> --- a/fs/nfs/localio.c
> +++ b/fs/nfs/localio.c
> @@ -279,14 +279,9 @@ nfs_local_open_fh(struct nfs_client *clp, const struct cred *cred,
>  		if (IS_ERR(new))
>  			return NULL;
>  		/* try to swap in the pointer */
> -		spin_lock(&clp->cl_uuid.lock);
> -		nf = rcu_dereference_protected(*pnf, 1);
> -		if (!nf) {
> -			nf = new;
> -			new = NULL;
> -			rcu_assign_pointer(*pnf, nf);
> -		}
> -		spin_unlock(&clp->cl_uuid.lock);
> +		nf = unrcu_pointer(cmpxchg(pnf, NULL, RCU_INITIALIZER(new)));
> +		if (!nf)
> +			swap(nf, new);
>  		rcu_read_lock();
>  	}
>  	nf = nfs_local_file_get(nf);
> diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> index 6a0bdea6d644..d72eecb85ea9 100644
> --- a/fs/nfs_common/nfslocalio.c
> +++ b/fs/nfs_common/nfslocalio.c
> @@ -285,28 +285,24 @@ void nfs_close_local_fh(struct nfs_file_localio *nfl)
>  		return;
>  	}
>  
> -	ro_nf = rcu_access_pointer(nfl->ro_file);
> -	rw_nf = rcu_access_pointer(nfl->rw_file);
> -	if (ro_nf || rw_nf) {
> -		spin_lock(&nfs_uuid->lock);
> -		if (ro_nf)
> -			ro_nf = rcu_dereference_protected(xchg(&nfl->ro_file, NULL), 1);
> -		if (rw_nf)
> -			rw_nf = rcu_dereference_protected(xchg(&nfl->rw_file, NULL), 1);
> -
> -		/* Remove nfl from nfs_uuid->files list */
> -		RCU_INIT_POINTER(nfl->nfs_uuid, NULL);
> -		list_del_init(&nfl->list);
> -		spin_unlock(&nfs_uuid->lock);
> -		rcu_read_unlock();
> +	ro_nf = unrcu_pointer(xchg(&nfl->ro_file, NULL));
> +	rw_nf = unrcu_pointer(xchg(&nfl->rw_file, NULL));
>  
> -		if (ro_nf)
> -			nfs_to_nfsd_file_put_local(ro_nf);
> -		if (rw_nf)
> -			nfs_to_nfsd_file_put_local(rw_nf);
> -		return;
> -	}
> +	spin_lock(&nfs_uuid->lock);
> +	/* Remove nfl from nfs_uuid->files list */
> +	list_del_init(&nfl->list);
> +	spin_unlock(&nfs_uuid->lock);
>  	rcu_read_unlock();
> +	/* Now we can allow racing nfs_close_local_fh() to
> +	 * skip the locking.
> +	 */
> +	RCU_INIT_POINTER(nfl->nfs_uuid, NULL);
> +
> +	if (ro_nf)
> +		nfs_to_nfsd_file_put_local(ro_nf);
> +	if (rw_nf)
> +		nfs_to_nfsd_file_put_local(rw_nf);
> +	return;
>  }

This return at the end of the function isn't needed.

>  EXPORT_SYMBOL_GPL(nfs_close_local_fh);
>  
> -- 
> 2.49.0
> 
>