| Message ID | 171010907730.13576.5751778995820664441@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4,v3] nfsd: perform all find_openstateowner_str calls in the one place. | expand |
On Mon, 2024-03-11 at 09:17 +1100, NeilBrown wrote: > Currently find_openstateowner_str lookups 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> > --- > > The previous version of this patch could deadlock on ->cl_lcok as it > called release_openowner() while holding that lock. > This version doesn't. > > Subsequent patch in the original series should be unchanged. If needed > I can resent them all after -rc1 is out. > > 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 ee9aa4843443..3e144b1a1386 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -541,7 +541,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; > @@ -558,18 +558,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) > { > @@ -4855,34 +4843,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); > +retry: > 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); > - > + oo = find_openstateowner_str(strhashval, open, clp); > + if (!oo && new) { > + hash_openowner(new, clp, strhashval); > + spin_unlock(&clp->cl_lock); > + return new; > + } > spin_unlock(&clp->cl_lock); > - return ret; > + > + if (oo && !(oo->oo_flags & NFS4_OO_CONFIRMED)) { > + /* Replace unconfirmed owners without checking for replay. */ > + release_openowner(oo); > + oo = NULL; > + } > + if (oo) { > + if (new) > + nfs4_free_stateowner(&new->oo_owner); > + return oo; > + } > + > + 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); > + goto retry; > } > > static struct nfs4_ol_stateid * > @@ -5331,27 +5331,14 @@ 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; > - } > + if (!oo) > + return nfserr_jukebox; > 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; > -alloc_stateid: > + > open->op_stp = nfs4_alloc_open_stateid(clp); > if (!open->op_stp) > return nfserr_jukebox; Bonus: this makes the spinlocking in nfsd4_process_open1 simpler too. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Mon, Mar 11, 2024 at 09:17:57AM +1100, NeilBrown wrote: > > Currently find_openstateowner_str lookups 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> > --- > > The previous version of this patch could deadlock on ->cl_lcok as it > called release_openowner() while holding that lock. > This version doesn't. > > Subsequent patch in the original series should be unchanged. If needed > I can resent them all after -rc1 is out. I have, from March 5, 2024: 1/4: nfsd: move nfsd4_cstate_assign_replay() earlier in open handling. 2/4: nfsd: perform all find_openstateowner_str calls in the one place. 3/4: nfsd: replace rp_mutex to avoid deadlock in move_to_close_lru() 4/4: nfsd: drop st_mutex_mutex before calling move_to_close_lru() Note that "nfsd: perform all find_open..." is 2/4, not 1/4. It applies cleanly to v6.9-rc2, but after it has been applied, the others do not apply cleanly. If I apply "nfsd: move nfsd4_cstate..." first, then "nfsd: perform all find_open..." does not apply cleanly. Please rebase this series on v6.9-rc2, test it, and re-post the full series as "v4". Thanks! > 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 ee9aa4843443..3e144b1a1386 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -541,7 +541,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; > @@ -558,18 +558,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) > { > @@ -4855,34 +4843,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); > +retry: > 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); > - > + oo = find_openstateowner_str(strhashval, open, clp); > + if (!oo && new) { > + hash_openowner(new, clp, strhashval); > + spin_unlock(&clp->cl_lock); > + return new; > + } > spin_unlock(&clp->cl_lock); > - return ret; > + > + if (oo && !(oo->oo_flags & NFS4_OO_CONFIRMED)) { > + /* Replace unconfirmed owners without checking for replay. */ > + release_openowner(oo); > + oo = NULL; > + } > + if (oo) { > + if (new) > + nfs4_free_stateowner(&new->oo_owner); > + return oo; > + } > + > + 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); > + goto retry; > } > > static struct nfs4_ol_stateid * > @@ -5331,27 +5331,14 @@ 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; > - } > + if (!oo) > + return nfserr_jukebox; > 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; > -alloc_stateid: > + > open->op_stp = nfs4_alloc_open_stateid(clp); > if (!open->op_stp) > return nfserr_jukebox; > -- > 2.43.0 >
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index ee9aa4843443..3e144b1a1386 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -541,7 +541,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; @@ -558,18 +558,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) { @@ -4855,34 +4843,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); +retry: 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); - + oo = find_openstateowner_str(strhashval, open, clp); + if (!oo && new) { + hash_openowner(new, clp, strhashval); + spin_unlock(&clp->cl_lock); + return new; + } spin_unlock(&clp->cl_lock); - return ret; + + if (oo && !(oo->oo_flags & NFS4_OO_CONFIRMED)) { + /* Replace unconfirmed owners without checking for replay. */ + release_openowner(oo); + oo = NULL; + } + if (oo) { + if (new) + nfs4_free_stateowner(&new->oo_owner); + return oo; + } + + 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); + goto retry; } static struct nfs4_ol_stateid * @@ -5331,27 +5331,14 @@ 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; - } + if (!oo) + return nfserr_jukebox; 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; -alloc_stateid: + open->op_stp = nfs4_alloc_open_stateid(clp); if (!open->op_stp) return nfserr_jukebox;
Currently find_openstateowner_str lookups 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> --- The previous version of this patch could deadlock on ->cl_lcok as it called release_openowner() while holding that lock. This version doesn't. Subsequent patch in the original series should be unchanged. If needed I can resent them all after -rc1 is out. fs/nfsd/nfs4state.c | 93 +++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 53 deletions(-)