Re: [PATCH 2/4] nfsd: perform all find_openstateowner_str calls in the one place.

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-03-04 at 15:40 +1100, NeilBrown wrote:
> Currently find_openstateowner_str looks are done both in
> nfsd4_process_open1() and alloc_init_open_stateowner() - the latter
> possibly being a surprise based on its name.
> 
> It would be easier to follow, and more conformant to common patterns, if
> the lookup was all in the one place.
> 
> So replace alloc_init_open_stateowner() with
> find_or_alloc_open_stateowner() and use the latter in
> nfsd4_process_open1() without any calls to find_openstateowner_str().
> 
> This means all finds are find_openstateowner_str_locked() and
> find_openstateowner_str() is no longer needed.  So discard
> find_openstateowner_str() and rename find_openstateowner_str_locked() to
> find_openstateowner_str().
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 93 +++++++++++++++++++--------------------------
>  1 file changed, 40 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2f1e465628b1..690d0e697320 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -539,7 +539,7 @@ same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
>  }
>  
>  static struct nfs4_openowner *
> -find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
> +find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
>  			struct nfs4_client *clp)
>  {
>  	struct nfs4_stateowner *so;
> @@ -556,18 +556,6 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
>  	return NULL;
>  }
>  
> -static struct nfs4_openowner *
> -find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
> -			struct nfs4_client *clp)
> -{
> -	struct nfs4_openowner *oo;
> -
> -	spin_lock(&clp->cl_lock);
> -	oo = find_openstateowner_str_locked(hashval, open, clp);
> -	spin_unlock(&clp->cl_lock);
> -	return oo;
> -}
> -
>  static inline u32
>  opaque_hashval(const void *ptr, int nbytes)
>  {
> @@ -4588,34 +4576,46 @@ nfsd4_find_and_lock_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
>  }
>  
>  static struct nfs4_openowner *
> -alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
> -			   struct nfsd4_compound_state *cstate)
> +find_or_alloc_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
> +			      struct nfsd4_compound_state *cstate)
>  {
>  	struct nfs4_client *clp = cstate->clp;
> -	struct nfs4_openowner *oo, *ret;
> +	struct nfs4_openowner *oo, *new = NULL;
>  
> -	oo = alloc_stateowner(openowner_slab, &open->op_owner, clp);
> -	if (!oo)
> -		return NULL;
> -	oo->oo_owner.so_ops = &openowner_ops;
> -	oo->oo_owner.so_is_open_owner = 1;
> -	oo->oo_owner.so_seqid = open->op_seqid;
> -	oo->oo_flags = 0;
> -	if (nfsd4_has_session(cstate))
> -		oo->oo_flags |= NFS4_OO_CONFIRMED;
> -	oo->oo_time = 0;
> -	oo->oo_last_closed_stid = NULL;
> -	INIT_LIST_HEAD(&oo->oo_close_lru);
> -	spin_lock(&clp->cl_lock);
> -	ret = find_openstateowner_str_locked(strhashval, open, clp);
> -	if (ret == NULL) {
> -		hash_openowner(oo, clp, strhashval);
> -		ret = oo;
> -	} else
> -		nfs4_free_stateowner(&oo->oo_owner);
> +	while (1) {
> +		spin_lock(&clp->cl_lock);
> +		oo = find_openstateowner_str(strhashval, open, clp);
> +		if (oo && !(oo->oo_flags & NFS4_OO_CONFIRMED)) {
> +			/* Replace unconfirmed owners without checking for replay. */
> +			release_openowner(oo);
> +			oo = NULL;
> +		}
> +		if (oo) {
> +			spin_unlock(&clp->cl_lock);
> +			if (new)
> +				nfs4_free_stateowner(&new->oo_owner);
> +			return oo;
> +		}
> +		if (new) {
> +			hash_openowner(new, clp, strhashval);
> +			spin_unlock(&clp->cl_lock);
> +			return new;
> +		}
> +		spin_unlock(&clp->cl_lock);
>  
> -	spin_unlock(&clp->cl_lock);
> -	return ret;
> +		new = alloc_stateowner(openowner_slab, &open->op_owner, clp);
> +		if (!new)
> +			return NULL;
> +		new->oo_owner.so_ops = &openowner_ops;
> +		new->oo_owner.so_is_open_owner = 1;
> +		new->oo_owner.so_seqid = open->op_seqid;
> +		new->oo_flags = 0;
> +		if (nfsd4_has_session(cstate))
> +			new->oo_flags |= NFS4_OO_CONFIRMED;
> +		new->oo_time = 0;
> +		new->oo_last_closed_stid = NULL;
> +		INIT_LIST_HEAD(&new->oo_close_lru);
> +	}

The while (1) makes the control flow a little weird here, but the logic
seems correct.

>  }
>  
>  static struct nfs4_ol_stateid *
> @@ -5064,28 +5064,15 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
>  	clp = cstate->clp;
>  
>  	strhashval = ownerstr_hashval(&open->op_owner);
> -	oo = find_openstateowner_str(strhashval, open, clp);
> +	oo = find_or_alloc_open_stateowner(strhashval, open, cstate);
>  	open->op_openowner = oo;
>  	if (!oo)
> -		goto new_owner;
> -	if (!(oo->oo_flags & NFS4_OO_CONFIRMED)) {
> -		/* Replace unconfirmed owners without checking for replay. */
> -		release_openowner(oo);
> -		open->op_openowner = NULL;
> -		goto new_owner;
> -	}
> +		return nfserr_jukebox;
>  	nfsd4_cstate_assign_replay(cstate, &oo->oo_owner);
>  	status = nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
>  	if (status)
>  		return status;
> -	goto alloc_stateid;
> -new_owner:
> -	oo = alloc_init_open_stateowner(strhashval, open, cstate);
> -	if (oo == NULL)
> -		return nfserr_jukebox;
> -	open->op_openowner = oo;
> -	nfsd4_cstate_assign_replay(cstate, &oo->oo_owner);
> -alloc_stateid:
> +
>  	open->op_stp = nfs4_alloc_open_stateid(clp);
>  	if (!open->op_stp)
>  		return nfserr_jukebox;

Nice cleanup overall:

Reviewed-by: Jeff Layton <jlayton@kernel.org>