Received-SPF: pass (google.com: domain of linux-nfs+bounces-2250-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
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>
Cc: "Jeff Layton" <jlayton@kernel.org>, linux-nfs@vger.kernel.org,
 "Olga Kornievskaia" <kolga@netapp.com>, "Dai Ngo" <Dai.Ngo@oracle.com>,
 "Tom Talpey" <tom@talpey.com>
Subject: Re: [PATCH 2/4] nfsd: perform all find_openstateowner_str calls in
 the one place.
In-reply-to: <Zeyw5c3WO-OjinBE@manet.1015granger.net>
References: <20240304224714.10370-1-neilb@suse.de>,
 <20240304224714.10370-3-neilb@suse.de>,
 <Zeyw5c3WO-OjinBE@manet.1015granger.net>
Date: Mon, 11 Mar 2024 09:13:33 +1100
Message-id: <171010881340.13576.4048275134557370137@noble.neil.brown.name>

On Sun, 10 Mar 2024, Chuck Lever wrote:
> On Tue, Mar 05, 2024 at 09:45:22AM +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.
> >=20
> > It would be easier to follow, and more conformant to common patterns, if
> > the lookup was all in the one place.
> >=20
> > 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().
> >=20
> > 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().
> >=20
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/nfsd/nfs4state.c | 93 +++++++++++++++++++--------------------------
> >  1 file changed, 40 insertions(+), 53 deletions(-)
> >=20
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index b9b3a2601675..0c1058e8cc4b 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -541,7 +541,7 @@ same_owner_str(struct nfs4_stateowner *sop, struct xd=
r_netobj *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,=
 struct 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_fil=
e *fp, struct nfsd4_open *open)
> >  }
> > =20
> >  static struct nfs4_openowner *
> > -alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *o=
pen,
> > -			   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 =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);
> > -	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);
> > +	while (1) {
> > +		spin_lock(&clp->cl_lock);
> > +		oo =3D 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 =3D 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);
> > =20
> > -	spin_unlock(&clp->cl_lock);
> > -	return ret;
> > +		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);
> > +	}
> >  }
> > =20
> >  static struct nfs4_ol_stateid *
> > @@ -5331,28 +5331,15 @@ nfsd4_process_open1(struct nfsd4_compound_state *=
cstate,
> >  	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;
> > -	}
> > +		return nfserr_jukebox;
> >  	nfsd4_cstate_assign_replay(cstate, &oo->oo_owner);
> >  	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;
> > -	nfsd4_cstate_assign_replay(cstate, &oo->oo_owner);
> > -alloc_stateid:
> > +
> >  	open->op_stp =3D nfs4_alloc_open_stateid(clp);
> >  	if (!open->op_stp)
> >  		return nfserr_jukebox;
> > --=20
> > 2.43.0
> >=20
>=20
> While running the NFSv4.0 pynfs tests with KASAN and lock debugging
> enabled, I get a number of CPU soft lockup warnings on the test
> server and then it wedges hard. I bisected to this patch.

Almost certainly this is the problem Dan Carpenter found and reported.

>=20
> Because we are just a day shy of the v6.9 merge window, I'm going to
> drop these four patches for v6.9. We can try them again for v6.10.

I'll post a revised version of this patch - it shouldn't affect the
subsequent patches (though I can resend them to if/when you want).

Up to you when it lands of course.

NeilBrown