[PATCH v1 0/7] Clean up XDR encoders for NFSv4 OPEN and LOCK

[PATCH v1 0/7] Clean up XDR encoders for NFSv4 OPEN and LOCK

[Chuck Lever]

Tidy up the server-side XDR encoders for NFSv4 OPEN and LOCK
results. Series applies to nfsd-next. See topic branch
"nfsd4-encoder-overhaul" in this repo:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git

---

Chuck Lever (7):
      NFSD: Add nfsd4_encode_lock_owner4()
      NFSD: Refactor nfsd4_encode_lock_denied()
      NFSD: Add nfsd4_encode_open_read_delegation4()
      NFSD: Add nfsd4_encode_open_write_delegation4()
      NFSD: Add nfsd4_encode_open_none_delegation4()
      NFSD: Add nfsd4_encode_open_delegation4()
      NFSD: Clean up nfsd4_encode_open()


 fs/nfsd/nfs4state.c |   6 +-
 fs/nfsd/nfs4xdr.c   | 305 +++++++++++++++++++++++++++-----------------
 fs/nfsd/xdr4.h      |   2 +-
 3 files changed, 189 insertions(+), 124 deletions(-)

--
Chuck Lever

[PATCH v1 1/7] NFSD: Add nfsd4_encode_lock_owner4()

[Chuck Lever]

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

To improve readability and better align the LOCK encoders with the
XDR specification, add an explicit encoder named for the lock_owner4
type.

In particular, to avoid code duplication, use
nfsd4_encode_clientid4() to encode the clientid in the lock owner
rather than open-coding it.

It looks to me like nfs4_set_lock_denied() already clears the
clientid if it won't return an owner (cf: the nevermind: label). The
code in the XDR encoder appears to be redundant and can safely be
removed.

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

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index d21aaa56c49a..9f7a6924ef5f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3976,6 +3976,20 @@ nfsd4_encode_getfh(struct nfsd4_compoundres *resp, __be32 nfserr,
 	return nfsd4_encode_nfs_fh4(xdr, &fhp->fh_handle);
 }
 
+static __be32
+nfsd4_encode_lock_owner4(struct xdr_stream *xdr, const clientid_t *clientid,
+			 const struct xdr_netobj *owner)
+{
+	__be32 status;
+
+	/* clientid */
+	status = nfsd4_encode_clientid4(xdr, clientid);
+	if (status != nfs_ok)
+		return status;
+	/* owner */
+	return nfsd4_encode_opaque(xdr, owner->data, owner->len);
+}
+
 /*
 * Including all fields other than the name, a LOCK4denied structure requires
 *   8(clientid) + 4(namelen) + 8(offset) + 8(length) + 4(type) = 32 bytes.
@@ -3984,10 +3998,10 @@ static __be32
 nfsd4_encode_lock_denied(struct xdr_stream *xdr, struct nfsd4_lock_denied *ld)
 {
 	struct xdr_netobj *conf = &ld->ld_owner;
-	__be32 *p;
+	__be32 *p, status;
 
 again:
-	p = xdr_reserve_space(xdr, 32 + XDR_LEN(conf->len));
+	p = xdr_reserve_space(xdr, XDR_UNIT * 5);
 	if (!p) {
 		/*
 		 * Don't fail to return the result just because we can't
@@ -4004,14 +4018,11 @@ nfsd4_encode_lock_denied(struct xdr_stream *xdr, struct nfsd4_lock_denied *ld)
 	p = xdr_encode_hyper(p, ld->ld_start);
 	p = xdr_encode_hyper(p, ld->ld_length);
 	*p++ = cpu_to_be32(ld->ld_type);
-	if (conf->len) {
-		p = xdr_encode_opaque_fixed(p, &ld->ld_clientid, 8);
-		p = xdr_encode_opaque(p, conf->data, conf->len);
-		kfree(conf->data);
-	}  else {  /* non - nfsv4 lock in conflict, no clientid nor owner */
-		p = xdr_encode_hyper(p, (u64)0); /* clientid */
-		*p++ = cpu_to_be32(0); /* length of owner name */
-	}
+	status = nfsd4_encode_lock_owner4(xdr, &ld->ld_clientid,
+					  &ld->ld_owner);
+	if (status != nfs_ok)
+		return status;
+
 	return nfserr_denied;
 }
 

[PATCH v1 2/7] NFSD: Refactor nfsd4_encode_lock_denied()

[Chuck Lever]

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

Use the modern XDR utility functions.

The LOCK and LOCKT encoder functions need to return nfserr_denied
when a lock is denied, but nfsd4_encode_lock4denied() should return
a status code that is consistent with other XDR encoders.

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

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 9f7a6924ef5f..ee8a7989f54f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3990,40 +3990,26 @@ nfsd4_encode_lock_owner4(struct xdr_stream *xdr, const clientid_t *clientid,
 	return nfsd4_encode_opaque(xdr, owner->data, owner->len);
 }
 
-/*
-* Including all fields other than the name, a LOCK4denied structure requires
-*   8(clientid) + 4(namelen) + 8(offset) + 8(length) + 4(type) = 32 bytes.
-*/
 static __be32
-nfsd4_encode_lock_denied(struct xdr_stream *xdr, struct nfsd4_lock_denied *ld)
+nfsd4_encode_lock4denied(struct xdr_stream *xdr,
+			 const struct nfsd4_lock_denied *ld)
 {
-	struct xdr_netobj *conf = &ld->ld_owner;
-	__be32 *p, status;
+	__be32 status;
 
-again:
-	p = xdr_reserve_space(xdr, XDR_UNIT * 5);
-	if (!p) {
-		/*
-		 * Don't fail to return the result just because we can't
-		 * return the conflicting open:
-		 */
-		if (conf->len) {
-			kfree(conf->data);
-			conf->len = 0;
-			conf->data = NULL;
-			goto again;
-		}
-		return nfserr_resource;
-	}
-	p = xdr_encode_hyper(p, ld->ld_start);
-	p = xdr_encode_hyper(p, ld->ld_length);
-	*p++ = cpu_to_be32(ld->ld_type);
-	status = nfsd4_encode_lock_owner4(xdr, &ld->ld_clientid,
-					  &ld->ld_owner);
+	/* offset */
+	status = nfsd4_encode_offset4(xdr, ld->ld_start);
 	if (status != nfs_ok)
 		return status;
-
-	return nfserr_denied;
+	/* length */
+	status = nfsd4_encode_length4(xdr, ld->ld_length);
+	if (status != nfs_ok)
+		return status;
+	/* locktype */
+	if (xdr_stream_encode_u32(xdr, ld->ld_type) != XDR_UNIT)
+		return nfserr_resource;
+	/* owner */
+	return nfsd4_encode_lock_owner4(xdr, &ld->ld_clientid,
+					&ld->ld_owner);
 }
 
 static __be32
@@ -4032,13 +4018,21 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr,
 {
 	struct nfsd4_lock *lock = &u->lock;
 	struct xdr_stream *xdr = resp->xdr;
+	__be32 status;
 
-	if (!nfserr)
-		nfserr = nfsd4_encode_stateid4(xdr, &lock->lk_resp_stateid);
-	else if (nfserr == nfserr_denied)
-		nfserr = nfsd4_encode_lock_denied(xdr, &lock->lk_denied);
-
-	return nfserr;
+	switch (nfserr) {
+	case nfs_ok:
+		/* resok4 */
+		status = nfsd4_encode_stateid4(xdr, &lock->lk_resp_stateid);
+		break;
+	case nfserr_denied:
+		/* denied */
+		status = nfsd4_encode_lock4denied(xdr, &lock->lk_denied);
+		break;
+	default:
+		return nfserr;
+	}
+	return status != nfs_ok ? status : nfserr;
 }
 
 static __be32
@@ -4047,9 +4041,14 @@ nfsd4_encode_lockt(struct nfsd4_compoundres *resp, __be32 nfserr,
 {
 	struct nfsd4_lockt *lockt = &u->lockt;
 	struct xdr_stream *xdr = resp->xdr;
+	__be32 status;
 
-	if (nfserr == nfserr_denied)
-		nfsd4_encode_lock_denied(xdr, &lockt->lt_denied);
+	if (nfserr == nfserr_denied) {
+		/* denied */
+		status = nfsd4_encode_lock4denied(xdr, &lockt->lt_denied);
+		if (status != nfs_ok)
+			return status;
+	}
 	return nfserr;
 }
 

[PATCH v1 3/7] NFSD: Add nfsd4_encode_open_read_delegation4()

[Chuck Lever]

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

Refactor nfsd4_encode_open() so the open_read_delegation4 type is
encoded in a separate function. This makes it more straightforward
to later add support for returning an nfsace4 in OPEN responses that
offer a delegation.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4state.c |    6 +++--
 fs/nfsd/nfs4xdr.c   |   61 ++++++++++++++++++++++++++++++++++++++-------------
 fs/nfsd/xdr4.h      |    2 +-
 3 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 22e95b9ae82f..b1118050ff52 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5688,11 +5688,11 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	struct path path;
 
 	cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
-	open->op_recall = 0;
+	open->op_recall = false;
 	switch (open->op_claim_type) {
 		case NFS4_OPEN_CLAIM_PREVIOUS:
 			if (!cb_up)
-				open->op_recall = 1;
+				open->op_recall = true;
 			break;
 		case NFS4_OPEN_CLAIM_NULL:
 			parent = currentfh;
@@ -5746,7 +5746,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&
 	    open->op_delegate_type != NFS4_OPEN_DELEGATE_NONE) {
 		dprintk("NFSD: WARNING: refusing delegation reclaim\n");
-		open->op_recall = 1;
+		open->op_recall = true;
 	}
 
 	/* 4.1 client asking for a delegation? */
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index ee8a7989f54f..f411fcc435f6 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4074,6 +4074,49 @@ nfsd4_encode_link(struct nfsd4_compoundres *resp, __be32 nfserr,
 	return nfsd4_encode_change_info4(xdr, &link->li_cinfo);
 }
 
+/*
+ * This implementation does not yet support returning an ACE in an
+ * OPEN that offers a delegation.
+ */
+static __be32
+nfsd4_encode_open_nfsace4(struct xdr_stream *xdr)
+{
+	__be32 status;
+
+	/* type */
+	status = nfsd4_encode_acetype4(xdr, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE);
+	if (status != nfs_ok)
+		return nfserr_resource;
+	/* flag */
+	status = nfsd4_encode_aceflag4(xdr, 0);
+	if (status != nfs_ok)
+		return nfserr_resource;
+	/* access mask */
+	status = nfsd4_encode_acemask4(xdr, 0);
+	if (status != nfs_ok)
+		return nfserr_resource;
+	/* who - empty for now */
+	if (xdr_stream_encode_u32(xdr, 0) != XDR_UNIT)
+		return nfserr_resource;
+	return nfs_ok;
+}
+
+static __be32
+nfsd4_encode_open_read_delegation4(struct xdr_stream *xdr, struct nfsd4_open *open)
+{
+	__be32 status;
+
+	/* stateid */
+	status = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
+	if (status != nfs_ok)
+		return status;
+	/* recall */
+	status = nfsd4_encode_bool(xdr, open->op_recall);
+	if (status != nfs_ok)
+		return status;
+	/* permissions */
+	return nfsd4_encode_open_nfsace4(xdr);
+}
 
 static __be32
 nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
@@ -4106,22 +4149,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
 	case NFS4_OPEN_DELEGATE_NONE:
 		break;
 	case NFS4_OPEN_DELEGATE_READ:
-		nfserr = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
-		if (nfserr)
-			return nfserr;
-		p = xdr_reserve_space(xdr, 20);
-		if (!p)
-			return nfserr_resource;
-		*p++ = cpu_to_be32(open->op_recall);
-
-		/*
-		 * TODO: ACE's in delegations
-		 */
-		*p++ = cpu_to_be32(NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE);
-		*p++ = cpu_to_be32(0);
-		*p++ = cpu_to_be32(0);
-		*p++ = cpu_to_be32(0);   /* XXX: is NULL principal ok? */
-		break;
+		/* read */
+		return nfsd4_encode_open_read_delegation4(xdr, open);
 	case NFS4_OPEN_DELEGATE_WRITE:
 		nfserr = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
 		if (nfserr)
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index aba07d5378fc..c142a9b5ab98 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -389,7 +389,7 @@ struct nfsd4_open {
 	u32		op_deleg_want;      /* request */
 	stateid_t	op_stateid;         /* response */
 	__be32		op_xdr_error;       /* see nfsd4_open_omfg() */
-	u32		op_recall;          /* recall */
+	bool		op_recall;          /* response */
 	struct nfsd4_change_info  op_cinfo; /* response */
 	u32		op_rflags;          /* response */
 	bool		op_truncate;        /* used during processing */

Re: [PATCH v1 3/7] NFSD: Add nfsd4_encode_open_read_delegation4()

[Jeff Layton]

On Fri, 2023-09-29 at 09:59 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Refactor nfsd4_encode_open() so the open_read_delegation4 type is
> encoded in a separate function. This makes it more straightforward
> to later add support for returning an nfsace4 in OPEN responses that
> offer a delegation.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4state.c |    6 +++--
>  fs/nfsd/nfs4xdr.c   |   61 ++++++++++++++++++++++++++++++++++++++-------------
>  fs/nfsd/xdr4.h      |    2 +-
>  3 files changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 22e95b9ae82f..b1118050ff52 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5688,11 +5688,11 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	struct path path;
>  
>  	cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
> -	open->op_recall = 0;
> +	open->op_recall = false;
>  	switch (open->op_claim_type) {
>  		case NFS4_OPEN_CLAIM_PREVIOUS:
>  			if (!cb_up)
> -				open->op_recall = 1;
> +				open->op_recall = true;
>  			break;
>  		case NFS4_OPEN_CLAIM_NULL:
>  			parent = currentfh;
> @@ -5746,7 +5746,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&
>  	    open->op_delegate_type != NFS4_OPEN_DELEGATE_NONE) {
>  		dprintk("NFSD: WARNING: refusing delegation reclaim\n");
> -		open->op_recall = 1;
> +		open->op_recall = true;
>  	}
>  
>  	/* 4.1 client asking for a delegation? */
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index ee8a7989f54f..f411fcc435f6 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4074,6 +4074,49 @@ nfsd4_encode_link(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	return nfsd4_encode_change_info4(xdr, &link->li_cinfo);
>  }
>  
> +/*
> + * This implementation does not yet support returning an ACE in an
> + * OPEN that offers a delegation.
> + */
> +static __be32
> +nfsd4_encode_open_nfsace4(struct xdr_stream *xdr)
> +{
> +	__be32 status;
> +
> +	/* type */
> +	status = nfsd4_encode_acetype4(xdr, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE);
> +	if (status != nfs_ok)
> +		return nfserr_resource;
> +	/* flag */
> +	status = nfsd4_encode_aceflag4(xdr, 0);
> +	if (status != nfs_ok)
> +		return nfserr_resource;
> +	/* access mask */
> +	status = nfsd4_encode_acemask4(xdr, 0);
> +	if (status != nfs_ok)
> +		return nfserr_resource;
> +	/* who - empty for now */
> +	if (xdr_stream_encode_u32(xdr, 0) != XDR_UNIT)
> +		return nfserr_resource;
> +	return nfs_ok;
> +}
> +
> +static __be32
> +nfsd4_encode_open_read_delegation4(struct xdr_stream *xdr, struct nfsd4_open *open)
> +{
> +	__be32 status;
> +
> +	/* stateid */
> +	status = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
> +	if (status != nfs_ok)
> +		return status;
> +	/* recall */
> +	status = nfsd4_encode_bool(xdr, open->op_recall);
> +	if (status != nfs_ok)
> +		return status;
> +	/* permissions */
> +	return nfsd4_encode_open_nfsace4(xdr);
> +}
>  
>  static __be32
>  nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
> @@ -4106,22 +4149,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	case NFS4_OPEN_DELEGATE_NONE:
>  		break;
>  	case NFS4_OPEN_DELEGATE_READ:
> -		nfserr = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
> -		if (nfserr)
> -			return nfserr;
> -		p = xdr_reserve_space(xdr, 20);
> -		if (!p)
> -			return nfserr_resource;
> -		*p++ = cpu_to_be32(open->op_recall);
> -
> -		/*
> -		 * TODO: ACE's in delegations
> -		 */
> -		*p++ = cpu_to_be32(NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE);
> -		*p++ = cpu_to_be32(0);
> -		*p++ = cpu_to_be32(0);
> -		*p++ = cpu_to_be32(0);   /* XXX: is NULL principal ok? */
> -		break;
> +		/* read */
> +		return nfsd4_encode_open_read_delegation4(xdr, open);
>  	case NFS4_OPEN_DELEGATE_WRITE:
>  		nfserr = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
>  		if (nfserr)
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index aba07d5378fc..c142a9b5ab98 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -389,7 +389,7 @@ struct nfsd4_open {
>  	u32		op_deleg_want;      /* request */
>  	stateid_t	op_stateid;         /* response */
>  	__be32		op_xdr_error;       /* see nfsd4_open_omfg() */
> -	u32		op_recall;          /* recall */
> +	bool		op_recall;          /* response */

nit: this would probably pack better if you moved op_recall down beside
op_truncate.

>  	struct nfsd4_change_info  op_cinfo; /* response */
>  	u32		op_rflags;          /* response */
>  	bool		op_truncate;        /* used during processing */
> 
> 

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH v1 3/7] NFSD: Add nfsd4_encode_open_read_delegation4()

[Chuck Lever]

On Fri, Sep 29, 2023 at 12:21:28PM -0400, Jeff Layton wrote:
> On Fri, 2023-09-29 at 09:59 -0400, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > Refactor nfsd4_encode_open() so the open_read_delegation4 type is
> > encoded in a separate function. This makes it more straightforward
> > to later add support for returning an nfsace4 in OPEN responses that
> > offer a delegation.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfs4state.c |    6 +++--
> >  fs/nfsd/nfs4xdr.c   |   61 ++++++++++++++++++++++++++++++++++++++-------------
> >  fs/nfsd/xdr4.h      |    2 +-
> >  3 files changed, 49 insertions(+), 20 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 22e95b9ae82f..b1118050ff52 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5688,11 +5688,11 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> >  	struct path path;
> >  
> >  	cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
> > -	open->op_recall = 0;
> > +	open->op_recall = false;
> >  	switch (open->op_claim_type) {
> >  		case NFS4_OPEN_CLAIM_PREVIOUS:
> >  			if (!cb_up)
> > -				open->op_recall = 1;
> > +				open->op_recall = true;
> >  			break;
> >  		case NFS4_OPEN_CLAIM_NULL:
> >  			parent = currentfh;
> > @@ -5746,7 +5746,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> >  	if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&
> >  	    open->op_delegate_type != NFS4_OPEN_DELEGATE_NONE) {
> >  		dprintk("NFSD: WARNING: refusing delegation reclaim\n");
> > -		open->op_recall = 1;
> > +		open->op_recall = true;
> >  	}
> >  
> >  	/* 4.1 client asking for a delegation? */
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index ee8a7989f54f..f411fcc435f6 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -4074,6 +4074,49 @@ nfsd4_encode_link(struct nfsd4_compoundres *resp, __be32 nfserr,
> >  	return nfsd4_encode_change_info4(xdr, &link->li_cinfo);
> >  }
> >  
> > +/*
> > + * This implementation does not yet support returning an ACE in an
> > + * OPEN that offers a delegation.
> > + */
> > +static __be32
> > +nfsd4_encode_open_nfsace4(struct xdr_stream *xdr)
> > +{
> > +	__be32 status;
> > +
> > +	/* type */
> > +	status = nfsd4_encode_acetype4(xdr, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE);
> > +	if (status != nfs_ok)
> > +		return nfserr_resource;
> > +	/* flag */
> > +	status = nfsd4_encode_aceflag4(xdr, 0);
> > +	if (status != nfs_ok)
> > +		return nfserr_resource;
> > +	/* access mask */
> > +	status = nfsd4_encode_acemask4(xdr, 0);
> > +	if (status != nfs_ok)
> > +		return nfserr_resource;
> > +	/* who - empty for now */
> > +	if (xdr_stream_encode_u32(xdr, 0) != XDR_UNIT)
> > +		return nfserr_resource;
> > +	return nfs_ok;
> > +}
> > +
> > +static __be32
> > +nfsd4_encode_open_read_delegation4(struct xdr_stream *xdr, struct nfsd4_open *open)
> > +{
> > +	__be32 status;
> > +
> > +	/* stateid */
> > +	status = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
> > +	if (status != nfs_ok)
> > +		return status;
> > +	/* recall */
> > +	status = nfsd4_encode_bool(xdr, open->op_recall);
> > +	if (status != nfs_ok)
> > +		return status;
> > +	/* permissions */
> > +	return nfsd4_encode_open_nfsace4(xdr);
> > +}
> >  
> >  static __be32
> >  nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
> > @@ -4106,22 +4149,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
> >  	case NFS4_OPEN_DELEGATE_NONE:
> >  		break;
> >  	case NFS4_OPEN_DELEGATE_READ:
> > -		nfserr = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
> > -		if (nfserr)
> > -			return nfserr;
> > -		p = xdr_reserve_space(xdr, 20);
> > -		if (!p)
> > -			return nfserr_resource;
> > -		*p++ = cpu_to_be32(open->op_recall);
> > -
> > -		/*
> > -		 * TODO: ACE's in delegations
> > -		 */
> > -		*p++ = cpu_to_be32(NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE);
> > -		*p++ = cpu_to_be32(0);
> > -		*p++ = cpu_to_be32(0);
> > -		*p++ = cpu_to_be32(0);   /* XXX: is NULL principal ok? */
> > -		break;
> > +		/* read */
> > +		return nfsd4_encode_open_read_delegation4(xdr, open);
> >  	case NFS4_OPEN_DELEGATE_WRITE:
> >  		nfserr = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
> >  		if (nfserr)
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index aba07d5378fc..c142a9b5ab98 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -389,7 +389,7 @@ struct nfsd4_open {
> >  	u32		op_deleg_want;      /* request */
> >  	stateid_t	op_stateid;         /* response */
> >  	__be32		op_xdr_error;       /* see nfsd4_open_omfg() */
> > -	u32		op_recall;          /* recall */
> > +	bool		op_recall;          /* response */
> 
> nit: this would probably pack better if you moved op_recall down beside
> op_truncate.

Fixed. All 15 pushed to nfsd-next with your Reviewed-by. Thanks!


> >  	struct nfsd4_change_info  op_cinfo; /* response */
> >  	u32		op_rflags;          /* response */
> >  	bool		op_truncate;        /* used during processing */
> > 
> > 
> 
> -- 
> Jeff Layton <jlayton@poochiereds.net>

-- 
Chuck Lever

[PATCH v1 4/7] NFSD: Add nfsd4_encode_open_write_delegation4()

[Chuck Lever]

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

Make it easier to adjust the XDR encoder to handle new features
related to write delegations.

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

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index f411fcc435f6..f7e5f54fda00 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4118,6 +4118,37 @@ nfsd4_encode_open_read_delegation4(struct xdr_stream *xdr, struct nfsd4_open *op
 	return nfsd4_encode_open_nfsace4(xdr);
 }
 
+static __be32
+nfsd4_encode_nfs_space_limit4(struct xdr_stream *xdr, u64 filesize)
+{
+	/* limitby */
+	if (xdr_stream_encode_u32(xdr, NFS4_LIMIT_SIZE) != XDR_UNIT)
+		return nfserr_resource;
+	/* filesize */
+	return nfsd4_encode_uint64_t(xdr, filesize);
+}
+
+static __be32
+nfsd4_encode_open_write_delegation4(struct xdr_stream *xdr,
+				    struct nfsd4_open *open)
+{
+	__be32 status;
+
+	/* stateid */
+	status = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
+	if (status != nfs_ok)
+		return status;
+	/* recall */
+	status = nfsd4_encode_bool(xdr, open->op_recall);
+	if (status != nfs_ok)
+		return status;
+	/* space_limit */
+	status = nfsd4_encode_nfs_space_limit4(xdr, 0);
+	if (status != nfs_ok)
+		return status;
+	return nfsd4_encode_open_nfsace4(xdr);
+}
+
 static __be32
 nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
 		  union nfsd4_op_u *u)
@@ -4152,32 +4183,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
 		/* read */
 		return nfsd4_encode_open_read_delegation4(xdr, open);
 	case NFS4_OPEN_DELEGATE_WRITE:
-		nfserr = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
-		if (nfserr)
-			return nfserr;
-
-		p = xdr_reserve_space(xdr, XDR_UNIT * 8);
-		if (!p)
-			return nfserr_resource;
-		*p++ = cpu_to_be32(open->op_recall);
-
-		/*
-		 * Always flush on close
-		 *
-		 * TODO: space_limit's in delegations
-		 */
-		*p++ = cpu_to_be32(NFS4_LIMIT_SIZE);
-		*p++ = xdr_zero;
-		*p++ = xdr_zero;
-
-		/*
-		 * TODO: ACE's in delegations
-		 */
-		*p++ = cpu_to_be32(NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE);
-		*p++ = cpu_to_be32(0);
-		*p++ = cpu_to_be32(0);
-		*p++ = cpu_to_be32(0);   /* XXX: is NULL principal ok? */
-		break;
+		/* write */
+		return nfsd4_encode_open_write_delegation4(xdr, open);
 	case NFS4_OPEN_DELEGATE_NONE_EXT: /* 4.1 */
 		switch (open->op_why_no_deleg) {
 		case WND4_CONTENTION:

[PATCH v1 5/7] NFSD: Add nfsd4_encode_open_none_delegation4()

[Chuck Lever]

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

To better align our implementation with the XDR specification,
refactor the part of nfsd4_encode_open() that encodes the
open_none_delegation4 type.

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

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index f7e5f54fda00..b6c5ccb9351f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4149,6 +4149,27 @@ nfsd4_encode_open_write_delegation4(struct xdr_stream *xdr,
 	return nfsd4_encode_open_nfsace4(xdr);
 }
 
+static __be32
+nfsd4_encode_open_none_delegation4(struct xdr_stream *xdr,
+				   struct nfsd4_open *open)
+{
+	__be32 status = nfs_ok;
+
+	/* ond_why */
+	if (xdr_stream_encode_u32(xdr, open->op_why_no_deleg) != XDR_UNIT)
+		return nfserr_resource;
+	switch (open->op_why_no_deleg) {
+	case WND4_CONTENTION:
+		/* ond_server_will_push_deleg */
+		status = nfsd4_encode_bool(xdr, false);
+		break;
+	case WND4_RESOURCE:
+		/* ond_server_will_signal_avail */
+		status = nfsd4_encode_bool(xdr, false);
+	}
+	return status;
+}
+
 static __be32
 nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
 		  union nfsd4_op_u *u)
@@ -4185,24 +4206,9 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
 	case NFS4_OPEN_DELEGATE_WRITE:
 		/* write */
 		return nfsd4_encode_open_write_delegation4(xdr, open);
-	case NFS4_OPEN_DELEGATE_NONE_EXT: /* 4.1 */
-		switch (open->op_why_no_deleg) {
-		case WND4_CONTENTION:
-		case WND4_RESOURCE:
-			p = xdr_reserve_space(xdr, 8);
-			if (!p)
-				return nfserr_resource;
-			*p++ = cpu_to_be32(open->op_why_no_deleg);
-			/* deleg signaling not supported yet: */
-			*p++ = cpu_to_be32(0);
-			break;
-		default:
-			p = xdr_reserve_space(xdr, 4);
-			if (!p)
-				return nfserr_resource;
-			*p++ = cpu_to_be32(open->op_why_no_deleg);
-		}
-		break;
+	case NFS4_OPEN_DELEGATE_NONE_EXT:
+		/* od_whynone */
+		return nfsd4_encode_open_none_delegation4(xdr, open);
 	default:
 		BUG();
 	}

[PATCH v1 6/7] NFSD: Add nfsd4_encode_open_delegation4()

[Chuck Lever]

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

To better align our implementation with the XDR specification,
refactor the part of nfsd4_encode_open() that encodes delegation
metadata.

As part of that refactor, remove an unnecessary BUG() call site and
a comment that appears to be stale.

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

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index b6c5ccb9351f..f37c370ceded 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4170,13 +4170,43 @@ nfsd4_encode_open_none_delegation4(struct xdr_stream *xdr,
 	return status;
 }
 
+static __be32
+nfsd4_encode_open_delegation4(struct xdr_stream *xdr, struct nfsd4_open *open)
+{
+	__be32 status;
+
+	/* delegation_type */
+	if (xdr_stream_encode_u32(xdr, open->op_delegate_type) != XDR_UNIT)
+		return nfserr_resource;
+	switch (open->op_delegate_type) {
+	case NFS4_OPEN_DELEGATE_NONE:
+		status = nfs_ok;
+		break;
+	case NFS4_OPEN_DELEGATE_READ:
+		/* read */
+		status = nfsd4_encode_open_read_delegation4(xdr, open);
+		break;
+	case NFS4_OPEN_DELEGATE_WRITE:
+		/* write */
+		status = nfsd4_encode_open_write_delegation4(xdr, open);
+		break;
+	case NFS4_OPEN_DELEGATE_NONE_EXT:
+		/* od_whynone */
+		status = nfsd4_encode_open_none_delegation4(xdr, open);
+		break;
+	default:
+		status = nfserr_serverfault;
+	}
+
+	return status;
+}
+
 static __be32
 nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
 		  union nfsd4_op_u *u)
 {
 	struct nfsd4_open *open = &u->open;
 	struct xdr_stream *xdr = resp->xdr;
-	__be32 *p;
 
 	nfserr = nfsd4_encode_stateid4(xdr, &open->op_stateid);
 	if (nfserr)
@@ -4192,28 +4222,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
 	if (nfserr)
 		return nfserr;
 
-	p = xdr_reserve_space(xdr, 4);
-	if (!p)
-		return nfserr_resource;
-
-	*p++ = cpu_to_be32(open->op_delegate_type);
-	switch (open->op_delegate_type) {
-	case NFS4_OPEN_DELEGATE_NONE:
-		break;
-	case NFS4_OPEN_DELEGATE_READ:
-		/* read */
-		return nfsd4_encode_open_read_delegation4(xdr, open);
-	case NFS4_OPEN_DELEGATE_WRITE:
-		/* write */
-		return nfsd4_encode_open_write_delegation4(xdr, open);
-	case NFS4_OPEN_DELEGATE_NONE_EXT:
-		/* od_whynone */
-		return nfsd4_encode_open_none_delegation4(xdr, open);
-	default:
-		BUG();
-	}
-	/* XXX save filehandle here */
-	return 0;
+	/* delegation */
+	return nfsd4_encode_open_delegation4(xdr, open);
 }
 
 static __be32

[PATCH v1 7/7] NFSD: Clean up nfsd4_encode_open()

[Chuck Lever]

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

Finish cleaning up nfsd4_encode_open().

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

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index f37c370ceded..143efc24551d 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4208,20 +4208,23 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
 	struct nfsd4_open *open = &u->open;
 	struct xdr_stream *xdr = resp->xdr;
 
+	/* stateid */
 	nfserr = nfsd4_encode_stateid4(xdr, &open->op_stateid);
-	if (nfserr)
+	if (nfserr != nfs_ok)
 		return nfserr;
+	/* cinfo */
 	nfserr = nfsd4_encode_change_info4(xdr, &open->op_cinfo);
-	if (nfserr)
+	if (nfserr != nfs_ok)
 		return nfserr;
-	if (xdr_stream_encode_u32(xdr, open->op_rflags) < 0)
-		return nfserr_resource;
-
+	/* rflags */
+	nfserr = nfsd4_encode_uint32_t(xdr, open->op_rflags);
+	if (nfserr != nfs_ok)
+		return nfserr;
+	/* attrset */
 	nfserr = nfsd4_encode_bitmap4(xdr, open->op_bmval[0],
 				      open->op_bmval[1], open->op_bmval[2]);
-	if (nfserr)
+	if (nfserr != nfs_ok)
 		return nfserr;
-
 	/* delegation */
 	return nfsd4_encode_open_delegation4(xdr, open);
 }

Re: [PATCH v1 0/7] Clean up XDR encoders for NFSv4 OPEN and LOCK

[Jeff Layton]

On Fri, 2023-09-29 at 09:58 -0400, Chuck Lever wrote:
> Tidy up the server-side XDR encoders for NFSv4 OPEN and LOCK
> results. Series applies to nfsd-next. See topic branch
> "nfsd4-encoder-overhaul" in this repo:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> 
> ---
> 
> Chuck Lever (7):
>       NFSD: Add nfsd4_encode_lock_owner4()
>       NFSD: Refactor nfsd4_encode_lock_denied()
>       NFSD: Add nfsd4_encode_open_read_delegation4()
>       NFSD: Add nfsd4_encode_open_write_delegation4()
>       NFSD: Add nfsd4_encode_open_none_delegation4()
>       NFSD: Add nfsd4_encode_open_delegation4()
>       NFSD: Clean up nfsd4_encode_open()
> 
> 
>  fs/nfsd/nfs4state.c |   6 +-
>  fs/nfsd/nfs4xdr.c   | 305 +++++++++++++++++++++++++++-----------------
>  fs/nfsd/xdr4.h      |   2 +-
>  3 files changed, 189 insertions(+), 124 deletions(-)
> 
> --
> Chuck Lever
> 

Looks pretty straightforward. I had one minor nit about the struct
packing but the rest looks fine. You can add:

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