[PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

In NFSD's pNFS flexfile layout implementation, struct pnfs_ff_layout
defines a struct nfs_fh field. This comes from <linux/nfs.h> :

> /*
>  * This is the kernel NFS client file handle representation
>  */
> #define NFS_MAXFHSIZE           128
> struct nfs_fh {
>         unsigned short          size;
>         unsigned char           data[NFS_MAXFHSIZE];
> };

But NFSD has an equivalent struct, knfsd_fh.

To reduce cross-subsystem header dependencies, avoid using a struct
defined by the kernel's NFS client implementation in NFSD's flexfile
layout implementation.

Cc: Thomas Haynes <loghyr@hammerspace.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/flexfilelayout.c    | 3 +--
 fs/nfsd/flexfilelayoutxdr.c | 4 ++--
 fs/nfsd/flexfilelayoutxdr.h | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/flexfilelayout.c b/fs/nfsd/flexfilelayout.c
index 0f1a35400cd5..971368877006 100644
--- a/fs/nfsd/flexfilelayout.c
+++ b/fs/nfsd/flexfilelayout.c
@@ -61,8 +61,7 @@ nfsd4_ff_proc_layoutget(struct svc_rqst *rqstp, struct inode *inode,
 	if (error)
 		goto out_error;
 
-	fl->fh.size = fhp->fh_handle.fh_size;
-	memcpy(fl->fh.data, &fhp->fh_handle.fh_raw, fl->fh.size);
+	fh_copy_shallow(&fl->fh, &fhp->fh_handle);
 
 	/* Give whole file layout segments */
 	seg->offset = 0;
diff --git a/fs/nfsd/flexfilelayoutxdr.c b/fs/nfsd/flexfilelayoutxdr.c
index f9f7e38cba13..88686532f03e 100644
--- a/fs/nfsd/flexfilelayoutxdr.c
+++ b/fs/nfsd/flexfilelayoutxdr.c
@@ -30,7 +30,7 @@ nfsd4_ff_encode_layoutget(struct xdr_stream *xdr,
 	struct ff_idmap uid;
 	struct ff_idmap gid;
 
-	fh_len = 4 + fl->fh.size;
+	fh_len = 4 + fl->fh.fh_size;
 
 	uid.len = sprintf(uid.buf, "%u", from_kuid(&init_user_ns, fl->uid));
 	gid.len = sprintf(gid.buf, "%u", from_kgid(&init_user_ns, fl->gid));
@@ -63,7 +63,7 @@ nfsd4_ff_encode_layoutget(struct xdr_stream *xdr,
 				    sizeof(stateid_opaque_t));
 
 	*p++ = cpu_to_be32(1);			/* single file handle */
-	p = xdr_encode_opaque(p, fl->fh.data, fl->fh.size);
+	p = xdr_encode_opaque(p, fl->fh.fh_raw, fl->fh.fh_size);
 
 	p = xdr_encode_opaque(p, uid.buf, uid.len);
 	p = xdr_encode_opaque(p, gid.buf, gid.len);
diff --git a/fs/nfsd/flexfilelayoutxdr.h b/fs/nfsd/flexfilelayoutxdr.h
index 6d5a1066a903..5dee1722834f 100644
--- a/fs/nfsd/flexfilelayoutxdr.h
+++ b/fs/nfsd/flexfilelayoutxdr.h
@@ -39,7 +39,7 @@ struct pnfs_ff_layout {
 	kgid_t				gid;
 	struct nfsd4_deviceid		deviceid;
 	stateid_t			stateid;
-	struct nfs_fh			fh;
+	struct knfsd_fh			fh;
 };
 
 __be32 nfsd4_ff_encode_getdeviceinfo(struct xdr_stream *xdr,
-- 
2.52.0

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[NeilBrown]

On Tue, 09 Dec 2025, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> In NFSD's pNFS flexfile layout implementation, struct pnfs_ff_layout
> defines a struct nfs_fh field. This comes from <linux/nfs.h> :
> 
> > /*
> >  * This is the kernel NFS client file handle representation
> >  */
> > #define NFS_MAXFHSIZE           128
> > struct nfs_fh {
> >         unsigned short          size;
> >         unsigned char           data[NFS_MAXFHSIZE];
> > };
> 
> But NFSD has an equivalent struct, knfsd_fh.
> 
> To reduce cross-subsystem header dependencies, avoid using a struct
> defined by the kernel's NFS client implementation in NFSD's flexfile
> layout implementation.

linux/nfs.h appears to be mostly generic-nfs stuff, rather than being
client specific.
If this change allowed us to remove "#include <linux/nfs.h>" from nfsd
code, then it would be a good change, but I don't see that it does.

Now that "struct knfsd_fh" doesn't encode info about the fh format that
knfsd uses, would it instead make sense to discard it and use "struct
nfs_fh" throughout NFSD?

Thanks,
NeilBrown

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[Chuck Lever]

On Mon, Dec 8, 2025, at 4:35 PM, NeilBrown wrote:
> On Tue, 09 Dec 2025, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>> 
>> In NFSD's pNFS flexfile layout implementation, struct pnfs_ff_layout
>> defines a struct nfs_fh field. This comes from <linux/nfs.h> :
>> 
>> > /*
>> >  * This is the kernel NFS client file handle representation
>> >  */
>> > #define NFS_MAXFHSIZE           128
>> > struct nfs_fh {
>> >         unsigned short          size;
>> >         unsigned char           data[NFS_MAXFHSIZE];
>> > };
>> 
>> But NFSD has an equivalent struct, knfsd_fh.
>> 
>> To reduce cross-subsystem header dependencies, avoid using a struct
>> defined by the kernel's NFS client implementation in NFSD's flexfile
>> layout implementation.
>
> linux/nfs.h appears to be mostly generic-nfs stuff, rather than being
> client specific.
> If this change allowed us to remove "#include <linux/nfs.h>" from nfsd
> code, then it would be a good change, but I don't see that it does.

This change is part of a longer series I'm working on that enables
the removal of linux/nfs.h from parts of the NFSD code base. The
other two spots that need attention are localio and server-to-server
copy. I'm not planning to do anything to localio.


> Now that "struct knfsd_fh" doesn't encode info about the fh format that
> knfsd uses, would it instead make sense to discard it and use "struct
> nfs_fh" throughout NFSD?

Based on the effort I've already put into dealing with just ssc, I
think that would be a monumental change, and I'm not convinced it's
worth the cost.

What's more, it would move in the wrong direction. Stapling the client
and server implementations together by using the same headers makes
the code more difficult to modify without touching both trees in a
rather invasive way.

What I'm trying to do right at the moment is convert NFSD's NFSv2 and
NFSv3 implementations over to use xdrgen. Getting rid of linux/nfs.h
and other such dependencies is a firm pre-requisite for that.


-- 
Chuck Lever

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[NeilBrown]

On Tue, 09 Dec 2025, Chuck Lever wrote:
> 
> On Mon, Dec 8, 2025, at 4:35 PM, NeilBrown wrote:
> > On Tue, 09 Dec 2025, Chuck Lever wrote:
> >> From: Chuck Lever <chuck.lever@oracle.com>
> >> 
> >> In NFSD's pNFS flexfile layout implementation, struct pnfs_ff_layout
> >> defines a struct nfs_fh field. This comes from <linux/nfs.h> :
> >> 
> >> > /*
> >> >  * This is the kernel NFS client file handle representation
> >> >  */
> >> > #define NFS_MAXFHSIZE           128
> >> > struct nfs_fh {
> >> >         unsigned short          size;
> >> >         unsigned char           data[NFS_MAXFHSIZE];
> >> > };
> >> 
> >> But NFSD has an equivalent struct, knfsd_fh.
> >> 
> >> To reduce cross-subsystem header dependencies, avoid using a struct
> >> defined by the kernel's NFS client implementation in NFSD's flexfile
> >> layout implementation.
> >
> > linux/nfs.h appears to be mostly generic-nfs stuff, rather than being
> > client specific.
> > If this change allowed us to remove "#include <linux/nfs.h>" from nfsd
> > code, then it would be a good change, but I don't see that it does.
> 
> This change is part of a longer series I'm working on that enables
> the removal of linux/nfs.h from parts of the NFSD code base. The
> other two spots that need attention are localio and server-to-server
> copy. I'm not planning to do anything to localio.

So why separate this patch out from the series?

> 
> 
> > Now that "struct knfsd_fh" doesn't encode info about the fh format that
> > knfsd uses, would it instead make sense to discard it and use "struct
> > nfs_fh" throughout NFSD?
> 
> Based on the effort I've already put into dealing with just ssc, I
> think that would be a monumental change, and I'm not convinced it's
> worth the cost.
> 
> What's more, it would move in the wrong direction. Stapling the client
> and server implementations together by using the same headers makes
> the code more difficult to modify without touching both trees in a
> rather invasive way.

Are we planning to get rid of nfs2.h nfs3.h nfs4.h too?

> 
> What I'm trying to do right at the moment is convert NFSD's NFSv2 and
> NFSv3 implementations over to use xdrgen. Getting rid of linux/nfs.h
> and other such dependencies is a firm pre-requisite for that.

This might be useful content to put in the commit message, though not as
useful as providing the whole series (once it is ready) - then we could
see *why* there is that firm pre-requisite.

I think this current patch could only stand on its own if it removed the
#include of nfs.h from nfsd.h and added it just the those .c files which
still needed it.  That would make it more clear that progress was being
made.

Thanks,
NeilBrown

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[Chuck Lever]

On Mon, Dec 8, 2025, at 6:00 PM, NeilBrown wrote:
> On Tue, 09 Dec 2025, Chuck Lever wrote:
>> 
>> On Mon, Dec 8, 2025, at 4:35 PM, NeilBrown wrote:
>> > On Tue, 09 Dec 2025, Chuck Lever wrote:
>> >> From: Chuck Lever <chuck.lever@oracle.com>
>> >> 
>> >> In NFSD's pNFS flexfile layout implementation, struct pnfs_ff_layout
>> >> defines a struct nfs_fh field. This comes from <linux/nfs.h> :
>> >> 
>> >> > /*
>> >> >  * This is the kernel NFS client file handle representation
>> >> >  */
>> >> > #define NFS_MAXFHSIZE           128
>> >> > struct nfs_fh {
>> >> >         unsigned short          size;
>> >> >         unsigned char           data[NFS_MAXFHSIZE];
>> >> > };
>> >> 
>> >> But NFSD has an equivalent struct, knfsd_fh.
>> >> 
>> >> To reduce cross-subsystem header dependencies, avoid using a struct
>> >> defined by the kernel's NFS client implementation in NFSD's flexfile
>> >> layout implementation.
>> >
>> > linux/nfs.h appears to be mostly generic-nfs stuff, rather than being
>> > client specific.
>> > If this change allowed us to remove "#include <linux/nfs.h>" from nfsd
>> > code, then it would be a good change, but I don't see that it does.
>> 
>> This change is part of a longer series I'm working on that enables
>> the removal of linux/nfs.h from parts of the NFSD code base. The
>> other two spots that need attention are localio and server-to-server
>> copy. I'm not planning to do anything to localio.
>
> So why separate this patch out from the series?

Because the next part of the series focuses on improvements to the nfs_ssc.c
shim, and are thus quite independent from this simple change. I really didn’t
want to post a long string of patches, and breaking this work up a bit makes it
easier for folks to get their minds around.


>> > Now that "struct knfsd_fh" doesn't encode info about the fh format that
>> > knfsd uses, would it instead make sense to discard it and use "struct
>> > nfs_fh" throughout NFSD?
>> 
>> Based on the effort I've already put into dealing with just ssc, I
>> think that would be a monumental change, and I'm not convinced it's
>> worth the cost.
>> 
>> What's more, it would move in the wrong direction. Stapling the client
>> and server implementations together by using the same headers makes
>> the code more difficult to modify without touching both trees in a
>> rather invasive way.
>
> Are we planning to get rid of nfs2.h nfs3.h nfs4.h too?

Probably. The most troublesome header is uapi/linux/nfs.h, which
is included by fs/nfsd/nfsd.h. But the other three will conflict to
various degrees with the xdrgen-generated headers.


>> What I'm trying to do right at the moment is convert NFSD's NFSv2 and
>> NFSv3 implementations over to use xdrgen. Getting rid of linux/nfs.h
>> and other such dependencies is a firm pre-requisite for that.
>
> This might be useful content to put in the commit message, though not as
> useful as providing the whole series (once it is ready) - then we could
> see *why* there is that firm pre-requisite.

You’re overthinking this. There’s no reason not to make this change,
no matter the underlying rationale; but the patch description does
mention the header rat’s nest that motivated it.


> I think this current patch could only stand on its own if it removed the
> #include of nfs.h from nfsd.h and added it just the those .c files which
> still needed it.  That would make it more clear that progress was being
> made.

That’s something that could be done in a separate patch. This one
focuses on one simple change, as patches are supposed to do.


-- 
Chuck Lever

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[Chuck Lever]

On Mon, Dec 8, 2025, at 6:13 PM, Chuck Lever wrote:
> On Mon, Dec 8, 2025, at 6:00 PM, NeilBrown wrote:
>> On Tue, 09 Dec 2025, Chuck Lever wrote:
>>> On Mon, Dec 8, 2025, at 4:35 PM, NeilBrown wrote:
>>> > Now that "struct knfsd_fh" doesn't encode info about the fh format that
>>> > knfsd uses, would it instead make sense to discard it and use "struct
>>> > nfs_fh" throughout NFSD?
>>> 
>>> Based on the effort I've already put into dealing with just ssc, I
>>> think that would be a monumental change, and I'm not convinced it's
>>> worth the cost.
>>> 
>>> What's more, it would move in the wrong direction. Stapling the client
>>> and server implementations together by using the same headers makes
>>> the code more difficult to modify without touching both trees in a
>>> rather invasive way.

OK. If we want to go the other way (replace knfsd_fh with nfs_fh
throughout NFSD) then I'd rather keep only the struct and the
CRC hash function in a single header to avoid pulling in a lot
of junk that only a few consumers need.

However there is still the problem of how large to make the
structure's data field. Right now it's NFS4_FHSIZE, but that means
you have to somehow include the header that defines NFS4_FHSIZE,
and that pulls in a lot more stuff than we really need or want.
It's difficult to separate the file handle structure from the rest
of the protocol.

NFS3_FHSIZE, for instance, would be defined in both linux/nfs3.h
and include/linux/sunrpc/xdrgen/nfs3.h . linux/nfs3.h includes
uapi/linux/nfs3.h. And fs/nfsd/nfsd.h includes linux/nfs.h,
linux/nfs3.h, and linux/nfs4.h, so there's no escape from the
uapi headers. (I'm not sure why NFS uapi headers have the
protocol bits in them; shouldn't they have only the admin UI
APIs?).

I don't think any of this would be terribly obvious to reviewers
even if the series had more patches. You basically have to go
spelunking to discover it.

A different way to slice this would be to make the subsystem-
agnostic NFS file handle merely a size and pointer to a buffer
(like struct xdr_netobj). Then both the client and server
implementations can use either static arrays or dynamically
allocated buffers, and they can get their maximum size
constants from wherever they like.

-- 
Chuck Lever

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[Jeff Layton]

On Mon, 2025-12-08 at 20:04 -0500, Chuck Lever wrote:
> 
> On Mon, Dec 8, 2025, at 6:13 PM, Chuck Lever wrote:
> > On Mon, Dec 8, 2025, at 6:00 PM, NeilBrown wrote:
> > > On Tue, 09 Dec 2025, Chuck Lever wrote:
> > > > On Mon, Dec 8, 2025, at 4:35 PM, NeilBrown wrote:
> > > > > Now that "struct knfsd_fh" doesn't encode info about the fh format that
> > > > > knfsd uses, would it instead make sense to discard it and use "struct
> > > > > nfs_fh" throughout NFSD?
> > > > 
> > > > Based on the effort I've already put into dealing with just ssc, I
> > > > think that would be a monumental change, and I'm not convinced it's
> > > > worth the cost.
> > > > 
> > > > What's more, it would move in the wrong direction. Stapling the client
> > > > and server implementations together by using the same headers makes
> > > > the code more difficult to modify without touching both trees in a
> > > > rather invasive way.
> 
> OK. If we want to go the other way (replace knfsd_fh with nfs_fh
> throughout NFSD) then I'd rather keep only the struct and the
> CRC hash function in a single header to avoid pulling in a lot
> of junk that only a few consumers need.
> 
> However there is still the problem of how large to make the
> structure's data field. Right now it's NFS4_FHSIZE, but that means
> you have to somehow include the header that defines NFS4_FHSIZE,
> and that pulls in a lot more stuff than we really need or want.
> It's difficult to separate the file handle structure from the rest
> of the protocol.
> 
> NFS3_FHSIZE, for instance, would be defined in both linux/nfs3.h
> and include/linux/sunrpc/xdrgen/nfs3.h . linux/nfs3.h includes
> uapi/linux/nfs3.h. And fs/nfsd/nfsd.h includes linux/nfs.h,
> linux/nfs3.h, and linux/nfs4.h, so there's no escape from the
> uapi headers. (I'm not sure why NFS uapi headers have the
> protocol bits in them; shouldn't they have only the admin UI
> APIs?).
> 

Amen. I too have wondered why we have so much stuff in the NFS uapi
headers. It would be good to just expose what we really need here.

I suspect that the primary consumer of these headers is nfs-utils.
Maybe we can try to strip out everything except what it needs?

> I don't think any of this would be terribly obvious to reviewers
> even if the series had more patches. You basically have to go
> spelunking to discover it.
> 
> A different way to slice this would be to make the subsystem-
> agnostic NFS file handle merely a size and pointer to a buffer
> (like struct xdr_netobj). Then both the client and server
> implementations can use either static arrays or dynamically
> allocated buffers, and they can get their maximum size
> constants from wherever they like.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[NeilBrown]

On Tue, 09 Dec 2025, Chuck Lever wrote:
> 
> On Mon, Dec 8, 2025, at 6:13 PM, Chuck Lever wrote:
> > On Mon, Dec 8, 2025, at 6:00 PM, NeilBrown wrote:
> >> On Tue, 09 Dec 2025, Chuck Lever wrote:
> >>> On Mon, Dec 8, 2025, at 4:35 PM, NeilBrown wrote:
> >>> > Now that "struct knfsd_fh" doesn't encode info about the fh format that
> >>> > knfsd uses, would it instead make sense to discard it and use "struct
> >>> > nfs_fh" throughout NFSD?
> >>> 
> >>> Based on the effort I've already put into dealing with just ssc, I
> >>> think that would be a monumental change, and I'm not convinced it's
> >>> worth the cost.
> >>> 
> >>> What's more, it would move in the wrong direction. Stapling the client
> >>> and server implementations together by using the same headers makes
> >>> the code more difficult to modify without touching both trees in a
> >>> rather invasive way.
> 
> OK. If we want to go the other way (replace knfsd_fh with nfs_fh
> throughout NFSD) then I'd rather keep only the struct and the
> CRC hash function in a single header to avoid pulling in a lot
> of junk that only a few consumers need.

This thing that I keep thinking of the is "enum nfs_stat".  I don't
think we want two copies of that in the kernel.  It is currently in
uapi/nfs.h and so in nfs.h

> 
> However there is still the problem of how large to make the
> structure's data field. Right now it's NFS4_FHSIZE, but that means
> you have to somehow include the header that defines NFS4_FHSIZE,
> and that pulls in a lot more stuff than we really need or want.
> It's difficult to separate the file handle structure from the rest
> of the protocol.

"128".
I don't think this *needs* to be NFS4_FHSIZE (though it will have the
same value).

#define NFS_MAXFHSIZE		128

works for me.

> 
> NFS3_FHSIZE, for instance, would be defined in both linux/nfs3.h
> and include/linux/sunrpc/xdrgen/nfs3.h .

Ok, I think this might be getting close to a central issue.
I think you want the .x file to become the source for all the various
constants and types with the generated .h files included where needed
and, significantly, any existing .h files which define the same name NOT
included in those places.

That sounds like a good long-term goal, but it isn't clear to me that we
want to jump straight there.

In the first instance, I think the main value of generating code from
xdr is the code - not the declarations.

What barriers are there to NOT using the .h files generated by xdrgen?

 linux/nfs3.h includes
> uapi/linux/nfs3.h. And fs/nfsd/nfsd.h includes linux/nfs.h,
> linux/nfs3.h, and linux/nfs4.h, so there's no escape from the
> uapi headers. (I'm not sure why NFS uapi headers have the
> protocol bits in them; shouldn't they have only the admin UI
> APIs?).
> 
> I don't think any of this would be terribly obvious to reviewers
> even if the series had more patches. You basically have to go
> spelunking to discover it.
> 
> A different way to slice this would be to make the subsystem-
> agnostic NFS file handle merely a size and pointer to a buffer
> (like struct xdr_netobj). Then both the client and server
> implementations can use either static arrays or dynamically
> allocated buffers, and they can get their maximum size
> constants from wherever they like.

I don't think the extra indirection of using the xdr_netobj approach for
filehandle is a good idea.

Thanks,
NeilBrown

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[Chuck Lever]

On Tue, Dec 9, 2025, at 4:42 PM, NeilBrown wrote:
> On Tue, 09 Dec 2025, Chuck Lever wrote:
>> 
>> On Mon, Dec 8, 2025, at 6:13 PM, Chuck Lever wrote:
>> > On Mon, Dec 8, 2025, at 6:00 PM, NeilBrown wrote:
>> >> On Tue, 09 Dec 2025, Chuck Lever wrote:
>> >>> On Mon, Dec 8, 2025, at 4:35 PM, NeilBrown wrote:
>> >>> > Now that "struct knfsd_fh" doesn't encode info about the fh format that
>> >>> > knfsd uses, would it instead make sense to discard it and use "struct
>> >>> > nfs_fh" throughout NFSD?
>> >>> 
>> >>> Based on the effort I've already put into dealing with just ssc, I
>> >>> think that would be a monumental change, and I'm not convinced it's
>> >>> worth the cost.
>> >>> 
>> >>> What's more, it would move in the wrong direction. Stapling the client
>> >>> and server implementations together by using the same headers makes
>> >>> the code more difficult to modify without touching both trees in a
>> >>> rather invasive way.
>> 
>> OK. If we want to go the other way (replace knfsd_fh with nfs_fh
>> throughout NFSD) then I'd rather keep only the struct and the
>> CRC hash function in a single header to avoid pulling in a lot
>> of junk that only a few consumers need.
>
> This thing that I keep thinking of the is "enum nfs_stat".  I don't
> think we want two copies of that in the kernel.  It is currently in
> uapi/nfs.h and so in nfs.h

Yes, and fs/nfsd/nfsd.h defines a bunch of little-endian
equivalents too. There are already multiple copies. Eventually
there should be Only One (tm), and I'd like that one to be
defined directly by the protocol specification.


>> However there is still the problem of how large to make the
>> structure's data field. Right now it's NFS4_FHSIZE, but that means
>> you have to somehow include the header that defines NFS4_FHSIZE,
>> and that pulls in a lot more stuff than we really need or want.
>> It's difficult to separate the file handle structure from the rest
>> of the protocol.
>
> "128".
> I don't think this *needs* to be NFS4_FHSIZE (though it will have the
> same value).
>
> #define NFS_MAXFHSIZE		128
>
> works for me.

Note that lockd uses "struct nfs_fh" as well, but in this case, the
128 bytes is overkill because NLM uses NFSv2 and NFSv3 file handles
only, which are 64 bytes at most.

I might be inclined to define an nlm_fh for this purpose.


>> NFS3_FHSIZE, for instance, would be defined in both linux/nfs3.h
>> and include/linux/sunrpc/xdrgen/nfs3.h .
>
> Ok, I think this might be getting close to a central issue.
> I think you want the .x file to become the source for all the various
> constants and types with the generated .h files included where needed
> and, significantly, any existing .h files which define the same name NOT
> included in those places.
>
> That sounds like a good long-term goal, but it isn't clear to me that we
> want to jump straight there.
>
> In the first instance, I think the main value of generating code from
> xdr is the code - not the declarations.

I don't agree with that. The .h files contain:

- Protocol definitions, using the same names as the RFC
- Function declarations for encoder and decoder functions
- Size constants for each on-the-wire data item for buffer
  length computations

IMO the .h files add critical value to the generated code. It's
boilerplate code that is straightforward to machine-generate in
either a C flavor or a Rust flavor, and human coders can get these
wrong.


> What barriers are there to NOT using the .h files generated by xdrgen?

A significant portion of what is in the human-generated .h files
would be dead code. We still need to include .h files that define
the XDR arguments and results structs, and they would need to be
based on the human-generated protocol headers.

That seems pretty messy.


>  linux/nfs3.h includes
>> uapi/linux/nfs3.h. And fs/nfsd/nfsd.h includes linux/nfs.h,
>> linux/nfs3.h, and linux/nfs4.h, so there's no escape from the
>> uapi headers. (I'm not sure why NFS uapi headers have the
>> protocol bits in them; shouldn't they have only the admin UI
>> APIs?).
>> 
>> I don't think any of this would be terribly obvious to reviewers
>> even if the series had more patches. You basically have to go
>> spelunking to discover it.
>> 
>> A different way to slice this would be to make the subsystem-
>> agnostic NFS file handle merely a size and pointer to a buffer
>> (like struct xdr_netobj). Then both the client and server
>> implementations can use either static arrays or dynamically
>> allocated buffers, and they can get their maximum size
>> constants from wherever they like.
>
> I don't think the extra indirection of using the xdr_netobj approach for
> filehandle is a good idea.

I'm talking about only the places where the client and server need
to communicate via local procedure call, like fs/nfs_common/nfs_ssc.c
or lockd. This would leave both sides to use their preferred storage
and internal API structures.

We have the same problem with nfs4_stateid versus stateid_t, and it
is also pretty convenient to shove those into an xdr_netobj to make
a function call between the client and server.


-- 
Chuck Lever

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[NeilBrown]

On Wed, 10 Dec 2025, Chuck Lever wrote:
> 
> On Tue, Dec 9, 2025, at 4:42 PM, NeilBrown wrote:
> > On Tue, 09 Dec 2025, Chuck Lever wrote:
> >> 
> >> On Mon, Dec 8, 2025, at 6:13 PM, Chuck Lever wrote:
> >> > On Mon, Dec 8, 2025, at 6:00 PM, NeilBrown wrote:
> >> >> On Tue, 09 Dec 2025, Chuck Lever wrote:
> >> >>> On Mon, Dec 8, 2025, at 4:35 PM, NeilBrown wrote:
> >> >>> > Now that "struct knfsd_fh" doesn't encode info about the fh format that
> >> >>> > knfsd uses, would it instead make sense to discard it and use "struct
> >> >>> > nfs_fh" throughout NFSD?
> >> >>> 
> >> >>> Based on the effort I've already put into dealing with just ssc, I
> >> >>> think that would be a monumental change, and I'm not convinced it's
> >> >>> worth the cost.
> >> >>> 
> >> >>> What's more, it would move in the wrong direction. Stapling the client
> >> >>> and server implementations together by using the same headers makes
> >> >>> the code more difficult to modify without touching both trees in a
> >> >>> rather invasive way.
> >> 
> >> OK. If we want to go the other way (replace knfsd_fh with nfs_fh
> >> throughout NFSD) then I'd rather keep only the struct and the
> >> CRC hash function in a single header to avoid pulling in a lot
> >> of junk that only a few consumers need.
> >
> > This thing that I keep thinking of the is "enum nfs_stat".  I don't
> > think we want two copies of that in the kernel.  It is currently in
> > uapi/nfs.h and so in nfs.h
> 
> Yes, and fs/nfsd/nfsd.h defines a bunch of little-endian
> equivalents too. There are already multiple copies. Eventually
> there should be Only One (tm), and I'd like that one to be
> defined directly by the protocol specification.
> 
> 
> >> However there is still the problem of how large to make the
> >> structure's data field. Right now it's NFS4_FHSIZE, but that means
> >> you have to somehow include the header that defines NFS4_FHSIZE,
> >> and that pulls in a lot more stuff than we really need or want.
> >> It's difficult to separate the file handle structure from the rest
> >> of the protocol.
> >
> > "128".
> > I don't think this *needs* to be NFS4_FHSIZE (though it will have the
> > same value).
> >
> > #define NFS_MAXFHSIZE		128
> >
> > works for me.
> 
> Note that lockd uses "struct nfs_fh" as well, but in this case, the
> 128 bytes is overkill because NLM uses NFSv2 and NFSv3 file handles
> only, which are 64 bytes at most.
> 
> I might be inclined to define an nlm_fh for this purpose.
> 
> 
> >> NFS3_FHSIZE, for instance, would be defined in both linux/nfs3.h
> >> and include/linux/sunrpc/xdrgen/nfs3.h .
> >
> > Ok, I think this might be getting close to a central issue.
> > I think you want the .x file to become the source for all the various
> > constants and types with the generated .h files included where needed
> > and, significantly, any existing .h files which define the same name NOT
> > included in those places.
> >
> > That sounds like a good long-term goal, but it isn't clear to me that we
> > want to jump straight there.
> >
> > In the first instance, I think the main value of generating code from
> > xdr is the code - not the declarations.
> 
> I don't agree with that. The .h files contain:
> 
> - Protocol definitions, using the same names as the RFC
> - Function declarations for encoder and decoder functions
> - Size constants for each on-the-wire data item for buffer
>   length computations
> 
> IMO the .h files add critical value to the generated code. It's
> boilerplate code that is straightforward to machine-generate in
> either a C flavor or a Rust flavor, and human coders can get these
> wrong.
> 
> 
> > What barriers are there to NOT using the .h files generated by xdrgen?
> 
> A significant portion of what is in the human-generated .h files
> would be dead code. We still need to include .h files that define
> the XDR arguments and results structs, and they would need to be
> based on the human-generated protocol headers.
> 
> That seems pretty messy.
> 
> 
> >  linux/nfs3.h includes
> >> uapi/linux/nfs3.h. And fs/nfsd/nfsd.h includes linux/nfs.h,
> >> linux/nfs3.h, and linux/nfs4.h, so there's no escape from the
> >> uapi headers. (I'm not sure why NFS uapi headers have the
> >> protocol bits in them; shouldn't they have only the admin UI
> >> APIs?).
> >> 
> >> I don't think any of this would be terribly obvious to reviewers
> >> even if the series had more patches. You basically have to go
> >> spelunking to discover it.
> >> 
> >> A different way to slice this would be to make the subsystem-
> >> agnostic NFS file handle merely a size and pointer to a buffer
> >> (like struct xdr_netobj). Then both the client and server
> >> implementations can use either static arrays or dynamically
> >> allocated buffers, and they can get their maximum size
> >> constants from wherever they like.
> >
> > I don't think the extra indirection of using the xdr_netobj approach for
> > filehandle is a good idea.
> 
> I'm talking about only the places where the client and server need
> to communicate via local procedure call, like fs/nfs_common/nfs_ssc.c
> or lockd. This would leave both sides to use their preferred storage
> and internal API structures.
> 
> We have the same problem with nfs4_stateid versus stateid_t, and it
> is also pretty convenient to shove those into an xdr_netobj to make
> a function call between the client and server.
> 

This all brings me back to something I said earlier.

I don't think I can properly review this patch without the context of
the follow-on work which appears to be the primary justification.

Thanks,
NeilBrown

Re: [PATCH v1] NFSD: Use struct knfsd_fh in struct pnfs_ff_layout

[Chuck Lever]

On Tue, Dec 9, 2025, at 4:42 PM, NeilBrown wrote:
> On Tue, 09 Dec 2025, Chuck Lever wrote:
>
>> NFS3_FHSIZE, for instance, would be defined in both linux/nfs3.h
>> and include/linux/sunrpc/xdrgen/nfs3.h .
>
> Ok, I think this might be getting close to a central issue.
> I think you want the .x file to become the source for all the various
> constants and types with the generated .h files included where needed
> and, significantly, any existing .h files which define the same name NOT
> included in those places.

Yes, that's the path I'm on. The largest confounding factor is the
headers that are shared between client and server implementations.


> That sounds like a good long-term goal, but it isn't clear to me that we
> want to jump straight there.

I don't intend to jump straight there... but if you want to see long
chains of patches that motivate some of these changes, some jumping
will have to occur (even if only as RFC).

Jumping (ie, flag days) might be necessary when replacing NFSv2
components because these are used throughout the implementation
of all three NFS versions. It's actually somewhat less complex for
NFSv3 and NFSv4.

-- 
Chuck Lever