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 <[email protected]>
---
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;
--
2.43.0
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 <[email protected]>
> ---
>
> 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 <[email protected]>
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 <[email protected]>
> ---
>
> 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
>
--
Chuck Lever