[PATCH v3 00/13] nfsd: implement the "delstid" draft

[PATCH v3 00/13] nfsd: implement the "delstid" draft

[Jeff Layton]

The first couple of patches are prep patches from Neil, with some small
cleanups. The first patch should probably go to mainline for v6.11
since it fixes a bug.

The OPEN_ARGUMENTS and OPEN_XOR_DELEG patches are pretty straightforward
and have survived some local testing. The delegated timestamp patches
took a few tries to get right, but seem to work. That part is hard to
test since it requires 2 clients. It would be nice to have pynfs tests
for that.

The main question I have about this series is the change to make nfs4.h
include the autogenerated headers. Does anyone have issues with that?
I'd especially appreciate feedback on the basic scheme from Chuck, Trond
and Anna.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v3:
- fix includes in nfs4xdr_gen.c
- drop ATTR_CTIME_DLG flag (just use ATTR_DELEG instead)
- proper handling for SETATTR (maybe? Outstanding q about stateid here)
- incorporate Neil's patches for handling non-delegation leases
- always return times from CB_GETATTR instead of from local vfs_getattr
- fix potential races vs. mgtimes by moving ctime handling into VFS layer
- Link to v2: https://lore.kernel.org/r/20240826-delstid-v2-0-e8ab5c0e39cc@kernel.org

Changes in v2:
- rebase onto Chuck's lkxdrgen branch, and reworked how autogenerated
  code is included
- declare nfsd_open_arguments as a global, so it doesn't have to be
  set up on the stack each time
- delegated timestamp support has been added
- Link to v1: https://lore.kernel.org/r/20240816-delstid-v1-0-c221c3dc14cd@kernel.org

---
Jeff Layton (11):
      nfsd: drop the ncf_cb_bmap field
      nfsd: drop the nfsd4_fattr_args "size" field
      nfsd: have nfsd4_deleg_getattr_conflict pass back write deleg pointer
      nfsd: add pragma public to delegated timestamp types
      nfsd: fix reported change attr on a write delegation
      nfs_common: make nfs4.h include generated nfs4_1.h
      nfsd: add support for FATTR4_OPEN_ARGUMENTS
      nfsd: implement OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION
      fs: handle delegated timestamps in setattr_copy_mgtime
      nfsd: add support for delegated timestamps
      nfsd: handle delegated timestamps in SETATTR

NeilBrown (2):
      nfsd: fix nfsd4_deleg_getattr_conflict in presence of third party lease
      nfsd: untangle code in nfsd4_deleg_getattr_conflict()

 {fs/nfsd => Documentation/sunrpc/xdr}/nfs4_1.x     |   2 +
 MAINTAINERS                                        |   1 +
 fs/attr.c                                          |  28 ++-
 fs/inode.c                                         |  74 +++++++
 fs/nfsd/Makefile                                   |   2 +-
 fs/nfsd/nfs4callback.c                             |  43 +++-
 fs/nfsd/nfs4proc.c                                 |  29 ++-
 fs/nfsd/nfs4state.c                                | 241 ++++++++++++++-------
 fs/nfsd/nfs4xdr.c                                  | 117 ++++++++--
 fs/nfsd/nfs4xdr_gen.c                              |  12 +-
 fs/nfsd/nfsd.h                                     |   5 +-
 fs/nfsd/state.h                                    |   6 +-
 fs/nfsd/xdr4cb.h                                   |  10 +-
 include/linux/fs.h                                 |   2 +
 include/linux/nfs4.h                               |   7 +-
 include/linux/nfs_xdr.h                            |   5 -
 .../linux/sunrpc/xdrgen/nfs4_1.h                   |  14 +-
 include/linux/time64.h                             |   5 +
 include/uapi/linux/nfs4.h                          |   7 +-
 19 files changed, 469 insertions(+), 141 deletions(-)
---
base-commit: 30e4927dc0506504ddbfed51698bc5f37b17343a
change-id: 20240815-delstid-93290691ad11

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v3 01/13] nfsd: fix nfsd4_deleg_getattr_conflict in presence of third party lease

[Jeff Layton]

From: NeilBrown <neilb@suse.de>

It is not safe to dereference fl->c.flc_owner without first confirming
fl->fl_lmops is the expected manager.  nfsd4_deleg_getattr_conflict()
tests fl_lmops but largely ignores the result and assumes that flc_owner
is an nfs4_delegation anyway.  This is wrong.

With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave
as it did before the changed mentioned below.  This the same as the
current code, but without any reference to a possible delegation.

Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
Signed-off-by: NeilBrown <neilb@suse.de>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b6bf39c64d78..eaa11d42d1b1 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8854,7 +8854,15 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 			 */
 			if (type == F_RDLCK)
 				break;
-			goto break_lease;
+
+			nfsd_stats_wdeleg_getattr_inc(nn);
+			spin_unlock(&ctx->flc_lock);
+
+			status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
+			if (status != nfserr_jukebox ||
+			    !nfsd_wait_for_delegreturn(rqstp, inode))
+				return status;
+			return 0;
 		}
 		if (type == F_WRLCK) {
 			struct nfs4_delegation *dp = fl->c.flc_owner;
@@ -8863,7 +8871,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 				spin_unlock(&ctx->flc_lock);
 				return 0;
 			}
-break_lease:
 			nfsd_stats_wdeleg_getattr_inc(nn);
 			dp = fl->c.flc_owner;
 			refcount_inc(&dp->dl_stid.sc_count);

-- 
2.46.0

Re: [PATCH v3 01/13] nfsd: fix nfsd4_deleg_getattr_conflict in presence of third party lease

[Chuck Lever]

On Thu, Aug 29, 2024 at 09:26:39AM -0400, Jeff Layton wrote:
> From: NeilBrown <neilb@suse.de>
> 
> It is not safe to dereference fl->c.flc_owner without first confirming
> fl->fl_lmops is the expected manager.  nfsd4_deleg_getattr_conflict()
> tests fl_lmops but largely ignores the result and assumes that flc_owner
> is an nfs4_delegation anyway.  This is wrong.
> 
> With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave
> as it did before the changed mentioned below.  This the same as the
> current code, but without any reference to a possible delegation.
> 
> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> Signed-off-by: NeilBrown <neilb@suse.de>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

I've already applied this to nfsd-fixes.

If I include this commit in both nfsd-fixes and nfsd-next then the
linux-next merge whines about duplicate patches. Stephen Rothwell
suggested git-merging nfsd-fixes and nfsd-next but I'm not quite
confident enough to try that.

Barring another solution, merging this series will have to wait a
few days before the two trees can sync up.


> ---
>  fs/nfsd/nfs4state.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b6bf39c64d78..eaa11d42d1b1 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -8854,7 +8854,15 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  			 */
>  			if (type == F_RDLCK)
>  				break;
> -			goto break_lease;
> +
> +			nfsd_stats_wdeleg_getattr_inc(nn);
> +			spin_unlock(&ctx->flc_lock);
> +
> +			status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> +			if (status != nfserr_jukebox ||
> +			    !nfsd_wait_for_delegreturn(rqstp, inode))
> +				return status;
> +			return 0;
>  		}
>  		if (type == F_WRLCK) {
>  			struct nfs4_delegation *dp = fl->c.flc_owner;
> @@ -8863,7 +8871,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  				spin_unlock(&ctx->flc_lock);
>  				return 0;
>  			}
> -break_lease:
>  			nfsd_stats_wdeleg_getattr_inc(nn);
>  			dp = fl->c.flc_owner;
>  			refcount_inc(&dp->dl_stid.sc_count);
> 
> -- 
> 2.46.0
> 

-- 
Chuck Lever

Re: [PATCH v3 01/13] nfsd: fix nfsd4_deleg_getattr_conflict in presence of third party lease

[NeilBrown]

On Fri, 30 Aug 2024, Chuck Lever wrote:
> On Thu, Aug 29, 2024 at 09:26:39AM -0400, Jeff Layton wrote:
> > From: NeilBrown <neilb@suse.de>
> > 
> > It is not safe to dereference fl->c.flc_owner without first confirming
> > fl->fl_lmops is the expected manager.  nfsd4_deleg_getattr_conflict()
> > tests fl_lmops but largely ignores the result and assumes that flc_owner
> > is an nfs4_delegation anyway.  This is wrong.
> > 
> > With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave
> > as it did before the changed mentioned below.  This the same as the
> > current code, but without any reference to a possible delegation.
> > 
> > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> I've already applied this to nfsd-fixes.
> 
> If I include this commit in both nfsd-fixes and nfsd-next then the
> linux-next merge whines about duplicate patches. Stephen Rothwell
> suggested git-merging nfsd-fixes and nfsd-next but I'm not quite
> confident enough to try that.
> 
> Barring another solution, merging this series will have to wait a
> few days before the two trees can sync up.

Hmmm....  I would probably always rebase nfsd-next on nfsd-fixes, which
I would rebase on the most recent of rc0, rc1, or the latest rc to
receive nfsd patches.

nfsd-fixes is currently based on 6.10-rc7, while -next is based on
6.11-rc5.

Why the 6.10 base??

NeilBrown

Re: [PATCH v3 01/13] nfsd: fix nfsd4_deleg_getattr_conflict in presence of third party lease

[Chuck Lever III]

> On Aug 30, 2024, at 2:01 AM, NeilBrown <neilb@suse.de> wrote:
> 
> On Fri, 30 Aug 2024, Chuck Lever wrote:
>> On Thu, Aug 29, 2024 at 09:26:39AM -0400, Jeff Layton wrote:
>>> From: NeilBrown <neilb@suse.de>
>>> 
>>> It is not safe to dereference fl->c.flc_owner without first confirming
>>> fl->fl_lmops is the expected manager.  nfsd4_deleg_getattr_conflict()
>>> tests fl_lmops but largely ignores the result and assumes that flc_owner
>>> is an nfs4_delegation anyway.  This is wrong.
>>> 
>>> With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave
>>> as it did before the changed mentioned below.  This the same as the
>>> current code, but without any reference to a possible delegation.
>>> 
>>> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> 
>> I've already applied this to nfsd-fixes.
>> 
>> If I include this commit in both nfsd-fixes and nfsd-next then the
>> linux-next merge whines about duplicate patches. Stephen Rothwell
>> suggested git-merging nfsd-fixes and nfsd-next but I'm not quite
>> confident enough to try that.
>> 
>> Barring another solution, merging this series will have to wait a
>> few days before the two trees can sync up.
> 
> Hmmm....  I would probably always rebase nfsd-next on nfsd-fixes, which
> I would rebase on the most recent of rc0, rc1, or the latest rc to
> receive nfsd patches.

That straightforward strategy leaves NFSD fixes scattered
throughout the merge history.


> nfsd-fixes is currently based on 6.10-rc7, while -next is based on
> 6.11-rc5.
> 
> Why the 6.10 base??

nfsd-fixes extends the branch I initially submitted to Linus
for the last merge window. That makes it easy to locate the
full set of NFSD commits that go into each kernel release
in the order they were submitted and keeps the merge
topology simpler.

      B - C - D - E       <<< nfsd
     /         \   \
  - A - X - Y - Z - AA -  <<< master

Or, that's the theory anyway. And generally it's not an
irritant except for right near the end of the development
cycle.

I'm not 100% wedded to this approach, and I am interested
in discussion and improvement.

Stephen Rothwell suggested that I provide a single merged
branch for development and for linux-next to pull. I almost
understand how to do that, but it's still a bit beyond my
current everyday tooling (stgit).


--
Chuck Lever

[PATCH v3 02/13] nfsd: untangle code in nfsd4_deleg_getattr_conflict()

[Jeff Layton]

From: NeilBrown <neilb@suse.de>

The code in nfsd4_deleg_getattr_conflict() is convoluted and buggy.

With this patch we:
 - properly handle non-nfsd leases.  We must not assume flc_owner is a
    delegation unless fl_lmops == &nfsd_lease_mng_ops
 - move the main code out of the for loop
 - have a single exit which calls nfs4_put_stid()
   (and other exits which don't need to call that)

[ jlayton: refactored on top of Neil's other patch: nfsd: fix
	   nfsd4_deleg_getattr_conflict in presence of third party lease ]

Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
Signed-off-by: NeilBrown <neilb@suse.de>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 131 +++++++++++++++++++++++++---------------------------
 1 file changed, 62 insertions(+), 69 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index eaa11d42d1b1..8835930ecee6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8831,6 +8831,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 	__be32 status;
 	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
 	struct file_lock_context *ctx;
+	struct nfs4_delegation *dp = NULL;
 	struct file_lease *fl;
 	struct iattr attrs;
 	struct nfs4_cb_fattr *ncf;
@@ -8840,84 +8841,76 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 	ctx = locks_inode_context(inode);
 	if (!ctx)
 		return 0;
+
+#define NON_NFSD_LEASE ((void *)1)
+
 	spin_lock(&ctx->flc_lock);
 	for_each_file_lock(fl, &ctx->flc_lease) {
-		unsigned char type = fl->c.flc_type;
-
 		if (fl->c.flc_flags == FL_LAYOUT)
 			continue;
-		if (fl->fl_lmops != &nfsd_lease_mng_ops) {
-			/*
-			 * non-nfs lease, if it's a lease with F_RDLCK then
-			 * we are done; there isn't any write delegation
-			 * on this inode
-			 */
-			if (type == F_RDLCK)
-				break;
-
-			nfsd_stats_wdeleg_getattr_inc(nn);
-			spin_unlock(&ctx->flc_lock);
-
-			status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
+		if (fl->c.flc_type == F_WRLCK) {
+			if (fl->fl_lmops == &nfsd_lease_mng_ops)
+				dp = fl->c.flc_owner;
+			else
+				dp = NON_NFSD_LEASE;
+		}
+		break;
+	}
+	if (dp == NULL || dp == NON_NFSD_LEASE ||
+	    dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
+		spin_unlock(&ctx->flc_lock);
+		if (dp == NON_NFSD_LEASE) {
+			status = nfserrno(nfsd_open_break_lease(inode,
+								NFSD_MAY_READ));
 			if (status != nfserr_jukebox ||
 			    !nfsd_wait_for_delegreturn(rqstp, inode))
 				return status;
-			return 0;
 		}
-		if (type == F_WRLCK) {
-			struct nfs4_delegation *dp = fl->c.flc_owner;
+		return 0;
+	}
 
-			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
-				spin_unlock(&ctx->flc_lock);
-				return 0;
-			}
-			nfsd_stats_wdeleg_getattr_inc(nn);
-			dp = fl->c.flc_owner;
-			refcount_inc(&dp->dl_stid.sc_count);
-			ncf = &dp->dl_cb_fattr;
-			nfs4_cb_getattr(&dp->dl_cb_fattr);
-			spin_unlock(&ctx->flc_lock);
-			wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
-					TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
-			if (ncf->ncf_cb_status) {
-				/* Recall delegation only if client didn't respond */
-				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
-				if (status != nfserr_jukebox ||
-						!nfsd_wait_for_delegreturn(rqstp, inode)) {
-					nfs4_put_stid(&dp->dl_stid);
-					return status;
-				}
-			}
-			if (!ncf->ncf_file_modified &&
-					(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
-					ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
-				ncf->ncf_file_modified = true;
-			if (ncf->ncf_file_modified) {
-				int err;
-
-				/*
-				 * Per section 10.4.3 of RFC 8881, the server would
-				 * not update the file's metadata with the client's
-				 * modified size
-				 */
-				attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
-				attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
-				inode_lock(inode);
-				err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
-				inode_unlock(inode);
-				if (err) {
-					nfs4_put_stid(&dp->dl_stid);
-					return nfserrno(err);
-				}
-				ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
-				*size = ncf->ncf_cur_fsize;
-				*modified = true;
-			}
-			nfs4_put_stid(&dp->dl_stid);
-			return 0;
+	nfsd_stats_wdeleg_getattr_inc(nn);
+	refcount_inc(&dp->dl_stid.sc_count);
+	ncf = &dp->dl_cb_fattr;
+	nfs4_cb_getattr(&dp->dl_cb_fattr);
+	spin_unlock(&ctx->flc_lock);
+
+	wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
+			    TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
+	if (ncf->ncf_cb_status) {
+		/* Recall delegation only if client didn't respond */
+		status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
+		if (status != nfserr_jukebox ||
+		    !nfsd_wait_for_delegreturn(rqstp, inode))
+			goto out_status;
+	}
+	if (!ncf->ncf_file_modified &&
+	    (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
+	     ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
+		ncf->ncf_file_modified = true;
+	if (ncf->ncf_file_modified) {
+		int err;
+
+		/*
+		 * Per section 10.4.3 of RFC 8881, the server would
+		 * not update the file's metadata with the client's
+		 * modified size
+		 */
+		attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
+		attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
+		inode_lock(inode);
+		err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
+		inode_unlock(inode);
+		if (err) {
+			status = nfserrno(err);
+			goto out_status;
 		}
-		break;
+		ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
+		*size = ncf->ncf_cur_fsize;
+		*modified = true;
 	}
-	spin_unlock(&ctx->flc_lock);
-	return 0;
+	status = 0;
+out_status:
+	nfs4_put_stid(&dp->dl_stid);
+	return status;
 }

-- 
2.46.0

[PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

[Jeff Layton]

This is always the same value, and in a later patch we're going to need
to set bits in WORD2. We can simplify this code and save a little space
in the delegation too. Just hardcode the bitmap in the callback encode
function.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4callback.c | 5 ++++-
 fs/nfsd/nfs4state.c    | 1 -
 fs/nfsd/state.h        | 1 -
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index b5b3ab9d719a..0c49e31d4350 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -364,10 +364,13 @@ encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
 	struct nfs4_delegation *dp =
 		container_of(fattr, struct nfs4_delegation, dl_cb_fattr);
 	struct knfsd_fh *fh = &dp->dl_stid.sc_file->fi_fhandle;
+	u32 bmap[1];
+
+	bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
 
 	encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
 	encode_nfs_fh4(xdr, fh);
-	encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
+	encode_bitmap4(xdr, bmap, ARRAY_SIZE(bmap));
 	hdr->nops++;
 }
 
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8835930ecee6..6844ae9ea350 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1183,7 +1183,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
 	nfsd4_init_cb(&dp->dl_cb_fattr.ncf_getattr, dp->dl_stid.sc_client,
 			&nfsd4_cb_getattr_ops, NFSPROC4_CLNT_CB_GETATTR);
 	dp->dl_cb_fattr.ncf_file_modified = false;
-	dp->dl_cb_fattr.ncf_cb_bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
 	get_nfs4_file(fp);
 	dp->dl_stid.sc_file = fp;
 	return dp;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 79c743c01a47..ac3a29224806 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -138,7 +138,6 @@ struct nfs4_cpntf_state {
 struct nfs4_cb_fattr {
 	struct nfsd4_callback ncf_getattr;
 	u32 ncf_cb_status;
-	u32 ncf_cb_bmap[1];
 
 	/* from CB_GETATTR reply */
 	u64 ncf_cb_change;

-- 
2.46.0

Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

[Chuck Lever]

On Thu, Aug 29, 2024 at 09:26:41AM -0400, Jeff Layton wrote:
> This is always the same value, and in a later patch we're going to need
> to set bits in WORD2. We can simplify this code and save a little space
> in the delegation too. Just hardcode the bitmap in the callback encode
> function.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

OK, lurching forward on this ;-)

 - The first patch in v3 was applied to v6.11-rc.
 - The second patch is now in nfsd-next.
 - I've squashed the sixth and eighth patches into nfsd-next.

I'm replying to this patch because this one seems like a step
backwards to me: the bitmap values are implementation-dependent,
and this code will eventually be automatically generated based only
on the protocol, not the local implementation. IMO, architecturally,
bitmap values should be set at the proc layer, not in the encoders.

I've gone back and forth on whether to just go with it for now, but
I thought I'd mention it here to see if this change is truly
necessary for what follows. If it can't be replaced, I will suck it
up and fix it up later when this encoder is converted to an xdrgen-
manufactured one.

I've tried to apply a few of the following patches in this series,
but by 9/13, things stop compiling. So I will let you rebase your
work on the current nfsd-next and redrive it, and we can go from
there.


> ---
>  fs/nfsd/nfs4callback.c | 5 ++++-
>  fs/nfsd/nfs4state.c    | 1 -
>  fs/nfsd/state.h        | 1 -
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index b5b3ab9d719a..0c49e31d4350 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -364,10 +364,13 @@ encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
>  	struct nfs4_delegation *dp =
>  		container_of(fattr, struct nfs4_delegation, dl_cb_fattr);
>  	struct knfsd_fh *fh = &dp->dl_stid.sc_file->fi_fhandle;
> +	u32 bmap[1];
> +
> +	bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
>  
>  	encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
>  	encode_nfs_fh4(xdr, fh);
> -	encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
> +	encode_bitmap4(xdr, bmap, ARRAY_SIZE(bmap));
>  	hdr->nops++;
>  }
>  
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 8835930ecee6..6844ae9ea350 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1183,7 +1183,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>  	nfsd4_init_cb(&dp->dl_cb_fattr.ncf_getattr, dp->dl_stid.sc_client,
>  			&nfsd4_cb_getattr_ops, NFSPROC4_CLNT_CB_GETATTR);
>  	dp->dl_cb_fattr.ncf_file_modified = false;
> -	dp->dl_cb_fattr.ncf_cb_bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
>  	get_nfs4_file(fp);
>  	dp->dl_stid.sc_file = fp;
>  	return dp;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 79c743c01a47..ac3a29224806 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -138,7 +138,6 @@ struct nfs4_cpntf_state {
>  struct nfs4_cb_fattr {
>  	struct nfsd4_callback ncf_getattr;
>  	u32 ncf_cb_status;
> -	u32 ncf_cb_bmap[1];
>  
>  	/* from CB_GETATTR reply */
>  	u64 ncf_cb_change;
> 
> -- 
> 2.46.0
> 



-- 
Chuck Lever

Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

[Jeff Layton]

On Wed, 2024-09-04 at 11:20 -0400, Chuck Lever wrote:
> On Thu, Aug 29, 2024 at 09:26:41AM -0400, Jeff Layton wrote:
> > This is always the same value, and in a later patch we're going to need
> > to set bits in WORD2. We can simplify this code and save a little space
> > in the delegation too. Just hardcode the bitmap in the callback encode
> > function.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> OK, lurching forward on this ;-)
> 
>  - The first patch in v3 was applied to v6.11-rc.
>  - The second patch is now in nfsd-next.
>  - I've squashed the sixth and eighth patches into nfsd-next.
> 
> I'm replying to this patch because this one seems like a step
> backwards to me: the bitmap values are implementation-dependent,
> and this code will eventually be automatically generated based only
> on the protocol, not the local implementation. IMO, architecturally,
> bitmap values should be set at the proc layer, not in the encoders.
> 
> I've gone back and forth on whether to just go with it for now, but
> I thought I'd mention it here to see if this change is truly
> necessary for what follows. If it can't be replaced, I will suck it
> up and fix it up later when this encoder is converted to an xdrgen-
> manufactured one.

It's not truly necessary, but I don't see why it's important that we
keep a record of the full-blown bitmap here. The ncf_cb_bmap field is a
field in the delegation record, and it has to be carried around in
perpetuity. This value is always set by the server and there are only a
few legit bit combinations that can be set in it.

We certainly can keep this bitmap around, but as I said in the
description, the delstid draft grows this bitmap to 3 words, and if we
want to be a purists about storing a bitmap, then that will also
require us to keep the bitmap size (in another 32-bit word), since we
don't always want to set anything in the third word. That's already 24
extra bits per delegation, and we'll be adding new fields for the
timestamps in a later patch.

I don't see the benefit here.

> I've tried to apply a few of the following patches in this series,
> but by 9/13, things stop compiling. So I will let you rebase your
> work on the current nfsd-next and redrive it, and we can go from
> there.
> 

Great. I'll rebase and re-test.

PS: I'm working on a couple of pynfs tests for CB_GETATTR as well. I'll
post them once I have them working. I've also found nfsd bugs in how we
currently proxy the attrs. I'll have patches for that in the next
posting (once my question on the IETF list is answered).

> 
> > ---
> >  fs/nfsd/nfs4callback.c | 5 ++++-
> >  fs/nfsd/nfs4state.c    | 1 -
> >  fs/nfsd/state.h        | 1 -
> >  3 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index b5b3ab9d719a..0c49e31d4350 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -364,10 +364,13 @@ encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
> >  	struct nfs4_delegation *dp =
> >  		container_of(fattr, struct nfs4_delegation, dl_cb_fattr);
> >  	struct knfsd_fh *fh = &dp->dl_stid.sc_file->fi_fhandle;
> > +	u32 bmap[1];
> > +
> > +	bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
> >  
> >  	encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
> >  	encode_nfs_fh4(xdr, fh);
> > -	encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
> > +	encode_bitmap4(xdr, bmap, ARRAY_SIZE(bmap));
> >  	hdr->nops++;
> >  }
> >  
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 8835930ecee6..6844ae9ea350 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1183,7 +1183,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
> >  	nfsd4_init_cb(&dp->dl_cb_fattr.ncf_getattr, dp->dl_stid.sc_client,
> >  			&nfsd4_cb_getattr_ops, NFSPROC4_CLNT_CB_GETATTR);
> >  	dp->dl_cb_fattr.ncf_file_modified = false;
> > -	dp->dl_cb_fattr.ncf_cb_bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
> >  	get_nfs4_file(fp);
> >  	dp->dl_stid.sc_file = fp;
> >  	return dp;
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index 79c743c01a47..ac3a29224806 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -138,7 +138,6 @@ struct nfs4_cpntf_state {
> >  struct nfs4_cb_fattr {
> >  	struct nfsd4_callback ncf_getattr;
> >  	u32 ncf_cb_status;
> > -	u32 ncf_cb_bmap[1];
> >  
> >  	/* from CB_GETATTR reply */
> >  	u64 ncf_cb_change;
> > 
> > -- 
> > 2.46.0
> > 
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

[Chuck Lever III]

> On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Wed, 2024-09-04 at 11:20 -0400, Chuck Lever wrote:
>> On Thu, Aug 29, 2024 at 09:26:41AM -0400, Jeff Layton wrote:
>>> This is always the same value, and in a later patch we're going to need
>>> to set bits in WORD2. We can simplify this code and save a little space
>>> in the delegation too. Just hardcode the bitmap in the callback encode
>>> function.
>>> 
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> 
>> OK, lurching forward on this ;-)
>> 
>> - The first patch in v3 was applied to v6.11-rc.
>> - The second patch is now in nfsd-next.
>> - I've squashed the sixth and eighth patches into nfsd-next.
>> 
>> I'm replying to this patch because this one seems like a step
>> backwards to me: the bitmap values are implementation-dependent,
>> and this code will eventually be automatically generated based only
>> on the protocol, not the local implementation. IMO, architecturally,
>> bitmap values should be set at the proc layer, not in the encoders.
>> 
>> I've gone back and forth on whether to just go with it for now, but
>> I thought I'd mention it here to see if this change is truly
>> necessary for what follows. If it can't be replaced, I will suck it
>> up and fix it up later when this encoder is converted to an xdrgen-
>> manufactured one.
> 
> It's not truly necessary, but I don't see why it's important that we
> keep a record of the full-blown bitmap here. The ncf_cb_bmap field is a
> field in the delegation record, and it has to be carried around in
> perpetuity. This value is always set by the server and there are only a
> few legit bit combinations that can be set in it.
> 
> We certainly can keep this bitmap around, but as I said in the
> description, the delstid draft grows this bitmap to 3 words, and if we
> want to be a purists about storing a bitmap,

Fwiw, it isn't purism about storing the bitmap, it's
purism about adopting machine-generated XDR marshaling/
unmarshaling code. The patch adds non-marshaling logic
in the encoder. Either we'll have to add a way to handle
that in xdrgen eventually, or we'll have to exclude this
encoder from machine generation. (This is a ways down the
road, of course)


> then that will also
> require us to keep the bitmap size (in another 32-bit word), since we
> don't always want to set anything in the third word. That's already 24
> extra bits per delegation, and we'll be adding new fields for the
> timestamps in a later patch.
> 
> I don't see the benefit here.

Understood, there's a memory scalability issue.

There are other ways to go about this that do not grow
the size of the delegation data structure, I think. For
instance, you could store the handful of actual valid
bitmap values in read-only memory, and have the proc
function select and reference one of them. IIRC the
client already does this for certain GETATTR operations.

So, leave this patch as is, and I will deal with it
later when we convert the callback XDR functions.


--
Chuck Lever

Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

[Jeff Layton]

On Wed, 2024-09-04 at 17:28 +0000, Chuck Lever III wrote:
> 
> > On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Wed, 2024-09-04 at 11:20 -0400, Chuck Lever wrote:
> > > On Thu, Aug 29, 2024 at 09:26:41AM -0400, Jeff Layton wrote:
> > > > This is always the same value, and in a later patch we're going to need
> > > > to set bits in WORD2. We can simplify this code and save a little space
> > > > in the delegation too. Just hardcode the bitmap in the callback encode
> > > > function.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > 
> > > OK, lurching forward on this ;-)
> > > 
> > > - The first patch in v3 was applied to v6.11-rc.
> > > - The second patch is now in nfsd-next.
> > > - I've squashed the sixth and eighth patches into nfsd-next.
> > > 
> > > I'm replying to this patch because this one seems like a step
> > > backwards to me: the bitmap values are implementation-dependent,
> > > and this code will eventually be automatically generated based only
> > > on the protocol, not the local implementation. IMO, architecturally,
> > > bitmap values should be set at the proc layer, not in the encoders.
> > > 
> > > I've gone back and forth on whether to just go with it for now, but
> > > I thought I'd mention it here to see if this change is truly
> > > necessary for what follows. If it can't be replaced, I will suck it
> > > up and fix it up later when this encoder is converted to an xdrgen-
> > > manufactured one.
> > 
> > It's not truly necessary, but I don't see why it's important that we
> > keep a record of the full-blown bitmap here. The ncf_cb_bmap field is a
> > field in the delegation record, and it has to be carried around in
> > perpetuity. This value is always set by the server and there are only a
> > few legit bit combinations that can be set in it.
> > 
> > We certainly can keep this bitmap around, but as I said in the
> > description, the delstid draft grows this bitmap to 3 words, and if we
> > want to be a purists about storing a bitmap,
> 
> Fwiw, it isn't purism about storing the bitmap, it's
> purism about adopting machine-generated XDR marshaling/
> unmarshaling code. The patch adds non-marshaling logic
> in the encoder. Either we'll have to add a way to handle
> that in xdrgen eventually, or we'll have to exclude this
> encoder from machine generation. (This is a ways down the
> road, of course)
>

Understood. I'll note that this function works with a nfs4_delegation
pointer too, which is not part of the protocol spec.

Once we move to autogenerated code, we will always have a hand-rolled
"glue" layer that morphs our internal representation of these sorts of
objects into a form that the xdrgen code requires. Storing this info as
a flag in the delegation makes more sense to me, as the glue layer
should massage that into the needed form.

> 
> > then that will also
> > require us to keep the bitmap size (in another 32-bit word), since we
> > don't always want to set anything in the third word. That's already 24
> > extra bits per delegation, and we'll be adding new fields for the
> > timestamps in a later patch.
> > 
> > I don't see the benefit here.
> 
> Understood, there's a memory scalability issue.
> 
> There are other ways to go about this that do not grow
> the size of the delegation data structure, I think. For
> instance, you could store the handful of actual valid
> bitmap values in read-only memory, and have the proc
> function select and reference one of them. IIRC the
> client already does this for certain GETATTR operations.
> 

Unfortunately, it turns out that the bitmap is a bit more complicated,
and we don't quite do it right today. I did a more careful read of
section 10.4.3 in RFC8881. It has this pseudocode:

    if (!modified) {
        do CB_GETATTR for change and size;

        if (cc != sc)
            modified = TRUE;
    } else {
        do CB_GETATTR for size;
    }

    if (modified) {
        sc = sc + 1;
        time_modify = time_metadata = current_time;
        update sc, time_modify, time_metadata into file's metadata;
    }

Note that if the thing is already considered to be modified, then it
says to not request FATTR4_CHANGE. I have a related question around
this on the IETF list too.

> So, leave this patch as is, and I will deal with it
> later when we convert the callback XDR functions.
> 

Thanks, will do.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

[Chuck Lever III]

> On Sep 4, 2024, at 1:39 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Wed, 2024-09-04 at 17:28 +0000, Chuck Lever III wrote:
>> 
>>> On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> On Wed, 2024-09-04 at 11:20 -0400, Chuck Lever wrote:
>>>> On Thu, Aug 29, 2024 at 09:26:41AM -0400, Jeff Layton wrote:
>>>>> This is always the same value, and in a later patch we're going to need
>>>>> to set bits in WORD2. We can simplify this code and save a little space
>>>>> in the delegation too. Just hardcode the bitmap in the callback encode
>>>>> function.
>>>>> 
>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>> 
>>>> OK, lurching forward on this ;-)
>>>> 
>>>> - The first patch in v3 was applied to v6.11-rc.
>>>> - The second patch is now in nfsd-next.
>>>> - I've squashed the sixth and eighth patches into nfsd-next.
>>>> 
>>>> I'm replying to this patch because this one seems like a step
>>>> backwards to me: the bitmap values are implementation-dependent,
>>>> and this code will eventually be automatically generated based only
>>>> on the protocol, not the local implementation. IMO, architecturally,
>>>> bitmap values should be set at the proc layer, not in the encoders.
>>>> 
>>>> I've gone back and forth on whether to just go with it for now, but
>>>> I thought I'd mention it here to see if this change is truly
>>>> necessary for what follows. If it can't be replaced, I will suck it
>>>> up and fix it up later when this encoder is converted to an xdrgen-
>>>> manufactured one.
>>> 
>>> It's not truly necessary, but I don't see why it's important that we
>>> keep a record of the full-blown bitmap here. The ncf_cb_bmap field is a
>>> field in the delegation record, and it has to be carried around in
>>> perpetuity. This value is always set by the server and there are only a
>>> few legit bit combinations that can be set in it.
>>> 
>>> We certainly can keep this bitmap around, but as I said in the
>>> description, the delstid draft grows this bitmap to 3 words, and if we
>>> want to be a purists about storing a bitmap,
>> 
>> Fwiw, it isn't purism about storing the bitmap, it's
>> purism about adopting machine-generated XDR marshaling/
>> unmarshaling code. The patch adds non-marshaling logic
>> in the encoder. Either we'll have to add a way to handle
>> that in xdrgen eventually, or we'll have to exclude this
>> encoder from machine generation. (This is a ways down the
>> road, of course)
>> 
> 
> Understood. I'll note that this function works with a nfs4_delegation
> pointer too, which is not part of the protocol spec.
> 
> Once we move to autogenerated code, we will always have a hand-rolled
> "glue" layer that morphs our internal representation of these sorts of
> objects into a form that the xdrgen code requires. Storing this info as
> a flag in the delegation makes more sense to me, as the glue layer
> should massage that into the needed form.

My thought was the existing proc functions would do
the massaging for us. But that's an abstract,
architectural kind of thought. I'm just starting to
integrate this idea into lockd.


--
Chuck Lever

Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

[NeilBrown]

On Thu, 05 Sep 2024, Chuck Lever III wrote:
> 
> 
> > On Sep 4, 2024, at 1:39 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Wed, 2024-09-04 at 17:28 +0000, Chuck Lever III wrote:
> >> 
> >>> On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
> >>> 
> >>> On Wed, 2024-09-04 at 11:20 -0400, Chuck Lever wrote:
> >>>> On Thu, Aug 29, 2024 at 09:26:41AM -0400, Jeff Layton wrote:
> >>>>> This is always the same value, and in a later patch we're going to need
> >>>>> to set bits in WORD2. We can simplify this code and save a little space
> >>>>> in the delegation too. Just hardcode the bitmap in the callback encode
> >>>>> function.
> >>>>> 
> >>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >>>> 
> >>>> OK, lurching forward on this ;-)
> >>>> 
> >>>> - The first patch in v3 was applied to v6.11-rc.
> >>>> - The second patch is now in nfsd-next.
> >>>> - I've squashed the sixth and eighth patches into nfsd-next.
> >>>> 
> >>>> I'm replying to this patch because this one seems like a step
> >>>> backwards to me: the bitmap values are implementation-dependent,
> >>>> and this code will eventually be automatically generated based only
> >>>> on the protocol, not the local implementation. IMO, architecturally,
> >>>> bitmap values should be set at the proc layer, not in the encoders.
> >>>> 
> >>>> I've gone back and forth on whether to just go with it for now, but
> >>>> I thought I'd mention it here to see if this change is truly
> >>>> necessary for what follows. If it can't be replaced, I will suck it
> >>>> up and fix it up later when this encoder is converted to an xdrgen-
> >>>> manufactured one.
> >>> 
> >>> It's not truly necessary, but I don't see why it's important that we
> >>> keep a record of the full-blown bitmap here. The ncf_cb_bmap field is a
> >>> field in the delegation record, and it has to be carried around in
> >>> perpetuity. This value is always set by the server and there are only a
> >>> few legit bit combinations that can be set in it.
> >>> 
> >>> We certainly can keep this bitmap around, but as I said in the
> >>> description, the delstid draft grows this bitmap to 3 words, and if we
> >>> want to be a purists about storing a bitmap,
> >> 
> >> Fwiw, it isn't purism about storing the bitmap, it's
> >> purism about adopting machine-generated XDR marshaling/
> >> unmarshaling code. The patch adds non-marshaling logic
> >> in the encoder. Either we'll have to add a way to handle
> >> that in xdrgen eventually, or we'll have to exclude this
> >> encoder from machine generation. (This is a ways down the
> >> road, of course)
> >> 
> > 
> > Understood. I'll note that this function works with a nfs4_delegation
> > pointer too, which is not part of the protocol spec.
> > 
> > Once we move to autogenerated code, we will always have a hand-rolled
> > "glue" layer that morphs our internal representation of these sorts of
> > objects into a form that the xdrgen code requires. Storing this info as
> > a flag in the delegation makes more sense to me, as the glue layer
> > should massage that into the needed form.
> 
> My thought was the existing proc functions would do
> the massaging for us. But that's an abstract,
> architectural kind of thought. I'm just starting to
> integrate this idea into lockd.

So presumably xdrgen defines a bunch of structs.  The proc code fills
those in (on the stack?) and passes them to the generated xdr code which
encodes them to the network stream??  That seems reasonable.

We still don't want ncf_cb_bmap in struct nfs4_cb_fattr, we only need it
in the generated struct.

Thanks,
NeilBrown

[PATCH v3 04/13] nfsd: drop the nfsd4_fattr_args "size" field

[Jeff Layton]

We already have a slot for this in the kstat structure. Just overwrite
that instead of keeping a copy.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4xdr.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index f118921250c3..d028daf77549 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2928,7 +2928,6 @@ struct nfsd4_fattr_args {
 	struct kstat		stat;
 	struct kstatfs		statfs;
 	struct nfs4_acl		*acl;
-	u64			size;
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
 	void			*context;
 	int			contextlen;
@@ -3047,7 +3046,7 @@ static __be32 nfsd4_encode_fattr4_change(struct xdr_stream *xdr,
 static __be32 nfsd4_encode_fattr4_size(struct xdr_stream *xdr,
 				       const struct nfsd4_fattr_args *args)
 {
-	return nfsd4_encode_uint64_t(xdr, args->size);
+	return nfsd4_encode_uint64_t(xdr, args->stat.size);
 }
 
 static __be32 nfsd4_encode_fattr4_fsid(struct xdr_stream *xdr,
@@ -3555,7 +3554,6 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 		if (status)
 			goto out;
 	}
-	args.size = 0;
 	if (attrmask[0] & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
 		status = nfsd4_deleg_getattr_conflict(rqstp, dentry,
 					&file_modified, &size);
@@ -3569,9 +3567,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 	if (err)
 		goto out_nfserr;
 	if (file_modified)
-		args.size = size;
-	else
-		args.size = args.stat.size;
+		args.stat.size = size;
 
 	if (!(args.stat.result_mask & STATX_BTIME))
 		/* underlying FS does not offer btime so we can't share it */

-- 
2.46.0

[PATCH v3 05/13] nfsd: have nfsd4_deleg_getattr_conflict pass back write deleg pointer

[Jeff Layton]

Currently we pass back the size and whether it has been modified, but
those just mirror values tracked inside the delegation. In a later
patch, we'll need to get at the timestamps in the delegation too, so
just pass back a reference to the write delegation, and use that to
properly override values in the iattr.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 17 ++++++++---------
 fs/nfsd/nfs4xdr.c   | 16 ++++++++++------
 fs/nfsd/state.h     |  2 +-
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6844ae9ea350..dce27420ae31 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8810,8 +8810,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
  * nfsd4_deleg_getattr_conflict - Recall if GETATTR causes conflict
  * @rqstp: RPC transaction context
  * @dentry: dentry of inode to be checked for a conflict
- * @modified: return true if file was modified
- * @size: new size of file if modified is true
+ * @pdp: returned WRITE delegation, if one was found
  *
  * This function is called when there is a conflict between a write
  * delegation and a change/size GETATTR from another client. The server
@@ -8821,11 +8820,12 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
  * 18.7.4.
  *
  * Returns 0 if there is no conflict; otherwise an nfs_stat
- * code is returned.
+ * code is returned. If @pdp is set to a non-NULL value, then the
+ * caller must put the reference.
  */
 __be32
 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
-				bool *modified, u64 *size)
+			     struct nfs4_delegation **pdp)
 {
 	__be32 status;
 	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
@@ -8836,10 +8836,9 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 	struct nfs4_cb_fattr *ncf;
 	struct inode *inode = d_inode(dentry);
 
-	*modified = false;
 	ctx = locks_inode_context(inode);
 	if (!ctx)
-		return 0;
+		return nfs_ok;
 
 #define NON_NFSD_LEASE ((void *)1)
 
@@ -8905,10 +8904,10 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 			goto out_status;
 		}
 		ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
-		*size = ncf->ncf_cur_fsize;
-		*modified = true;
+		*pdp = dp;
+		return nfs_ok;
 	}
-	status = 0;
+	status = nfs_ok;
 out_status:
 	nfs4_put_stid(&dp->dl_stid);
 	return status;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index d028daf77549..ccaee73de72b 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3511,6 +3511,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 		    int ignore_crossmnt)
 {
 	DECLARE_BITMAP(attr_bitmap, ARRAY_SIZE(nfsd4_enc_fattr4_encode_ops));
+	struct nfs4_delegation *dp = NULL;
 	struct nfsd4_fattr_args args;
 	struct svc_fh *tempfh = NULL;
 	int starting_len = xdr->buf->len;
@@ -3525,8 +3526,6 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 		.dentry	= dentry,
 	};
 	unsigned long bit;
-	bool file_modified = false;
-	u64 size = 0;
 
 	WARN_ON_ONCE(bmval[1] & NFSD_WRITEONLY_ATTRS_WORD1);
 	WARN_ON_ONCE(!nfsd_attrs_supported(minorversion, bmval));
@@ -3555,8 +3554,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 			goto out;
 	}
 	if (attrmask[0] & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
-		status = nfsd4_deleg_getattr_conflict(rqstp, dentry,
-					&file_modified, &size);
+		status = nfsd4_deleg_getattr_conflict(rqstp, dentry, &dp);
 		if (status)
 			goto out;
 	}
@@ -3564,10 +3562,16 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 	err = vfs_getattr(&path, &args.stat,
 			  STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
 			  AT_STATX_SYNC_AS_STAT);
+	if (dp) {
+		struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr;
+
+		if (ncf->ncf_file_modified)
+			args.stat.size = ncf->ncf_cur_fsize;
+
+		nfs4_put_stid(&dp->dl_stid);
+	}
 	if (err)
 		goto out_nfserr;
-	if (file_modified)
-		args.stat.size = size;
 
 	if (!(args.stat.result_mask & STATX_BTIME))
 		/* underlying FS does not offer btime so we can't share it */
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index ac3a29224806..c7c7ec21e510 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -781,5 +781,5 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
 }
 
 extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
-		struct dentry *dentry, bool *file_modified, u64 *size);
+		struct dentry *dentry, struct nfs4_delegation **pdp);
 #endif   /* NFSD4_STATE_H */

-- 
2.46.0

[PATCH v3 06/13] nfsd: add pragma public to delegated timestamp types

[Jeff Layton]

In a later patch we're going to need the ability to decode the delegated
timestamp fields.  Make the decoders available for the delgated
timestamp types, and regenerate the source and header files.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4_1.x      |  2 ++
 fs/nfsd/nfs4xdr_gen.c | 10 +++++-----
 fs/nfsd/nfs4xdr_gen.h |  8 +++++++-
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4_1.x b/fs/nfsd/nfs4_1.x
index d2fde450de5e..fc37d1ecba0f 100644
--- a/fs/nfsd/nfs4_1.x
+++ b/fs/nfsd/nfs4_1.x
@@ -150,6 +150,8 @@ const OPEN4_RESULT_NO_OPEN_STATEID = 0x00000010;
  */
 typedef nfstime4        fattr4_time_deleg_access;
 typedef nfstime4        fattr4_time_deleg_modify;
+pragma public 		fattr4_time_deleg_access;
+pragma public		fattr4_time_deleg_modify;
 
 
 %/*
diff --git a/fs/nfsd/nfs4xdr_gen.c b/fs/nfsd/nfs4xdr_gen.c
index 2503f58f2f47..6833d0ad35a8 100644
--- a/fs/nfsd/nfs4xdr_gen.c
+++ b/fs/nfsd/nfs4xdr_gen.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 // Generated by xdrgen. Manual edits will be lost.
-// XDR specification modification time: Tue Aug 27 12:10:19 2024
+// XDR specification modification time: Wed Aug 28 09:57:28 2024
 
 #include "nfs4xdr_gen.h"
 
@@ -120,13 +120,13 @@ xdrgen_decode_fattr4_open_arguments(struct xdr_stream *xdr, fattr4_open_argument
 	return xdrgen_decode_open_arguments4(xdr, ptr);
 };
 
-static bool __maybe_unused
+bool
 xdrgen_decode_fattr4_time_deleg_access(struct xdr_stream *xdr, fattr4_time_deleg_access *ptr)
 {
 	return xdrgen_decode_nfstime4(xdr, ptr);
 };
 
-static bool __maybe_unused
+bool
 xdrgen_decode_fattr4_time_deleg_modify(struct xdr_stream *xdr, fattr4_time_deleg_modify *ptr)
 {
 	return xdrgen_decode_nfstime4(xdr, ptr);
@@ -223,13 +223,13 @@ xdrgen_encode_fattr4_open_arguments(struct xdr_stream *xdr, const fattr4_open_ar
 	return xdrgen_encode_open_arguments4(xdr, value);
 };
 
-static bool __maybe_unused
+bool
 xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const fattr4_time_deleg_access *value)
 {
 	return xdrgen_encode_nfstime4(xdr, value);
 };
 
-static bool __maybe_unused
+bool
 xdrgen_encode_fattr4_time_deleg_modify(struct xdr_stream *xdr, const fattr4_time_deleg_modify *value)
 {
 	return xdrgen_encode_nfstime4(xdr, value);
diff --git a/fs/nfsd/nfs4xdr_gen.h b/fs/nfsd/nfs4xdr_gen.h
index edcc052626de..5465db4fb32b 100644
--- a/fs/nfsd/nfs4xdr_gen.h
+++ b/fs/nfsd/nfs4xdr_gen.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Generated by xdrgen. Manual edits will be lost. */
-/* XDR specification modification time: Tue Aug 27 12:10:19 2024 */
+/* XDR specification modification time: Wed Aug 28 09:57:28 2024 */
 
 #ifndef _LINUX_NFS4_XDRGEN_H
 #define _LINUX_NFS4_XDRGEN_H
@@ -88,8 +88,14 @@ enum { OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION = 0x200000 };
 enum { OPEN4_RESULT_NO_OPEN_STATEID = 0x00000010 };
 
 typedef struct nfstime4 fattr4_time_deleg_access;
+bool xdrgen_decode_fattr4_time_deleg_access(struct xdr_stream *xdr, fattr4_time_deleg_access *ptr);
+bool xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const fattr4_time_deleg_access *value);
+
 
 typedef struct nfstime4 fattr4_time_deleg_modify;
+bool xdrgen_decode_fattr4_time_deleg_modify(struct xdr_stream *xdr, fattr4_time_deleg_modify *ptr);
+bool xdrgen_encode_fattr4_time_deleg_modify(struct xdr_stream *xdr, const fattr4_time_deleg_modify *value);
+
 
 enum { FATTR4_TIME_DELEG_ACCESS = 84 };
 

-- 
2.46.0

Re: [PATCH v3 06/13] nfsd: add pragma public to delegated timestamp types

[Chuck Lever]

On Thu, Aug 29, 2024 at 09:26:44AM -0400, Jeff Layton wrote:
> In a later patch we're going to need the ability to decode the delegated
> timestamp fields.  Make the decoders available for the delgated
> timestamp types, and regenerate the source and header files.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4_1.x      |  2 ++
>  fs/nfsd/nfs4xdr_gen.c | 10 +++++-----
>  fs/nfsd/nfs4xdr_gen.h |  8 +++++++-
>  3 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4_1.x b/fs/nfsd/nfs4_1.x
> index d2fde450de5e..fc37d1ecba0f 100644
> --- a/fs/nfsd/nfs4_1.x
> +++ b/fs/nfsd/nfs4_1.x
> @@ -150,6 +150,8 @@ const OPEN4_RESULT_NO_OPEN_STATEID = 0x00000010;
>   */
>  typedef nfstime4        fattr4_time_deleg_access;
>  typedef nfstime4        fattr4_time_deleg_modify;
> +pragma public 		fattr4_time_deleg_access;
> +pragma public		fattr4_time_deleg_modify;
>  
>  
>  %/*
> diff --git a/fs/nfsd/nfs4xdr_gen.c b/fs/nfsd/nfs4xdr_gen.c
> index 2503f58f2f47..6833d0ad35a8 100644
> --- a/fs/nfsd/nfs4xdr_gen.c
> +++ b/fs/nfsd/nfs4xdr_gen.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Generated by xdrgen. Manual edits will be lost.
> -// XDR specification modification time: Tue Aug 27 12:10:19 2024
> +// XDR specification modification time: Wed Aug 28 09:57:28 2024
>  
>  #include "nfs4xdr_gen.h"
>  
> @@ -120,13 +120,13 @@ xdrgen_decode_fattr4_open_arguments(struct xdr_stream *xdr, fattr4_open_argument
>  	return xdrgen_decode_open_arguments4(xdr, ptr);
>  };
>  
> -static bool __maybe_unused
> +bool
>  xdrgen_decode_fattr4_time_deleg_access(struct xdr_stream *xdr, fattr4_time_deleg_access *ptr)
>  {
>  	return xdrgen_decode_nfstime4(xdr, ptr);
>  };
>  
> -static bool __maybe_unused
> +bool
>  xdrgen_decode_fattr4_time_deleg_modify(struct xdr_stream *xdr, fattr4_time_deleg_modify *ptr)
>  {
>  	return xdrgen_decode_nfstime4(xdr, ptr);
> @@ -223,13 +223,13 @@ xdrgen_encode_fattr4_open_arguments(struct xdr_stream *xdr, const fattr4_open_ar
>  	return xdrgen_encode_open_arguments4(xdr, value);
>  };
>  
> -static bool __maybe_unused
> +bool
>  xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const fattr4_time_deleg_access *value)
>  {
>  	return xdrgen_encode_nfstime4(xdr, value);
>  };
>  
> -static bool __maybe_unused
> +bool
>  xdrgen_encode_fattr4_time_deleg_modify(struct xdr_stream *xdr, const fattr4_time_deleg_modify *value)
>  {
>  	return xdrgen_encode_nfstime4(xdr, value);
> diff --git a/fs/nfsd/nfs4xdr_gen.h b/fs/nfsd/nfs4xdr_gen.h
> index edcc052626de..5465db4fb32b 100644
> --- a/fs/nfsd/nfs4xdr_gen.h
> +++ b/fs/nfsd/nfs4xdr_gen.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /* Generated by xdrgen. Manual edits will be lost. */
> -/* XDR specification modification time: Tue Aug 27 12:10:19 2024 */
> +/* XDR specification modification time: Wed Aug 28 09:57:28 2024 */
>  
>  #ifndef _LINUX_NFS4_XDRGEN_H
>  #define _LINUX_NFS4_XDRGEN_H
> @@ -88,8 +88,14 @@ enum { OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION = 0x200000 };
>  enum { OPEN4_RESULT_NO_OPEN_STATEID = 0x00000010 };
>  
>  typedef struct nfstime4 fattr4_time_deleg_access;
> +bool xdrgen_decode_fattr4_time_deleg_access(struct xdr_stream *xdr, fattr4_time_deleg_access *ptr);
> +bool xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const fattr4_time_deleg_access *value);
> +
>  
>  typedef struct nfstime4 fattr4_time_deleg_modify;
> +bool xdrgen_decode_fattr4_time_deleg_modify(struct xdr_stream *xdr, fattr4_time_deleg_modify *ptr);
> +bool xdrgen_encode_fattr4_time_deleg_modify(struct xdr_stream *xdr, const fattr4_time_deleg_modify *value);
> +
>  
>  enum { FATTR4_TIME_DELEG_ACCESS = 84 };
>  
> 
> -- 
> 2.46.0
> 

I can squash this into 2/2 of the xdrgen series in the meantime.

-- 
Chuck Lever

[PATCH v3 07/13] nfsd: fix reported change attr on a write delegation

[Jeff Layton]

If there's a write deleg outstanding and there are no writes yet from
the client, we'll currently just report the old, original change attr.

RFC 8881, section 10.4.3 describes a way to report the change attribute.

When there are no writes from the client yet, but the reported change
attribute in a CB_GETATTR indicates that the file has been modified,
then report the initial change attr + 1.

Once there have been writes from the client, use the value in the inode
instead.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4xdr.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index ccaee73de72b..a9827bb8a2f0 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2928,6 +2928,7 @@ struct nfsd4_fattr_args {
 	struct kstat		stat;
 	struct kstatfs		statfs;
 	struct nfs4_acl		*acl;
+	u64			change_attr;
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
 	void			*context;
 	int			contextlen;
@@ -3027,7 +3028,6 @@ static __be32 nfsd4_encode_fattr4_change(struct xdr_stream *xdr,
 					 const struct nfsd4_fattr_args *args)
 {
 	const struct svc_export *exp = args->exp;
-	u64 c;
 
 	if (unlikely(exp->ex_flags & NFSEXP_V4ROOT)) {
 		u32 flush_time = convert_to_wallclock(exp->cd->flush_time);
@@ -3038,9 +3038,7 @@ static __be32 nfsd4_encode_fattr4_change(struct xdr_stream *xdr,
 			return nfserr_resource;
 		return nfs_ok;
 	}
-
-	c = nfsd4_change_attribute(&args->stat, d_inode(args->dentry));
-	return nfsd4_encode_changeid4(xdr, c);
+	return nfsd4_encode_changeid4(xdr, args->change_attr);
 }
 
 static __be32 nfsd4_encode_fattr4_size(struct xdr_stream *xdr,
@@ -3562,12 +3560,16 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 	err = vfs_getattr(&path, &args.stat,
 			  STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
 			  AT_STATX_SYNC_AS_STAT);
+	args.change_attr = nfsd4_change_attribute(&args.stat, d_inode(dentry));
 	if (dp) {
 		struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr;
 
-		if (ncf->ncf_file_modified)
+		if (ncf->ncf_file_modified) {
 			args.stat.size = ncf->ncf_cur_fsize;
-
+			/* If there have been no changes, report the initial cinfo + 1 */
+			if (args.change_attr == ncf->ncf_initial_cinfo)
+				args.change_attr = ncf->ncf_initial_cinfo + 1;
+		}
 		nfs4_put_stid(&dp->dl_stid);
 	}
 	if (err)

-- 
2.46.0

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

[Jeff Layton]

Long term, we'd like to move to autogenerating a lot of our XDR code.
Both the client and server include include/linux/nfs4.h. That file is
hand-rolled and some of the symbols in it conflict with the
autogenerated symbols from the spec.

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

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

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

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

-- 
2.46.0

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

[Chuck Lever]

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

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


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

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

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


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

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


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

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

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


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

Ditto. Resist The Urge (tm).


>  #include <linux/types.h>
>  #include <linux/sunrpc/svc.h>
> @@ -103,4 +103,4 @@ enum { FATTR4_TIME_DELEG_MODIFY = 85 };
>  
>  enum { OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 0x100000 };
>  
> -#endif /* _LINUX_NFS4_XDRGEN_H */
> +#endif /* _LINUX_XDRGEN_NFS4_H */
> 
> -- 
> 2.46.0
> 

-- 
Chuck Lever

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

[Chuck Lever]

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

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


> >  #include <linux/types.h>
> >  #include <linux/sunrpc/svc.h>
> > @@ -103,4 +103,4 @@ enum { FATTR4_TIME_DELEG_MODIFY = 85 };
> >  
> >  enum { OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 0x100000 };
> >  
> > -#endif /* _LINUX_NFS4_XDRGEN_H */
> > +#endif /* _LINUX_XDRGEN_NFS4_H */

-- 
Chuck Lever

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

[Jeff Layton]

On Thu, 2024-08-29 at 11:13 -0400, Chuck Lever wrote:
> On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
> > Long term, we'd like to move to autogenerating a lot of our XDR code.
> > Both the client and server include include/linux/nfs4.h. That file is
> > hand-rolled and some of the symbols in it conflict with the
> > autogenerated symbols from the spec.
> > 
> > Move nfs4_1.x to Documentation/sunrpc/xdr.
> 
> I can change 2/2 in the xdrgen series to land this file under
> Documentation/sunrpc/xdr.
> 

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

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

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

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

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

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


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

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

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

No problem. This part is a Q&D hack job to get everything working with
minimal changes. Changing the tool to generate the right thing would be
a better long-term solution (once we settle on where these files will
go, etc.)
 
> >  static bool __maybe_unused
> >  xdrgen_decode_int64_t(struct xdr_stream *xdr, int64_t *ptr)
> > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> > index 8d7430d9f218..b90719244775 100644
> > --- a/include/linux/nfs4.h
> > +++ b/include/linux/nfs4.h
> > @@ -17,6 +17,7 @@
> >  #include <linux/uidgid.h>
> >  #include <uapi/linux/nfs4.h>
> >  #include <linux/sunrpc/msg_prot.h>
> > +#include <linux/sunrpc/xdrgen/nfs4_1.h>
> >  
> >  enum nfs4_acl_whotype {
> >  	NFS4_ACL_WHO_NAMED = 0,
> > @@ -512,12 +513,6 @@ enum {
> >  	FATTR4_XATTR_SUPPORT		= 82,
> >  };
> >  
> > -enum {
> > -	FATTR4_TIME_DELEG_ACCESS	= 84,
> > -	FATTR4_TIME_DELEG_MODIFY	= 85,
> > -	FATTR4_OPEN_ARGUMENTS		= 86,
> > -};
> > -
> >  /*
> >   * The following internal definitions enable processing the above
> >   * attribute bits within 32-bit word boundaries.
> > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > index 45623af3e7b8..d3fe47baf110 100644
> > --- a/include/linux/nfs_xdr.h
> > +++ b/include/linux/nfs_xdr.h
> > @@ -1315,11 +1315,6 @@ struct nfs4_fsid_present_res {
> >  
> >  #endif /* CONFIG_NFS_V4 */
> >  
> > -struct nfstime4 {
> > -	u64	seconds;
> > -	u32	nseconds;
> > -};
> > -
> >  #ifdef CONFIG_NFS_V4_1
> >  
> >  struct pnfs_commit_bucket {
> > diff --git a/fs/nfsd/nfs4xdr_gen.h b/include/linux/sunrpc/xdrgen/nfs4_1.h
> > similarity index 96%
> > rename from fs/nfsd/nfs4xdr_gen.h
> > rename to include/linux/sunrpc/xdrgen/nfs4_1.h
> > index 5465db4fb32b..5faee67281b8 100644
> > --- a/fs/nfsd/nfs4xdr_gen.h
> > +++ b/include/linux/sunrpc/xdrgen/nfs4_1.h
> > @@ -2,8 +2,8 @@
> >  /* Generated by xdrgen. Manual edits will be lost. */
> >  /* XDR specification modification time: Wed Aug 28 09:57:28 2024 */
> >  
> > -#ifndef _LINUX_NFS4_XDRGEN_H
> > -#define _LINUX_NFS4_XDRGEN_H
> > +#ifndef _LINUX_XDRGEN_NFS4_H
> > +#define _LINUX_XDRGEN_NFS4_H
> 
> Ditto. Resist The Urge (tm).
> 
> 
> >  #include <linux/types.h>
> >  #include <linux/sunrpc/svc.h>
> > @@ -103,4 +103,4 @@ enum { FATTR4_TIME_DELEG_MODIFY = 85 };
> >  
> >  enum { OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 0x100000 };
> >  
> > -#endif /* _LINUX_NFS4_XDRGEN_H */
> > +#endif /* _LINUX_XDRGEN_NFS4_H */
> > 
> > -- 
> > 2.46.0
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

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

[Chuck Lever III]

> On Aug 29, 2024, at 2:26 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Thu, 2024-08-29 at 11:13 -0400, Chuck Lever wrote:
>> On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
>> 
>>> Create a new include/linux/sunrpc/xdrgen directory in which we can
>>> keep autogenerated header files.
>> 
>> I think the header files will have different content for the client
>> and server. For example, the server-side header has function
>> declarations for the procedure argument and result XDR functions,
>> the client doesn't (because those functions are all static on the
>> client side).
>> 
>> Not sure we're ready for this level of sharing between client and
>> server.
>> 
> 
> Does that matter though?

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

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


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

The tool already does that now:

  xdrgen header --definitions

gives you the protocol pieces, and

  xdrgen header --declarations

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

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


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

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


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

That should work.


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

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


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

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

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


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

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

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


--
Chuck Lever

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

[Chuck Lever III]

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

OK, that makes sense.

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

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

The header file is now split between

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

and

  fs/nfsd/nfs4xdr_gen.h (function declarations)


--
Chuck Lever

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

[Jeff Layton]

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

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

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

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

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

Excellent. I'll rebase onto that later today.
-- 
Jeff Layton <jlayton@kernel.org>

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

[Jeff Layton]

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

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

I won't bother reposting it just yet, since it's fundamentally the same
code. The only real difference is in the patch that adapts nfs4.h to
include the autogenerated header.
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v3 09/13] nfsd: add support for FATTR4_OPEN_ARGUMENTS

[Jeff Layton]

Add support for FATTR4_OPEN_ARGUMENTS. This a new mechanism for the
client to discover what OPEN features the server supports.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/Makefile  |  2 +-
 fs/nfsd/nfs4xdr.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfsd.h    |  3 ++-
 3 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/Makefile b/fs/nfsd/Makefile
index b8736a82e57c..d6ab3ae7d0a0 100644
--- a/fs/nfsd/Makefile
+++ b/fs/nfsd/Makefile
@@ -18,7 +18,7 @@ nfsd-$(CONFIG_NFSD_V2) += nfsproc.o nfsxdr.o
 nfsd-$(CONFIG_NFSD_V2_ACL) += nfs2acl.o
 nfsd-$(CONFIG_NFSD_V3_ACL) += nfs3acl.o
 nfsd-$(CONFIG_NFSD_V4)	+= nfs4proc.o nfs4xdr.o nfs4state.o nfs4idmap.o \
-			   nfs4acl.o nfs4callback.o nfs4recover.o
+			   nfs4acl.o nfs4callback.o nfs4recover.o nfs4xdr_gen.o
 nfsd-$(CONFIG_NFSD_PNFS) += nfs4layouts.o
 nfsd-$(CONFIG_NFSD_BLOCKLAYOUT) += blocklayout.o blocklayoutxdr.o
 nfsd-$(CONFIG_NFSD_SCSILAYOUT) += blocklayout.o blocklayoutxdr.o
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a9827bb8a2f0..c25dbfa0e7ea 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3396,6 +3396,54 @@ static __be32 nfsd4_encode_fattr4_xattr_support(struct xdr_stream *xdr,
 	return nfsd4_encode_bool(xdr, err == 0);
 }
 
+#define NFSD_OA_SHARE_ACCESS	(BIT(OPEN_ARGS_SHARE_ACCESS_READ)	| \
+				 BIT(OPEN_ARGS_SHARE_ACCESS_WRITE)	| \
+				 BIT(OPEN_ARGS_SHARE_ACCESS_BOTH))
+
+#define NFSD_OA_SHARE_DENY	(BIT(OPEN_ARGS_SHARE_DENY_NONE)		| \
+				 BIT(OPEN_ARGS_SHARE_DENY_READ)		| \
+				 BIT(OPEN_ARGS_SHARE_DENY_WRITE)	| \
+				 BIT(OPEN_ARGS_SHARE_DENY_BOTH))
+
+#define NFSD_OA_SHARE_ACCESS_WANT	(BIT(OPEN_ARGS_SHARE_ACCESS_WANT_ANY_DELEG)		| \
+					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_NO_DELEG)		| \
+					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL))
+
+#define NFSD_OA_OPEN_CLAIM	(BIT(OPEN_ARGS_OPEN_CLAIM_NULL)		| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_PREVIOUS)	| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_DELEGATE_CUR)	| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_DELEGATE_PREV)| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_FH)		| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_DELEG_CUR_FH)	| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_DELEG_PREV_FH))
+
+#define NFSD_OA_CREATE_MODE	(BIT(OPEN_ARGS_CREATEMODE_UNCHECKED4)	| \
+				 BIT(OPEN_ARGS_CREATE_MODE_GUARDED)	| \
+				 BIT(OPEN_ARGS_CREATEMODE_EXCLUSIVE4)	| \
+				 BIT(OPEN_ARGS_CREATE_MODE_EXCLUSIVE4_1))
+
+static uint32_t oa_share_access = NFSD_OA_SHARE_ACCESS;
+static uint32_t oa_share_deny = NFSD_OA_SHARE_DENY;
+static uint32_t oa_share_access_want = NFSD_OA_SHARE_ACCESS_WANT;
+static uint32_t oa_open_claim = NFSD_OA_OPEN_CLAIM;
+static uint32_t oa_create_mode = NFSD_OA_CREATE_MODE;
+
+static const struct open_arguments4 nfsd_open_arguments = {
+	.oa_share_access = { .count = 1, .element = &oa_share_access },
+	.oa_share_deny = { .count = 1, .element = &oa_share_deny },
+	.oa_share_access_want = { .count = 1, .element = &oa_share_access_want },
+	.oa_open_claim = { .count = 1, .element = &oa_open_claim },
+	.oa_create_mode = { .count = 1, .element = &oa_create_mode },
+};
+
+static __be32 nfsd4_encode_fattr4_open_arguments(struct xdr_stream *xdr,
+						 const struct nfsd4_fattr_args *args)
+{
+	if (!xdrgen_encode_fattr4_open_arguments(xdr, &nfsd_open_arguments))
+		return nfserr_resource;
+	return nfs_ok;
+}
+
 static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
 	[FATTR4_SUPPORTED_ATTRS]	= nfsd4_encode_fattr4_supported_attrs,
 	[FATTR4_TYPE]			= nfsd4_encode_fattr4_type,
@@ -3496,6 +3544,7 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
 
 	[FATTR4_MODE_UMASK]		= nfsd4_encode_fattr4__noop,
 	[FATTR4_XATTR_SUPPORT]		= nfsd4_encode_fattr4_xattr_support,
+	[FATTR4_OPEN_ARGUMENTS]		= nfsd4_encode_fattr4_open_arguments,
 };
 
 /*
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 4ccbf014a2c7..c98fb104ba7d 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -454,7 +454,8 @@ enum {
 	(NFSD4_1_SUPPORTED_ATTRS_WORD2 | \
 	FATTR4_WORD2_MODE_UMASK | \
 	NFSD4_2_SECURITY_ATTRS | \
-	FATTR4_WORD2_XATTR_SUPPORT)
+	FATTR4_WORD2_XATTR_SUPPORT | \
+	FATTR4_WORD2_OPEN_ARGUMENTS)
 
 extern const u32 nfsd_suppattrs[3][3];
 

-- 
2.46.0

[PATCH v3 10/13] nfsd: implement OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION

[Jeff Layton]

Allow clients to request getting a delegation xor an open stateid if a
delegation isn't available. This allows the client to avoid sending a
final CLOSE for the (useless) open stateid, when it is granted a
delegation.

This is done by moving the increment of the open stateid and unlocking
of the st_mutex until after we acquire a delegation. If we get a
delegation, we zero out the op_stateid field and set the NO_OPEN_STATEID
flag. If the open stateid was brand new, then unhash it too in this case
since it won't be needed.

If we can't get a delegation or the new flag wasn't requested, then just
increment and copy the open stateid as usual.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c       | 28 ++++++++++++++++++++++++----
 fs/nfsd/nfs4xdr.c         |  5 +++--
 include/uapi/linux/nfs4.h |  7 +++++--
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index dce27420ae31..c4e76427af92 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6035,6 +6035,17 @@ static void nfsd4_deleg_xgrade_none_ext(struct nfsd4_open *open,
 	 */
 }
 
+/* Are we only returning a delegation stateid? */
+static bool open_xor_delegation(struct nfsd4_open *open)
+{
+	if (!(open->op_deleg_want & OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION))
+		return false;
+	if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ &&
+	    open->op_delegate_type != NFS4_OPEN_DELEGATE_WRITE)
+		return false;
+	return true;
+}
+
 /**
  * nfsd4_process_open2 - finish open processing
  * @rqstp: the RPC transaction being executed
@@ -6057,6 +6068,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	struct nfs4_delegation *dp = NULL;
 	__be32 status;
 	bool new_stp = false;
+	bool deleg_only = false;
 
 	/*
 	 * Lookup file; if found, lookup stateid and check open request,
@@ -6111,9 +6123,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 			open->op_odstate = NULL;
 	}
 
-	nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
-	mutex_unlock(&stp->st_mutex);
-
 	if (nfsd4_has_session(&resp->cstate)) {
 		if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
 			open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE_EXT;
@@ -6127,7 +6136,18 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	* OPEN succeeds even if we fail.
 	*/
 	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
+	deleg_only = open_xor_delegation(open);
 nodeleg:
+	if (deleg_only) {
+		memcpy(&open->op_stateid, &zero_stateid, sizeof(open->op_stateid));
+		open->op_rflags |= OPEN4_RESULT_NO_OPEN_STATEID;
+		if (new_stp)
+			release_open_stateid(stp);
+	} else {
+		nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
+	}
+	mutex_unlock(&stp->st_mutex);
+
 	status = nfs_ok;
 	trace_nfsd_open(&stp->st_stid.sc_stateid);
 out:
@@ -6143,7 +6163,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	/*
 	* To finish the open response, we just need to set the rflags.
 	*/
-	open->op_rflags = NFS4_OPEN_RESULT_LOCKTYPE_POSIX;
+	open->op_rflags |= NFS4_OPEN_RESULT_LOCKTYPE_POSIX;
 	if (nfsd4_has_session(&resp->cstate))
 		open->op_rflags |= NFS4_OPEN_RESULT_MAY_NOTIFY_LOCK;
 	else if (!(open->op_openowner->oo_flags & NFS4_OO_CONFIRMED))
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index c25dbfa0e7ea..11c6079e7dea 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1066,7 +1066,7 @@ static __be32 nfsd4_decode_share_access(struct nfsd4_compoundargs *argp, u32 *sh
 		return nfs_ok;
 	if (!argp->minorversion)
 		return nfserr_bad_xdr;
-	switch (w & NFS4_SHARE_WANT_MASK) {
+	switch (w & NFS4_SHARE_WANT_TYPE_MASK) {
 	case NFS4_SHARE_WANT_NO_PREFERENCE:
 	case NFS4_SHARE_WANT_READ_DELEG:
 	case NFS4_SHARE_WANT_WRITE_DELEG:
@@ -3407,7 +3407,8 @@ static __be32 nfsd4_encode_fattr4_xattr_support(struct xdr_stream *xdr,
 
 #define NFSD_OA_SHARE_ACCESS_WANT	(BIT(OPEN_ARGS_SHARE_ACCESS_WANT_ANY_DELEG)		| \
 					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_NO_DELEG)		| \
-					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL))
+					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL)		| \
+					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION))
 
 #define NFSD_OA_OPEN_CLAIM	(BIT(OPEN_ARGS_OPEN_CLAIM_NULL)		| \
 				 BIT(OPEN_ARGS_OPEN_CLAIM_PREVIOUS)	| \
diff --git a/include/uapi/linux/nfs4.h b/include/uapi/linux/nfs4.h
index caf4db2fcbb9..4273e0249fcb 100644
--- a/include/uapi/linux/nfs4.h
+++ b/include/uapi/linux/nfs4.h
@@ -58,7 +58,7 @@
 #define NFS4_SHARE_DENY_BOTH	0x0003
 
 /* nfs41 */
-#define NFS4_SHARE_WANT_MASK		0xFF00
+#define NFS4_SHARE_WANT_TYPE_MASK	0xFF00
 #define NFS4_SHARE_WANT_NO_PREFERENCE	0x0000
 #define NFS4_SHARE_WANT_READ_DELEG	0x0100
 #define NFS4_SHARE_WANT_WRITE_DELEG	0x0200
@@ -66,13 +66,16 @@
 #define NFS4_SHARE_WANT_NO_DELEG	0x0400
 #define NFS4_SHARE_WANT_CANCEL		0x0500
 
-#define NFS4_SHARE_WHEN_MASK		0xF0000
+#define NFS4_SHARE_WHEN_MASK				0xF0000
 #define NFS4_SHARE_SIGNAL_DELEG_WHEN_RESRC_AVAIL	0x10000
 #define NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED		0x20000
 
+#define NFS4_SHARE_WANT_MOD_MASK			0xF00000
 #define NFS4_SHARE_WANT_DELEG_TIMESTAMPS		0x100000
 #define NFS4_SHARE_WANT_OPEN_XOR_DELEGATION		0x200000
 
+#define NFS4_SHARE_WANT_MASK	(NFS4_SHARE_WANT_TYPE_MASK | NFS4_SHARE_WANT_MOD_MASK)
+
 #define NFS4_CDFC4_FORE	0x1
 #define NFS4_CDFC4_BACK 0x2
 #define NFS4_CDFC4_BOTH 0x3

-- 
2.46.0

[PATCH v3 11/13] fs: handle delegated timestamps in setattr_copy_mgtime

[Jeff Layton]

When updating the ctime on an inode for a SETATTR with a multigrain
filesystem, we usually want to take the latest time we can get for the
ctime. The exception to this rule is when there is a nfsd write
delegation and the server is proxying timestamps from the client.

When nfsd gets a CB_GETATTR response, we want to update the timestamp
value in the inode to the values that the client is tracking. The client
doesn't send a ctime value (since that's always determined by the
exported filesystem), but it can send a mtime value. In the case where
it does, then we may need to update the ctime to a value commensurate
with that instead of the current time.

If ATTR_DELEG is set, then use ia_ctime value instead of setting the
timestamp to the current time.

With the addition of delegated timestamps we can also receive a request
to update only the atime, but we may not need to set the ctime. Trust
the ATTR_CTIME flag in the update and only update the ctime when it's
set.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/attr.c          | 28 +++++++++++++--------
 fs/inode.c         | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 3 files changed, 94 insertions(+), 10 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 3bcbc45708a3..392eb62aa609 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -286,16 +286,20 @@ static void setattr_copy_mgtime(struct inode *inode, const struct iattr *attr)
 	unsigned int ia_valid = attr->ia_valid;
 	struct timespec64 now;
 
-	/*
-	 * If the ctime isn't being updated then nothing else should be
-	 * either.
-	 */
-	if (!(ia_valid & ATTR_CTIME)) {
-		WARN_ON_ONCE(ia_valid & (ATTR_ATIME|ATTR_MTIME));
-		return;
+	if (ia_valid & ATTR_CTIME) {
+		/*
+		 * In the case of an update for a write delegation, we must respect
+		 * the value in ia_ctime and not use the current time.
+		 */
+		if (ia_valid & ATTR_DELEG)
+			now = inode_set_ctime_deleg(inode, attr->ia_ctime);
+		else
+			now = inode_set_ctime_current(inode);
+	} else {
+		/* If ATTR_CTIME isn't set, then ATTR_MTIME shouldn't be either. */
+		WARN_ON_ONCE(ia_valid & ATTR_MTIME);
 	}
 
-	now = inode_set_ctime_current(inode);
 	if (ia_valid & ATTR_ATIME_SET)
 		inode_set_atime_to_ts(inode, attr->ia_atime);
 	else if (ia_valid & ATTR_ATIME)
@@ -354,8 +358,12 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
 		inode_set_atime_to_ts(inode, attr->ia_atime);
 	if (ia_valid & ATTR_MTIME)
 		inode_set_mtime_to_ts(inode, attr->ia_mtime);
-	if (ia_valid & ATTR_CTIME)
-		inode_set_ctime_to_ts(inode, attr->ia_ctime);
+	if (ia_valid & ATTR_CTIME) {
+		if (ia_valid & ATTR_DELEG)
+			inode_set_ctime_deleg(inode, attr->ia_ctime);
+		else
+			inode_set_ctime_to_ts(inode, attr->ia_ctime);
+	}
 }
 EXPORT_SYMBOL(setattr_copy);
 
diff --git a/fs/inode.c b/fs/inode.c
index 01f7df1973bd..f0fbfd470d8e 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2835,6 +2835,80 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
 }
 EXPORT_SYMBOL(inode_set_ctime_current);
 
+/**
+ * inode_set_ctime_deleg - try to update the ctime on a delegated inode
+ * @inode: inode to update
+ * @update: timespec64 to set the ctime
+ *
+ * Attempt to atomically update the ctime on behalf of a delegation holder.
+ *
+ * The nfs server can call back the holder of a delegation to get updated
+ * inode attributes, including the mtime. When updating the mtime we may
+ * need to update the ctime to a value at least equal to that.
+ *
+ * This can race with concurrent updates to the inode, in which
+ * case we just don't do the update.
+ *
+ * Note that this works even when multigrain timestamps are not enabled,
+ * so use it in either case.
+ */
+struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 update)
+{
+	ktime_t now, floor = atomic64_read(&ctime_floor);
+	struct timespec64 now_ts, cur_ts;
+	u32 cur, old;
+
+	/* pairs with try_cmpxchg below */
+	cur = smp_load_acquire(&inode->i_ctime_nsec);
+	cur_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
+	cur_ts.tv_sec = inode->i_ctime_sec;
+
+	/* If the update is older than the existing value, skip it. */
+	if (timespec64_compare(&update, &cur_ts) <= 0)
+		return cur_ts;
+
+	now = coarse_ctime(floor);
+	now_ts = ktime_to_timespec64(now);
+
+	/* Clamp the update to "now" if it's in the future */
+	if (timespec64_compare(&update, &now_ts) > 0)
+		update = now_ts;
+
+	update = timestamp_truncate(update, inode);
+
+	/* No need to update if the values are already the same */
+	if (timespec64_equal(&update, &cur_ts))
+		return cur_ts;
+
+	/*
+	 * Try to swap the nsec value into place. If it fails, that means
+	 * we raced with an update due to a write or similar activity. That
+	 * stamp takes precedence, so just skip the update.
+	 */
+retry:
+	old = cur;
+	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, update.tv_nsec)) {
+		inode->i_ctime_sec = update.tv_sec;
+		mgtime_counter_inc(mg_ctime_swaps);
+		return update;
+	}
+
+	/*
+	 * Was the change due to someone marking the old ctime QUERIED?
+	 * If so then retry the swap. This can only happen once since
+	 * the only way to clear I_CTIME_QUERIED is to stamp the inode
+	 * with a new ctime.
+	 */
+	if (!(old & I_CTIME_QUERIED) && (cur == (old | I_CTIME_QUERIED)))
+		goto retry;
+
+	/* Otherwise, it was a new timestamp. */
+	cur_ts.tv_sec = inode->i_ctime_sec;
+	cur_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
+	return cur_ts;
+}
+EXPORT_SYMBOL(inode_set_ctime_deleg);
+
 /**
  * in_group_or_capable - check whether caller is CAP_FSETID privileged
  * @idmap:	idmap of the mount @inode was found from
diff --git a/include/linux/fs.h b/include/linux/fs.h
index eff688e75f2f..ea7ed437d2b1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1544,6 +1544,8 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb,
 
 struct timespec64 current_time(struct inode *inode);
 struct timespec64 inode_set_ctime_current(struct inode *inode);
+struct timespec64 inode_set_ctime_deleg(struct inode *inode,
+					struct timespec64 update);
 
 static inline time64_t inode_get_atime_sec(const struct inode *inode)
 {

-- 
2.46.0

Re: [PATCH v3 11/13] fs: handle delegated timestamps in setattr_copy_mgtime

[Jan Kara]

On Thu 29-08-24 09:26:49, Jeff Layton wrote:
> When updating the ctime on an inode for a SETATTR with a multigrain
> filesystem, we usually want to take the latest time we can get for the
> ctime. The exception to this rule is when there is a nfsd write
> delegation and the server is proxying timestamps from the client.
> 
> When nfsd gets a CB_GETATTR response, we want to update the timestamp
> value in the inode to the values that the client is tracking. The client
> doesn't send a ctime value (since that's always determined by the
> exported filesystem), but it can send a mtime value. In the case where
> it does, then we may need to update the ctime to a value commensurate
> with that instead of the current time.
> 
> If ATTR_DELEG is set, then use ia_ctime value instead of setting the
> timestamp to the current time.
> 
> With the addition of delegated timestamps we can also receive a request
> to update only the atime, but we may not need to set the ctime. Trust
> the ATTR_CTIME flag in the update and only update the ctime when it's
> set.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/attr.c          | 28 +++++++++++++--------
>  fs/inode.c         | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |  2 ++
>  3 files changed, 94 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index 3bcbc45708a3..392eb62aa609 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -286,16 +286,20 @@ static void setattr_copy_mgtime(struct inode *inode, const struct iattr *attr)
>  	unsigned int ia_valid = attr->ia_valid;
>  	struct timespec64 now;
>  
> -	/*
> -	 * If the ctime isn't being updated then nothing else should be
> -	 * either.
> -	 */
> -	if (!(ia_valid & ATTR_CTIME)) {
> -		WARN_ON_ONCE(ia_valid & (ATTR_ATIME|ATTR_MTIME));
> -		return;
> +	if (ia_valid & ATTR_CTIME) {
> +		/*
> +		 * In the case of an update for a write delegation, we must respect
> +		 * the value in ia_ctime and not use the current time.
> +		 */
> +		if (ia_valid & ATTR_DELEG)
> +			now = inode_set_ctime_deleg(inode, attr->ia_ctime);
> +		else
> +			now = inode_set_ctime_current(inode);
> +	} else {
> +		/* If ATTR_CTIME isn't set, then ATTR_MTIME shouldn't be either. */
> +		WARN_ON_ONCE(ia_valid & ATTR_MTIME);
>  	}
>  
> -	now = inode_set_ctime_current(inode);
>  	if (ia_valid & ATTR_ATIME_SET)
>  		inode_set_atime_to_ts(inode, attr->ia_atime);
>  	else if (ia_valid & ATTR_ATIME)
> @@ -354,8 +358,12 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
>  		inode_set_atime_to_ts(inode, attr->ia_atime);
>  	if (ia_valid & ATTR_MTIME)
>  		inode_set_mtime_to_ts(inode, attr->ia_mtime);
> -	if (ia_valid & ATTR_CTIME)
> -		inode_set_ctime_to_ts(inode, attr->ia_ctime);
> +	if (ia_valid & ATTR_CTIME) {
> +		if (ia_valid & ATTR_DELEG)
> +			inode_set_ctime_deleg(inode, attr->ia_ctime);
> +		else
> +			inode_set_ctime_to_ts(inode, attr->ia_ctime);
> +	}
>  }
>  EXPORT_SYMBOL(setattr_copy);
>  
> diff --git a/fs/inode.c b/fs/inode.c
> index 01f7df1973bd..f0fbfd470d8e 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2835,6 +2835,80 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
>  }
>  EXPORT_SYMBOL(inode_set_ctime_current);
>  
> +/**
> + * inode_set_ctime_deleg - try to update the ctime on a delegated inode
> + * @inode: inode to update
> + * @update: timespec64 to set the ctime
> + *
> + * Attempt to atomically update the ctime on behalf of a delegation holder.
> + *
> + * The nfs server can call back the holder of a delegation to get updated
> + * inode attributes, including the mtime. When updating the mtime we may
> + * need to update the ctime to a value at least equal to that.
> + *
> + * This can race with concurrent updates to the inode, in which
> + * case we just don't do the update.
> + *
> + * Note that this works even when multigrain timestamps are not enabled,
> + * so use it in either case.
> + */
> +struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 update)
> +{
> +	ktime_t now, floor = atomic64_read(&ctime_floor);
> +	struct timespec64 now_ts, cur_ts;
> +	u32 cur, old;
> +
> +	/* pairs with try_cmpxchg below */
> +	cur = smp_load_acquire(&inode->i_ctime_nsec);
> +	cur_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
> +	cur_ts.tv_sec = inode->i_ctime_sec;
> +
> +	/* If the update is older than the existing value, skip it. */
> +	if (timespec64_compare(&update, &cur_ts) <= 0)
> +		return cur_ts;
> +
> +	now = coarse_ctime(floor);
> +	now_ts = ktime_to_timespec64(now);
> +
> +	/* Clamp the update to "now" if it's in the future */
> +	if (timespec64_compare(&update, &now_ts) > 0)
> +		update = now_ts;
> +
> +	update = timestamp_truncate(update, inode);
> +
> +	/* No need to update if the values are already the same */
> +	if (timespec64_equal(&update, &cur_ts))
> +		return cur_ts;
> +
> +	/*
> +	 * Try to swap the nsec value into place. If it fails, that means
> +	 * we raced with an update due to a write or similar activity. That
> +	 * stamp takes precedence, so just skip the update.
> +	 */
> +retry:
> +	old = cur;
> +	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, update.tv_nsec)) {
> +		inode->i_ctime_sec = update.tv_sec;
> +		mgtime_counter_inc(mg_ctime_swaps);
> +		return update;
> +	}
> +
> +	/*
> +	 * Was the change due to someone marking the old ctime QUERIED?
> +	 * If so then retry the swap. This can only happen once since
> +	 * the only way to clear I_CTIME_QUERIED is to stamp the inode
> +	 * with a new ctime.
> +	 */
> +	if (!(old & I_CTIME_QUERIED) && (cur == (old | I_CTIME_QUERIED)))
> +		goto retry;
> +
> +	/* Otherwise, it was a new timestamp. */
> +	cur_ts.tv_sec = inode->i_ctime_sec;
> +	cur_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
> +	return cur_ts;
> +}
> +EXPORT_SYMBOL(inode_set_ctime_deleg);
> +
>  /**
>   * in_group_or_capable - check whether caller is CAP_FSETID privileged
>   * @idmap:	idmap of the mount @inode was found from
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index eff688e75f2f..ea7ed437d2b1 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1544,6 +1544,8 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb,
>  
>  struct timespec64 current_time(struct inode *inode);
>  struct timespec64 inode_set_ctime_current(struct inode *inode);
> +struct timespec64 inode_set_ctime_deleg(struct inode *inode,
> +					struct timespec64 update);
>  
>  static inline time64_t inode_get_atime_sec(const struct inode *inode)
>  {
> 
> -- 
> 2.46.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 12/13] nfsd: add support for delegated timestamps

[Jeff Layton]

Add support for the delegated timestamps on write delegations. This
allows the server to proxy timestamps from the delegation holder to
other clients that are doing GETATTRs vs. the same inode.

Add a new flag to nfs4_delegation for indicating that the client can
provide timestamps in the CB_GETATTR response. Set that when the client
sets the appropriate flag in the open request.

Add timespec64 fields to nfs4_cb_fattr and decode the timestamps into
those. Vet those timestamps according to the delstid spec and update
the inode attrs if necessary.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4callback.c | 42 ++++++++++++++++++++++----
 fs/nfsd/nfs4state.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++----
 fs/nfsd/nfs4xdr.c      | 13 +++++++-
 fs/nfsd/nfsd.h         |  2 ++
 fs/nfsd/state.h        |  3 ++
 fs/nfsd/xdr4cb.h       | 10 +++++--
 include/linux/time64.h |  5 ++++
 7 files changed, 141 insertions(+), 15 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 0c49e31d4350..11bf7439a253 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -93,12 +93,35 @@ static int decode_cb_fattr4(struct xdr_stream *xdr, uint32_t *bitmap,
 {
 	fattr->ncf_cb_change = 0;
 	fattr->ncf_cb_fsize = 0;
+	fattr->ncf_cb_atime.tv_sec = 0;
+	fattr->ncf_cb_atime.tv_nsec = 0;
+	fattr->ncf_cb_mtime.tv_sec = 0;
+	fattr->ncf_cb_mtime.tv_nsec = 0;
+
 	if (bitmap[0] & FATTR4_WORD0_CHANGE)
 		if (xdr_stream_decode_u64(xdr, &fattr->ncf_cb_change) < 0)
 			return -NFSERR_BAD_XDR;
 	if (bitmap[0] & FATTR4_WORD0_SIZE)
 		if (xdr_stream_decode_u64(xdr, &fattr->ncf_cb_fsize) < 0)
 			return -NFSERR_BAD_XDR;
+	if (bitmap[2] & FATTR4_WORD2_TIME_DELEG_ACCESS) {
+		fattr4_time_deleg_access access;
+
+		if (!xdrgen_decode_fattr4_time_deleg_access(xdr, &access))
+			return -NFSERR_BAD_XDR;
+		fattr->ncf_cb_atime.tv_sec = access.seconds;
+		fattr->ncf_cb_atime.tv_nsec = access.nseconds;
+
+	}
+	if (bitmap[2] & FATTR4_WORD2_TIME_DELEG_MODIFY) {
+		fattr4_time_deleg_modify modify;
+
+		if (!xdrgen_decode_fattr4_time_deleg_modify(xdr, &modify))
+			return -NFSERR_BAD_XDR;
+		fattr->ncf_cb_mtime.tv_sec = modify.seconds;
+		fattr->ncf_cb_mtime.tv_nsec = modify.nseconds;
+
+	}
 	return 0;
 }
 
@@ -364,13 +387,18 @@ encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
 	struct nfs4_delegation *dp =
 		container_of(fattr, struct nfs4_delegation, dl_cb_fattr);
 	struct knfsd_fh *fh = &dp->dl_stid.sc_file->fi_fhandle;
-	u32 bmap[1];
+	u32 bmap[3];
+	u32 bmap_size = 1;
 
 	bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
-
+	if (dp->dl_deleg_ts) {
+		bmap[1] = 0;
+		bmap[2] = FATTR4_WORD2_TIME_DELEG_ACCESS | FATTR4_WORD2_TIME_DELEG_MODIFY;
+		bmap_size = 3;
+	}
 	encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
 	encode_nfs_fh4(xdr, fh);
-	encode_bitmap4(xdr, bmap, ARRAY_SIZE(bmap));
+	encode_bitmap4(xdr, bmap, bmap_size);
 	hdr->nops++;
 }
 
@@ -595,7 +623,7 @@ static int nfs4_xdr_dec_cb_getattr(struct rpc_rqst *rqstp,
 	struct nfs4_cb_compound_hdr hdr;
 	int status;
 	u32 bitmap[3] = {0};
-	u32 attrlen;
+	u32 attrlen, maxlen;
 	struct nfs4_cb_fattr *ncf =
 		container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
 
@@ -614,7 +642,11 @@ static int nfs4_xdr_dec_cb_getattr(struct rpc_rqst *rqstp,
 		return -NFSERR_BAD_XDR;
 	if (xdr_stream_decode_u32(xdr, &attrlen) < 0)
 		return -NFSERR_BAD_XDR;
-	if (attrlen > (sizeof(ncf->ncf_cb_change) + sizeof(ncf->ncf_cb_fsize)))
+	maxlen = sizeof(ncf->ncf_cb_change) + sizeof(ncf->ncf_cb_fsize);
+	if (bitmap[2] != 0)
+		maxlen += (sizeof(ncf->ncf_cb_mtime.tv_sec) +
+			   sizeof(ncf->ncf_cb_mtime.tv_nsec)) * 2;
+	if (attrlen > maxlen)
 		return -NFSERR_BAD_XDR;
 	status = decode_cb_fattr4(xdr, bitmap, ncf);
 	return status;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c4e76427af92..06dff11c3e51 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5982,6 +5982,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 
 	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
 
+	if (open->op_deleg_want & NFS4_SHARE_WANT_DELEG_TIMESTAMPS)
+		dp->dl_deleg_ts = true;
 	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
 		open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE;
 		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
@@ -8826,6 +8828,78 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
 	get_stateid(cstate, &u->write.wr_stateid);
 }
 
+/**
+ * set_cb_time - vet and set the timespec for a cb_getattr update
+ * @cb: timestamp from the CB_GETATTR response
+ * @orig: original timestamp in the inode
+ * @now: current time
+ *
+ * Given a timestamp in a CB_GETATTR response, check it against the
+ * current timestamp in the inode and the current time. Returns true
+ * if the inode's timestamp needs to be updated, and false otherwise.
+ * @cb may also be changed if the timestamp needs to be clamped.
+ */
+static bool set_cb_time(struct timespec64 *cb, const struct timespec64 *orig,
+			const struct timespec64 *now)
+{
+
+	/*
+	 * "When the time presented is before the original time, then the
+	 *  update is ignored." Also no need to update if there is no change.
+	 */
+	if (timespec64_compare(cb, orig) <= 0)
+		return false;
+
+	/*
+	 * "When the time presented is in the future, the server can either
+	 *  clamp the new time to the current time, or it may
+	 *  return NFS4ERR_DELAY to the client, allowing it to retry."
+	 */
+	if (timespec64_compare(cb, now) > 0) {
+		/* clamp it */
+		*cb = *now;
+	}
+
+	return true;
+}
+
+static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation *dp)
+{
+	struct inode *inode = d_inode(dentry);
+	struct timespec64 now = current_time(inode);
+	struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr;
+	struct iattr attrs = { };
+	int ret;
+
+	if (dp->dl_deleg_ts) {
+		struct timespec64 atime = inode_get_atime(inode);
+		struct timespec64 mtime = inode_get_mtime(inode);
+
+		attrs.ia_atime = ncf->ncf_cb_atime;
+		attrs.ia_mtime = ncf->ncf_cb_mtime;
+
+		if (set_cb_time(&attrs.ia_atime, &atime, &now))
+			attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
+
+		if (set_cb_time(&attrs.ia_mtime, &mtime, &now)) {
+			attrs.ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET;
+			attrs.ia_ctime = attrs.ia_mtime;
+		}
+	} else {
+		attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME;
+		attrs.ia_mtime = attrs.ia_ctime = now;
+	}
+
+	if (!attrs.ia_valid)
+		return 0;
+
+	attrs.ia_valid |= ATTR_DELEG;
+	inode_lock(inode);
+	ret = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
+	inode_unlock(inode);
+	return ret;
+}
+
 /**
  * nfsd4_deleg_getattr_conflict - Recall if GETATTR causes conflict
  * @rqstp: RPC transaction context
@@ -8852,7 +8926,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 	struct file_lock_context *ctx;
 	struct nfs4_delegation *dp = NULL;
 	struct file_lease *fl;
-	struct iattr attrs;
 	struct nfs4_cb_fattr *ncf;
 	struct inode *inode = d_inode(dentry);
 
@@ -8914,11 +8987,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 		 * not update the file's metadata with the client's
 		 * modified size
 		 */
-		attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
-		attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
-		inode_lock(inode);
-		err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
-		inode_unlock(inode);
+		err = cb_getattr_update_times(dentry, dp);
 		if (err) {
 			status = nfserrno(err);
 			goto out_status;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 11c6079e7dea..557f4c8767ff 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3408,6 +3408,7 @@ static __be32 nfsd4_encode_fattr4_xattr_support(struct xdr_stream *xdr,
 #define NFSD_OA_SHARE_ACCESS_WANT	(BIT(OPEN_ARGS_SHARE_ACCESS_WANT_ANY_DELEG)		| \
 					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_NO_DELEG)		| \
 					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL)		| \
+					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS)	| \
 					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION))
 
 #define NFSD_OA_OPEN_CLAIM	(BIT(OPEN_ARGS_OPEN_CLAIM_NULL)		| \
@@ -3601,7 +3602,11 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 		if (status)
 			goto out;
 	}
-	if (attrmask[0] & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
+	if ((attrmask[0] & (FATTR4_WORD0_CHANGE |
+			    FATTR4_WORD0_SIZE)) ||
+	    (attrmask[1] & (FATTR4_WORD1_TIME_ACCESS |
+			    FATTR4_WORD1_TIME_MODIFY |
+			    FATTR4_WORD1_TIME_METADATA))) {
 		status = nfsd4_deleg_getattr_conflict(rqstp, dentry, &dp);
 		if (status)
 			goto out;
@@ -3619,7 +3624,13 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 			/* If there have been no changes, report the initial cinfo + 1 */
 			if (args.change_attr == ncf->ncf_initial_cinfo)
 				args.change_attr = ncf->ncf_initial_cinfo + 1;
+			if (!timespec64_is_epoch(&ncf->ncf_cb_mtime))
+				args.stat.mtime = ncf->ncf_cb_mtime;
 		}
+
+		if (!timespec64_is_epoch(&ncf->ncf_cb_atime))
+			args.stat.atime = ncf->ncf_cb_atime;
+
 		nfs4_put_stid(&dp->dl_stid);
 	}
 	if (err)
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index c98fb104ba7d..18b8d383f73a 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -455,6 +455,8 @@ enum {
 	FATTR4_WORD2_MODE_UMASK | \
 	NFSD4_2_SECURITY_ATTRS | \
 	FATTR4_WORD2_XATTR_SUPPORT | \
+	FATTR4_WORD2_TIME_DELEG_ACCESS | \
+	FATTR4_WORD2_TIME_DELEG_MODIFY | \
 	FATTR4_WORD2_OPEN_ARGUMENTS)
 
 extern const u32 nfsd_suppattrs[3][3];
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index c7c7ec21e510..874fcab2b183 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -142,6 +142,8 @@ struct nfs4_cb_fattr {
 	/* from CB_GETATTR reply */
 	u64 ncf_cb_change;
 	u64 ncf_cb_fsize;
+	struct timespec64 ncf_cb_mtime;
+	struct timespec64 ncf_cb_atime;
 
 	unsigned long ncf_cb_flags;
 	bool ncf_file_modified;
@@ -185,6 +187,7 @@ struct nfs4_delegation {
 	int			dl_retries;
 	struct nfsd4_callback	dl_recall;
 	bool			dl_recalled;
+	bool			dl_deleg_ts;
 
 	/* for CB_GETATTR */
 	struct nfs4_cb_fattr    dl_cb_fattr;
diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
index e8b00309c449..f1a315cd31b7 100644
--- a/fs/nfsd/xdr4cb.h
+++ b/fs/nfsd/xdr4cb.h
@@ -59,16 +59,20 @@
  * 1: CB_GETATTR opcode (32-bit)
  * N: file_handle
  * 1: number of entry in attribute array (32-bit)
- * 1: entry 0 in attribute array (32-bit)
+ * 3: entry 0-2 in attribute array (32-bit * 3)
  */
 #define NFS4_enc_cb_getattr_sz		(cb_compound_enc_hdr_sz +       \
 					cb_sequence_enc_sz +            \
-					1 + enc_nfs4_fh_sz + 1 + 1)
+					1 + enc_nfs4_fh_sz + 1 + 3)
 /*
  * 4: fattr_bitmap_maxsz
  * 1: attribute array len
  * 2: change attr (64-bit)
  * 2: size (64-bit)
+ * 2: atime.seconds (64-bit)
+ * 1: atime.nanoseconds (32-bit)
+ * 2: mtime.seconds (64-bit)
+ * 1: mtime.nanoseconds (32-bit)
  */
 #define NFS4_dec_cb_getattr_sz		(cb_compound_dec_hdr_sz  +      \
-			cb_sequence_dec_sz + 4 + 1 + 2 + 2 + op_dec_sz)
+			cb_sequence_dec_sz + 4 + 1 + 2 + 2 + 2 + 1 + 2 + 1 + op_dec_sz)
diff --git a/include/linux/time64.h b/include/linux/time64.h
index f1bcea8c124a..9934331c7b86 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -49,6 +49,11 @@ static inline int timespec64_equal(const struct timespec64 *a,
 	return (a->tv_sec == b->tv_sec) && (a->tv_nsec == b->tv_nsec);
 }
 
+static inline bool timespec64_is_epoch(const struct timespec64 *ts)
+{
+	return ts->tv_sec == 0 && ts->tv_nsec == 0;
+}
+
 /*
  * lhs < rhs:  return <0
  * lhs == rhs: return 0

-- 
2.46.0

[PATCH v3 13/13] nfsd: handle delegated timestamps in SETATTR

[Jeff Layton]

Allow SETATTR to handle delegated timestamps. This patch assumes that
only the delegation holder has the ability to set the timestamps in this
way, so we only allow this if the SETATTR stateid refers to the
delegation.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4proc.c | 29 ++++++++++++++++++++++++++---
 fs/nfsd/nfs4xdr.c  | 20 ++++++++++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 963a02e179a0..f715dd29de60 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1142,18 +1142,41 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		.na_iattr	= &setattr->sa_iattr,
 		.na_seclabel	= &setattr->sa_label,
 	};
+	struct nfs4_stid *st = NULL;
 	struct inode *inode;
 	__be32 status = nfs_ok;
-	bool save_no_wcc;
+	bool save_no_wcc, deleg_attrs;
 	int err;
 
-	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
+	deleg_attrs = setattr->sa_bmval[2] & (FATTR4_WORD2_TIME_DELEG_ACCESS |
+					      FATTR4_WORD2_TIME_DELEG_MODIFY);
+
+	if (deleg_attrs || (setattr->sa_iattr.ia_valid & ATTR_SIZE)) {
 		status = nfs4_preprocess_stateid_op(rqstp, cstate,
 				&cstate->current_fh, &setattr->sa_stateid,
-				WR_STATE, NULL, NULL);
+				WR_STATE, NULL, &st);
 		if (status)
 			return status;
 	}
+
+	/*
+	 * If client is trying to set delegated timestamps, ensure that the
+	 * stateid refers to a write delegation.
+	 */
+	if (deleg_attrs) {
+		status = nfserr_bad_stateid;
+		if (st->sc_type & SC_TYPE_DELEG) {
+			struct nfs4_delegation *dp = delegstateid(st);
+
+			if (dp->dl_type == NFS4_OPEN_DELEGATE_WRITE)
+				status = nfs_ok;
+		}
+	}
+	if (st)
+		nfs4_put_stid(st);
+	if (status)
+		return status;
+
 	err = fh_want_write(&cstate->current_fh);
 	if (err)
 		return nfserrno(err);
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 557f4c8767ff..3b46014f911b 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -520,6 +520,26 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 		*umask = mask & S_IRWXUGO;
 		iattr->ia_valid |= ATTR_MODE;
 	}
+	if (bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS) {
+		fattr4_time_deleg_access access;
+
+		if (!xdrgen_decode_fattr4_time_deleg_access(argp->xdr, &access))
+			return nfserr_bad_xdr;
+		iattr->ia_atime.tv_sec = access.seconds;
+		iattr->ia_atime.tv_nsec = access.nseconds;
+		iattr->ia_valid |= ATTR_ATIME | ATTR_ATIME_SET | ATTR_DELEG;
+	}
+	if (bmval[2] & FATTR4_WORD2_TIME_DELEG_MODIFY) {
+		fattr4_time_deleg_modify modify;
+
+		if (!xdrgen_decode_fattr4_time_deleg_modify(argp->xdr, &modify))
+			return nfserr_bad_xdr;
+		iattr->ia_mtime.tv_sec = modify.seconds;
+		iattr->ia_mtime.tv_nsec = modify.nseconds;
+		iattr->ia_ctime.tv_sec = modify.seconds;
+		iattr->ia_ctime.tv_nsec = modify.seconds;
+		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
+	}
 
 	/* request sanity: did attrlist4 contain the expected number of words? */
 	if (attrlist4_count != xdr_stream_pos(argp->xdr) - starting_pos)

-- 
2.46.0