diff mbox series

[v3,08/13] nfs_common: make nfs4.h include generated nfs4_1.h

Message ID 20240829-delstid-v3-8-271c60806c5d@kernel.org (mailing list archive)
State New
Headers show
Series nfsd: implement the "delstid" draft | expand

Commit Message

Jeff Layton Aug. 29, 2024, 1:26 p.m. UTC
Long term, we'd like to move to autogenerating a lot of our XDR code.
Both the client and server include include/linux/nfs4.h. That file is
hand-rolled and some of the symbols in it conflict with the
autogenerated symbols from the spec.

Move nfs4_1.x to Documentation/sunrpc/xdr. Create a new
include/linux/sunrpc/xdrgen directory in which we can keep autogenerated
header files. Move the new, generated nfs4xdr_gen.h file to nfs4_1.h in
that directory. Have include/linux/nfs4.h include the newly renamed file
and then remove conflicting definitions from it and nfs_xdr.h.

For now, the .x file from which we're generating the header is fairly
small and just covers the delstid draft, but we can expand that in the
future and just remove conflicting definitions as we go.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 {fs/nfsd => Documentation/sunrpc/xdr}/nfs4_1.x                | 0
 MAINTAINERS                                                   | 1 +
 fs/nfsd/nfs4xdr_gen.c                                         | 2 +-
 include/linux/nfs4.h                                          | 7 +------
 include/linux/nfs_xdr.h                                       | 5 -----
 fs/nfsd/nfs4xdr_gen.h => include/linux/sunrpc/xdrgen/nfs4_1.h | 6 +++---
 6 files changed, 6 insertions(+), 15 deletions(-)

Comments

Chuck Lever Aug. 29, 2024, 3:13 p.m. UTC | #1
On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
> Long term, we'd like to move to autogenerating a lot of our XDR code.
> Both the client and server include include/linux/nfs4.h. That file is
> hand-rolled and some of the symbols in it conflict with the
> autogenerated symbols from the spec.
> 
> Move nfs4_1.x to Documentation/sunrpc/xdr.

I can change 2/2 in the xdrgen series to land this file under
Documentation/sunrpc/xdr.


> Create a new include/linux/sunrpc/xdrgen directory in which we can
> keep autogenerated header files.

I think the header files will have different content for the client
and server. For example, the server-side header has function
declarations for the procedure argument and result XDR functions,
the client doesn't (because those functions are all static on the
client side).

Not sure we're ready for this level of sharing between client and
server.


> Move the new, generated nfs4xdr_gen.h file to nfs4_1.h in
> that directory.

I'd rather keep the current file name to indicate that it's
generated code.


> Have include/linux/nfs4.h include the newly renamed file
> and then remove conflicting definitions from it and nfs_xdr.h.
> 
> For now, the .x file from which we're generating the header is fairly
> small and just covers the delstid draft, but we can expand that in the
> future and just remove conflicting definitions as we go.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  {fs/nfsd => Documentation/sunrpc/xdr}/nfs4_1.x                | 0
>  MAINTAINERS                                                   | 1 +
>  fs/nfsd/nfs4xdr_gen.c                                         | 2 +-
>  include/linux/nfs4.h                                          | 7 +------
>  include/linux/nfs_xdr.h                                       | 5 -----
>  fs/nfsd/nfs4xdr_gen.h => include/linux/sunrpc/xdrgen/nfs4_1.h | 6 +++---
>  6 files changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4_1.x b/Documentation/sunrpc/xdr/nfs4_1.x
> similarity index 100%
> rename from fs/nfsd/nfs4_1.x
> rename to Documentation/sunrpc/xdr/nfs4_1.x
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a70b7c9c3533..e85114273238 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12175,6 +12175,7 @@ S:	Supported
>  B:	https://bugzilla.kernel.org
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
>  F:	Documentation/filesystems/nfs/
> +F:	Documentation/sunrpc/xdr/
>  F:	fs/lockd/
>  F:	fs/nfs_common/
>  F:	fs/nfsd/
> diff --git a/fs/nfsd/nfs4xdr_gen.c b/fs/nfsd/nfs4xdr_gen.c
> index 6833d0ad35a8..00e803781c87 100644
> --- a/fs/nfsd/nfs4xdr_gen.c
> +++ b/fs/nfsd/nfs4xdr_gen.c
> @@ -2,7 +2,7 @@
>  // Generated by xdrgen. Manual edits will be lost.
>  // XDR specification modification time: Wed Aug 28 09:57:28 2024
>  
> -#include "nfs4xdr_gen.h"
> +#include <linux/sunrpc/xdrgen/nfs4_1.h>

Please don't hand-edit these files. That makes it impossible to just
run the xdrgen tool and get a new version, which is the real goal.

If you need different generated content, change the tool to generate
what you need (or feel free to ask me to get out my whittling
knife).


>  static bool __maybe_unused
>  xdrgen_decode_int64_t(struct xdr_stream *xdr, int64_t *ptr)
> diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> index 8d7430d9f218..b90719244775 100644
> --- a/include/linux/nfs4.h
> +++ b/include/linux/nfs4.h
> @@ -17,6 +17,7 @@
>  #include <linux/uidgid.h>
>  #include <uapi/linux/nfs4.h>
>  #include <linux/sunrpc/msg_prot.h>
> +#include <linux/sunrpc/xdrgen/nfs4_1.h>
>  
>  enum nfs4_acl_whotype {
>  	NFS4_ACL_WHO_NAMED = 0,
> @@ -512,12 +513,6 @@ enum {
>  	FATTR4_XATTR_SUPPORT		= 82,
>  };
>  
> -enum {
> -	FATTR4_TIME_DELEG_ACCESS	= 84,
> -	FATTR4_TIME_DELEG_MODIFY	= 85,
> -	FATTR4_OPEN_ARGUMENTS		= 86,
> -};
> -
>  /*
>   * The following internal definitions enable processing the above
>   * attribute bits within 32-bit word boundaries.
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 45623af3e7b8..d3fe47baf110 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1315,11 +1315,6 @@ struct nfs4_fsid_present_res {
>  
>  #endif /* CONFIG_NFS_V4 */
>  
> -struct nfstime4 {
> -	u64	seconds;
> -	u32	nseconds;
> -};
> -
>  #ifdef CONFIG_NFS_V4_1
>  
>  struct pnfs_commit_bucket {
> diff --git a/fs/nfsd/nfs4xdr_gen.h b/include/linux/sunrpc/xdrgen/nfs4_1.h
> similarity index 96%
> rename from fs/nfsd/nfs4xdr_gen.h
> rename to include/linux/sunrpc/xdrgen/nfs4_1.h
> index 5465db4fb32b..5faee67281b8 100644
> --- a/fs/nfsd/nfs4xdr_gen.h
> +++ b/include/linux/sunrpc/xdrgen/nfs4_1.h
> @@ -2,8 +2,8 @@
>  /* Generated by xdrgen. Manual edits will be lost. */
>  /* XDR specification modification time: Wed Aug 28 09:57:28 2024 */
>  
> -#ifndef _LINUX_NFS4_XDRGEN_H
> -#define _LINUX_NFS4_XDRGEN_H
> +#ifndef _LINUX_XDRGEN_NFS4_H
> +#define _LINUX_XDRGEN_NFS4_H

Ditto. Resist The Urge (tm).


>  #include <linux/types.h>
>  #include <linux/sunrpc/svc.h>
> @@ -103,4 +103,4 @@ enum { FATTR4_TIME_DELEG_MODIFY = 85 };
>  
>  enum { OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 0x100000 };
>  
> -#endif /* _LINUX_NFS4_XDRGEN_H */
> +#endif /* _LINUX_XDRGEN_NFS4_H */
> 
> -- 
> 2.46.0
>
Chuck Lever Aug. 29, 2024, 3:28 p.m. UTC | #2
On Thu, Aug 29, 2024 at 11:13:49AM -0400, Chuck Lever wrote:
> On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
> > Long term, we'd like to move to autogenerating a lot of our XDR code.
> > Both the client and server include include/linux/nfs4.h. That file is
> > hand-rolled and some of the symbols in it conflict with the
> > autogenerated symbols from the spec.
> > 
> > Move nfs4_1.x to Documentation/sunrpc/xdr.
> 
> I can change 2/2 in the xdrgen series to land this file under
> Documentation/sunrpc/xdr.
> 
> 
> > Create a new include/linux/sunrpc/xdrgen directory in which we can
> > keep autogenerated header files.
> 
> I think the header files will have different content for the client
> and server. For example, the server-side header has function
> declarations for the procedure argument and result XDR functions,
> the client doesn't (because those functions are all static on the
> client side).
> 
> Not sure we're ready for this level of sharing between client and
> server.
> 
> 
> > Move the new, generated nfs4xdr_gen.h file to nfs4_1.h in
> > that directory.
> 
> I'd rather keep the current file name to indicate that it's
> generated code.
> 
> 
> > Have include/linux/nfs4.h include the newly renamed file
> > and then remove conflicting definitions from it and nfs_xdr.h.
> > 
> > For now, the .x file from which we're generating the header is fairly
> > small and just covers the delstid draft, but we can expand that in the
> > future and just remove conflicting definitions as we go.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  {fs/nfsd => Documentation/sunrpc/xdr}/nfs4_1.x                | 0
> >  MAINTAINERS                                                   | 1 +
> >  fs/nfsd/nfs4xdr_gen.c                                         | 2 +-
> >  include/linux/nfs4.h                                          | 7 +------
> >  include/linux/nfs_xdr.h                                       | 5 -----
> >  fs/nfsd/nfs4xdr_gen.h => include/linux/sunrpc/xdrgen/nfs4_1.h | 6 +++---
> >  6 files changed, 6 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4_1.x b/Documentation/sunrpc/xdr/nfs4_1.x
> > similarity index 100%
> > rename from fs/nfsd/nfs4_1.x
> > rename to Documentation/sunrpc/xdr/nfs4_1.x
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a70b7c9c3533..e85114273238 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12175,6 +12175,7 @@ S:	Supported
> >  B:	https://bugzilla.kernel.org
> >  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> >  F:	Documentation/filesystems/nfs/
> > +F:	Documentation/sunrpc/xdr/
> >  F:	fs/lockd/
> >  F:	fs/nfs_common/
> >  F:	fs/nfsd/
> > diff --git a/fs/nfsd/nfs4xdr_gen.c b/fs/nfsd/nfs4xdr_gen.c
> > index 6833d0ad35a8..00e803781c87 100644
> > --- a/fs/nfsd/nfs4xdr_gen.c
> > +++ b/fs/nfsd/nfs4xdr_gen.c
> > @@ -2,7 +2,7 @@
> >  // Generated by xdrgen. Manual edits will be lost.
> >  // XDR specification modification time: Wed Aug 28 09:57:28 2024
> >  
> > -#include "nfs4xdr_gen.h"
> > +#include <linux/sunrpc/xdrgen/nfs4_1.h>
> 
> Please don't hand-edit these files. That makes it impossible to just
> run the xdrgen tool and get a new version, which is the real goal.
> 
> If you need different generated content, change the tool to generate
> what you need (or feel free to ask me to get out my whittling
> knife).
> 
> 
> >  static bool __maybe_unused
> >  xdrgen_decode_int64_t(struct xdr_stream *xdr, int64_t *ptr)
> > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> > index 8d7430d9f218..b90719244775 100644
> > --- a/include/linux/nfs4.h
> > +++ b/include/linux/nfs4.h
> > @@ -17,6 +17,7 @@
> >  #include <linux/uidgid.h>
> >  #include <uapi/linux/nfs4.h>
> >  #include <linux/sunrpc/msg_prot.h>
> > +#include <linux/sunrpc/xdrgen/nfs4_1.h>
> >  
> >  enum nfs4_acl_whotype {
> >  	NFS4_ACL_WHO_NAMED = 0,
> > @@ -512,12 +513,6 @@ enum {
> >  	FATTR4_XATTR_SUPPORT		= 82,
> >  };
> >  
> > -enum {
> > -	FATTR4_TIME_DELEG_ACCESS	= 84,
> > -	FATTR4_TIME_DELEG_MODIFY	= 85,
> > -	FATTR4_OPEN_ARGUMENTS		= 86,
> > -};
> > -
> >  /*
> >   * The following internal definitions enable processing the above
> >   * attribute bits within 32-bit word boundaries.
> > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > index 45623af3e7b8..d3fe47baf110 100644
> > --- a/include/linux/nfs_xdr.h
> > +++ b/include/linux/nfs_xdr.h
> > @@ -1315,11 +1315,6 @@ struct nfs4_fsid_present_res {
> >  
> >  #endif /* CONFIG_NFS_V4 */
> >  
> > -struct nfstime4 {
> > -	u64	seconds;
> > -	u32	nseconds;
> > -};
> > -
> >  #ifdef CONFIG_NFS_V4_1
> >  
> >  struct pnfs_commit_bucket {
> > diff --git a/fs/nfsd/nfs4xdr_gen.h b/include/linux/sunrpc/xdrgen/nfs4_1.h
> > similarity index 96%
> > rename from fs/nfsd/nfs4xdr_gen.h
> > rename to include/linux/sunrpc/xdrgen/nfs4_1.h
> > index 5465db4fb32b..5faee67281b8 100644
> > --- a/fs/nfsd/nfs4xdr_gen.h
> > +++ b/include/linux/sunrpc/xdrgen/nfs4_1.h
> > @@ -2,8 +2,8 @@
> >  /* Generated by xdrgen. Manual edits will be lost. */
> >  /* XDR specification modification time: Wed Aug 28 09:57:28 2024 */
> >  
> > -#ifndef _LINUX_NFS4_XDRGEN_H
> > -#define _LINUX_NFS4_XDRGEN_H
> > +#ifndef _LINUX_XDRGEN_NFS4_H
> > +#define _LINUX_XDRGEN_NFS4_H
> 
> Ditto. Resist The Urge (tm).

Actually, renaming the header guard macro makes sense. I can adjust
xdrgen to do that.


> >  #include <linux/types.h>
> >  #include <linux/sunrpc/svc.h>
> > @@ -103,4 +103,4 @@ enum { FATTR4_TIME_DELEG_MODIFY = 85 };
> >  
> >  enum { OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 0x100000 };
> >  
> > -#endif /* _LINUX_NFS4_XDRGEN_H */
> > +#endif /* _LINUX_XDRGEN_NFS4_H */
Jeff Layton Aug. 29, 2024, 6:26 p.m. UTC | #3
On Thu, 2024-08-29 at 11:13 -0400, Chuck Lever wrote:
> On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
> > Long term, we'd like to move to autogenerating a lot of our XDR code.
> > Both the client and server include include/linux/nfs4.h. That file is
> > hand-rolled and some of the symbols in it conflict with the
> > autogenerated symbols from the spec.
> > 
> > Move nfs4_1.x to Documentation/sunrpc/xdr.
> 
> I can change 2/2 in the xdrgen series to land this file under
> Documentation/sunrpc/xdr.
> 

Thanks, and also thanks for making the timestamp handling functions
public too.

> 
> > Create a new include/linux/sunrpc/xdrgen directory in which we can
> > keep autogenerated header files.
> 
> I think the header files will have different content for the client
> and server. For example, the server-side header has function
> declarations for the procedure argument and result XDR functions,
> the client doesn't (because those functions are all static on the
> client side).
> 
> Not sure we're ready for this level of sharing between client and
> server.
> 

Does that matter though? Sure the client might be exposed to some
server encoding/decoding functions, but it doesn't have to use them.
The constant and type definitions are the same between the client and
server. I think there is value in having a single source for that, like
we have today with nfs4.h.

If we do decide that it's a problem, we can just split things up
further:

1. one header file with constant, struct and type definitions
2. one with server-side encode/decode prototypes that includes #1
3. one with client-side encode/decode prototypes that includes #1

include/linux/nfs4.h could still include #1 as well, but the client and
server could include the appropriate headers instead of or in addition
to it.


> > Move the new, generated nfs4xdr_gen.h file to nfs4_1.h in
> > that directory.
> 
> I'd rather keep the current file name to indicate that it's
> generated code.
> 

I figured the "xdrgen" directory name would convey that. This naming
also makes it clearer that it's generated from nfs4_1.x. That said, I'm
not too particular here. Can you lay out how you think we ought to
arrange things?

> 
> > Have include/linux/nfs4.h include the newly renamed file
> > and then remove conflicting definitions from it and nfs_xdr.h.
> > 
> > For now, the .x file from which we're generating the header is fairly
> > small and just covers the delstid draft, but we can expand that in the
> > future and just remove conflicting definitions as we go.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  {fs/nfsd => Documentation/sunrpc/xdr}/nfs4_1.x                | 0
> >  MAINTAINERS                                                   | 1 +
> >  fs/nfsd/nfs4xdr_gen.c                                         | 2 +-
> >  include/linux/nfs4.h                                          | 7 +------
> >  include/linux/nfs_xdr.h                                       | 5 -----
> >  fs/nfsd/nfs4xdr_gen.h => include/linux/sunrpc/xdrgen/nfs4_1.h | 6 +++---
> >  6 files changed, 6 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4_1.x b/Documentation/sunrpc/xdr/nfs4_1.x
> > similarity index 100%
> > rename from fs/nfsd/nfs4_1.x
> > rename to Documentation/sunrpc/xdr/nfs4_1.x
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a70b7c9c3533..e85114273238 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12175,6 +12175,7 @@ S:	Supported
> >  B:	https://bugzilla.kernel.org
> >  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> >  F:	Documentation/filesystems/nfs/
> > +F:	Documentation/sunrpc/xdr/
> >  F:	fs/lockd/
> >  F:	fs/nfs_common/
> >  F:	fs/nfsd/
> > diff --git a/fs/nfsd/nfs4xdr_gen.c b/fs/nfsd/nfs4xdr_gen.c
> > index 6833d0ad35a8..00e803781c87 100644
> > --- a/fs/nfsd/nfs4xdr_gen.c
> > +++ b/fs/nfsd/nfs4xdr_gen.c
> > @@ -2,7 +2,7 @@
> >  // Generated by xdrgen. Manual edits will be lost.
> >  // XDR specification modification time: Wed Aug 28 09:57:28 2024
> >  
> > -#include "nfs4xdr_gen.h"
> > +#include <linux/sunrpc/xdrgen/nfs4_1.h>
> 
> Please don't hand-edit these files. That makes it impossible to just
> run the xdrgen tool and get a new version, which is the real goal.
> 
> If you need different generated content, change the tool to generate
> what you need (or feel free to ask me to get out my whittling
> knife).
> 
> 

No problem. This part is a Q&D hack job to get everything working with
minimal changes. Changing the tool to generate the right thing would be
a better long-term solution (once we settle on where these files will
go, etc.)
 
> >  static bool __maybe_unused
> >  xdrgen_decode_int64_t(struct xdr_stream *xdr, int64_t *ptr)
> > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> > index 8d7430d9f218..b90719244775 100644
> > --- a/include/linux/nfs4.h
> > +++ b/include/linux/nfs4.h
> > @@ -17,6 +17,7 @@
> >  #include <linux/uidgid.h>
> >  #include <uapi/linux/nfs4.h>
> >  #include <linux/sunrpc/msg_prot.h>
> > +#include <linux/sunrpc/xdrgen/nfs4_1.h>
> >  
> >  enum nfs4_acl_whotype {
> >  	NFS4_ACL_WHO_NAMED = 0,
> > @@ -512,12 +513,6 @@ enum {
> >  	FATTR4_XATTR_SUPPORT		= 82,
> >  };
> >  
> > -enum {
> > -	FATTR4_TIME_DELEG_ACCESS	= 84,
> > -	FATTR4_TIME_DELEG_MODIFY	= 85,
> > -	FATTR4_OPEN_ARGUMENTS		= 86,
> > -};
> > -
> >  /*
> >   * The following internal definitions enable processing the above
> >   * attribute bits within 32-bit word boundaries.
> > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > index 45623af3e7b8..d3fe47baf110 100644
> > --- a/include/linux/nfs_xdr.h
> > +++ b/include/linux/nfs_xdr.h
> > @@ -1315,11 +1315,6 @@ struct nfs4_fsid_present_res {
> >  
> >  #endif /* CONFIG_NFS_V4 */
> >  
> > -struct nfstime4 {
> > -	u64	seconds;
> > -	u32	nseconds;
> > -};
> > -
> >  #ifdef CONFIG_NFS_V4_1
> >  
> >  struct pnfs_commit_bucket {
> > diff --git a/fs/nfsd/nfs4xdr_gen.h b/include/linux/sunrpc/xdrgen/nfs4_1.h
> > similarity index 96%
> > rename from fs/nfsd/nfs4xdr_gen.h
> > rename to include/linux/sunrpc/xdrgen/nfs4_1.h
> > index 5465db4fb32b..5faee67281b8 100644
> > --- a/fs/nfsd/nfs4xdr_gen.h
> > +++ b/include/linux/sunrpc/xdrgen/nfs4_1.h
> > @@ -2,8 +2,8 @@
> >  /* Generated by xdrgen. Manual edits will be lost. */
> >  /* XDR specification modification time: Wed Aug 28 09:57:28 2024 */
> >  
> > -#ifndef _LINUX_NFS4_XDRGEN_H
> > -#define _LINUX_NFS4_XDRGEN_H
> > +#ifndef _LINUX_XDRGEN_NFS4_H
> > +#define _LINUX_XDRGEN_NFS4_H
> 
> Ditto. Resist The Urge (tm).
> 
> 
> >  #include <linux/types.h>
> >  #include <linux/sunrpc/svc.h>
> > @@ -103,4 +103,4 @@ enum { FATTR4_TIME_DELEG_MODIFY = 85 };
> >  
> >  enum { OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 0x100000 };
> >  
> > -#endif /* _LINUX_NFS4_XDRGEN_H */
> > +#endif /* _LINUX_XDRGEN_NFS4_H */
> > 
> > -- 
> > 2.46.0
> > 
>
Chuck Lever Aug. 29, 2024, 7:02 p.m. UTC | #4
> On Aug 29, 2024, at 2:26 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Thu, 2024-08-29 at 11:13 -0400, Chuck Lever wrote:
>> On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
>> 
>>> Create a new include/linux/sunrpc/xdrgen directory in which we can
>>> keep autogenerated header files.
>> 
>> I think the header files will have different content for the client
>> and server. For example, the server-side header has function
>> declarations for the procedure argument and result XDR functions,
>> the client doesn't (because those functions are all static on the
>> client side).
>> 
>> Not sure we're ready for this level of sharing between client and
>> server.
>> 
> 
> Does that matter though?

IMHO it does. Client and server are rather separate
code bases, and when we have attempted to share things
extensively between them in the past, it locks the other
side into something that doesn't fit well.

I'd rather be further along with client side code
generation before making decisions about how to fit
these pieces together. What I've toyed with so far
suggests there are going to be a few substantive
differences with the server side.


> The constant and type definitions are the same between the client and
> server. I think there is value in having a single source for that, like
> we have today with nfs4.h.
> 
> If we do decide that it's a problem, we can just split things up
> further:

The tool already does that now:

  xdrgen header --definitions

gives you the protocol pieces, and

  xdrgen header --declarations

gives you the function declarations. Specify both
together and you get what you have in nfs4xdr_gen.h
today. It might not be the most natural way to split
these up, but <shrug>.

(Note that this is going to get worse when Rust is
introduced into the mix).


> 1. one header file with constant, struct and type definitions
> 2. one with server-side encode/decode prototypes that includes #1
> 3. one with client-side encode/decode prototypes that includes #1

Right, that's already the direction I'm going with
xdrgen. I think we're on the same page with that.


> include/linux/nfs4.h could still include #1 as well, but the client and
> server could include the appropriate headers instead of or in addition
> to it.

That should work.


>>> Move the new, generated nfs4xdr_gen.h file to nfs4_1.h in
>>> that directory.
>> 
>> I'd rather keep the current file name to indicate that it's
>> generated code.
>> 
> 
> I figured the "xdrgen" directory name would convey that.

It does only if we move the header file to
include/linux/sunrpc/xdrgen. I'm not sold on that idea yet
though I guess it works for the protocol pieces that the
Linux community doesn't directly control (ie --definitions).


> This naming also makes it clearer that it's generated from
> nfs4_1.x.

Well the generated boilerplate could contain the spec file
name -- it has the spec file's timestamp already as a kind
of janky version number. Commit hash might be better.

Note that the spec authors expect implementations to catenate
all the disparate .x pieces (ie, all the minor versions and
all the pNFS layout types) together into a single all-singing-
all-dancing nfs4.x. Looking at it now, I'm not convinced we
want to stay with the name nfs4_1.x. Also that's why I used
the output file name nfs4xdr_gen.[ch].


> That said, I'm not too particular here. Can you lay out
> how you think we ought to arrange things?

I'm still musing about it. I'll mock something up in the
2/2 xdrgen patch for more feedback.

Now that we've got most of the review of LOCALIO done, I'll
have a bit more time to help you get xdrgen ready for server-
side delstid. I will try not to hold you up.


--
Chuck Lever
Chuck Lever Aug. 30, 2024, 2:48 p.m. UTC | #5
> On Aug 29, 2024, at 2:26 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Thu, 2024-08-29 at 11:13 -0400, Chuck Lever wrote:
>> On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
>>> 
>>> index 6833d0ad35a8..00e803781c87 100644
>>> --- a/fs/nfsd/nfs4xdr_gen.c
>>> +++ b/fs/nfsd/nfs4xdr_gen.c
>>> @@ -2,7 +2,7 @@
>>> // Generated by xdrgen. Manual edits will be lost.
>>> // XDR specification modification time: Wed Aug 28 09:57:28 2024
>>> 
>>> -#include "nfs4xdr_gen.h"
>>> +#include <linux/sunrpc/xdrgen/nfs4_1.h>
>> 
>> Please don't hand-edit these files. That makes it impossible to just
>> run the xdrgen tool and get a new version, which is the real goal.
>> 
>> If you need different generated content, change the tool to generate
>> what you need (or feel free to ask me to get out my whittling
>> knife).
> 
> No problem. This part is a Q&D hack job to get everything working with
> minimal changes. Changing the tool to generate the right thing would be
> a better long-term solution (once we settle on where these files will
> go, etc.)

OK, that makes sense.

Going forward I will watch for such Q&D edits in the generated
files and try to address those in xdrgen. I would like to avoid
actually /committing/ such edits, though, because IMO we are
pretty active here right now and can get the "long term" fix
done quickly.

Meanwhile, I've updated the xdrgen patches in nfsd-next to
address many (or maybe all) of your requests from yesterday.

The header file is now split between

  include/linux/sunrpc/xdrgen/nfs4.h (protocol definitions)

and

  fs/nfsd/nfs4xdr_gen.h (function declarations)


--
Chuck Lever
Jeff Layton Aug. 30, 2024, 3:44 p.m. UTC | #6
On Fri, 2024-08-30 at 14:48 +0000, Chuck Lever III wrote:
> 
> > On Aug 29, 2024, at 2:26 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Thu, 2024-08-29 at 11:13 -0400, Chuck Lever wrote:
> > > On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
> > > > 
> > > > index 6833d0ad35a8..00e803781c87 100644
> > > > --- a/fs/nfsd/nfs4xdr_gen.c
> > > > +++ b/fs/nfsd/nfs4xdr_gen.c
> > > > @@ -2,7 +2,7 @@
> > > > // Generated by xdrgen. Manual edits will be lost.
> > > > // XDR specification modification time: Wed Aug 28 09:57:28 2024
> > > > 
> > > > -#include "nfs4xdr_gen.h"
> > > > +#include <linux/sunrpc/xdrgen/nfs4_1.h>
> > > 
> > > Please don't hand-edit these files. That makes it impossible to just
> > > run the xdrgen tool and get a new version, which is the real goal.
> > > 
> > > If you need different generated content, change the tool to generate
> > > what you need (or feel free to ask me to get out my whittling
> > > knife).
> > 
> > No problem. This part is a Q&D hack job to get everything working with
> > minimal changes. Changing the tool to generate the right thing would be
> > a better long-term solution (once we settle on where these files will
> > go, etc.)
> 
> OK, that makes sense.
> 
> Going forward I will watch for such Q&D edits in the generated
> files and try to address those in xdrgen. I would like to avoid
> actually /committing/ such edits, though, because IMO we are
> pretty active here right now and can get the "long term" fix
> done quickly.
> 

Sorry I wasn't clear about that. That part is really just a PoC. The
goal would be to autogenerate those files.

Once we settle on locations, we can add a "make xdrgen" target that
runs xdrgen over all of the files in Documentation/sunrpc/xdr and
regenerates all of the headers and source files.

Then we can edit the .x files, do a "make xdrgen" and then commit the
resulting updates.

> Meanwhile, I've updated the xdrgen patches in nfsd-next to
> address many (or maybe all) of your requests from yesterday.
> 
> The header file is now split between
> 
>   include/linux/sunrpc/xdrgen/nfs4.h (protocol definitions)
> 
> and
> 
>   fs/nfsd/nfs4xdr_gen.h (function declarations)
> 

Excellent. I'll rebase onto that later today.
Jeff Layton Aug. 30, 2024, 5:48 p.m. UTC | #7
On Fri, 2024-08-30 at 11:44 -0400, Jeff Layton wrote:
> On Fri, 2024-08-30 at 14:48 +0000, Chuck Lever III wrote:
> > 
> > > On Aug 29, 2024, at 2:26 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > On Thu, 2024-08-29 at 11:13 -0400, Chuck Lever wrote:
> > > > On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
> > > > > 
> > > > > index 6833d0ad35a8..00e803781c87 100644
> > > > > --- a/fs/nfsd/nfs4xdr_gen.c
> > > > > +++ b/fs/nfsd/nfs4xdr_gen.c
> > > > > @@ -2,7 +2,7 @@
> > > > > // Generated by xdrgen. Manual edits will be lost.
> > > > > // XDR specification modification time: Wed Aug 28 09:57:28 2024
> > > > > 
> > > > > -#include "nfs4xdr_gen.h"
> > > > > +#include <linux/sunrpc/xdrgen/nfs4_1.h>
> > > > 
> > > > Please don't hand-edit these files. That makes it impossible to just
> > > > run the xdrgen tool and get a new version, which is the real goal.
> > > > 
> > > > If you need different generated content, change the tool to generate
> > > > what you need (or feel free to ask me to get out my whittling
> > > > knife).
> > > 
> > > No problem. This part is a Q&D hack job to get everything working with
> > > minimal changes. Changing the tool to generate the right thing would be
> > > a better long-term solution (once we settle on where these files will
> > > go, etc.)
> > 
> > OK, that makes sense.
> > 
> > Going forward I will watch for such Q&D edits in the generated
> > files and try to address those in xdrgen. I would like to avoid
> > actually /committing/ such edits, though, because IMO we are
> > pretty active here right now and can get the "long term" fix
> > done quickly.
> > 
> 
> Sorry I wasn't clear about that. That part is really just a PoC. The
> goal would be to autogenerate those files.
> 
> Once we settle on locations, we can add a "make xdrgen" target that
> runs xdrgen over all of the files in Documentation/sunrpc/xdr and
> regenerates all of the headers and source files.
> 
> Then we can edit the .x files, do a "make xdrgen" and then commit the
> resulting updates.
> 
> > Meanwhile, I've updated the xdrgen patches in nfsd-next to
> > address many (or maybe all) of your requests from yesterday.
> > 
> > The header file is now split between
> > 
> >   include/linux/sunrpc/xdrgen/nfs4.h (protocol definitions)
> > 
> > and
> > 
> >   fs/nfsd/nfs4xdr_gen.h (function declarations)
> > 
> 
> Excellent. I'll rebase onto that later today.

...and on that note, I pushed a rebased version of this into my
"delstid" branch.

I won't bother reposting it just yet, since it's fundamentally the same
code. The only real difference is in the patch that adapts nfs4.h to
include the autogenerated header.
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4_1.x b/Documentation/sunrpc/xdr/nfs4_1.x
similarity index 100%
rename from fs/nfsd/nfs4_1.x
rename to Documentation/sunrpc/xdr/nfs4_1.x
diff --git a/MAINTAINERS b/MAINTAINERS
index a70b7c9c3533..e85114273238 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12175,6 +12175,7 @@  S:	Supported
 B:	https://bugzilla.kernel.org
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
 F:	Documentation/filesystems/nfs/
+F:	Documentation/sunrpc/xdr/
 F:	fs/lockd/
 F:	fs/nfs_common/
 F:	fs/nfsd/
diff --git a/fs/nfsd/nfs4xdr_gen.c b/fs/nfsd/nfs4xdr_gen.c
index 6833d0ad35a8..00e803781c87 100644
--- a/fs/nfsd/nfs4xdr_gen.c
+++ b/fs/nfsd/nfs4xdr_gen.c
@@ -2,7 +2,7 @@ 
 // Generated by xdrgen. Manual edits will be lost.
 // XDR specification modification time: Wed Aug 28 09:57:28 2024
 
-#include "nfs4xdr_gen.h"
+#include <linux/sunrpc/xdrgen/nfs4_1.h>
 
 static bool __maybe_unused
 xdrgen_decode_int64_t(struct xdr_stream *xdr, int64_t *ptr)
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index 8d7430d9f218..b90719244775 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -17,6 +17,7 @@ 
 #include <linux/uidgid.h>
 #include <uapi/linux/nfs4.h>
 #include <linux/sunrpc/msg_prot.h>
+#include <linux/sunrpc/xdrgen/nfs4_1.h>
 
 enum nfs4_acl_whotype {
 	NFS4_ACL_WHO_NAMED = 0,
@@ -512,12 +513,6 @@  enum {
 	FATTR4_XATTR_SUPPORT		= 82,
 };
 
-enum {
-	FATTR4_TIME_DELEG_ACCESS	= 84,
-	FATTR4_TIME_DELEG_MODIFY	= 85,
-	FATTR4_OPEN_ARGUMENTS		= 86,
-};
-
 /*
  * The following internal definitions enable processing the above
  * attribute bits within 32-bit word boundaries.
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 45623af3e7b8..d3fe47baf110 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1315,11 +1315,6 @@  struct nfs4_fsid_present_res {
 
 #endif /* CONFIG_NFS_V4 */
 
-struct nfstime4 {
-	u64	seconds;
-	u32	nseconds;
-};
-
 #ifdef CONFIG_NFS_V4_1
 
 struct pnfs_commit_bucket {
diff --git a/fs/nfsd/nfs4xdr_gen.h b/include/linux/sunrpc/xdrgen/nfs4_1.h
similarity index 96%
rename from fs/nfsd/nfs4xdr_gen.h
rename to include/linux/sunrpc/xdrgen/nfs4_1.h
index 5465db4fb32b..5faee67281b8 100644
--- a/fs/nfsd/nfs4xdr_gen.h
+++ b/include/linux/sunrpc/xdrgen/nfs4_1.h
@@ -2,8 +2,8 @@ 
 /* Generated by xdrgen. Manual edits will be lost. */
 /* XDR specification modification time: Wed Aug 28 09:57:28 2024 */
 
-#ifndef _LINUX_NFS4_XDRGEN_H
-#define _LINUX_NFS4_XDRGEN_H
+#ifndef _LINUX_XDRGEN_NFS4_H
+#define _LINUX_XDRGEN_NFS4_H
 
 #include <linux/types.h>
 #include <linux/sunrpc/svc.h>
@@ -103,4 +103,4 @@  enum { FATTR4_TIME_DELEG_MODIFY = 85 };
 
 enum { OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 0x100000 };
 
-#endif /* _LINUX_NFS4_XDRGEN_H */
+#endif /* _LINUX_XDRGEN_NFS4_H */