Received-SPF: pass (google.com: domain of linux-nfs+bounces-2251-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
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 1/4  v3] nfsd: perform all find_openstateowner_str calls in
 the one place.
Date: Mon, 11 Mar 2024 09:17:57 +1100
Message-id: <171010907730.13576.5751778995820664441@noble.neil.brown.name>


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_ne=
tobj *owner)
 }
=20
 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, str=
uct nfsd4_open *open,
 	return NULL;
 }
=20
-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 =3D 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 *f=
p, struct nfsd4_open *open)
 }
=20
 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 *op=
en,
+			      struct nfsd4_compound_state *cstate)
 {
 	struct nfs4_client *clp =3D cstate->clp;
-	struct nfs4_openowner *oo, *ret;
+	struct nfs4_openowner *oo, *new =3D NULL;
=20
-	oo =3D alloc_stateowner(openowner_slab, &open->op_owner, clp);
-	if (!oo)
-		return NULL;
-	oo->oo_owner.so_ops =3D &openowner_ops;
-	oo->oo_owner.so_is_open_owner =3D 1;
-	oo->oo_owner.so_seqid =3D open->op_seqid;
-	oo->oo_flags =3D 0;
-	if (nfsd4_has_session(cstate))
-		oo->oo_flags |=3D NFS4_OO_CONFIRMED;
-	oo->oo_time =3D 0;
-	oo->oo_last_closed_stid =3D NULL;
-	INIT_LIST_HEAD(&oo->oo_close_lru);
+retry:
 	spin_lock(&clp->cl_lock);
-	ret =3D find_openstateowner_str_locked(strhashval, open, clp);
-	if (ret =3D=3D NULL) {
-		hash_openowner(oo, clp, strhashval);
-		ret =3D oo;
-	} else
-		nfs4_free_stateowner(&oo->oo_owner);
-
+	oo =3D 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 =3D NULL;
+	}
+	if (oo) {
+		if (new)
+			nfs4_free_stateowner(&new->oo_owner);
+		return oo;
+	}
+
+	new =3D alloc_stateowner(openowner_slab, &open->op_owner, clp);
+	if (!new)
+		return NULL;
+	new->oo_owner.so_ops =3D &openowner_ops;
+	new->oo_owner.so_is_open_owner =3D 1;
+	new->oo_owner.so_seqid =3D open->op_seqid;
+	new->oo_flags =3D 0;
+	if (nfsd4_has_session(cstate))
+		new->oo_flags |=3D NFS4_OO_CONFIRMED;
+	new->oo_time =3D 0;
+	new->oo_last_closed_stid =3D NULL;
+	INIT_LIST_HEAD(&new->oo_close_lru);
+	goto retry;
 }
=20
 static struct nfs4_ol_stateid *
@@ -5331,27 +5331,14 @@ nfsd4_process_open1(struct nfsd4_compound_state *csta=
te,
 	clp =3D cstate->clp;
=20
 	strhashval =3D ownerstr_hashval(&open->op_owner);
-	oo =3D find_openstateowner_str(strhashval, open, clp);
+	oo =3D find_or_alloc_open_stateowner(strhashval, open, cstate);
 	open->op_openowner =3D 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 =3D NULL;
-		goto new_owner;
-	}
+	if (!oo)
+		return nfserr_jukebox;
 	status =3D nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
 	if (status)
 		return status;
-	goto alloc_stateid;
-new_owner:
-	oo =3D alloc_init_open_stateowner(strhashval, open, cstate);
-	if (oo =3D=3D NULL)
-		return nfserr_jukebox;
-	open->op_openowner =3D oo;
-alloc_stateid:
+
 	open->op_stp =3D nfs4_alloc_open_stateid(clp);
 	if (!open->op_stp)
 		return nfserr_jukebox;
--=20
2.43.0