[PATCH RFC 0/4] Encode NFSv4 attributes via a branch table

[PATCH RFC 0/4] Encode NFSv4 attributes via a branch table

[Chuck Lever]

Here's something just for fun. I've converted nfsd4_encode_fattr4()
to use a bitmask loop, calling an encode helper for each attribute
to be encoded. Rotten tomatoes and gold stars are both acceptible.

I was hoping for a bit of a performance gain because encode_fattr4
is called so very often, but there was not much difference at all.

The main benefit here is the hard scope boundary for each of the
separate attribute encoders -- that makes for safer code that is
easier to reason about, and might even be more straightforward to
convert to machine-generated code, if we ever want to do that.

And notably it will automatically encode the attributes in bitmask
order.

There are a few readability improvements that could be done, like
defining meaningfully-named macros for the bit positions. The ones
we have now are not directly usable for table indices. It might get
us another step closer to the XDR specification if we could find a
way to encode the whole bitmask in a single loop.

---

Chuck Lever (4):
      NFSD: Add struct nfsd4_fattr_args
      NFSD: Encode attributes in WORD0 using a bitmask loop
      NFSD: Encode attributes in WORD1 using a bitmask loop
      NFSD: Encode attributes in WORD2 using a bitmask loop


 fs/nfsd/nfs4xdr.c | 1089 +++++++++++++++++++++++++++------------------
 1 file changed, 657 insertions(+), 432 deletions(-)

--
Chuck Lever

[PATCH RFC 1/4] NFSD: Add struct nfsd4_fattr_args

[Chuck Lever]

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

I'm about to split nfsd4_encode_fattr() into a number of smaller
functions. Instead of passing a large number of arguments to each
of the smaller functions, create a struct that can gather the
common argument variables into something with a handle on it.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c |  111 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 62 insertions(+), 49 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 26b1343c8035..a22a6015afe5 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2938,6 +2938,15 @@ nfsd4_encode_bitmap(struct xdr_stream *xdr, u32 bmval0, u32 bmval1, u32 bmval2)
 	return nfserr_resource;
 }
 
+struct nfsd4_fattr_args {
+	struct svc_fh		*fhp;
+	struct kstat		stat;
+	struct kstatfs		statfs;
+	struct nfs4_acl		*acl;
+	u32			rdattr_err;
+	bool			contextsupport;
+};
+
 /*
  * Note: @fhp can be NULL; in this case, we might have to compose the filehandle
  * ourselves.
@@ -2948,26 +2957,22 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		struct dentry *dentry, u32 *bmval,
 		struct svc_rqst *rqstp, int ignore_crossmnt)
 {
+	struct nfsd4_fattr_args args;
 	u32 bmval0 = bmval[0];
 	u32 bmval1 = bmval[1];
 	u32 bmval2 = bmval[2];
-	struct kstat stat;
 	struct svc_fh *tempfh = NULL;
-	struct kstatfs statfs;
 	__be32 *p, *attrlen_p;
 	int starting_len = xdr->buf->len;
 	int attrlen_offset;
 	u32 dummy;
 	u64 dummy64;
-	u32 rdattr_err = 0;
 	__be32 status;
 	int err;
-	struct nfs4_acl *acl = NULL;
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
 	void *context = NULL;
 	int contextlen;
 #endif
-	bool contextsupport = false;
 	struct nfsd4_compoundres *resp = rqstp->rq_resp;
 	u32 minorversion = resp->cstate.minorversion;
 	struct path path = {
@@ -2979,25 +2984,28 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 	BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
 	BUG_ON(!nfsd_attrs_supported(minorversion, bmval));
 
+	args.rdattr_err = 0;
 	if (exp->ex_fslocs.migrated) {
-		status = fattr_handle_absent_fs(&bmval0, &bmval1, &bmval2, &rdattr_err);
+		status = fattr_handle_absent_fs(&bmval0, &bmval1, &bmval2,
+						&args.rdattr_err);
 		if (status)
 			goto out;
 	}
 
-	err = vfs_getattr(&path, &stat,
+	err = vfs_getattr(&path, &args.stat,
 			  STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
 			  AT_STATX_SYNC_AS_STAT);
 	if (err)
 		goto out_nfserr;
-	if (!(stat.result_mask & STATX_BTIME))
+
+	if (!(args.stat.result_mask & STATX_BTIME))
 		/* underlying FS does not offer btime so we can't share it */
 		bmval1 &= ~FATTR4_WORD1_TIME_CREATE;
 	if ((bmval0 & (FATTR4_WORD0_FILES_AVAIL | FATTR4_WORD0_FILES_FREE |
 			FATTR4_WORD0_FILES_TOTAL | FATTR4_WORD0_MAXNAME)) ||
 	    (bmval1 & (FATTR4_WORD1_SPACE_AVAIL | FATTR4_WORD1_SPACE_FREE |
 		       FATTR4_WORD1_SPACE_TOTAL))) {
-		err = vfs_statfs(&path, &statfs);
+		err = vfs_statfs(&path, &args.statfs);
 		if (err)
 			goto out_nfserr;
 	}
@@ -3010,10 +3018,13 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		status = fh_compose(tempfh, exp, dentry, NULL);
 		if (status)
 			goto out;
-		fhp = tempfh;
-	}
+		args.fhp = tempfh;
+	} else
+		args.fhp = fhp;
+
+	args.acl = NULL;
 	if (bmval0 & FATTR4_WORD0_ACL) {
-		err = nfsd4_get_nfs4_acl(rqstp, dentry, &acl);
+		err = nfsd4_get_nfs4_acl(rqstp, dentry, &args.acl);
 		if (err == -EOPNOTSUPP)
 			bmval0 &= ~FATTR4_WORD0_ACL;
 		else if (err == -EINVAL) {
@@ -3023,6 +3034,8 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 			goto out_nfserr;
 	}
 
+	args.contextsupport = false;
+
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
 	if ((bmval2 & FATTR4_WORD2_SECURITY_LABEL) ||
 	     bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
@@ -3031,7 +3044,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 						&context, &contextlen);
 		else
 			err = -EOPNOTSUPP;
-		contextsupport = (err == 0);
+		args.contextsupport = (err == 0);
 		if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) {
 			if (err == -EOPNOTSUPP)
 				bmval2 &= ~FATTR4_WORD2_SECURITY_LABEL;
@@ -3057,7 +3070,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 
 		if (!IS_POSIXACL(dentry->d_inode))
 			supp[0] &= ~FATTR4_WORD0_ACL;
-		if (!contextsupport)
+		if (!args.contextsupport)
 			supp[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
 		if (!supp[2]) {
 			p = xdr_reserve_space(xdr, 12);
@@ -3080,7 +3093,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = xdr_reserve_space(xdr, 4);
 		if (!p)
 			goto out_resource;
-		dummy = nfs4_file_type(stat.mode);
+		dummy = nfs4_file_type(args.stat.mode);
 		if (dummy == NF4BAD) {
 			status = nfserr_serverfault;
 			goto out;
@@ -3101,13 +3114,13 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		p = encode_change(p, &stat, d_inode(dentry), exp);
+		p = encode_change(p, &args.stat, d_inode(dentry), exp);
 	}
 	if (bmval0 & FATTR4_WORD0_SIZE) {
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		p = xdr_encode_hyper(p, stat.size);
+		p = xdr_encode_hyper(p, args.stat.size);
 	}
 	if (bmval0 & FATTR4_WORD0_LINK_SUPPORT) {
 		p = xdr_reserve_space(xdr, 4);
@@ -3134,16 +3147,16 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		if (exp->ex_fslocs.migrated) {
 			p = xdr_encode_hyper(p, NFS4_REFERRAL_FSID_MAJOR);
 			p = xdr_encode_hyper(p, NFS4_REFERRAL_FSID_MINOR);
-		} else switch(fsid_source(fhp)) {
+		} else switch(fsid_source(args.fhp)) {
 		case FSIDSOURCE_FSID:
 			p = xdr_encode_hyper(p, (u64)exp->ex_fsid);
 			p = xdr_encode_hyper(p, (u64)0);
 			break;
 		case FSIDSOURCE_DEV:
 			*p++ = cpu_to_be32(0);
-			*p++ = cpu_to_be32(MAJOR(stat.dev));
+			*p++ = cpu_to_be32(MAJOR(args.stat.dev));
 			*p++ = cpu_to_be32(0);
-			*p++ = cpu_to_be32(MINOR(stat.dev));
+			*p++ = cpu_to_be32(MINOR(args.stat.dev));
 			break;
 		case FSIDSOURCE_UUID:
 			p = xdr_encode_opaque_fixed(p, exp->ex_uuid,
@@ -3167,12 +3180,12 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = xdr_reserve_space(xdr, 4);
 		if (!p)
 			goto out_resource;
-		*p++ = cpu_to_be32(rdattr_err);
+		*p++ = cpu_to_be32(args.rdattr_err);
 	}
 	if (bmval0 & FATTR4_WORD0_ACL) {
 		struct nfs4_ace *ace;
 
-		if (acl == NULL) {
+		if (args.acl == NULL) {
 			p = xdr_reserve_space(xdr, 4);
 			if (!p)
 				goto out_resource;
@@ -3183,9 +3196,9 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = xdr_reserve_space(xdr, 4);
 		if (!p)
 			goto out_resource;
-		*p++ = cpu_to_be32(acl->naces);
+		*p++ = cpu_to_be32(args.acl->naces);
 
-		for (ace = acl->aces; ace < acl->aces + acl->naces; ace++) {
+		for (ace = args.acl->aces; ace < args.acl->aces + args.acl->naces; ace++) {
 			p = xdr_reserve_space(xdr, 4*3);
 			if (!p)
 				goto out_resource;
@@ -3231,35 +3244,35 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		*p++ = cpu_to_be32(1);
 	}
 	if (bmval0 & FATTR4_WORD0_FILEHANDLE) {
-		p = xdr_reserve_space(xdr, fhp->fh_handle.fh_size + 4);
+		p = xdr_reserve_space(xdr, args.fhp->fh_handle.fh_size + 4);
 		if (!p)
 			goto out_resource;
-		p = xdr_encode_opaque(p, &fhp->fh_handle.fh_raw,
-					fhp->fh_handle.fh_size);
+		p = xdr_encode_opaque(p, &args.fhp->fh_handle.fh_raw,
+					args.fhp->fh_handle.fh_size);
 	}
 	if (bmval0 & FATTR4_WORD0_FILEID) {
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		p = xdr_encode_hyper(p, stat.ino);
+		p = xdr_encode_hyper(p, args.stat.ino);
 	}
 	if (bmval0 & FATTR4_WORD0_FILES_AVAIL) {
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		p = xdr_encode_hyper(p, (u64) statfs.f_ffree);
+		p = xdr_encode_hyper(p, (u64) args.statfs.f_ffree);
 	}
 	if (bmval0 & FATTR4_WORD0_FILES_FREE) {
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		p = xdr_encode_hyper(p, (u64) statfs.f_ffree);
+		p = xdr_encode_hyper(p, (u64) args.statfs.f_ffree);
 	}
 	if (bmval0 & FATTR4_WORD0_FILES_TOTAL) {
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		p = xdr_encode_hyper(p, (u64) statfs.f_files);
+		p = xdr_encode_hyper(p, (u64) args.statfs.f_files);
 	}
 	if (bmval0 & FATTR4_WORD0_FS_LOCATIONS) {
 		status = nfsd4_encode_fs_locations(xdr, rqstp, exp);
@@ -3288,7 +3301,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = xdr_reserve_space(xdr, 4);
 		if (!p)
 			goto out_resource;
-		*p++ = cpu_to_be32(statfs.f_namelen);
+		*p++ = cpu_to_be32(args.statfs.f_namelen);
 	}
 	if (bmval0 & FATTR4_WORD0_MAXREAD) {
 		p = xdr_reserve_space(xdr, 8);
@@ -3306,7 +3319,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = xdr_reserve_space(xdr, 4);
 		if (!p)
 			goto out_resource;
-		*p++ = cpu_to_be32(stat.mode & S_IALLUGO);
+		*p++ = cpu_to_be32(args.stat.mode & S_IALLUGO);
 	}
 	if (bmval1 & FATTR4_WORD1_NO_TRUNC) {
 		p = xdr_reserve_space(xdr, 4);
@@ -3318,15 +3331,15 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = xdr_reserve_space(xdr, 4);
 		if (!p)
 			goto out_resource;
-		*p++ = cpu_to_be32(stat.nlink);
+		*p++ = cpu_to_be32(args.stat.nlink);
 	}
 	if (bmval1 & FATTR4_WORD1_OWNER) {
-		status = nfsd4_encode_user(xdr, rqstp, stat.uid);
+		status = nfsd4_encode_user(xdr, rqstp, args.stat.uid);
 		if (status)
 			goto out;
 	}
 	if (bmval1 & FATTR4_WORD1_OWNER_GROUP) {
-		status = nfsd4_encode_group(xdr, rqstp, stat.gid);
+		status = nfsd4_encode_group(xdr, rqstp, args.stat.gid);
 		if (status)
 			goto out;
 	}
@@ -3334,39 +3347,39 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		*p++ = cpu_to_be32((u32) MAJOR(stat.rdev));
-		*p++ = cpu_to_be32((u32) MINOR(stat.rdev));
+		*p++ = cpu_to_be32((u32) MAJOR(args.stat.rdev));
+		*p++ = cpu_to_be32((u32) MINOR(args.stat.rdev));
 	}
 	if (bmval1 & FATTR4_WORD1_SPACE_AVAIL) {
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		dummy64 = (u64)statfs.f_bavail * (u64)statfs.f_bsize;
+		dummy64 = (u64)args.statfs.f_bavail * (u64)args.statfs.f_bsize;
 		p = xdr_encode_hyper(p, dummy64);
 	}
 	if (bmval1 & FATTR4_WORD1_SPACE_FREE) {
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		dummy64 = (u64)statfs.f_bfree * (u64)statfs.f_bsize;
+		dummy64 = (u64)args.statfs.f_bfree * (u64)args.statfs.f_bsize;
 		p = xdr_encode_hyper(p, dummy64);
 	}
 	if (bmval1 & FATTR4_WORD1_SPACE_TOTAL) {
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		dummy64 = (u64)statfs.f_blocks * (u64)statfs.f_bsize;
+		dummy64 = (u64)args.statfs.f_blocks * (u64)args.statfs.f_bsize;
 		p = xdr_encode_hyper(p, dummy64);
 	}
 	if (bmval1 & FATTR4_WORD1_SPACE_USED) {
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
 			goto out_resource;
-		dummy64 = (u64)stat.blocks << 9;
+		dummy64 = (u64)args.stat.blocks << 9;
 		p = xdr_encode_hyper(p, dummy64);
 	}
 	if (bmval1 & FATTR4_WORD1_TIME_ACCESS) {
-		status = nfsd4_encode_nfstime4(xdr, &stat.atime);
+		status = nfsd4_encode_nfstime4(xdr, &args.stat.atime);
 		if (status)
 			goto out;
 	}
@@ -3377,22 +3390,22 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = encode_time_delta(p, d_inode(dentry));
 	}
 	if (bmval1 & FATTR4_WORD1_TIME_METADATA) {
-		status = nfsd4_encode_nfstime4(xdr, &stat.ctime);
+		status = nfsd4_encode_nfstime4(xdr, &args.stat.ctime);
 		if (status)
 			goto out;
 	}
 	if (bmval1 & FATTR4_WORD1_TIME_MODIFY) {
-		status = nfsd4_encode_nfstime4(xdr, &stat.mtime);
+		status = nfsd4_encode_nfstime4(xdr, &args.stat.mtime);
 		if (status)
 			goto out;
 	}
 	if (bmval1 & FATTR4_WORD1_TIME_CREATE) {
-		status = nfsd4_encode_nfstime4(xdr, &stat.btime);
+		status = nfsd4_encode_nfstime4(xdr, &args.stat.btime);
 		if (status)
 			goto out;
 	}
 	if (bmval1 & FATTR4_WORD1_MOUNTED_ON_FILEID) {
-		u64 ino = stat.ino;
+		u64 ino = args.stat.ino;
 
 		p = xdr_reserve_space(xdr, 8);
 		if (!p)
@@ -3427,7 +3440,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		p = xdr_reserve_space(xdr, 4);
 		if (!p)
 			goto out_resource;
-		*p++ = cpu_to_be32(stat.blksize);
+		*p++ = cpu_to_be32(args.stat.blksize);
 	}
 #endif /* CONFIG_NFSD_PNFS */
 	if (bmval2 & FATTR4_WORD2_SUPPATTR_EXCLCREAT) {
@@ -3468,7 +3481,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 	if (context)
 		security_release_secctx(context, contextlen);
 #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
-	kfree(acl);
+	kfree(args.acl);
 	if (tempfh) {
 		fh_put(tempfh);
 		kfree(tempfh);

[PATCH RFC 2/4] NFSD: Encode attributes in WORD0 using a bitmask loop

[Chuck Lever]

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

Replace the current implementation with a branch table. This creates
hard function scope boundaries, limiting side effects and reducing
instruction cache footprint (uncalled encoders remain out of the
cache).

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c |  598 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 341 insertions(+), 257 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a22a6015afe5..20e09e5510c9 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2939,7 +2939,10 @@ nfsd4_encode_bitmap(struct xdr_stream *xdr, u32 bmval0, u32 bmval1, u32 bmval2)
 }
 
 struct nfsd4_fattr_args {
+	struct svc_rqst		*rqstp;
 	struct svc_fh		*fhp;
+	struct svc_export	*exp;
+	struct dentry		*dentry;
 	struct kstat		stat;
 	struct kstatfs		statfs;
 	struct nfs4_acl		*acl;
@@ -2947,6 +2950,332 @@ struct nfsd4_fattr_args {
 	bool			contextsupport;
 };
 
+typedef __be32(*nfsd4_enc_attr)(struct xdr_stream *xdr,
+				struct nfsd4_fattr_args *args);
+
+static __be32 nfsd4_encode_fattr4__noop(struct xdr_stream *xdr,
+					struct nfsd4_fattr_args *args)
+{
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4__true(struct xdr_stream *xdr,
+					struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_bool(xdr, true) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4__false(struct xdr_stream *xdr,
+					 struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_bool(xdr, false) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_supported_attrs(struct xdr_stream *xdr,
+						  struct nfsd4_fattr_args *args)
+{
+	struct nfsd4_compoundres *resp = args->rqstp->rq_resp;
+	u32 minorversion = resp->cstate.minorversion;
+	u32 supp[3];
+
+	memcpy(supp, nfsd_suppattrs[minorversion], sizeof(supp));
+	if (!IS_POSIXACL(args->dentry->d_inode))
+		supp[0] &= ~FATTR4_WORD0_ACL;
+	if (!args->contextsupport)
+		supp[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
+
+	return nfsd4_encode_bitmap(xdr, supp[0], supp[1], supp[2]);
+}
+
+static __be32 nfsd4_encode_fattr4_type(struct xdr_stream *xdr,
+				       struct nfsd4_fattr_args *args)
+{
+	u32 type = nfs4_file_type(args->stat.mode);
+	__be32 *p;
+
+	if (type == NF4BAD)
+		return nfserr_serverfault;
+
+	p = xdr_reserve_space(xdr, XDR_UNIT);
+	if (!p)
+		return nfserr_resource;
+
+	*p = cpu_to_be32(type);
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_fh_expire_type(struct xdr_stream *xdr,
+						 struct nfsd4_fattr_args *args)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, XDR_UNIT);
+	if (!p)
+		return nfserr_resource;
+
+	if (args->exp->ex_flags & NFSEXP_NOSUBTREECHECK)
+		*p = cpu_to_be32(NFS4_FH_PERSISTENT);
+	else
+		*p = cpu_to_be32(NFS4_FH_PERSISTENT | NFS4_FH_VOL_RENAME);
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_change(struct xdr_stream *xdr,
+					 struct nfsd4_fattr_args *args)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, XDR_UNIT * 2);
+	if (!p)
+		return nfserr_resource;
+
+	encode_change(p, &args->stat, d_inode(args->dentry), args->exp);
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_size(struct xdr_stream *xdr,
+				       struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u64(xdr, args->stat.size) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_fsid(struct xdr_stream *xdr,
+				       struct nfsd4_fattr_args *args)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, XDR_UNIT * 4);
+	if (!p)
+		return nfserr_resource;
+
+	if (unlikely(args->exp->ex_fslocs.migrated)) {
+		p = xdr_encode_hyper(p, NFS4_REFERRAL_FSID_MAJOR);
+		xdr_encode_hyper(p, NFS4_REFERRAL_FSID_MINOR);
+		return nfs_ok;
+	}
+
+	switch (fsid_source(args->fhp)) {
+	case FSIDSOURCE_FSID:
+		p = xdr_encode_hyper(p, (u64)args->exp->ex_fsid);
+		xdr_encode_hyper(p, (u64)0);
+		break;
+	case FSIDSOURCE_DEV:
+		*p++ = cpu_to_be32(0);
+		*p++ = cpu_to_be32(MAJOR(args->stat.dev));
+		*p++ = cpu_to_be32(0);
+		*p   = cpu_to_be32(MINOR(args->stat.dev));
+		break;
+	case FSIDSOURCE_UUID:
+		xdr_encode_opaque_fixed(p, args->exp->ex_uuid, EX_UUID_LEN);
+		break;
+	}
+
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_lease_time(struct xdr_stream *xdr,
+					     struct nfsd4_fattr_args *args)
+{
+	struct nfsd_net *nn = net_generic(SVC_NET(args->rqstp), nfsd_net_id);
+
+	if (xdr_stream_encode_u32(xdr, nn->nfsd4_lease) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_rdattr_error(struct xdr_stream *xdr,
+					       struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u32(xdr, args->rdattr_err) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_acl(struct xdr_stream *xdr,
+				      struct nfsd4_fattr_args *args)
+{
+	struct nfs4_acl *acl = args->acl;
+	struct nfs4_ace *ace;
+	__be32 *p, status;
+
+	if (!acl) {
+		if (xdr_stream_encode_item_absent(xdr) < 0)
+			return nfserr_resource;
+		p = xdr_reserve_space(xdr, XDR_UNIT);
+		if (!p)
+			return nfserr_resource;
+		*p = cpu_to_be32(IS_POSIXACL(d_inode(args->dentry)) ?
+				 ACL4_SUPPORT_ALLOW_ACL | ACL4_SUPPORT_DENY_ACL : 0);
+		return nfs_ok;
+	}
+
+	if (xdr_stream_encode_u32(xdr, acl->naces) < 0)
+		return nfserr_resource;
+
+	for (ace = acl->aces; ace < acl->aces + acl->naces; ace++) {
+		p = xdr_reserve_space(xdr, XDR_UNIT * 3);
+		if (!p)
+			return nfserr_resource;
+
+		*p++ = cpu_to_be32(ace->type);
+		*p++ = cpu_to_be32(ace->flag);
+		*p   = cpu_to_be32(ace->access_mask & NFS4_ACE_MASK_ALL);
+		status = nfsd4_encode_aclname(xdr, args->rqstp, ace);
+		if (status)
+			return status;
+	}
+
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_aclsupport(struct xdr_stream *xdr,
+					     struct nfsd4_fattr_args *args)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, XDR_UNIT);
+	if (!p)
+		return nfserr_resource;
+
+	*p = cpu_to_be32(IS_POSIXACL(d_inode(args->dentry)) ?
+			 ACL4_SUPPORT_ALLOW_ACL | ACL4_SUPPORT_DENY_ACL : 0);
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_filehandle(struct xdr_stream *xdr,
+					     struct nfsd4_fattr_args *args)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, args->fhp->fh_handle.fh_size + 4);
+	if (!p)
+		return nfserr_resource;
+
+	xdr_encode_opaque(p, &args->fhp->fh_handle.fh_raw,
+			  args->fhp->fh_handle.fh_size);
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_fileid(struct xdr_stream *xdr,
+					 struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u64(xdr, args->stat.ino) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_files_avail(struct xdr_stream *xdr,
+					      struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u64(xdr, args->statfs.f_ffree) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_files_free(struct xdr_stream *xdr,
+					     struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u64(xdr, args->statfs.f_ffree) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_files_total(struct xdr_stream *xdr,
+					      struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u64(xdr, args->statfs.f_files) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_fs_locations(struct xdr_stream *xdr,
+					       struct nfsd4_fattr_args *args)
+{
+	return nfsd4_encode_fs_locations(xdr, args->rqstp, args->exp);
+}
+
+static __be32 nfsd4_encode_fattr4_maxfilesize(struct xdr_stream *xdr,
+					      struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u64(xdr,
+				  args->exp->ex_path.mnt->mnt_sb->s_maxbytes) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_maxlink(struct xdr_stream *xdr,
+					  struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u32(xdr, 255) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_maxname(struct xdr_stream *xdr,
+					  struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u32(xdr, args->statfs.f_namelen) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_maxread(struct xdr_stream *xdr,
+					  struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u64(xdr, svc_max_payload(args->rqstp)) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_maxwrite(struct xdr_stream *xdr,
+					   struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u64(xdr, svc_max_payload(args->rqstp)) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static const nfsd4_enc_attr nfsd4_enc_fattr4_word0_ops[] = {
+	[0]		= nfsd4_encode_fattr4_supported_attrs,
+	[1]		= nfsd4_encode_fattr4_type,
+	[2]		= nfsd4_encode_fattr4_fh_expire_type,
+	[3]		= nfsd4_encode_fattr4_change,
+	[4]		= nfsd4_encode_fattr4_size,
+	[5]		= nfsd4_encode_fattr4__true,	/* link support */
+	[6]		= nfsd4_encode_fattr4__true,	/* symlink support */
+	[7]		= nfsd4_encode_fattr4__false,	/* named attributes */
+	[8]		= nfsd4_encode_fattr4_fsid,
+	[9]		= nfsd4_encode_fattr4__true,	/* unique handles */
+	[10]		= nfsd4_encode_fattr4_lease_time,
+	[11]		= nfsd4_encode_fattr4_rdattr_error,
+	[12]		= nfsd4_encode_fattr4_acl,
+	[13]		= nfsd4_encode_fattr4_aclsupport,
+	[14]		= nfsd4_encode_fattr4__noop,	/* archive */
+	[15]		= nfsd4_encode_fattr4__true,	/* can set time */
+	[16]		= nfsd4_encode_fattr4__false,	/* case insensitive */
+	[17]		= nfsd4_encode_fattr4__true,	/* case preserving */
+	[18]		= nfsd4_encode_fattr4__true,	/* chown restricted */
+	[19]		= nfsd4_encode_fattr4_filehandle,
+	[20]		= nfsd4_encode_fattr4_fileid,
+	[21]		= nfsd4_encode_fattr4_files_avail,
+	[22]		= nfsd4_encode_fattr4_files_free,
+	[23]		= nfsd4_encode_fattr4_files_total,
+	[24]		= nfsd4_encode_fattr4_fs_locations,
+	[25]		= nfsd4_encode_fattr4__noop,	/* hidden */
+	[26]		= nfsd4_encode_fattr4__true,	/* homogeneous */
+	[27]		= nfsd4_encode_fattr4_maxfilesize,
+	[28]		= nfsd4_encode_fattr4_maxlink,
+	[29]		= nfsd4_encode_fattr4_maxname,
+	[30]		= nfsd4_encode_fattr4_maxread,
+	[31]		= nfsd4_encode_fattr4_maxwrite,
+};
+
 /*
  * Note: @fhp can be NULL; in this case, we might have to compose the filehandle
  * ourselves.
@@ -2962,27 +3291,27 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 	u32 bmval1 = bmval[1];
 	u32 bmval2 = bmval[2];
 	struct svc_fh *tempfh = NULL;
-	__be32 *p, *attrlen_p;
 	int starting_len = xdr->buf->len;
-	int attrlen_offset;
-	u32 dummy;
 	u64 dummy64;
-	__be32 status;
-	int err;
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
 	void *context = NULL;
 	int contextlen;
 #endif
 	struct nfsd4_compoundres *resp = rqstp->rq_resp;
 	u32 minorversion = resp->cstate.minorversion;
+	int err, i, attrlen_offset;
+	__be32 status, *attrlen_p;
 	struct path path = {
 		.mnt	= exp->ex_path.mnt,
 		.dentry	= dentry,
 	};
-	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
+	unsigned long mask;
+	__be32 *p;
 
-	BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
-	BUG_ON(!nfsd_attrs_supported(minorversion, bmval));
+	args.rqstp = rqstp;
+	args.fhp = fhp;
+	args.exp = exp;
+	args.dentry = dentry;
 
 	args.rdattr_err = 0;
 	if (exp->ex_fslocs.migrated) {
@@ -3063,258 +3392,13 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 	if (!attrlen_p)
 		goto out_resource;
 
-	if (bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
-		u32 supp[3];
-
-		memcpy(supp, nfsd_suppattrs[minorversion], sizeof(supp));
-
-		if (!IS_POSIXACL(dentry->d_inode))
-			supp[0] &= ~FATTR4_WORD0_ACL;
-		if (!args.contextsupport)
-			supp[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
-		if (!supp[2]) {
-			p = xdr_reserve_space(xdr, 12);
-			if (!p)
-				goto out_resource;
-			*p++ = cpu_to_be32(2);
-			*p++ = cpu_to_be32(supp[0]);
-			*p++ = cpu_to_be32(supp[1]);
-		} else {
-			p = xdr_reserve_space(xdr, 16);
-			if (!p)
-				goto out_resource;
-			*p++ = cpu_to_be32(3);
-			*p++ = cpu_to_be32(supp[0]);
-			*p++ = cpu_to_be32(supp[1]);
-			*p++ = cpu_to_be32(supp[2]);
-		}
-	}
-	if (bmval0 & FATTR4_WORD0_TYPE) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		dummy = nfs4_file_type(args.stat.mode);
-		if (dummy == NF4BAD) {
-			status = nfserr_serverfault;
-			goto out;
-		}
-		*p++ = cpu_to_be32(dummy);
-	}
-	if (bmval0 & FATTR4_WORD0_FH_EXPIRE_TYPE) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		if (exp->ex_flags & NFSEXP_NOSUBTREECHECK)
-			*p++ = cpu_to_be32(NFS4_FH_PERSISTENT);
-		else
-			*p++ = cpu_to_be32(NFS4_FH_PERSISTENT|
-						NFS4_FH_VOL_RENAME);
-	}
-	if (bmval0 & FATTR4_WORD0_CHANGE) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		p = encode_change(p, &args.stat, d_inode(dentry), exp);
-	}
-	if (bmval0 & FATTR4_WORD0_SIZE) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		p = xdr_encode_hyper(p, args.stat.size);
-	}
-	if (bmval0 & FATTR4_WORD0_LINK_SUPPORT) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(1);
-	}
-	if (bmval0 & FATTR4_WORD0_SYMLINK_SUPPORT) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(1);
-	}
-	if (bmval0 & FATTR4_WORD0_NAMED_ATTR) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(0);
-	}
-	if (bmval0 & FATTR4_WORD0_FSID) {
-		p = xdr_reserve_space(xdr, 16);
-		if (!p)
-			goto out_resource;
-		if (exp->ex_fslocs.migrated) {
-			p = xdr_encode_hyper(p, NFS4_REFERRAL_FSID_MAJOR);
-			p = xdr_encode_hyper(p, NFS4_REFERRAL_FSID_MINOR);
-		} else switch(fsid_source(args.fhp)) {
-		case FSIDSOURCE_FSID:
-			p = xdr_encode_hyper(p, (u64)exp->ex_fsid);
-			p = xdr_encode_hyper(p, (u64)0);
-			break;
-		case FSIDSOURCE_DEV:
-			*p++ = cpu_to_be32(0);
-			*p++ = cpu_to_be32(MAJOR(args.stat.dev));
-			*p++ = cpu_to_be32(0);
-			*p++ = cpu_to_be32(MINOR(args.stat.dev));
-			break;
-		case FSIDSOURCE_UUID:
-			p = xdr_encode_opaque_fixed(p, exp->ex_uuid,
-								EX_UUID_LEN);
-			break;
-		}
-	}
-	if (bmval0 & FATTR4_WORD0_UNIQUE_HANDLES) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(0);
-	}
-	if (bmval0 & FATTR4_WORD0_LEASE_TIME) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(nn->nfsd4_lease);
-	}
-	if (bmval0 & FATTR4_WORD0_RDATTR_ERROR) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(args.rdattr_err);
-	}
-	if (bmval0 & FATTR4_WORD0_ACL) {
-		struct nfs4_ace *ace;
-
-		if (args.acl == NULL) {
-			p = xdr_reserve_space(xdr, 4);
-			if (!p)
-				goto out_resource;
-
-			*p++ = cpu_to_be32(0);
-			goto out_acl;
-		}
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(args.acl->naces);
-
-		for (ace = args.acl->aces; ace < args.acl->aces + args.acl->naces; ace++) {
-			p = xdr_reserve_space(xdr, 4*3);
-			if (!p)
-				goto out_resource;
-			*p++ = cpu_to_be32(ace->type);
-			*p++ = cpu_to_be32(ace->flag);
-			*p++ = cpu_to_be32(ace->access_mask &
-							NFS4_ACE_MASK_ALL);
-			status = nfsd4_encode_aclname(xdr, rqstp, ace);
-			if (status)
-				goto out;
-		}
-	}
-out_acl:
-	if (bmval0 & FATTR4_WORD0_ACLSUPPORT) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(IS_POSIXACL(dentry->d_inode) ?
-			ACL4_SUPPORT_ALLOW_ACL|ACL4_SUPPORT_DENY_ACL : 0);
-	}
-	if (bmval0 & FATTR4_WORD0_CANSETTIME) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(1);
-	}
-	if (bmval0 & FATTR4_WORD0_CASE_INSENSITIVE) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(0);
-	}
-	if (bmval0 & FATTR4_WORD0_CASE_PRESERVING) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(1);
-	}
-	if (bmval0 & FATTR4_WORD0_CHOWN_RESTRICTED) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(1);
-	}
-	if (bmval0 & FATTR4_WORD0_FILEHANDLE) {
-		p = xdr_reserve_space(xdr, args.fhp->fh_handle.fh_size + 4);
-		if (!p)
-			goto out_resource;
-		p = xdr_encode_opaque(p, &args.fhp->fh_handle.fh_raw,
-					args.fhp->fh_handle.fh_size);
-	}
-	if (bmval0 & FATTR4_WORD0_FILEID) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		p = xdr_encode_hyper(p, args.stat.ino);
-	}
-	if (bmval0 & FATTR4_WORD0_FILES_AVAIL) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		p = xdr_encode_hyper(p, (u64) args.statfs.f_ffree);
-	}
-	if (bmval0 & FATTR4_WORD0_FILES_FREE) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		p = xdr_encode_hyper(p, (u64) args.statfs.f_ffree);
-	}
-	if (bmval0 & FATTR4_WORD0_FILES_TOTAL) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		p = xdr_encode_hyper(p, (u64) args.statfs.f_files);
-	}
-	if (bmval0 & FATTR4_WORD0_FS_LOCATIONS) {
-		status = nfsd4_encode_fs_locations(xdr, rqstp, exp);
+	mask = bmval0;
+	for_each_set_bit(i, &mask, 32) {
+		status = nfsd4_enc_fattr4_word0_ops[i](xdr, &args);
 		if (status)
 			goto out;
 	}
-	if (bmval0 & FATTR4_WORD0_HOMOGENEOUS) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(1);
-	}
-	if (bmval0 & FATTR4_WORD0_MAXFILESIZE) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		p = xdr_encode_hyper(p, exp->ex_path.mnt->mnt_sb->s_maxbytes);
-	}
-	if (bmval0 & FATTR4_WORD0_MAXLINK) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(255);
-	}
-	if (bmval0 & FATTR4_WORD0_MAXNAME) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(args.statfs.f_namelen);
-	}
-	if (bmval0 & FATTR4_WORD0_MAXREAD) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		p = xdr_encode_hyper(p, (u64) svc_max_payload(rqstp));
-	}
-	if (bmval0 & FATTR4_WORD0_MAXWRITE) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		p = xdr_encode_hyper(p, (u64) svc_max_payload(rqstp));
-	}
+
 	if (bmval1 & FATTR4_WORD1_MODE) {
 		p = xdr_reserve_space(xdr, 4);
 		if (!p)

[PATCH RFC 3/4] NFSD: Encode attributes in WORD1 using a bitmask loop

[Chuck Lever]

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

Replace the current implementation with a branch table. This creates
hard function scope boundaries, limiting side effects and reducing
instruction cache footprint (uncalled encoders remain out of the
cache).

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c |  301 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 190 insertions(+), 111 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 20e09e5510c9..8335ca1e2da0 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2948,6 +2948,7 @@ struct nfsd4_fattr_args {
 	struct nfs4_acl		*acl;
 	u32			rdattr_err;
 	bool			contextsupport;
+	bool			ignore_crossmnt;
 };
 
 typedef __be32(*nfsd4_enc_attr)(struct xdr_stream *xdr,
@@ -3276,6 +3277,191 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_word0_ops[] = {
 	[31]		= nfsd4_encode_fattr4_maxwrite,
 };
 
+static __be32 nfsd4_encode_fattr4_mode(struct xdr_stream *xdr,
+				       struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u32(xdr, args->stat.mode & S_IALLUGO) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_numlinks(struct xdr_stream *xdr,
+					   struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u32(xdr, args->stat.nlink) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_owner(struct xdr_stream *xdr,
+					struct nfsd4_fattr_args *args)
+{
+	return nfsd4_encode_user(xdr, args->rqstp, args->stat.uid);
+}
+
+static __be32 nfsd4_encode_fattr4_owner_group(struct xdr_stream *xdr,
+					      struct nfsd4_fattr_args *args)
+{
+	return nfsd4_encode_group(xdr, args->rqstp, args->stat.gid);
+}
+
+static __be32 nfsd4_encode_fattr4_rawdev(struct xdr_stream *xdr,
+					 struct nfsd4_fattr_args *args)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, 8);
+	if (!p)
+		return nfserr_resource;
+	*p++ = cpu_to_be32((u32) MAJOR(args->stat.rdev));
+	*p   = cpu_to_be32((u32) MINOR(args->stat.rdev));
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_space_avail(struct xdr_stream *xdr,
+					      struct nfsd4_fattr_args *args)
+{
+	u64 space = (u64)args->statfs.f_bavail * (u64)args->statfs.f_bsize;
+
+	if (xdr_stream_encode_u64(xdr, space) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_space_free(struct xdr_stream *xdr,
+					     struct nfsd4_fattr_args *args)
+{
+	u64 space = (u64)args->statfs.f_bfree * (u64)args->statfs.f_bsize;
+
+	if (xdr_stream_encode_u64(xdr, space) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_space_total(struct xdr_stream *xdr,
+					      struct nfsd4_fattr_args *args)
+{
+	u64 space = (u64)args->statfs.f_blocks * (u64)args->statfs.f_bsize;
+
+	if (xdr_stream_encode_u64(xdr, space) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_space_used(struct xdr_stream *xdr,
+					     struct nfsd4_fattr_args *args)
+{
+	u64 space = (u64)args->stat.blocks << 9;
+
+	if (xdr_stream_encode_u64(xdr, space) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_time_access(struct xdr_stream *xdr,
+					     struct nfsd4_fattr_args *args)
+{
+	return nfsd4_encode_nfstime4(xdr, &args->stat.atime);
+}
+
+static __be32 nfsd4_encode_fattr4_time_create(struct xdr_stream *xdr,
+					      struct nfsd4_fattr_args *args)
+{
+	return nfsd4_encode_nfstime4(xdr, &args->stat.btime);
+}
+
+static __be32 nfsd4_encode_fattr4_time_delta(struct xdr_stream *xdr,
+					     struct nfsd4_fattr_args *args)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, XDR_UNIT * 3);
+	if (!p)
+		return nfserr_resource;
+
+	encode_time_delta(p, d_inode(args->dentry));
+	return nfs_ok;
+}
+
+static __be32 nfsd4_encode_fattr4_time_metadata(struct xdr_stream *xdr,
+						struct nfsd4_fattr_args *args)
+{
+	return nfsd4_encode_nfstime4(xdr, &args->stat.ctime);
+}
+
+static __be32 nfsd4_encode_fattr4_time_modify(struct xdr_stream *xdr,
+					      struct nfsd4_fattr_args *args)
+{
+	return nfsd4_encode_nfstime4(xdr, &args->stat.mtime);
+}
+
+static __be32 nfsd4_encode_fattr4_mounted_on_fileid(struct xdr_stream *xdr,
+						    struct nfsd4_fattr_args *args)
+{
+	u64 ino = args->stat.ino;
+	int err;
+
+	if (!args->ignore_crossmnt &&
+	    args->dentry == args->exp->ex_path.mnt->mnt_root) {
+		err = nfsd4_get_mounted_on_ino(args->exp, &ino);
+		if (err)
+			return nfserrno(err);
+	}
+
+	if (xdr_stream_encode_u64(xdr, ino) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+#ifdef CONFIG_NFSD_PNFS
+
+static __be32 nfsd4_encode_fattr4_layout_types(struct xdr_stream *xdr,
+					       struct nfsd4_fattr_args *args)
+{
+	return nfsd4_encode_layout_types(xdr, args->exp->ex_layout_types);
+}
+
+#endif
+
+static const nfsd4_enc_attr nfsd4_enc_fattr4_word1_ops[] = {
+	[0]		= nfsd4_encode_fattr4__noop,	/* mime type */
+	[1]		= nfsd4_encode_fattr4_mode,
+	[2]		= nfsd4_encode_fattr4__true,	/* no trunc */
+	[3]		= nfsd4_encode_fattr4_numlinks,
+	[4]		= nfsd4_encode_fattr4_owner,
+	[5]		= nfsd4_encode_fattr4_owner_group,
+	[6]		= nfsd4_encode_fattr4__noop,	/* quota_hard */
+	[7]		= nfsd4_encode_fattr4__noop,	/* quota_soft */
+	[8]		= nfsd4_encode_fattr4__noop,	/* quota_used */
+	[9]		= nfsd4_encode_fattr4_rawdev,
+	[10]		= nfsd4_encode_fattr4_space_avail,
+	[11]		= nfsd4_encode_fattr4_space_free,
+	[12]		= nfsd4_encode_fattr4_space_total,
+	[13]		= nfsd4_encode_fattr4_space_used,
+	[14]		= nfsd4_encode_fattr4__noop,	/* system */
+	[15]		= nfsd4_encode_fattr4_time_access,
+	[16]		= nfsd4_encode_fattr4__noop,	/* time_access_set */
+	[17]		= nfsd4_encode_fattr4__noop,	/* time_backup */
+	[18]		= nfsd4_encode_fattr4_time_create,
+	[19]		= nfsd4_encode_fattr4_time_delta,
+	[20]		= nfsd4_encode_fattr4_time_metadata,
+	[21]		= nfsd4_encode_fattr4_time_modify,
+	[22]		= nfsd4_encode_fattr4__noop,	/* time_modify_set */
+	[23]		= nfsd4_encode_fattr4_mounted_on_fileid,
+	[24]		= nfsd4_encode_fattr4__noop,	/* dir notif delay */
+	[25]		= nfsd4_encode_fattr4__noop,	/* dirent notif delay */
+	[26]		= nfsd4_encode_fattr4__noop,	/* dacl */
+	[27]		= nfsd4_encode_fattr4__noop,	/* sacl */
+	[28]		= nfsd4_encode_fattr4__noop,	/* change policy */
+	[29]		= nfsd4_encode_fattr4__noop,	/* fs status */
+#ifdef CONFIG_NFSD_PNFS
+	[30]		= nfsd4_encode_fattr4_layout_types,
+#else
+	[30]		= nfsd4_encode_fattr4__noop,
+#endif
+	[31]		= nfsd4_encode_fattr4__noop,	/* layout hint */
+};
+
 /*
  * Note: @fhp can be NULL; in this case, we might have to compose the filehandle
  * ourselves.
@@ -3292,7 +3478,6 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 	u32 bmval2 = bmval[2];
 	struct svc_fh *tempfh = NULL;
 	int starting_len = xdr->buf->len;
-	u64 dummy64;
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
 	void *context = NULL;
 	int contextlen;
@@ -3312,6 +3497,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 	args.fhp = fhp;
 	args.exp = exp;
 	args.dentry = dentry;
+	args.ignore_crossmnt = (ignore_crossmnt != 0);
 
 	args.rdattr_err = 0;
 	if (exp->ex_fslocs.migrated) {
@@ -3399,121 +3585,14 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 			goto out;
 	}
 
-	if (bmval1 & FATTR4_WORD1_MODE) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(args.stat.mode & S_IALLUGO);
-	}
-	if (bmval1 & FATTR4_WORD1_NO_TRUNC) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(1);
-	}
-	if (bmval1 & FATTR4_WORD1_NUMLINKS) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(args.stat.nlink);
-	}
-	if (bmval1 & FATTR4_WORD1_OWNER) {
-		status = nfsd4_encode_user(xdr, rqstp, args.stat.uid);
-		if (status)
-			goto out;
-	}
-	if (bmval1 & FATTR4_WORD1_OWNER_GROUP) {
-		status = nfsd4_encode_group(xdr, rqstp, args.stat.gid);
-		if (status)
-			goto out;
-	}
-	if (bmval1 & FATTR4_WORD1_RAWDEV) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32((u32) MAJOR(args.stat.rdev));
-		*p++ = cpu_to_be32((u32) MINOR(args.stat.rdev));
-	}
-	if (bmval1 & FATTR4_WORD1_SPACE_AVAIL) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		dummy64 = (u64)args.statfs.f_bavail * (u64)args.statfs.f_bsize;
-		p = xdr_encode_hyper(p, dummy64);
-	}
-	if (bmval1 & FATTR4_WORD1_SPACE_FREE) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		dummy64 = (u64)args.statfs.f_bfree * (u64)args.statfs.f_bsize;
-		p = xdr_encode_hyper(p, dummy64);
-	}
-	if (bmval1 & FATTR4_WORD1_SPACE_TOTAL) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		dummy64 = (u64)args.statfs.f_blocks * (u64)args.statfs.f_bsize;
-		p = xdr_encode_hyper(p, dummy64);
-	}
-	if (bmval1 & FATTR4_WORD1_SPACE_USED) {
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-			goto out_resource;
-		dummy64 = (u64)args.stat.blocks << 9;
-		p = xdr_encode_hyper(p, dummy64);
-	}
-	if (bmval1 & FATTR4_WORD1_TIME_ACCESS) {
-		status = nfsd4_encode_nfstime4(xdr, &args.stat.atime);
-		if (status)
-			goto out;
-	}
-	if (bmval1 & FATTR4_WORD1_TIME_DELTA) {
-		p = xdr_reserve_space(xdr, 12);
-		if (!p)
-			goto out_resource;
-		p = encode_time_delta(p, d_inode(dentry));
-	}
-	if (bmval1 & FATTR4_WORD1_TIME_METADATA) {
-		status = nfsd4_encode_nfstime4(xdr, &args.stat.ctime);
-		if (status)
-			goto out;
-	}
-	if (bmval1 & FATTR4_WORD1_TIME_MODIFY) {
-		status = nfsd4_encode_nfstime4(xdr, &args.stat.mtime);
-		if (status)
-			goto out;
-	}
-	if (bmval1 & FATTR4_WORD1_TIME_CREATE) {
-		status = nfsd4_encode_nfstime4(xdr, &args.stat.btime);
+	mask = bmval1;
+	for_each_set_bit(i, &mask, 32) {
+		status = nfsd4_enc_fattr4_word1_ops[i](xdr, &args);
 		if (status)
 			goto out;
 	}
-	if (bmval1 & FATTR4_WORD1_MOUNTED_ON_FILEID) {
-		u64 ino = args.stat.ino;
 
-		p = xdr_reserve_space(xdr, 8);
-		if (!p)
-                	goto out_resource;
-		/*
-		 * Get ino of mountpoint in parent filesystem, if not ignoring
-		 * crossmount and this is the root of a cross-mounted
-		 * filesystem.
-		 */
-		if (ignore_crossmnt == 0 &&
-		    dentry == exp->ex_path.mnt->mnt_root) {
-			err = nfsd4_get_mounted_on_ino(exp, &ino);
-			if (err)
-				goto out_nfserr;
-		}
-		p = xdr_encode_hyper(p, ino);
-	}
 #ifdef CONFIG_NFSD_PNFS
-	if (bmval1 & FATTR4_WORD1_FS_LAYOUT_TYPES) {
-		status = nfsd4_encode_layout_types(xdr, exp->ex_layout_types);
-		if (status)
-			goto out;
-	}
-
 	if (bmval2 & FATTR4_WORD2_LAYOUT_TYPES) {
 		status = nfsd4_encode_layout_types(xdr, exp->ex_layout_types);
 		if (status)

[PATCH RFC 4/4] NFSD: Encode attributes in WORD2 using a bitmask loop

[Chuck Lever]

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

Replace the current implementation with a branch table. This creates
hard function scope boundaries, limiting side effects and reducing
instruction cache footprint (uncalled encoders remain out of the
cache).

This also makes it obvious which attributes are not supported by the
Linux NFS server.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c |  153 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 101 insertions(+), 52 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 8335ca1e2da0..31dccf6d1caa 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2946,6 +2946,10 @@ struct nfsd4_fattr_args {
 	struct kstat		stat;
 	struct kstatfs		statfs;
 	struct nfs4_acl		*acl;
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+	void			*context;
+	int			contextlen;
+#endif
 	u32			rdattr_err;
 	bool			contextsupport;
 	bool			ignore_crossmnt;
@@ -3421,6 +3425,14 @@ static __be32 nfsd4_encode_fattr4_layout_types(struct xdr_stream *xdr,
 	return nfsd4_encode_layout_types(xdr, args->exp->ex_layout_types);
 }
 
+static __be32 nfsd4_encode_fattr4_layout_blksize(struct xdr_stream *xdr,
+						 struct nfsd4_fattr_args *args)
+{
+	if (xdr_stream_encode_u32(xdr, args->stat.blksize) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
 #endif
 
 static const nfsd4_enc_attr nfsd4_enc_fattr4_word1_ops[] = {
@@ -3462,6 +3474,84 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_word1_ops[] = {
 	[31]		= nfsd4_encode_fattr4__noop,	/* layout hint */
 };
 
+static __be32 nfsd4_encode_fattr4_suppattr_exclcreat(struct xdr_stream *xdr,
+						     struct nfsd4_fattr_args *args)
+{
+	struct nfsd4_compoundres *resp = args->rqstp->rq_resp;
+	u32 minorversion = resp->cstate.minorversion;
+	u32 supp[3];
+
+	memcpy(supp, nfsd_suppattrs[minorversion], sizeof(supp));
+	supp[0] &= NFSD_SUPPATTR_EXCLCREAT_WORD0;
+	supp[1] &= NFSD_SUPPATTR_EXCLCREAT_WORD1;
+	supp[2] &= NFSD_SUPPATTR_EXCLCREAT_WORD2;
+
+	return nfsd4_encode_bitmap(xdr, supp[0], supp[1], supp[2]);
+}
+
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+static __be32 nfsd4_encode_fattr4_sec_label(struct xdr_stream *xdr,
+					    struct nfsd4_fattr_args *args)
+{
+	return nfsd4_encode_security_label(xdr, args->rqstp,
+					   args->context, args->contextlen);
+}
+#endif
+
+static __be32 nfsd4_encode_fattr4_xattr_support(struct xdr_stream *xdr,
+						struct nfsd4_fattr_args *args)
+{
+	int err = xattr_supports_user_prefix(d_inode(args->dentry));
+
+	if (xdr_stream_encode_bool(xdr, err == 0) < 0)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static const nfsd4_enc_attr nfsd4_enc_fattr4_word2_ops[] = {
+#ifdef CONFIG_NFSD_PNFS
+	[0]		= nfsd4_encode_fattr4_layout_types,
+	[1]		= nfsd4_encode_fattr4_layout_blksize,
+#else
+	[0]		= nfsd4_encode_fattr4__noop,
+	[1]		= nfsd4_encode_fattr4__noop,
+#endif
+	[2]		= nfsd4_encode_fattr4__noop,	/* layout alignment */
+	[3]		= nfsd4_encode_fattr4__noop,	/* fslocations info */
+	[4]		= nfsd4_encode_fattr4__noop,	/* mds threshold */
+	[5]		= nfsd4_encode_fattr4__noop,	/* retention get */
+	[6]		= nfsd4_encode_fattr4__noop,	/* retention set */
+	[7]		= nfsd4_encode_fattr4__noop,	/* retentevt get */
+	[8]		= nfsd4_encode_fattr4__noop,	/* retentevt set */
+	[9]		= nfsd4_encode_fattr4__noop,	/* retention hold */
+	[10]		= nfsd4_encode_fattr4__noop,	/* mode set mask */
+	[11]		= nfsd4_encode_fattr4_suppattr_exclcreat,
+	[12]		= nfsd4_encode_fattr4__noop,	/* fs charset cap */
+	[13]		= nfsd4_encode_fattr4__noop,	/* clone blksize */
+	[14]		= nfsd4_encode_fattr4__noop,	/* space freed */
+	[15]		= nfsd4_encode_fattr4__noop,	/* change attr type */
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+	[16]		= nfsd4_encode_fattr4_sec_label,
+#else
+	[16]		= nfsd4_encode_fattr4__noop,
+#endif
+	[17]		= nfsd4_encode_fattr4__noop,	/* mode_umask */
+	[18]		= nfsd4_encode_fattr4_xattr_support,
+	[19]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[20]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[21]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[22]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[23]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[24]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[25]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[26]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[27]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[28]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[29]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[30]		= nfsd4_encode_fattr4__noop,	/* reserved */
+	[31]		= nfsd4_encode_fattr4__noop,	/* reserved */
+};
+
 /*
  * Note: @fhp can be NULL; in this case, we might have to compose the filehandle
  * ourselves.
@@ -3478,12 +3568,6 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 	u32 bmval2 = bmval[2];
 	struct svc_fh *tempfh = NULL;
 	int starting_len = xdr->buf->len;
-#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
-	void *context = NULL;
-	int contextlen;
-#endif
-	struct nfsd4_compoundres *resp = rqstp->rq_resp;
-	u32 minorversion = resp->cstate.minorversion;
 	int err, i, attrlen_offset;
 	__be32 status, *attrlen_p;
 	struct path path = {
@@ -3491,7 +3575,6 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		.dentry	= dentry,
 	};
 	unsigned long mask;
-	__be32 *p;
 
 	args.rqstp = rqstp;
 	args.fhp = fhp;
@@ -3552,11 +3635,12 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 	args.contextsupport = false;
 
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+	args.context = NULL;
 	if ((bmval2 & FATTR4_WORD2_SECURITY_LABEL) ||
 	     bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
 		if (exp->ex_flags & NFSEXP_SECURITY_LABEL)
 			err = security_inode_getsecctx(d_inode(dentry),
-						&context, &contextlen);
+						&args.context, &args.contextlen);
 		else
 			err = -EOPNOTSUPP;
 		args.contextsupport = (err == 0);
@@ -3592,48 +3676,13 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 			goto out;
 	}
 
-#ifdef CONFIG_NFSD_PNFS
-	if (bmval2 & FATTR4_WORD2_LAYOUT_TYPES) {
-		status = nfsd4_encode_layout_types(xdr, exp->ex_layout_types);
-		if (status)
-			goto out;
-	}
-
-	if (bmval2 & FATTR4_WORD2_LAYOUT_BLKSIZE) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		*p++ = cpu_to_be32(args.stat.blksize);
-	}
-#endif /* CONFIG_NFSD_PNFS */
-	if (bmval2 & FATTR4_WORD2_SUPPATTR_EXCLCREAT) {
-		u32 supp[3];
-
-		memcpy(supp, nfsd_suppattrs[minorversion], sizeof(supp));
-		supp[0] &= NFSD_SUPPATTR_EXCLCREAT_WORD0;
-		supp[1] &= NFSD_SUPPATTR_EXCLCREAT_WORD1;
-		supp[2] &= NFSD_SUPPATTR_EXCLCREAT_WORD2;
-
-		status = nfsd4_encode_bitmap(xdr, supp[0], supp[1], supp[2]);
-		if (status)
-			goto out;
-	}
-
-#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
-	if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) {
-		status = nfsd4_encode_security_label(xdr, rqstp, context,
-								contextlen);
-		if (status)
-			goto out;
-	}
-#endif
-
-	if (bmval2 & FATTR4_WORD2_XATTR_SUPPORT) {
-		p = xdr_reserve_space(xdr, 4);
-		if (!p)
-			goto out_resource;
-		err = xattr_supports_user_prefix(d_inode(dentry));
-		*p++ = cpu_to_be32(err == 0);
+	if (bmval2) {
+		mask = bmval2;
+		for_each_set_bit(i, &mask, 32) {
+			status = nfsd4_enc_fattr4_word2_ops[i](xdr, &args);
+			if (status)
+				goto out;
+		}
 	}
 
 	*attrlen_p = cpu_to_be32(xdr->buf->len - attrlen_offset - XDR_UNIT);
@@ -3641,8 +3690,8 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 
 out:
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
-	if (context)
-		security_release_secctx(context, contextlen);
+	if (args.context)
+		security_release_secctx(args.context, args.contextlen);
 #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
 	kfree(args.acl);
 	if (tempfh) {

Re: [PATCH RFC 0/4] Encode NFSv4 attributes via a branch table

[NeilBrown]

On Fri, 30 Jun 2023, Chuck Lever wrote:
> Here's something just for fun. I've converted nfsd4_encode_fattr4()
> to use a bitmask loop, calling an encode helper for each attribute
> to be encoded. Rotten tomatoes and gold stars are both acceptible.

Tomatoes or stars .... it is a hard choice :-)

I wonder what the compiler does with this code.
If it unrolls the loop and inlines the functions - which it probably can
do as the array of pointers is declared const - you end up with much the
same result as the current code.

And I wonder where the compiler puts the code in each conditional now.
If it assumes an if() is unlikely, then it would all be out-of-line
which sounds like part of your goal (or maybe just a nice-to-have).

If the compiler does, or can be convinced to, do the unroll and inline
and unlikely optimisations, then I think I'd give this a goal star.

I guess in practice some of the attributes are "likely" and many are
"unlikely".  With the current code we could easily annotate that if we
wanted to and thought (or measured) there was any value.  With the
looping code we cannot really annotate the likelihood of each.

The code-generation idea is intriguing.  Even if we didn't reach that
goal, having the code highly structured as though it were auto-generated
would be no bad thing.

Thanks,
NeilBrown

Re: [PATCH RFC 0/4] Encode NFSv4 attributes via a branch table

[Chuck Lever III]

> On Jul 3, 2023, at 12:48 AM, NeilBrown <neilb@suse.de> wrote:
> 
> On Fri, 30 Jun 2023, Chuck Lever wrote:
>> Here's something just for fun. I've converted nfsd4_encode_fattr4()
>> to use a bitmask loop, calling an encode helper for each attribute
>> to be encoded. Rotten tomatoes and gold stars are both acceptible.
> 
> Tomatoes or stars .... it is a hard choice :-)
> 
> I wonder what the compiler does with this code.
> If it unrolls the loop and inlines the functions - which it probably can
> do as the array of pointers is declared const - you end up with much the
> same result as the current code.
> 
> And I wonder where the compiler puts the code in each conditional now.
> If it assumes an if() is unlikely, then it would all be out-of-line
> which sounds like part of your goal (or maybe just a nice-to-have).
> 
> If the compiler does, or can be convinced to, do the unroll and inline
> and unlikely optimisations, then I think I'd give this a goal star.
> 
> I guess in practice some of the attributes are "likely" and many are
> "unlikely".

This is absolutely the case.

My first attempt at optimizing nfsd4_encode_fattr() was to build
a miniature version that handled just the frequently-requested
combinations of attributes. It made very little difference.

The conclusions that I drew from that are:

- The number of conditional branches in here doesn't seem to be
  the costly part of encode_fattr().

- The frequently-requested attributes are expensive to process
  for some reason. Size is easy, but getting the user and
  group are not as quick as I would have hoped.

- It's not the efficiency of encode_fattr() that is the issue,
  it's the frequency of its use. That's something the server
  can't do much about.


> With the current code we could easily annotate that if we
> wanted to and thought (or measured) there was any value.  With the
> looping code we cannot really annotate the likelihood of each.

Nope, likelihood annotation isn't really possible with a bitmask
loop. But my understanding is that unlikely() means really
really really unlikely, as in "this code is an error case that
is almost never used". And that's not actually the case for most
of these attributes.


> The code-generation idea is intriguing.  Even if we didn't reach that
> goal, having the code highly structured as though it were auto-generated
> would be no bad thing.

Maybe it just calms my yearning for an ordered universe to deal
with these attributes in the same way that we deal with COMPOUND
operations.

I appreciate the review!


--
Chuck Lever

Re: [PATCH RFC 0/4] Encode NFSv4 attributes via a branch table

[NeilBrown]

On Mon, 03 Jul 2023, Chuck Lever III wrote:
> 
> > On Jul 3, 2023, at 12:48 AM, NeilBrown <neilb@suse.de> wrote:
> > 
> > I guess in practice some of the attributes are "likely" and many are
> > "unlikely".
> 
> This is absolutely the case.
> 
> My first attempt at optimizing nfsd4_encode_fattr() was to build
> a miniature version that handled just the frequently-requested
> combinations of attributes. It made very little difference.

My understanding of the effect of these code-movement optimisations is
that they affect whole-system performance rather than micro-benchmarks. 
So they are quite hard to measure.
Modern CPUs are quite good at predicting code flow, so the benefit of
code movement is not a reduction in pipeline stalls, but a reduction in
cache usage.  The latter affects the whole system more-or-less equally.


> 
> The conclusions that I drew from that are:
> 
> - The number of conditional branches in here doesn't seem to be
>   the costly part of encode_fattr().
> 
> - The frequently-requested attributes are expensive to process
>   for some reason. Size is easy, but getting the user and
>   group are not as quick as I would have hoped.
> 
> - It's not the efficiency of encode_fattr() that is the issue,
>   it's the frequency of its use. That's something the server
>   can't do much about.

There probably needs a protocol revision to improve this.  I imagine a
GETATTR request including a CTIME value with the implication that if the
CTIME hasn't changed, then there is no need to return any attributes.

> 
> 
> > With the current code we could easily annotate that if we
> > wanted to and thought (or measured) there was any value.  With the
> > looping code we cannot really annotate the likelihood of each.
> 
> Nope, likelihood annotation isn't really possible with a bitmask
> loop. But my understanding is that unlikely() means really
> really really unlikely, as in "this code is an error case that
> is almost never used". And that's not actually the case for most
> of these attributes.

My understanding of unlikely() (which is largely compatible with yours)
is that it tells the compile to pessimise code dependant on the
condition being true.  Or more accurately: when there is an optimisation
trade off between code for the 'true' case and any other code, preference
the other code.
So it is definitely good to say errors are unlikely, because there is no
need to optimise for them.  You might also say a fast-path condition is
likely() because that path is more worthy of optimisation.

> 
> 
> > The code-generation idea is intriguing.  Even if we didn't reach that
> > goal, having the code highly structured as though it were auto-generated
> > would be no bad thing.
> 
> Maybe it just calms my yearning for an ordered universe to deal
> with these attributes in the same way that we deal with COMPOUND
> operations.

There are worse reasons for refactoring code:-)

Thanks,
NeilBrown

> 
> I appreciate the review!
> 
> 
> --
> Chuck Lever
> 
> 
> 

Re: [PATCH RFC 0/4] Encode NFSv4 attributes via a branch table

[Chuck Lever III]

> On Jul 3, 2023, at 5:33 PM, NeilBrown <neilb@suse.de> wrote:
> 
> On Mon, 03 Jul 2023, Chuck Lever III wrote:
>> 
>> - It's not the efficiency of encode_fattr() that is the issue,
>>  it's the frequency of its use. That's something the server
>>  can't do much about.
> 
> There probably needs a protocol revision to improve this.  I imagine a
> GETATTR request including a CTIME value with the implication that if the
> CTIME hasn't changed, then there is no need to return any attributes.

You can precede the GETATTR with an NVERIFY operation.

https://datatracker.ietf.org/doc/html/rfc8881#name-operation-17-nverify-verify


--
Chuck Lever