Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
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 5/9] nfsd: allow admin-revoked state to appear in
 /proc/fs/nfsd/clients/*/states
In-reply-to: <ced58cba1234c662ddf7cfeaef5ce4b1a702dcfa.camel@kernel.org>
References: <20231117022121.23310-1-neilb@suse.de>,
 <20231117022121.23310-6-neilb@suse.de>,
 <ced58cba1234c662ddf7cfeaef5ce4b1a702dcfa.camel@kernel.org>
Date:   Mon, 20 Nov 2023 09:36:45 +1100
Message-id: <170043340577.19300.13599388686680253695@noble.neil.brown.name>
Precedence: bulk

On Fri, 17 Nov 2023, Jeff Layton wrote:
> On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > Change the "show" functions to show some content even if a file cannot
> > be found.
> > This is primarily useful for debugging - to ensure states are being
> > removed eventually.
> >=20
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/nfsd/nfs4state.c | 81 +++++++++++++++++++++++----------------------
> >  1 file changed, 41 insertions(+), 40 deletions(-)
> >=20
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index a2b3320a6ba8..8debd148840f 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -2680,17 +2680,10 @@ static int nfs4_show_open(struct seq_file *s, str=
uct nfs4_stid *st)
> >  	struct nfs4_stateowner *oo;
> >  	unsigned int access, deny;
> > =20
> > -	if (st->sc_type !=3D NFS4_OPEN_STID && st->sc_type !=3D NFS4_LOCK_STID)
> > -		return 0; /* XXX: or SEQ_SKIP? */
> >  	ols =3D openlockstateid(st);
> >  	oo =3D ols->st_stateowner;
> >  	nf =3D st->sc_file;
> > =20
> > -	spin_lock(&nf->fi_lock);
> > -	file =3D find_any_file_locked(nf);
> > -	if (!file)
> > -		goto out;
> > -
> >  	seq_printf(s, "- ");
> >  	nfs4_show_stateid(s, &st->sc_stateid);
> >  	seq_printf(s, ": { type: open, ");
> > @@ -2705,14 +2698,19 @@ static int nfs4_show_open(struct seq_file *s, str=
uct nfs4_stid *st)
> >  		deny & NFS4_SHARE_ACCESS_READ ? "r" : "-",
> >  		deny & NFS4_SHARE_ACCESS_WRITE ? "w" : "-");
> > =20
> > -	nfs4_show_superblock(s, file);
> > -	seq_printf(s, ", ");
> > -	nfs4_show_fname(s, file);
> > -	seq_printf(s, ", ");
> > +	spin_lock(&nf->fi_lock);
> > +	file =3D find_any_file_locked(nf);
> > +	if (file) {
> > +		nfs4_show_superblock(s, file);
> > +		seq_puts(s, ", ");
> > +		nfs4_show_fname(s, file);
> > +		seq_puts(s, ", ");
> > +	}
> > +	spin_unlock(&nf->fi_lock);
> >  	nfs4_show_owner(s, oo);
> > +	if (st->sc_status & NFS4_STID_ADMIN_REVOKED)
> > +		seq_puts(s, ", admin-revoked");
> >  	seq_printf(s, " }\n");
> > -out:
> > -	spin_unlock(&nf->fi_lock);
> >  	return 0;
> >  }
> > =20
> > @@ -2726,30 +2724,31 @@ static int nfs4_show_lock(struct seq_file *s, str=
uct nfs4_stid *st)
> >  	ols =3D openlockstateid(st);
> >  	oo =3D ols->st_stateowner;
> >  	nf =3D st->sc_file;
> > -	spin_lock(&nf->fi_lock);
> > -	file =3D find_any_file_locked(nf);
> > -	if (!file)
> > -		goto out;
> > =20
> >  	seq_printf(s, "- ");
> >  	nfs4_show_stateid(s, &st->sc_stateid);
> >  	seq_printf(s, ": { type: lock, ");
> > =20
> > -	/*
> > -	 * Note: a lock stateid isn't really the same thing as a lock,
> > -	 * it's the locking state held by one owner on a file, and there
> > -	 * may be multiple (or no) lock ranges associated with it.
> > -	 * (Same for the matter is true of open stateids.)
> > -	 */
> > +	spin_lock(&nf->fi_lock);
> > +	file =3D find_any_file_locked(nf);
> > +	if (file) {
> > +		/*
> > +		 * Note: a lock stateid isn't really the same thing as a lock,
> > +		 * it's the locking state held by one owner on a file, and there
> > +		 * may be multiple (or no) lock ranges associated with it.
> > +		 * (Same for the matter is true of open stateids.)
> > +		 */
> > =20
> > -	nfs4_show_superblock(s, file);
> > -	/* XXX: open stateid? */
> > -	seq_printf(s, ", ");
> > -	nfs4_show_fname(s, file);
> > -	seq_printf(s, ", ");
> > +		nfs4_show_superblock(s, file);
> > +		/* XXX: open stateid? */
> > +		seq_puts(s, ", ");
> > +		nfs4_show_fname(s, file);
> > +		seq_puts(s, ", ");
> > +	}
> >  	nfs4_show_owner(s, oo);
> > +	if (st->sc_status & NFS4_STID_ADMIN_REVOKED)
> > +		seq_puts(s, ", admin-revoked");
> >  	seq_printf(s, " }\n");
> > -out:
> >  	spin_unlock(&nf->fi_lock);
> >  	return 0;
> >  }
> > @@ -2762,27 +2761,29 @@ static int nfs4_show_deleg(struct seq_file *s, st=
ruct nfs4_stid *st)
> > =20
> >  	ds =3D delegstateid(st);
> >  	nf =3D st->sc_file;
> > -	spin_lock(&nf->fi_lock);
> > -	file =3D nf->fi_deleg_file;
> > -	if (!file)
> > -		goto out;
> > =20
> >  	seq_printf(s, "- ");
> >  	nfs4_show_stateid(s, &st->sc_stateid);
> >  	seq_printf(s, ": { type: deleg, ");
> > =20
> >  	/* Kinda dead code as long as we only support read delegs: */
>=20
> nit: You can probably remove the above comment since we now support
> write delegations.

Done.  Thanks for the suggestion.

NeilBrown


>=20
> > -	seq_printf(s, "access: %s, ",
> > -		ds->dl_type =3D=3D NFS4_OPEN_DELEGATE_READ ? "r" : "w");
> > +	seq_printf(s, "access: %s",
> > +		   ds->dl_type =3D=3D NFS4_OPEN_DELEGATE_READ ? "r" : "w");
> > =20
> >  	/* XXX: lease time, whether it's being recalled. */
> > =20
> > -	nfs4_show_superblock(s, file);
> > -	seq_printf(s, ", ");
> > -	nfs4_show_fname(s, file);
> > -	seq_printf(s, " }\n");
> > -out:
> > +	spin_lock(&nf->fi_lock);
> > +	file =3D nf->fi_deleg_file;
> > +	if (file) {
> > +		seq_puts(s, ", ");
> > +		nfs4_show_superblock(s, file);
> > +		seq_puts(s, ", ");
> > +		nfs4_show_fname(s, file);
> > +	}
> >  	spin_unlock(&nf->fi_lock);
> > +	if (st->sc_status & NFS4_STID_ADMIN_REVOKED)
> > +		seq_puts(s, ", admin-revoked");
> > +	seq_puts(s, " }\n");
> >  	return 0;
> >  }
> > =20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>
>=20