[RFC PATCH 0/2] Remove union from struct knfsd_fh

[RFC PATCH 0/2] Remove union from struct knfsd_fh

[Chuck Lever]

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

This is a low-risk way to simplify the knfsd_fh struct, based on
Christoph's suggestion of using array indices rather than u8
fields.

Compile-tested only.

Chuck Lever (2):
  NFSD: Access a knfsd_fh's fsid by pointer
  NFSD: Simplify struct knfsd_fh

 fs/nfsd/nfs4layouts.c |  4 ++--
 fs/nfsd/nfsfh.c       | 16 +++++++++-------
 fs/nfsd/nfsfh.h       | 26 +++++++++++++++-----------
 3 files changed, 26 insertions(+), 20 deletions(-)

-- 
2.49.0

[RFC PATCH 1/2] NFSD: Access a knfsd_fh's fsid by pointer

[Chuck Lever]

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

I'm about to remove the union in struct knfsd_fh. First step is to
add an accessor function for the file handle's fsid portion.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4layouts.c |  4 ++--
 fs/nfsd/nfsfh.c       | 16 +++++++++-------
 fs/nfsd/nfsfh.h       | 11 +++++++++--
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 290271ac4245..aea905fcaf87 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -65,7 +65,7 @@ nfsd4_alloc_devid_map(const struct svc_fh *fhp)
 		return;
 
 	map->fsid_type = fh->fh_fsid_type;
-	memcpy(&map->fsid, fh->fh_fsid, fsid_len);
+	memcpy(&map->fsid, fh_fsid(fh), fsid_len);
 
 	spin_lock(&nfsd_devid_lock);
 	if (fhp->fh_export->ex_devid_map)
@@ -75,7 +75,7 @@ nfsd4_alloc_devid_map(const struct svc_fh *fhp)
 		list_for_each_entry(old, &nfsd_devid_hash[i], hash) {
 			if (old->fsid_type != fh->fh_fsid_type)
 				continue;
-			if (memcmp(old->fsid, fh->fh_fsid,
+			if (memcmp(old->fsid, fh_fsid(fh),
 					key_len(old->fsid_type)))
 				continue;
 
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index aef474f1b84b..4565112d0324 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -172,6 +172,8 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
 	if (len == 0)
 		return error;
 	if (fh->fh_fsid_type == FSID_MAJOR_MINOR) {
+		u32 *fsid = fh_fsid(fh);
+
 		/* deprecated, convert to type 3 */
 		len = key_len(FSID_ENCODE_DEV)/4;
 		fh->fh_fsid_type = FSID_ENCODE_DEV;
@@ -181,17 +183,17 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
 		 * confuses sparse, so we must use __force here to
 		 * keep it from complaining.
 		 */
-		fh->fh_fsid[0] = new_encode_dev(MKDEV(ntohl((__force __be32)fh->fh_fsid[0]),
-						      ntohl((__force __be32)fh->fh_fsid[1])));
-		fh->fh_fsid[1] = fh->fh_fsid[2];
+		fsid[0] = new_encode_dev(MKDEV(ntohl((__force __be32)fsid[0]),
+						      ntohl((__force __be32)fsid[1])));
+		fsid[1] = fsid[2];
 	}
 	data_left -= len;
 	if (data_left < 0)
 		return error;
 	exp = rqst_exp_find(rqstp ? &rqstp->rq_chandle : NULL,
 			    net, client, gssclient,
-			    fh->fh_fsid_type, fh->fh_fsid);
-	fid = (struct fid *)(fh->fh_fsid + len);
+			    fh->fh_fsid_type, fh_fsid(fh));
+	fid = (struct fid *)(fh_fsid(fh) + len);
 
 	error = nfserr_stale;
 	if (IS_ERR(exp)) {
@@ -463,7 +465,7 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp,
 {
 	if (dentry != exp->ex_path.dentry) {
 		struct fid *fid = (struct fid *)
-			(fhp->fh_handle.fh_fsid + fhp->fh_handle.fh_size/4 - 1);
+			(fh_fsid(&fhp->fh_handle) + fhp->fh_handle.fh_size/4 - 1);
 		int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4;
 		int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 :
 				EXPORT_FH_CONNECTABLE;
@@ -614,7 +616,7 @@ fh_compose(struct svc_fh *fhp, struct svc_export *exp, struct dentry *dentry,
 	fhp->fh_handle.fh_auth_type = 0;
 
 	mk_fsid(fhp->fh_handle.fh_fsid_type,
-		fhp->fh_handle.fh_fsid,
+		fh_fsid(&fhp->fh_handle),
 		ex_dev,
 		d_inode(exp->ex_path.dentry)->i_ino,
 		exp->ex_fsid, exp->ex_uuid);
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 760e77f3630b..4569b5950b55 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -56,11 +56,15 @@ struct knfsd_fh {
 			u8		fh_auth_type;	/* deprecated */
 			u8		fh_fsid_type;
 			u8		fh_fileid_type;
-			u32		fh_fsid[NFS4_FHSIZE / 4 - 1];
 		};
 	};
 };
 
+static inline u32 *fh_fsid(const struct knfsd_fh *fh)
+{
+	return (u32 *)&fh->fh_raw[4];
+}
+
 static inline __u32 ino_t_to_u32(ino_t ino)
 {
 	return (__u32) ino;
@@ -260,9 +264,12 @@ static inline bool fh_match(const struct knfsd_fh *fh1,
 static inline bool fh_fsid_match(const struct knfsd_fh *fh1,
 				 const struct knfsd_fh *fh2)
 {
+	u32 *fsid1 = fh_fsid(fh1);
+	u32 *fsid2 = fh_fsid(fh2);
+
 	if (fh1->fh_fsid_type != fh2->fh_fsid_type)
 		return false;
-	if (memcmp(fh1->fh_fsid, fh2->fh_fsid, key_len(fh1->fh_fsid_type)) != 0)
+	if (memcmp(fsid1, fsid2, key_len(fh1->fh_fsid_type)) != 0)
 		return false;
 	return true;
 }
-- 
2.49.0

Re: [RFC PATCH 1/2] NFSD: Access a knfsd_fh's fsid by pointer

[Christoph Hellwig]

On Fri, Jun 20, 2025 at 08:31:54AM -0400, Chuck Lever wrote:
> +		fsid[0] = new_encode_dev(MKDEV(ntohl((__force __be32)fsid[0]),
> +						      ntohl((__force __be32)fsid[1])));

Maybe avoid the overly long line by reducing the somewhat pointlessly
large indentation?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[RFC PATCH 2/2] NFSD: Simplify struct knfsd_fh

[Chuck Lever]

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

Compilers are allowed to insert padding and reorder the
fields in a struct, so using a union of an array and a
struct in struct knfsd_fh is not reliable.

The position of elements in an array is more reliable.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfsfh.h | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 4569b5950b55..1cf979722521 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -49,17 +49,14 @@ struct knfsd_fh {
 					 * Points to the current size while
 					 * building a new file handle.
 					 */
-	union {
-		char			fh_raw[NFS4_FHSIZE];
-		struct {
-			u8		fh_version;	/* == 1 */
-			u8		fh_auth_type;	/* deprecated */
-			u8		fh_fsid_type;
-			u8		fh_fileid_type;
-		};
-	};
+	u8		fh_raw[NFS4_FHSIZE];
 };
 
+#define fh_version		fh_raw[0]
+#define fh_auth_type		fh_raw[1]
+#define fh_fsid_type		fh_raw[2]
+#define fh_fileid_type		fh_raw[3]
+
 static inline u32 *fh_fsid(const struct knfsd_fh *fh)
 {
 	return (u32 *)&fh->fh_raw[4];
-- 
2.49.0

Re: [RFC PATCH 2/2] NFSD: Simplify struct knfsd_fh

[Christoph Hellwig]

> +#define fh_version		fh_raw[0]
> +#define fh_auth_type		fh_raw[1]
> +#define fh_fsid_type		fh_raw[2]
> +#define fh_fileid_type		fh_raw[3]

I guess that works, even if I find struct field defines a bit confusing.

Either way it is better than what we had before, so:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [RFC PATCH 0/2] Remove union from struct knfsd_fh

[Jeff Layton]

On Fri, 2025-06-20 at 08:31 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> This is a low-risk way to simplify the knfsd_fh struct, based on
> Christoph's suggestion of using array indices rather than u8
> fields.
> 
> Compile-tested only.
> 
> Chuck Lever (2):
>   NFSD: Access a knfsd_fh's fsid by pointer
>   NFSD: Simplify struct knfsd_fh
> 
>  fs/nfsd/nfs4layouts.c |  4 ++--
>  fs/nfsd/nfsfh.c       | 16 +++++++++-------
>  fs/nfsd/nfsfh.h       | 26 +++++++++++++++-----------
>  3 files changed, 26 insertions(+), 20 deletions(-)


This all looks pretty sane to me. Overall, just a simpler way to define
this struct.

Reviewed-by: Jeff Layton <jlayton@kernel.org>