Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4556 bytes --]

> 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 to help
> > > > > > > the user to maintain backward compatibility.
> > > > > > 
> > > > > > 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 for a good
> > > > > > user experience.
> > > > > 
> > > > > There is no UX consideration here. A user browsing the file directly
> > > > > will not care about the version.
> > > > > 
> > > > > 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.
> > > > > 
> > > > > This is what we typically get with a made-up format that isn't .ini
> > > > > 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.
> > > > > 
> > > > 
> > > > It shouldn't be a variable number of tokens per line.
> > > 
> > > That's how NFSv4 COMPOUND operations are displayed. For example:
> > > 
> > > 0x5d58666f 0x000000d1 0x000186a3 NFSv4 COMPOUND 0000062034739371 192.168.103.67 0 192.168.103.56 20049 OP_SEQUENCE OP_PUTFH OP_READ
> > > 
> > > The list of operations in the displayed compound are currently
> > > blank-separated tokens at the end of each row.
> > > 
> > 
> > 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?
> > 
> > IMO, the list of operations should be one field, separated by a distinct
> > delimiter (like ':').
> > 
> > > 
> > > > 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.
> > > 
> > > They could be wrapped in curly braces, or separated by commas, to
> > > make them all one token.
> > > 
> > > I haven't looked at NFSv3 output yet, but I expect those extra
> > > tokens won't even be there in that case.
> > > 
> > 
> > That's probably another bug. Anything not a v4 COMPOUND should have
> > something as a placeholder. It could just be a single '-' character.
> 
> Confirmed, rows reporting NFSv3 procedures have nothing on the end.
> 
> 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?
> 
> 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 != NFS_PROGRAM.
> 
> 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.

> 
> 
> > > JSON, yaml, or xml would all address the extensibility problem, just
> > > as an alternative thought.
> > > 
> > 
> > It would probably be fairly simple to output well-formed yaml instead.
> > JSON and XML are a bit more of a pain.
> > 
> > For now, we can change the output. We do need to have this settled
> > before this goes to Linus' tree though.
> 
> 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 COMPOUND)?
  The yaml approach downside is we will need to add some specific code since 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 info.

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

Regards,
Lorenzo

> 
> 
> -- 
> Chuck Lever

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]