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
