On Tue, 2024-03-05 at 08:41 +1100, NeilBrown wrote:
> On Mon, 04 Mar 2024, Jeff Layton wrote:
> > 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@xxxxxxx>
> > > ---
> > > 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.
>
> Would you prefer a goto? That is more consistent with common patterns.
> I'll do that.
>
Ok, that works. I don't have a strong preference either way, so whatever
you think is easiest on the eyes.
> >
> > > }
> > >
> > > 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@xxxxxxxxxx>
>
> Thanks.
> BTW I've moved this to the start of the series - but kept your SoB.
> That avoid having one patch add code that the next patch removes.
>
Sounds good.
--
Jeff Layton <jlayton@xxxxxxxxxx>
