Received-SPF: pass (google.com: domain of linux-nfs+bounces-2149-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: "Jeff Layton" <jlayton@kernel.org>
Cc: "Chuck Lever" <chuck.lever@oracle.com>, 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/3] nfsd: move nfsd4_cstate_assign_replay() earlier in
 open handling.
In-reply-to: <b2f720cb8993fbf0072a3d02434214c5f3b613fa.camel@kernel.org>
References: <20240301000950.2306-1-neilb@suse.de>,
 <20240301000950.2306-2-neilb@suse.de>,
 <b2f720cb8993fbf0072a3d02434214c5f3b613fa.camel@kernel.org>
Date: Mon, 04 Mar 2024 10:59:53 +1100
Message-id: <170951039351.24797.11983130591432693683@noble.neil.brown.name>

On Fri, 01 Mar 2024, Jeff Layton wrote:
> On Fri, 2024-03-01 at 11:07 +1100, NeilBrown wrote:
> > Rather than taking the rp_mutex in nfsd4_cleanup_open_state() (which
> > seems counter-intuitive), take it and assign rp_owner as soon as
> > possible.
> >=20
> > This will support a future change when nfsd4_cstate_assign_replay() might
> > fail.
> >=20
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/nfsd/nfs4state.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> >=20
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 7d6c657e0409..e625f738f7b0 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5066,15 +5066,15 @@ nfsd4_process_open1(struct nfsd4_compound_state *=
cstate,
> >  	strhashval =3D ownerstr_hashval(&open->op_owner);
> >  	oo =3D find_openstateowner_str(strhashval, open, clp);
> >  	open->op_openowner =3D oo;
> > -	if (!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;
> >  	}
> > +	nfsd4_cstate_assign_replay(cstate, &oo->oo_owner);
> >  	status =3D nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
> >  	if (status)
> >  		return status;
> > @@ -5084,6 +5084,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cs=
tate,
> >  	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)
> > @@ -5835,12 +5836,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct=
 svc_fh *current_fh, struct nf
> >  void nfsd4_cleanup_open_state(struct nfsd4_compound_state *cstate,
> >  			      struct nfsd4_open *open)
> >  {
> > -	if (open->op_openowner) {
> > -		struct nfs4_stateowner *so =3D &open->op_openowner->oo_owner;
> > -
> > -		nfsd4_cstate_assign_replay(cstate, so);
> > -		nfs4_put_stateowner(so);
> > -	}
> > +	if (cstate->replay_owner)
> > +		nfs4_put_stateowner(cstate->replay_owner);
>=20
> The above delta doesn't look right. The replay_owner won't be set on
> v4.1+ mounts, but op_openowner will still hold a valid reference that
> will now leak.

Yes, of course.  I was over-thinking and making a mess of it.

Fixed,
thanks.

NeilBrown

>=20
> >  	if (open->op_file)
> >  		kmem_cache_free(file_slab, open->op_file);
> >  	if (open->op_stp)
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>
>=20
>=20