Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 9 Aug 2023 09:49:55 +0200
From:   Lorenzo Bianconi <lorenzo@kernel.org>
To:     Chuck Lever <chuck.lever@oracle.com>
Cc:     Jeff Layton <jlayton@kernel.org>, NeilBrown <neilb@suse.de>,
        linux-nfs@vger.kernel.org, lorenzo.bianconi@redhat.com
Subject: Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler
Message-ID: <ZNNFI7baJW+XGTJS@lore-rh-laptop>
References: <6431d0ea2295a1e128f83cd76a419dee161e4c44.1691482815.git.lorenzo@kernel.org>
 <169149440399.32308.1010201101079709026@noble.neil.brown.name>
 <ZNJCIRjI64YIY+I0@tissot.1015granger.net>
 <ea598236b2da9f1aa9b587ca797afaa9de5545c7.camel@kernel.org>
 <ZNJLQIxweTaEsu16@tissot.1015granger.net>
 <ed02b06f96eeeca4d499583f2bdf31a433921aa1.camel@kernel.org>
 <ZNKBvgAMnOsDiaKQ@tissot.1015granger.net>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="7ZJEOpuJxZ9Bix7h"
Content-Disposition: inline
In-Reply-To: <ZNKBvgAMnOsDiaKQ@tissot.1015granger.net>
Precedence: bulk


--7ZJEOpuJxZ9Bix7h
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

> On Tue, Aug 08, 2023 at 10:20:44AM -0400, Jeff Layton wrote:
> > On Tue, 2023-08-08 at 10:03 -0400, Chuck Lever wrote:
> > > On Tue, Aug 08, 2023 at 09:48:42AM -0400, Jeff Layton wrote:
> > > > On Tue, 2023-08-08 at 09:24 -0400, Chuck Lever wrote:
> > > > > On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > > > > > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > > > > > Introduce version field to nfsd_rpc_status handler in order t=
o help
> > > > > > > the user to maintain backward compatibility.
> > > > > >=20
> > > > > > I wonder if this really helps.  What do I do if I see a version=
 that I
> > > > > > don't understand?  Ignore the whole file?  That doesn't make fo=
r a good
> > > > > > user experience.
> > > > >=20
> > > > > There is no UX consideration here. A user browsing the file direc=
tly
> > > > > will not care about the version.
> > > > >=20
> > > > > This file is intended to be parsable by scripts and they have to
> > > > > keep up with the occasional changes in format. Scripts can handle=
 an
> > > > > unrecogized version however they like.
> > > > >=20
> > > > > This is what we typically get with a made-up format that isn't .i=
ni
> > > > > or JSON or XML. The file format isn't self-documenting. The final
> > > > > field on each row is a variable number of tokens, so it will be
> > > > > nearly impossible to simply add another field without breaking
> > > > > something.
> > > > >=20
> > > >=20
> > > > It shouldn't be a variable number of tokens per line.
> > >=20
> > > That's how NFSv4 COMPOUND operations are displayed. For example:
> > >=20
> > > 0x5d58666f 0x000000d1 0x000186a3 NFSv4 COMPOUND 0000062034739371 192.=
168.103.67 0 192.168.103.56 20049 OP_SEQUENCE OP_PUTFH OP_READ
> > >=20
> > > The list of operations in the displayed compound are currently
> > > blank-separated tokens at the end of each row.
> > >=20
> >=20
> > Oh! That's a bug in missed in my latest review then. The operations
> > field was delimited by ':' chars at one point. Lorenzo, did you mean to
> > change that?
> >=20
> > IMO, the list of operations should be one field, separated by a distinct
> > delimiter (like ':').
> >=20
> > >=20
> > > > If there is, then that's a bug, IMO. We do want it to be simple to
> > > > just add a new field, published version info notwithstanding.
> > >=20
> > > They could be wrapped in curly braces, or separated by commas, to
> > > make them all one token.
> > >=20
> > > I haven't looked at NFSv3 output yet, but I expect those extra
> > > tokens won't even be there in that case.
> > >=20
> >=20
> > That's probably another bug. Anything not a v4 COMPOUND should have
> > something as a placeholder. It could just be a single '-' character.
>=20
> Confirmed, rows reporting NFSv3 procedures have nothing on the end.
>=20
> I'll also note that rq_prog and the "NFSv" string are problematic.
> Is it the case that all RPCs handled in this thread pool are going
> to be NFS requests?
>=20
> If we expect non-NFS requests to be handled in this thread pool
> (like svc_wake_up or NFSACL) then the loop should simply skip
> threads whose rq_prog !=3D NFS_PROGRAM.
>=20
> And, if the rpc_status file is supposed to display only NFS
> requests (and I believe the answer to that is yes), then let's drop
> the rq_prog field, since it will always show the same value.

ack, I will fix it.

>=20
>=20
> > > JSON, yaml, or xml would all address the extensibility problem, just
> > > as an alternative thought.
> > >=20
> >=20
> > It would probably be fairly simple to output well-formed yaml instead.
> > JSON and XML are a bit more of a pain.
> >=20
> > For now, we can change the output. We do need to have this settled
> > before this goes to Linus' tree though.
>=20
> Lorenzo, I'll drop the v5 of this series from nfsd-next. When you're
> ready, please send another version with the discussed changes
> squashed in.

ack, fine to me. Just a couple of questions:
- do we want to expose the output in yaml or is it enough to fix the NFSv4 =
COMPOUND
  parsing using ":" as sub-delimiter (and add a placeholder for non NFSv4 C=
OMPOUND)?
  The yaml approach downside is we will need to add some specific code sinc=
e afaik
  there isn't any yaml code we can rely on in the kernel, right?
- what about netlink? I would say we can have both of them (cat + netlink) =
so
  the user does not need to have a specific userspace tool to decode the in=
fo.

I will work on v6 as soon as we have agreed on the points above.

Regards,
Lorenzo

>=20
>=20
> --=20
> Chuck Lever

--7ZJEOpuJxZ9Bix7h
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCZNNFHwAKCRA6cBh0uS2t
rP4PAP9l+fqXJFmsz6PUXGpnmR/yruRVJCuOseEH4E2uAm000QEAxbE1NKvSjSNR
CggdvrMpAB8PZTlhYYdS1Jyg58lDYgk=
=NFPd
-----END PGP SIGNATURE-----

--7ZJEOpuJxZ9Bix7h--