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@xxxxxxxxxx>
> ---
> 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
>
>
