| Message ID | 47c144cfa1859ab089527e67c8540eb920427c64.1694436263.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | add rpc_status netlink support for NFSD | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, 2023-09-11 at 14:49 +0200, Lorenzo Bianconi wrote: > Introduce nfsd_server.yaml specs to generate uAPI and netlink > code for nfsd server. > Add rpc-status specs to define message reported by the nfsd server > dumping the pending RPC requests. > > Tested-by: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > Documentation/netlink/specs/nfsd_server.yaml | 97 ++++++++++++++++++++ > 1 file changed, 97 insertions(+) > create mode 100644 Documentation/netlink/specs/nfsd_server.yaml > > diff --git a/Documentation/netlink/specs/nfsd_server.yaml b/Documentation/netlink/specs/nfsd_server.yaml > new file mode 100644 > index 000000000000..e681b493847b > --- /dev/null > +++ b/Documentation/netlink/specs/nfsd_server.yaml > @@ -0,0 +1,97 @@ > +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) > + > +name: nfsd_server > + > +doc: > + nfsd server configuration over generic netlink. > + > +attribute-sets: > + - > + name: rpc-status-comp-op-attr > + enum-name: nfsd-rpc-status-comp-attr > + name-prefix: nfsd-attr-rpc-status-comp- > + attributes: > + - > + name: unspec > + type: unused > + value: 0 > + - > + name: op > + type: u32 > + - > + name: rpc-status-attr > + enum-name: nfsd-rpc-status-attr > + name-prefix: nfsd-attr-rpc-status- > + attributes: > + - > + name: unspec > + type: unused > + value: 0 > + - > + name: xid > + type: u32 > + byte-order: big-endian > + - > + name: flags > + type: u32 > + - > + name: prog > + type: u32 > + - > + name: version > + type: u8 > + - > + name: proc > + type: u32 > + - > + name: service_time > + type: s64 > + - > + name: pad > + type: pad > + - > + name: saddr4 > + type: u32 > + byte-order: big-endian > + display-hint: ipv4 > + - > + name: daddr4 > + type: u32 > + byte-order: big-endian > + display-hint: ipv4 > + - > + name: saddr6 > + type: binary > + display-hint: ipv6 > + - > + name: daddr6 > + type: binary > + display-hint: ipv6 > + - > + name: sport > + type: u16 > + byte-order: big-endian > + - > + name: dport > + type: u16 > + byte-order: big-endian > + - > + name: compond-op > + type: array-nest > + nested-attributes: rpc-status-comp-op-attr Is there a way to do unions in netlink? We're sending down the list of COMPOUND proc operations here for NFSv4, but it might be interesting to send down other info for other program/version/procedures in the future. > + > +operations: > + enum-name: nfsd-commands > + name-prefix: nfsd-cmd- > + list: > + - > + name: unspec > + doc: unused > + value: 0 > + - > + name: rpc-status-get > + doc: dump pending nfsd rpc > + attribute-set: rpc-status-attr > + dump: > + pre: nfsd-server-nl-rpc-status-get-start > + post: nfsd-server-nl-rpc-status-get-done Looks like a good first stab though. Acked-by: Jeff Layton <jlayton@kernel.org>
On Mon, Sep 11, 2023 at 02:49:44PM +0200, Lorenzo Bianconi wrote: > Introduce nfsd_server.yaml specs to generate uAPI and netlink > code for nfsd server. > Add rpc-status specs to define message reported by the nfsd server > dumping the pending RPC requests. > > Tested-by: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > Documentation/netlink/specs/nfsd_server.yaml | 97 ++++++++++++++++++++ > 1 file changed, 97 insertions(+) > create mode 100644 Documentation/netlink/specs/nfsd_server.yaml I've had a look... the series is simple and short. Thanks! My only quibbles right now are cosmetic and naming-related, all of which can be addressed when I apply these. So I'm going to wait for other review comments to see if we need another version or whether I can apply v8 with by-hand clean-ups. Comments below are what I might change when applying this one. This is not (yet) a request for a new version. > diff --git a/Documentation/netlink/specs/nfsd_server.yaml b/Documentation/netlink/specs/nfsd_server.yaml > new file mode 100644 > index 000000000000..e681b493847b > --- /dev/null > +++ b/Documentation/netlink/specs/nfsd_server.yaml > @@ -0,0 +1,97 @@ > +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) > + > +name: nfsd_server IMHO "nfsd_server" is redundant. "nfsd" should work. > + > +doc: > + nfsd server configuration over generic netlink. > + > +attribute-sets: > + - > + name: rpc-status-comp-op-attr > + enum-name: nfsd-rpc-status-comp-attr > + name-prefix: nfsd-attr-rpc-status-comp- > + attributes: > + - > + name: unspec > + type: unused > + value: 0 I don't recall whether a zero-value definition is explicitly necessary. Maybe "value-start: 1" would work rather than these three lines? Why is zero a special attribute value? > + - > + name: op > + type: u32 > + - > + name: rpc-status-attr > + enum-name: nfsd-rpc-status-attr > + name-prefix: nfsd-attr-rpc-status- Specifying all three of these name settings seems a bit cluttered. > + attributes: > + - > + name: unspec > + type: unused > + value: 0 > + - > + name: xid > + type: u32 > + byte-order: big-endian > + - > + name: flags > + type: u32 > + - > + name: prog > + type: u32 > + - > + name: version > + type: u8 > + - > + name: proc > + type: u32 > + - > + name: service_time > + type: s64 > + - > + name: pad > + type: pad > + - > + name: saddr4 > + type: u32 > + byte-order: big-endian > + display-hint: ipv4 > + - > + name: daddr4 > + type: u32 > + byte-order: big-endian > + display-hint: ipv4 > + - > + name: saddr6 > + type: binary > + display-hint: ipv6 > + - > + name: daddr6 > + type: binary > + display-hint: ipv6 > + - > + name: sport > + type: u16 > + byte-order: big-endian > + - > + name: dport > + type: u16 > + byte-order: big-endian > + - > + name: compond-op s/compond-op/compound-op > + type: array-nest > + nested-attributes: rpc-status-comp-op-attr So, this is supposed to be a counted array of op numbers? Is there an existing type that could be used for this instead? > + > +operations: > + enum-name: nfsd-commands > + name-prefix: nfsd-cmd- > + list: > + - > + name: unspec > + doc: unused > + value: 0 > + - > + name: rpc-status-get > + doc: dump pending nfsd rpc > + attribute-set: rpc-status-attr > + dump: > + pre: nfsd-server-nl-rpc-status-get-start > + post: nfsd-server-nl-rpc-status-get-done
> On Mon, Sep 11, 2023 at 02:49:44PM +0200, Lorenzo Bianconi wrote: > > Introduce nfsd_server.yaml specs to generate uAPI and netlink > > code for nfsd server. > > Add rpc-status specs to define message reported by the nfsd server > > dumping the pending RPC requests. > > > > Tested-by: Jeff Layton <jlayton@kernel.org> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > Documentation/netlink/specs/nfsd_server.yaml | 97 ++++++++++++++++++++ > > 1 file changed, 97 insertions(+) > > create mode 100644 Documentation/netlink/specs/nfsd_server.yaml > > I've had a look... the series is simple and short. Thanks! > > My only quibbles right now are cosmetic and naming-related, all > of which can be addressed when I apply these. So I'm going to > wait for other review comments to see if we need another version > or whether I can apply v8 with by-hand clean-ups. > > Comments below are what I might change when applying this one. > This is not (yet) a request for a new version. Hi Chuck, thx for the review. Please let me know if I need to post a v9. > > > > diff --git a/Documentation/netlink/specs/nfsd_server.yaml b/Documentation/netlink/specs/nfsd_server.yaml > > new file mode 100644 > > index 000000000000..e681b493847b > > --- /dev/null > > +++ b/Documentation/netlink/specs/nfsd_server.yaml > > @@ -0,0 +1,97 @@ > > +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) > > + > > +name: nfsd_server > > IMHO "nfsd_server" is redundant. "nfsd" should work. ack, fine > > > > + > > +doc: > > + nfsd server configuration over generic netlink. > > + > > +attribute-sets: > > + - > > + name: rpc-status-comp-op-attr > > + enum-name: nfsd-rpc-status-comp-attr > > + name-prefix: nfsd-attr-rpc-status-comp- > > + attributes: > > + - > > + name: unspec > > + type: unused > > + value: 0 > > I don't recall whether a zero-value definition is explicitly > necessary. Maybe "value-start: 1" would work rather than > these three lines? Why is zero a special attribute value? I do not think it is mandatory, I added them just to be aligned with other netlink definitions, but we do not use them. > > > > + - > > + name: op > > + type: u32 > > + - > > + name: rpc-status-attr > > + enum-name: nfsd-rpc-status-attr > > + name-prefix: nfsd-attr-rpc-status- > > Specifying all three of these name settings seems a bit > cluttered. ack, I would say we can get rid of enum-name, agree? > > > > + attributes: > > + - > > + name: unspec > > + type: unused > > + value: 0 > > + - > > + name: xid > > + type: u32 > > + byte-order: big-endian > > + - > > + name: flags > > + type: u32 > > + - > > + name: prog > > + type: u32 > > + - > > + name: version > > + type: u8 > > + - > > + name: proc > > + type: u32 > > + - > > + name: service_time > > + type: s64 > > + - > > + name: pad > > + type: pad > > + - > > + name: saddr4 > > + type: u32 > > + byte-order: big-endian > > + display-hint: ipv4 > > + - > > + name: daddr4 > > + type: u32 > > + byte-order: big-endian > > + display-hint: ipv4 > > + - > > + name: saddr6 > > + type: binary > > + display-hint: ipv6 > > + - > > + name: daddr6 > > + type: binary > > + display-hint: ipv6 > > + - > > + name: sport > > + type: u16 > > + byte-order: big-endian > > + - > > + name: dport > > + type: u16 > > + byte-order: big-endian > > + - > > + name: compond-op > > s/compond-op/compound-op ack > > > + type: array-nest > > + nested-attributes: rpc-status-comp-op-attr > > So, this is supposed to be a counted array of op numbers? Is there > an existing type that could be used for this instead? I think the attribute-sets available types are defined here: https://github.com/torvalds/linux/blob/master/Documentation/netlink/genetlink-c.yaml#L151 Regards, Lorenzo > > > > + > > +operations: > > + enum-name: nfsd-commands > > + name-prefix: nfsd-cmd- > > + list: > > + - > > + name: unspec > > + doc: unused > > + value: 0 > > + - > > + name: rpc-status-get > > + doc: dump pending nfsd rpc > > + attribute-set: rpc-status-attr > > + dump: > > + pre: nfsd-server-nl-rpc-status-get-start > > + post: nfsd-server-nl-rpc-status-get-done > > -- > Chuck Lever >
On Mon, 11 Sep 2023 14:49:44 +0200 Lorenzo Bianconi wrote: > Introduce nfsd_server.yaml specs to generate uAPI and netlink > code for nfsd server. > Add rpc-status specs to define message reported by the nfsd server > dumping the pending RPC requests. Sorry for the delay, some minor "take it or leave it" nits below. > +doc: > + nfsd server configuration over generic netlink. > + > +attribute-sets: > + - > + name: rpc-status-comp-op-attr > + enum-name: nfsd-rpc-status-comp-attr > + name-prefix: nfsd-attr-rpc-status-comp- > + attributes: > + - > + name: unspec > + type: unused > + value: 0 the unused attrs can usually be skipped, the specs now start with value of 1 by default. Same for the unused command. > + - > + name: dport > + type: u16 > + byte-order: big-endian > + - > + name: compond-op > + type: array-nest Avoid array-nests if you can, they are legacy (does this spec pass JSON schema validation?). There's only one attribute in the nest, can you use - name: op type: u32 multi-attr: true ? > + nested-attributes: rpc-status-comp-op-attr > + - > + name: rpc-status-get > + doc: dump pending nfsd rpc > + attribute-set: rpc-status-attr > + dump: > + pre: nfsd-server-nl-rpc-status-get-start > + post: nfsd-server-nl-rpc-status-get-done No attributes listed? User space C codegen will need those to make sense of the commands.
Hi Jakub- > On Oct 3, 2023, at 1:55 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 11 Sep 2023 14:49:44 +0200 Lorenzo Bianconi wrote: >> Introduce nfsd_server.yaml specs to generate uAPI and netlink >> code for nfsd server. >> Add rpc-status specs to define message reported by the nfsd server >> dumping the pending RPC requests. > > Sorry for the delay, some minor "take it or leave it" nits below. > >> +doc: >> + nfsd server configuration over generic netlink. >> + >> +attribute-sets: >> + - >> + name: rpc-status-comp-op-attr >> + enum-name: nfsd-rpc-status-comp-attr >> + name-prefix: nfsd-attr-rpc-status-comp- >> + attributes: >> + - >> + name: unspec >> + type: unused >> + value: 0 > > the unused attrs can usually be skipped, the specs now start with value > of 1 by default. Same for the unused command. > >> + - >> + name: dport >> + type: u16 >> + byte-order: big-endian >> + - >> + name: compond-op >> + type: array-nest > > Avoid array-nests if you can, they are legacy (does this spec pass JSON > schema validation?). > > There's only one attribute in the nest, can you use > > - > name: op > type: u32 > multi-attr: true > > ? I've made similar modifications to Lorenzo's original contribution. The current updated version of this spec is here: https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-next&id=55f55c77f624066895470306898190f090e00cda >> + nested-attributes: rpc-status-comp-op-attr > >> + - >> + name: rpc-status-get >> + doc: dump pending nfsd rpc >> + attribute-set: rpc-status-attr >> + dump: >> + pre: nfsd-server-nl-rpc-status-get-start >> + post: nfsd-server-nl-rpc-status-get-done > > No attributes listed? User space C codegen will need those to make > sense of the commands. -- Chuck Lever
On Tue, 3 Oct 2023 18:40:29 +0000 Chuck Lever III wrote: > I've made similar modifications to Lorenzo's original > contribution. The current updated version of this spec > is here: > > https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-next&id=55f55c77f624066895470306898190f090e00cda Great! I noticed too late :) Just the attr listing is missing in the spec, then.
> On Oct 3, 2023, at 3:02 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 3 Oct 2023 18:40:29 +0000 Chuck Lever III wrote: >> I've made similar modifications to Lorenzo's original >> contribution. The current updated version of this spec >> is here: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-next&id=55f55c77f624066895470306898190f090e00cda > > Great! I noticed too late :) > Just the attr listing is missing in the spec, then. To ensure that I understand you correctly: diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml index 403d3e3a04f3..f6a9f3da6291 100644 --- a/Documentation/netlink/specs/nfsd.yaml +++ b/Documentation/netlink/specs/nfsd.yaml @@ -72,3 +72,19 @@ operations: dump: pre: nfsd-nl-rpc-status-get-start post: nfsd-nl-rpc-status-get-done + reply: + attributes: + - xid + - flags + - prog + - version + - proc + - service_time + - pad + - saddr4 + - daddr4 + - saddr6 + - daddr6 + - sport + - dport + - compound-ops Yes? -- Chuck Lever
On Tue, 3 Oct 2023 23:00:09 +0000 Chuck Lever III wrote: > To ensure that I understand you correctly: > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml > index 403d3e3a04f3..f6a9f3da6291 100644 > --- a/Documentation/netlink/specs/nfsd.yaml > +++ b/Documentation/netlink/specs/nfsd.yaml > @@ -72,3 +72,19 @@ operations: > dump: > pre: nfsd-nl-rpc-status-get-start > post: nfsd-nl-rpc-status-get-done > + reply: > + attributes: > + - xid > + - flags > + - prog > + - version > + - proc > + - service_time > + - pad > + - saddr4 > + - daddr4 > + - saddr6 > + - daddr6 > + - sport > + - dport > + - compound-ops Yup! (the pad can be skipped since it's not rendered, anyway).
diff --git a/Documentation/netlink/specs/nfsd_server.yaml b/Documentation/netlink/specs/nfsd_server.yaml new file mode 100644 index 000000000000..e681b493847b --- /dev/null +++ b/Documentation/netlink/specs/nfsd_server.yaml @@ -0,0 +1,97 @@ +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) + +name: nfsd_server + +doc: + nfsd server configuration over generic netlink. + +attribute-sets: + - + name: rpc-status-comp-op-attr + enum-name: nfsd-rpc-status-comp-attr + name-prefix: nfsd-attr-rpc-status-comp- + attributes: + - + name: unspec + type: unused + value: 0 + - + name: op + type: u32 + - + name: rpc-status-attr + enum-name: nfsd-rpc-status-attr + name-prefix: nfsd-attr-rpc-status- + attributes: + - + name: unspec + type: unused + value: 0 + - + name: xid + type: u32 + byte-order: big-endian + - + name: flags + type: u32 + - + name: prog + type: u32 + - + name: version + type: u8 + - + name: proc + type: u32 + - + name: service_time + type: s64 + - + name: pad + type: pad + - + name: saddr4 + type: u32 + byte-order: big-endian + display-hint: ipv4 + - + name: daddr4 + type: u32 + byte-order: big-endian + display-hint: ipv4 + - + name: saddr6 + type: binary + display-hint: ipv6 + - + name: daddr6 + type: binary + display-hint: ipv6 + - + name: sport + type: u16 + byte-order: big-endian + - + name: dport + type: u16 + byte-order: big-endian + - + name: compond-op + type: array-nest + nested-attributes: rpc-status-comp-op-attr + +operations: + enum-name: nfsd-commands + name-prefix: nfsd-cmd- + list: + - + name: unspec + doc: unused + value: 0 + - + name: rpc-status-get + doc: dump pending nfsd rpc + attribute-set: rpc-status-attr + dump: + pre: nfsd-server-nl-rpc-status-get-start + post: nfsd-server-nl-rpc-status-get-done