| Message ID | 20240304044304.3657-3-neilb@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: fix dadlock in move_to_close_lru() | expand |
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>
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@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. Would you prefer a goto? That is more consistent with common patterns. I'll do that. > > > } > > > > 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> 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. Thanks, NeilBrown >
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@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. > > 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@kernel.org> > > 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.
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); + } } 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;
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(-)