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.
> > 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.
--
Chuck Lever
> 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
On Wed, Aug 09, 2023 at 09:49:55AM +0200, Lorenzo Bianconi wrote:
> > 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?
Would you mind spinning a series with the simple delimiter changes
and the other things we've discussed so far? It seems we have some
items that still need sorting before tackling netlink v. sysfs.
> - 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.
The trend in network subsystems is to use netlink and a purpose-
built tool, no "cat" support. The trouble with "cat" is we can't
seem to decide on where to put the output file.
Also, I notice that rq_flags appears for each in-flight request in
raw hexadecimal form. That's not especially user-friendly and cries
out for a tool to interpret the bits in that field. (Actually IIRC
there is now a tool that can take a yaml-defined netlink protocol
and perform each of the protocol's operations and spit out the
raw results).
I'm wondering if having support for "cat" is really just an old
habit we need to discard.
Re: netlink... folks should keep in mind that the output would not
be yaml. netlink uses yaml to define the netlink protocol, which is
quite like SunRPC. This still meets our extensibility requirements,
IMO.
Creating a netlink protocol would provide a vehicle for exporting
other information. Once there is an NFSD-specific netlink protocol,
it's straightforward to define an additional netlink procedure for
any of the information that are now available in /proc/fs/nfsd/
files.
> I will work on v6 as soon as we have agreed on the points above.
Thanks, I appreciate it!
--
Chuck Lever