All of lore.kernel.org
 help / color / mirror / Atom feed
From: NeilBrown <neilb@suse.de>
To: Chuck Lever <chuck.lever@oracle.com>, Jeff Layton <jlayton@kernel.org>
Cc: linux-nfs@vger.kernel.org, Olga Kornievskaia <kolga@netapp.com>,
	Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>
Subject: [PATCH 2/4] nfsd: perform all find_openstateowner_str calls in the one place.
Date: Mon,  4 Mar 2024 15:40:20 +1100	[thread overview]
Message-ID: <20240304044304.3657-3-neilb@suse.de> (raw)
In-Reply-To: <20240304044304.3657-1-neilb@suse.de>

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);
+	}
 }
 
 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;
-- 
2.43.0


  parent reply	other threads:[~2024-03-04  4:43 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-04  4:40 [PATCH 0/4 v2] nfsd: fix dadlock in move_to_close_lru() NeilBrown
2024-03-04  4:40 ` [PATCH 1/4] nfsd: move nfsd4_cstate_assign_replay() earlier in open handling NeilBrown
2024-03-04 12:42   ` Jeff Layton
2024-03-04  4:40 ` NeilBrown [this message]
2024-03-04 12:46   ` [PATCH 2/4] nfsd: perform all find_openstateowner_str calls in the one place Jeff Layton
2024-03-04 21:41     ` NeilBrown
2024-03-04 23:04       ` Jeff Layton
2024-03-04  4:40 ` [PATCH 3/4] nfsd: replace rp_mutex to avoid deadlock in move_to_close_lru() NeilBrown
2024-03-04 12:50   ` Jeff Layton
2024-03-04 14:09   ` Chuck Lever
2024-03-04 21:45     ` NeilBrown
2024-03-04 22:03       ` Chuck Lever
2024-03-04 22:36         ` NeilBrown
2024-03-04 22:54           ` Chuck Lever
2024-03-04 23:04             ` NeilBrown
2024-03-04 23:11             ` Jeff Layton
2024-03-04 23:30               ` Chuck Lever
2024-03-05  0:05                 ` NeilBrown
2024-03-05  0:11                 ` Jeff Layton
2024-03-04  4:40 ` [PATCH 4/4] nfsd: drop st_mutex_mutex before calling move_to_close_lru() NeilBrown
2024-03-04 12:52   ` Jeff Layton
2024-03-04 22:45 [PATCH 0/4 v3] nfsd: fix deadlock in move_to_close_lru() NeilBrown
2024-03-04 22:45 ` [PATCH 2/4] nfsd: perform all find_openstateowner_str calls in the one place NeilBrown
2024-03-09 18:56   ` Chuck Lever
2024-03-10 22:13     ` NeilBrown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240304044304.3657-3-neilb@suse.de \
    --to=neilb@suse.de \
    --cc=Dai.Ngo@oracle.com \
    --cc=chuck.lever@oracle.com \
    --cc=jlayton@kernel.org \
    --cc=kolga@netapp.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=tom@talpey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.