Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
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 1/9] nfsd: hold ->cl_lock for hash_delegation_locked()
In-reply-to: <ZVqciNeBnfAaHZ6a@tissot.1015granger.net>
References: <20231117022121.23310-1-neilb@suse.de>,
 <20231117022121.23310-2-neilb@suse.de>,
 <40e1bf417c635ce303f9e42ddb0e3dbd90022477.camel@kernel.org>,
 <170042987584.19300.7721851585544522693@noble.neil.brown.name>,
 <ZVqciNeBnfAaHZ6a@tissot.1015granger.net>
Date:   Mon, 20 Nov 2023 10:41:52 +1100
Message-id: <170043731218.19300.6947617887297970977@noble.neil.brown.name>
Precedence: bulk

On Mon, 20 Nov 2023, Chuck Lever wrote:
> On Mon, Nov 20, 2023 at 08:37:55AM +1100, NeilBrown wrote:
> > On Fri, 17 Nov 2023, Jeff Layton wrote:
> > > On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > > > The protocol for creating a new state in nfsd is to allocated the sta=
te
> > > > leaving it largely uninitialised, add that state to the ->cl_stateids
> > > > idr so as to reserve a state id, then complete initialisation of the
> > > > state and only set ->sc_type to non-zero once the state is fully
> > > > initialised.
> > > >=20
> > > > If a state is found in the idr with ->sc_type =3D=3D 0, it is ignored.
> > > > The ->cl_lock list is used to avoid races - it is held while checking
> > > > sc_type during lookup, and held when a non-zero value is stored in
> > > > ->sc_type.
> > > >=20
> > > > ... except... hash_delegation_locked() finalises the initialisation o=
f a
> > > > delegation state, but does NOT hold ->cl_lock.
> > > >=20
> > > > So this patch takes ->cl_lock at the appropriate time w.r.t other loc=
ks,
> > > > and so ensures there are no races (which are extremely unlikely in any
> > > > case).
> > > >=20
> > > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > > ---
> > > >  fs/nfsd/nfs4state.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >=20
> > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > index 65fd5510323a..6368788a7d4e 100644
> > > > --- a/fs/nfsd/nfs4state.c
> > > > +++ b/fs/nfsd/nfs4state.c
> > > > @@ -1317,6 +1317,7 @@ hash_delegation_locked(struct nfs4_delegation *=
dp, struct nfs4_file *fp)
> > > > =20
> > > >  	lockdep_assert_held(&state_lock);
> > > >  	lockdep_assert_held(&fp->fi_lock);
> > > > +	lockdep_assert_held(&clp->cl_lock);
> > > > =20
> > > >  	if (nfs4_delegation_exists(clp, fp))
> > > >  		return -EAGAIN;
> > > > @@ -5609,12 +5610,14 @@ nfs4_set_delegation(struct nfsd4_open *open, =
struct nfs4_ol_stateid *stp,
> > > >  		goto out_unlock;
> > > > =20
> > > >  	spin_lock(&state_lock);
> > > > +	spin_lock(&clp->cl_lock);
> > > >  	spin_lock(&fp->fi_lock);
> > > >  	if (fp->fi_had_conflict)
> > > >  		status =3D -EAGAIN;
> > > >  	else
> > > >  		status =3D hash_delegation_locked(dp, fp);
> > > >  	spin_unlock(&fp->fi_lock);
> > > > +	spin_unlock(&clp->cl_lock);
> > > >  	spin_unlock(&state_lock);
> > > > =20
> > > >  	if (status)
> > >=20
> > > I know it's (supposedly) an unlikely race, but should we send this to
> > > stable?
> >=20
> > I don't know.  Once upon a time, "stable" meant something.  There was a
> > clear list of rules.  Those seem to have been torn up.  Now it seems to
> > be whatever some machine-learning tool chooses.
> > If that tool chooses this patch (which I suspect it will), I won't
> > object.  But I don't think it is worth encouraging it.
>=20
> We've asked Sasha and GregKH not to use AUTOSEL on NFSD patches,
> promising that we will explicitly mark anything that should be
> back-ported.

Oh good - I didn't know that.
Thanks.

NeilBrown