[PATCH v6 0/5] NFSD: add support for NFSv4.1+ write delegation

[PATCH v6 0/5] NFSD: add support for NFSv4.1+ write delegation

[Dai Ngo]

The NFSv4 server currently supports read delegation using VFS lease
which is implemented using file_lock. 

This patch series add write delegation support for NFSv4.1+ client by:

    . remove the check for F_WRLCK in generic_add_lease to allow
      file_lock to be used for write delegation.  

    . grant write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
      if there is no conflict with other OPENs.

Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
are handled the same as read delegation using notify_change, try_break_deleg.

The write delegation support is for NFSv4.1+ client only since the NFSv4.0
Linux client behavior is not compliant with RFC 7530 Section 16.7.5. It
expects the server to look ahead in the compound to find a stateid in order
to determine whether the client that sends the GETATTR is the same client
that holds the write delegation. RFC 7530 spec does not call for the server
to look ahead in order to service the GETATTR op.

Changes since v1:

[PATCH 3/4] NFSD: add supports for CB_GETATTR callback
- remove WARN_ON_ONCE from encode_bitmap4
- replace decode_bitmap4 with xdr_stream_decode_uint32_array
- replace xdr_inline_decode and xdr_decode_hyper in decode_cb_getattr
   with xdr_stream_decode_u64. Also remove the un-needed likely().
- modify signature of encode_cb_getattr4args to take pointer to
   nfs4_cb_fattr
- replace decode_attr_length with xdr_stream_decode_u32
- rename decode_cb_getattr to decode_cb_fattr4
- fold the initialization of cb_cinfo and cb_fsize into decode_cb_fattr4
- rename ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
  in fs/nfsd/nfs4xdr.c
- correct NFS4_dec_cb_getattr_sz and update size description

[PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
- change nfs4_handle_wrdeleg_conflict returns __be32 to fix test robot
- change ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
  in fs/nfsd/nfs4xdr.c

Changes since v2:

[PATCH 2/4] NFSD: enable support for write delegation
- rename 'deleg' to 'dl_type' in nfs4_set_delegation
- remove 'wdeleg' in nfs4_open_delegation

- drop [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
  and [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
  for futher clarification of the benefits of these patches

Changes since v3:

- recall write delegation when there is GETATTR from 2nd client
- add trace point to track when write delegation is granted 

Changes since v4:
- squash 4/4 into 2/4
- apply 1/4 last instead of first
- combine nfs4_wrdeleg_filelock and nfs4_handle_wrdeleg_conflict to
  nfsd4_deleg_getattr_conflict and move it to fs/nfsd/nfs4state.c
- check for lock belongs to delegation before proceed and do it
  under the fl_lock
- check and skip FL_LAYOUT file_locks

Changes since v5:
- [patch 2/5] disable write delegation for NFSv4.0 client

- [patch 4/5] allow client to use write delegation stateid for READ (same
  behavior as Solaris server)

  When the server receives a READ request with write delegation stateid
  the server may returns the NFS4ERR_OPENMODE or allows the READ to proceed
  to accommodate clients whose WRITE implementation may unavoidably do reads
  (e.g., due to buffer cache constraints).  Per RFC 8881 section 9.1.2. Use
  of the Stateid and Locking

  Returning NFS4ERR_OPENMODE causes the client and server to enter an infinite
  loop of READ, NFS4ERR_OPENMODE, TEST_STATEID, READs, NFS4ERR_OPENMODEs,
  TEST_STATEID, READs, NFS4ERR_OPENMODEs. The Linux NFS client can not recover
  from NFS4ERR_OPENMODE for READ request if the file was opened with
  OPEN4_SHARE_ACCESS_WRITE. This READ was initiated internally from the NFS
  client and not from the read(2) system call.

- pass git regression test with 40 threads

[PATCH v6 1/5] locks: allow support for write delegation

[Dai Ngo]

Remove the check for F_WRLCK in generic_add_lease to allow file_lock
to be used for write delegation.

First consumer is NFSD.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/locks.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index df8b26a42524..08fb0b4fd4f8 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
 	if (is_deleg && !inode_trylock(inode))
 		return -EAGAIN;
 
-	if (is_deleg && arg == F_WRLCK) {
-		/* Write delegations are not currently supported: */
-		inode_unlock(inode);
-		WARN_ON_ONCE(1);
-		return -EINVAL;
-	}
-
 	percpu_down_read(&file_rwsem);
 	spin_lock(&ctx->flc_lock);
 	time_out_leases(inode, &dispose);
-- 
2.39.3

[PATCH v6 2/5] NFSD: Enable write delegation support for NFSv4.1+ client

[Dai Ngo]

This patch grants write delegations for OPEN with NFS4_SHARE_ACCESS_WRITE
if there is no conflict with other OPENs.

Write delegation conflicts with another OPEN, REMOVE, RENAME and SETATTR
are handled the same as read delegation using notify_change,
try_break_deleg.

The write delegation support is for NFSv4.1+ client only since the NFSv4.0
Linux client behavior is not compliant with RFC 7530 Section 16.7.5. It
expects the server to look ahead in the compound to find a stateid in order
to determine whether the client that sends the GETATTR is the same client
that holds the write delegation. RFC 7530 spec does not call for the server
to look ahead in order to service the GETATTR op.

Tracepoint added to track whether read or write delegation is granted.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++-----------
 fs/nfsd/trace.h     |  1 +
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6e61fa3acaf1..f971919b04c7 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh)
 
 static struct nfs4_delegation *
 alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
-		 struct nfs4_clnt_odstate *odstate)
+		struct nfs4_clnt_odstate *odstate, u32 dl_type)
 {
 	struct nfs4_delegation *dp;
 	long n;
@@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
 	INIT_LIST_HEAD(&dp->dl_recall_lru);
 	dp->dl_clnt_odstate = odstate;
 	get_clnt_odstate(odstate);
-	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
+	dp->dl_type = dl_type;
 	dp->dl_retries = 1;
 	dp->dl_recalled = false;
 	nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
@@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	struct nfs4_delegation *dp;
 	struct nfsd_file *nf;
 	struct file_lock *fl;
+	u32 dl_type;
 
 	/*
 	 * The fi_had_conflict and nfs_get_existing_delegation checks
@@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	if (fp->fi_had_conflict)
 		return ERR_PTR(-EAGAIN);
 
-	nf = find_readable_file(fp);
+	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
+		nf = find_writeable_file(fp);
+		dl_type = NFS4_OPEN_DELEGATE_WRITE;
+	} else {
+		nf = find_readable_file(fp);
+		dl_type = NFS4_OPEN_DELEGATE_READ;
+	}
 	if (!nf) {
 		/*
 		 * We probably could attempt another open and get a read
@@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 		return ERR_PTR(status);
 
 	status = -ENOMEM;
-	dp = alloc_init_deleg(clp, fp, odstate);
+	dp = alloc_init_deleg(clp, fp, odstate, dl_type);
 	if (!dp)
 		goto out_delegees;
 
-	fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
+	fl = nfs4_alloc_init_lease(dp, dl_type);
 	if (!fl)
 		goto out_clnt_odstate;
 
@@ -5570,8 +5577,13 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
 /*
  * Attempt to hand out a delegation.
  *
- * Note we don't support write delegations, and won't until the vfs has
- * proper support for them.
+ * Note we don't support write delegations for NFSv4.0 client since the Linux
+ * client behavior is not compliant with RFC 7530 Section 16.7.5 with regard
+ * to handle the conflict GETATTR. It expects the server to look ahead in the
+ * compound (PUTFH, GETATTR, DELEGRETURN) to find a stateid in order to
+ * determine whether the client that sends the GETATTR is the same with the
+ * client that holds the write delegation. RFC 7530 spec does not call for
+ * the server to look ahead in order to service the conflict GETATTR op.
  */
 static void
 nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
@@ -5590,8 +5602,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 		case NFS4_OPEN_CLAIM_PREVIOUS:
 			if (!cb_up)
 				open->op_recall = 1;
-			if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ)
-				goto out_no_deleg;
 			break;
 		case NFS4_OPEN_CLAIM_NULL:
 			parent = currentfh;
@@ -5606,6 +5616,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 				goto out_no_deleg;
 			if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED))
 				goto out_no_deleg;
+			if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE &&
+					!clp->cl_minorversion)
+				goto out_no_deleg;
 			break;
 		default:
 			goto out_no_deleg;
@@ -5616,8 +5629,13 @@ 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));
 
-	trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
-	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
+	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);
+	} else {
+		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
+		trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
+	}
 	nfs4_put_stid(&dp->dl_stid);
 	return;
 out_no_deleg:
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 72a906a053dc..56f28364cc6b 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -607,6 +607,7 @@ DEFINE_STATEID_EVENT(layout_recall_release);
 
 DEFINE_STATEID_EVENT(open);
 DEFINE_STATEID_EVENT(deleg_read);
+DEFINE_STATEID_EVENT(deleg_write);
 DEFINE_STATEID_EVENT(deleg_return);
 DEFINE_STATEID_EVENT(deleg_recall);
 
-- 
2.39.3

Re: [PATCH v6 2/5] NFSD: Enable write delegation support for NFSv4.1+ client

[Chuck Lever]

On Wed, Jun 28, 2023 at 07:36:13PM -0700, Dai Ngo wrote:
> This patch grants write delegations for OPEN with NFS4_SHARE_ACCESS_WRITE
> if there is no conflict with other OPENs.
> 
> Write delegation conflicts with another OPEN, REMOVE, RENAME and SETATTR
> are handled the same as read delegation using notify_change,
> try_break_deleg.
> 
> The write delegation support is for NFSv4.1+ client only since the NFSv4.0
> Linux client behavior is not compliant with RFC 7530 Section 16.7.5. It
> expects the server to look ahead in the compound to find a stateid in order
> to determine whether the client that sends the GETATTR is the same client
> that holds the write delegation. RFC 7530 spec does not call for the server
> to look ahead in order to service the GETATTR op.

Here (and the comment below) I would rather state this issue in
terms of protocol constraints.

"The NFSv4.0 protocol does not enable a server to determine that a
conflicting GETATTR originated from the client holding the
delegation versus coming from some other client. With NFSv4.1 and
later, the SEQUENCE operation that begins each COMPOUND contains a
client ID, so delegation recall can be safely squelched in this case.

With NFSv4.0, therefore, the server must recall or send a CB_GETATTR
(per RFC 7530 Section 16.7.5) even when the GETATTR originates from
the client holding that delegation.

An NFSv4.0 client can trigger a pathological situation if it always
sends a DELEGRETURN preceded by a conflicting GETATTR in the same
COMPOUND. COMPOUND execution will always stop at the GETATTR and the
DELEGRETURN will never get executed. The server eventually revokes
the delegation, which can result in loss of open or lock state."

Comments and further edits welcome!


> Tracepoint added to track whether read or write delegation is granted.
> 
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++-----------
>  fs/nfsd/trace.h     |  1 +
>  2 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6e61fa3acaf1..f971919b04c7 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh)
>  
>  static struct nfs4_delegation *
>  alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
> -		 struct nfs4_clnt_odstate *odstate)
> +		struct nfs4_clnt_odstate *odstate, u32 dl_type)
>  {
>  	struct nfs4_delegation *dp;
>  	long n;
> @@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>  	INIT_LIST_HEAD(&dp->dl_recall_lru);
>  	dp->dl_clnt_odstate = odstate;
>  	get_clnt_odstate(odstate);
> -	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
> +	dp->dl_type = dl_type;
>  	dp->dl_retries = 1;
>  	dp->dl_recalled = false;
>  	nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
> @@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	struct nfs4_delegation *dp;
>  	struct nfsd_file *nf;
>  	struct file_lock *fl;
> +	u32 dl_type;
>  
>  	/*
>  	 * The fi_had_conflict and nfs_get_existing_delegation checks
> @@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	if (fp->fi_had_conflict)
>  		return ERR_PTR(-EAGAIN);
>  
> -	nf = find_readable_file(fp);
> +	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> +		nf = find_writeable_file(fp);
> +		dl_type = NFS4_OPEN_DELEGATE_WRITE;
> +	} else {
> +		nf = find_readable_file(fp);
> +		dl_type = NFS4_OPEN_DELEGATE_READ;
> +	}
>  	if (!nf) {
>  		/*
>  		 * We probably could attempt another open and get a read
> @@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  		return ERR_PTR(status);
>  
>  	status = -ENOMEM;
> -	dp = alloc_init_deleg(clp, fp, odstate);
> +	dp = alloc_init_deleg(clp, fp, odstate, dl_type);
>  	if (!dp)
>  		goto out_delegees;
>  
> -	fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
> +	fl = nfs4_alloc_init_lease(dp, dl_type);
>  	if (!fl)
>  		goto out_clnt_odstate;
>  
> @@ -5570,8 +5577,13 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
>  /*
>   * Attempt to hand out a delegation.
>   *
> - * Note we don't support write delegations, and won't until the vfs has
> - * proper support for them.
> + * Note we don't support write delegations for NFSv4.0 client since the Linux
> + * client behavior is not compliant with RFC 7530 Section 16.7.5 with regard
> + * to handle the conflict GETATTR. It expects the server to look ahead in the
> + * compound (PUTFH, GETATTR, DELEGRETURN) to find a stateid in order to
> + * determine whether the client that sends the GETATTR is the same with the
> + * client that holds the write delegation. RFC 7530 spec does not call for
> + * the server to look ahead in order to service the conflict GETATTR op.
>   */
>  static void
>  nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> @@ -5590,8 +5602,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  		case NFS4_OPEN_CLAIM_PREVIOUS:
>  			if (!cb_up)
>  				open->op_recall = 1;
> -			if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ)
> -				goto out_no_deleg;
>  			break;
>  		case NFS4_OPEN_CLAIM_NULL:
>  			parent = currentfh;
> @@ -5606,6 +5616,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  				goto out_no_deleg;
>  			if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED))
>  				goto out_no_deleg;
> +			if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE &&
> +					!clp->cl_minorversion)
> +				goto out_no_deleg;
>  			break;
>  		default:
>  			goto out_no_deleg;
> @@ -5616,8 +5629,13 @@ 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));
>  
> -	trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
> -	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> +	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);
> +	} else {
> +		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> +		trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
> +	}
>  	nfs4_put_stid(&dp->dl_stid);
>  	return;
>  out_no_deleg:
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 72a906a053dc..56f28364cc6b 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -607,6 +607,7 @@ DEFINE_STATEID_EVENT(layout_recall_release);
>  
>  DEFINE_STATEID_EVENT(open);
>  DEFINE_STATEID_EVENT(deleg_read);
> +DEFINE_STATEID_EVENT(deleg_write);
>  DEFINE_STATEID_EVENT(deleg_return);
>  DEFINE_STATEID_EVENT(deleg_recall);
>  
> -- 
> 2.39.3
> 

Re: [PATCH v6 2/5] NFSD: Enable write delegation support for NFSv4.1+ client

[dai.ngo]

Thank you Chuck and Jeff for reviewing the patch.

On 6/29/23 7:54 AM, Chuck Lever wrote:
> On Wed, Jun 28, 2023 at 07:36:13PM -0700, Dai Ngo wrote:
>> This patch grants write delegations for OPEN with NFS4_SHARE_ACCESS_WRITE
>> if there is no conflict with other OPENs.
>>
>> Write delegation conflicts with another OPEN, REMOVE, RENAME and SETATTR
>> are handled the same as read delegation using notify_change,
>> try_break_deleg.
>>
>> The write delegation support is for NFSv4.1+ client only since the NFSv4.0
>> Linux client behavior is not compliant with RFC 7530 Section 16.7.5. It
>> expects the server to look ahead in the compound to find a stateid in order
>> to determine whether the client that sends the GETATTR is the same client
>> that holds the write delegation. RFC 7530 spec does not call for the server
>> to look ahead in order to service the GETATTR op.
> Here (and the comment below) I would rather state this issue in
> terms of protocol constraints.
>
> "The NFSv4.0 protocol does not enable a server to determine that a
> conflicting GETATTR originated from the client holding the
> delegation versus coming from some other client. With NFSv4.1 and
> later, the SEQUENCE operation that begins each COMPOUND contains a
> client ID, so delegation recall can be safely squelched in this case.
>
> With NFSv4.0, therefore, the server must recall or send a CB_GETATTR
> (per RFC 7530 Section 16.7.5) even when the GETATTR originates from
> the client holding that delegation.
>
> An NFSv4.0 client can trigger a pathological situation if it always
> sends a DELEGRETURN preceded by a conflicting GETATTR in the same
> COMPOUND. COMPOUND execution will always stop at the GETATTR and the
> DELEGRETURN will never get executed. The server eventually revokes
> the delegation, which can result in loss of open or lock state."
>
> Comments and further edits welcome!

I will update the comment with this explanation in v7.

-Dai

>
>
>> Tracepoint added to track whether read or write delegation is granted.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++-----------
>>   fs/nfsd/trace.h     |  1 +
>>   2 files changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 6e61fa3acaf1..f971919b04c7 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh)
>>   
>>   static struct nfs4_delegation *
>>   alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>> -		 struct nfs4_clnt_odstate *odstate)
>> +		struct nfs4_clnt_odstate *odstate, u32 dl_type)
>>   {
>>   	struct nfs4_delegation *dp;
>>   	long n;
>> @@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>>   	INIT_LIST_HEAD(&dp->dl_recall_lru);
>>   	dp->dl_clnt_odstate = odstate;
>>   	get_clnt_odstate(odstate);
>> -	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
>> +	dp->dl_type = dl_type;
>>   	dp->dl_retries = 1;
>>   	dp->dl_recalled = false;
>>   	nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
>> @@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>   	struct nfs4_delegation *dp;
>>   	struct nfsd_file *nf;
>>   	struct file_lock *fl;
>> +	u32 dl_type;
>>   
>>   	/*
>>   	 * The fi_had_conflict and nfs_get_existing_delegation checks
>> @@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>   	if (fp->fi_had_conflict)
>>   		return ERR_PTR(-EAGAIN);
>>   
>> -	nf = find_readable_file(fp);
>> +	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>> +		nf = find_writeable_file(fp);
>> +		dl_type = NFS4_OPEN_DELEGATE_WRITE;
>> +	} else {
>> +		nf = find_readable_file(fp);
>> +		dl_type = NFS4_OPEN_DELEGATE_READ;
>> +	}
>>   	if (!nf) {
>>   		/*
>>   		 * We probably could attempt another open and get a read
>> @@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>   		return ERR_PTR(status);
>>   
>>   	status = -ENOMEM;
>> -	dp = alloc_init_deleg(clp, fp, odstate);
>> +	dp = alloc_init_deleg(clp, fp, odstate, dl_type);
>>   	if (!dp)
>>   		goto out_delegees;
>>   
>> -	fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
>> +	fl = nfs4_alloc_init_lease(dp, dl_type);
>>   	if (!fl)
>>   		goto out_clnt_odstate;
>>   
>> @@ -5570,8 +5577,13 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
>>   /*
>>    * Attempt to hand out a delegation.
>>    *
>> - * Note we don't support write delegations, and won't until the vfs has
>> - * proper support for them.
>> + * Note we don't support write delegations for NFSv4.0 client since the Linux
>> + * client behavior is not compliant with RFC 7530 Section 16.7.5 with regard
>> + * to handle the conflict GETATTR. It expects the server to look ahead in the
>> + * compound (PUTFH, GETATTR, DELEGRETURN) to find a stateid in order to
>> + * determine whether the client that sends the GETATTR is the same with the
>> + * client that holds the write delegation. RFC 7530 spec does not call for
>> + * the server to look ahead in order to service the conflict GETATTR op.
>>    */
>>   static void
>>   nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> @@ -5590,8 +5602,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>   		case NFS4_OPEN_CLAIM_PREVIOUS:
>>   			if (!cb_up)
>>   				open->op_recall = 1;
>> -			if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ)
>> -				goto out_no_deleg;
>>   			break;
>>   		case NFS4_OPEN_CLAIM_NULL:
>>   			parent = currentfh;
>> @@ -5606,6 +5616,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>   				goto out_no_deleg;
>>   			if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED))
>>   				goto out_no_deleg;
>> +			if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE &&
>> +					!clp->cl_minorversion)
>> +				goto out_no_deleg;
>>   			break;
>>   		default:
>>   			goto out_no_deleg;
>> @@ -5616,8 +5629,13 @@ 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));
>>   
>> -	trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
>> -	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
>> +	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);
>> +	} else {
>> +		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
>> +		trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
>> +	}
>>   	nfs4_put_stid(&dp->dl_stid);
>>   	return;
>>   out_no_deleg:
>> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
>> index 72a906a053dc..56f28364cc6b 100644
>> --- a/fs/nfsd/trace.h
>> +++ b/fs/nfsd/trace.h
>> @@ -607,6 +607,7 @@ DEFINE_STATEID_EVENT(layout_recall_release);
>>   
>>   DEFINE_STATEID_EVENT(open);
>>   DEFINE_STATEID_EVENT(deleg_read);
>> +DEFINE_STATEID_EVENT(deleg_write);
>>   DEFINE_STATEID_EVENT(deleg_return);
>>   DEFINE_STATEID_EVENT(deleg_recall);
>>   
>> -- 
>> 2.39.3
>>

[PATCH v6 3/5] NFSD: handle GETATTR conflict with write delegation

[Dai Ngo]

If the GETATTR request on a file that has write delegation in effect and
the request attributes include the change info and size attribute then
the write delegation is recalled. If the delegation is returned within
30ms then the GETATTR is serviced as normal otherwise the NFS4ERR_DELAY
error is returned for the GETATTR.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfs4xdr.c   |  5 ++++
 fs/nfsd/state.h     |  3 +++
 3 files changed, 68 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f971919b04c7..2d2656c41ffb 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8361,3 +8361,63 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
 {
 	get_stateid(cstate, &u->write.wr_stateid);
 }
+
+/**
+ * nfsd4_deleg_getattr_conflict - Trigger recall if GETATTR causes conflict
+ * @rqstp: RPC transaction context
+ * @inode: file to be checked for a conflict
+ *
+ * This function is called when there is a conflict between a write
+ * delegation and a change/size GETATR from another client. The server
+ * must either use the CB_GETATTR to get the current values of the
+ * attributes from the client that hold the delegation or recall the
+ * delegation before replying to the GETATTR. See RFC 8881 section
+ * 18.7.4.
+ *
+ * Returns 0 if there is no conflict; otherwise an nfs_stat
+ * code is returned.
+ */
+__be32
+nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
+{
+	__be32 status;
+	struct file_lock_context *ctx;
+	struct file_lock *fl;
+	struct nfs4_delegation *dp;
+
+	ctx = locks_inode_context(inode);
+	if (!ctx)
+		return 0;
+	spin_lock(&ctx->flc_lock);
+	list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
+		if (fl->fl_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 (fl->fl_type == F_RDLCK)
+				break;
+			goto break_lease;
+		}
+		if (fl->fl_type == F_WRLCK) {
+			dp = fl->fl_owner;
+			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
+				spin_unlock(&ctx->flc_lock);
+				return 0;
+			}
+break_lease:
+			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;
+		}
+		break;
+	}
+	spin_unlock(&ctx->flc_lock);
+	return 0;
+}
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 76db2fe29624..b35855c8beb6 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2966,6 +2966,11 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
 		if (status)
 			goto out;
 	}
+	if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
+		status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry));
+		if (status)
+			goto out;
+	}
 
 	err = vfs_getattr(&path, &stat,
 			  STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index d49d3060ed4f..cbddcf484dba 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -732,4 +732,7 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
 	cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
 	return clp->cl_state == NFSD4_EXPIRABLE;
 }
+
+extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
+				struct inode *inode);
 #endif   /* NFSD4_STATE_H */
-- 
2.39.3

Re: [PATCH v6 3/5] NFSD: handle GETATTR conflict with write delegation

[Chuck Lever]

On Wed, Jun 28, 2023 at 07:36:14PM -0700, Dai Ngo wrote:
> If the GETATTR request on a file that has write delegation in effect and
> the request attributes include the change info and size attribute then
> the write delegation is recalled. If the delegation is returned within
> 30ms then the GETATTR is serviced as normal otherwise the NFS4ERR_DELAY
> error is returned for the GETATTR.
>
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>  fs/nfsd/nfs4xdr.c   |  5 ++++
>  fs/nfsd/state.h     |  3 +++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f971919b04c7..2d2656c41ffb 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -8361,3 +8361,63 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
>  {
>  	get_stateid(cstate, &u->write.wr_stateid);
>  }
> +
> +/**
> + * nfsd4_deleg_getattr_conflict - Trigger recall if GETATTR causes conflict
> + * @rqstp: RPC transaction context
> + * @inode: file to be checked for a conflict
> + *
> + * This function is called when there is a conflict between a write
> + * delegation and a change/size GETATR from another client. The server

/GETATR/GETATTR/

> + * must either use the CB_GETATTR to get the current values of the
> + * attributes from the client that hold the delegation or recall the
> + * delegation before replying to the GETATTR. See RFC 8881 section
> + * 18.7.4.

Since you have mentioned CB_GETATTR here, you should also clarify
that our implementation currently does not use it, but eventually we
might implement CB_GETATTR to avoid recalling the delegation due to
this kind of conflict.

Thanks for the thorough comments!


> + *
> + * Returns 0 if there is no conflict; otherwise an nfs_stat
> + * code is returned.
> + */
> +__be32
> +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
> +{
> +	__be32 status;
> +	struct file_lock_context *ctx;
> +	struct file_lock *fl;
> +	struct nfs4_delegation *dp;
> +
> +	ctx = locks_inode_context(inode);
> +	if (!ctx)
> +		return 0;
> +	spin_lock(&ctx->flc_lock);
> +	list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
> +		if (fl->fl_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 (fl->fl_type == F_RDLCK)
> +				break;
> +			goto break_lease;
> +		}
> +		if (fl->fl_type == F_WRLCK) {
> +			dp = fl->fl_owner;
> +			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> +				spin_unlock(&ctx->flc_lock);
> +				return 0;
> +			}
> +break_lease:
> +			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;
> +		}
> +		break;
> +	}
> +	spin_unlock(&ctx->flc_lock);
> +	return 0;
> +}
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 76db2fe29624..b35855c8beb6 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2966,6 +2966,11 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>  		if (status)
>  			goto out;
>  	}
> +	if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
> +		status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry));
> +		if (status)
> +			goto out;
> +	}
>  
>  	err = vfs_getattr(&path, &stat,
>  			  STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index d49d3060ed4f..cbddcf484dba 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -732,4 +732,7 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
>  	cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
>  	return clp->cl_state == NFSD4_EXPIRABLE;
>  }
> +
> +extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
> +				struct inode *inode);
>  #endif   /* NFSD4_STATE_H */
> -- 
> 2.39.3
> 

Re: [PATCH v6 3/5] NFSD: handle GETATTR conflict with write delegation

[dai.ngo]

On 6/29/23 8:00 AM, Chuck Lever wrote:
> On Wed, Jun 28, 2023 at 07:36:14PM -0700, Dai Ngo wrote:
>> If the GETATTR request on a file that has write delegation in effect and
>> the request attributes include the change info and size attribute then
>> the write delegation is recalled. If the delegation is returned within
>> 30ms then the GETATTR is serviced as normal otherwise the NFS4ERR_DELAY
>> error is returned for the GETATTR.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>   fs/nfsd/nfs4xdr.c   |  5 ++++
>>   fs/nfsd/state.h     |  3 +++
>>   3 files changed, 68 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index f971919b04c7..2d2656c41ffb 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -8361,3 +8361,63 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
>>   {
>>   	get_stateid(cstate, &u->write.wr_stateid);
>>   }
>> +
>> +/**
>> + * nfsd4_deleg_getattr_conflict - Trigger recall if GETATTR causes conflict
>> + * @rqstp: RPC transaction context
>> + * @inode: file to be checked for a conflict
>> + *
>> + * This function is called when there is a conflict between a write
>> + * delegation and a change/size GETATR from another client. The server
> /GETATR/GETATTR/

will fix.

>
>> + * must either use the CB_GETATTR to get the current values of the
>> + * attributes from the client that hold the delegation or recall the
>> + * delegation before replying to the GETATTR. See RFC 8881 section
>> + * 18.7.4.
> Since you have mentioned CB_GETATTR here, you should also clarify
> that our implementation currently does not use it, but eventually we
> might implement CB_GETATTR to avoid recalling the delegation due to
> this kind of conflict.

will do.

-Dai

>
> Thanks for the thorough comments!
>
>
>> + *
>> + * Returns 0 if there is no conflict; otherwise an nfs_stat
>> + * code is returned.
>> + */
>> +__be32
>> +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
>> +{
>> +	__be32 status;
>> +	struct file_lock_context *ctx;
>> +	struct file_lock *fl;
>> +	struct nfs4_delegation *dp;
>> +
>> +	ctx = locks_inode_context(inode);
>> +	if (!ctx)
>> +		return 0;
>> +	spin_lock(&ctx->flc_lock);
>> +	list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
>> +		if (fl->fl_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 (fl->fl_type == F_RDLCK)
>> +				break;
>> +			goto break_lease;
>> +		}
>> +		if (fl->fl_type == F_WRLCK) {
>> +			dp = fl->fl_owner;
>> +			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
>> +				spin_unlock(&ctx->flc_lock);
>> +				return 0;
>> +			}
>> +break_lease:
>> +			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;
>> +		}
>> +		break;
>> +	}
>> +	spin_unlock(&ctx->flc_lock);
>> +	return 0;
>> +}
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 76db2fe29624..b35855c8beb6 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -2966,6 +2966,11 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>>   		if (status)
>>   			goto out;
>>   	}
>> +	if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
>> +		status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry));
>> +		if (status)
>> +			goto out;
>> +	}
>>   
>>   	err = vfs_getattr(&path, &stat,
>>   			  STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index d49d3060ed4f..cbddcf484dba 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -732,4 +732,7 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
>>   	cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
>>   	return clp->cl_state == NFSD4_EXPIRABLE;
>>   }
>> +
>> +extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
>> +				struct inode *inode);
>>   #endif   /* NFSD4_STATE_H */
>> -- 
>> 2.39.3
>>

[PATCH v6 4/5] NFSD: allow client to use write delegation stateid for READ

[Dai Ngo]

Allow NFSv4 client to use write delegation stateid for READ operation.
Per RFC 8881 section 9.1.2. Use of the Stateid and Locking.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4proc.c | 16 ++++++++++++++--
 fs/nfsd/nfs4xdr.c  |  9 +++++++++
 fs/nfsd/xdr4.h     |  2 ++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 5ae670807449..3fa66cb38780 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -942,8 +942,18 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	/* check stateid */
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
 					&read->rd_stateid, RD_STATE,
-					&read->rd_nf, NULL);
-
+					&read->rd_nf, &read->rd_wd_stid);
+	/*
+	 * rd_wd_stid is needed for nfsd4_encode_read to allow write
+	 * delegation stateid used for read. Its refcount is decremented
+	 * by nfsd4_read_release when read is done.
+	 */
+	if (!status && (read->rd_wd_stid->sc_type != NFS4_DELEG_STID ||
+			delegstateid(read->rd_wd_stid)->dl_type !=
+			NFS4_OPEN_DELEGATE_WRITE)) {
+		nfs4_put_stid(read->rd_wd_stid);
+		read->rd_wd_stid = NULL;
+	}
 	read->rd_rqstp = rqstp;
 	read->rd_fhp = &cstate->current_fh;
 	return status;
@@ -953,6 +963,8 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 static void
 nfsd4_read_release(union nfsd4_op_u *u)
 {
+	if (u->read.rd_wd_stid)
+		nfs4_put_stid(u->read.rd_wd_stid);
 	if (u->read.rd_nf)
 		nfsd_file_put(u->read.rd_nf);
 	trace_nfsd_read_done(u->read.rd_rqstp, u->read.rd_fhp,
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index b35855c8beb6..833634cdc761 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4125,6 +4125,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
 	struct file *file;
 	int starting_len = xdr->buf->len;
 	__be32 *p;
+	fmode_t o_fmode = 0;
 
 	if (nfserr)
 		return nfserr;
@@ -4144,10 +4145,18 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
 	maxcount = min_t(unsigned long, read->rd_length,
 			 (xdr->buf->buflen - xdr->buf->len));
 
+	if (read->rd_wd_stid) {
+		/* allow READ using write delegation stateid */
+		o_fmode = file->f_mode;
+		file->f_mode |= FMODE_READ;
+	}
 	if (file->f_op->splice_read && splice_ok)
 		nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
 	else
 		nfserr = nfsd4_encode_readv(resp, read, file, maxcount);
+	if (o_fmode)
+		file->f_mode = o_fmode;
+
 	if (nfserr) {
 		xdr_truncate_encode(xdr, starting_len);
 		return nfserr;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 510978e602da..3ccc40f9274a 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -307,6 +307,8 @@ struct nfsd4_read {
 	struct svc_rqst		*rd_rqstp;          /* response */
 	struct svc_fh		*rd_fhp;            /* response */
 	u32			rd_eof;             /* response */
+
+	struct nfs4_stid	*rd_wd_stid;		/* internal */
 };
 
 struct nfsd4_readdir {
-- 
2.39.3

Re: [PATCH v6 4/5] NFSD: allow client to use write delegation stateid for READ

[Chuck Lever]

On Wed, Jun 28, 2023 at 07:36:15PM -0700, Dai Ngo wrote:
> Allow NFSv4 client to use write delegation stateid for READ operation.
> Per RFC 8881 section 9.1.2. Use of the Stateid and Locking.

I'm wondering if this fix should precede 2/5 to prevent breakage
during a bisect. Jeff, what do you think?


> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4proc.c | 16 ++++++++++++++--
>  fs/nfsd/nfs4xdr.c  |  9 +++++++++
>  fs/nfsd/xdr4.h     |  2 ++
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 5ae670807449..3fa66cb38780 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -942,8 +942,18 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	/* check stateid */
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					&read->rd_stateid, RD_STATE,
> -					&read->rd_nf, NULL);
> -
> +					&read->rd_nf, &read->rd_wd_stid);
> +	/*
> +	 * rd_wd_stid is needed for nfsd4_encode_read to allow write
> +	 * delegation stateid used for read. Its refcount is decremented
> +	 * by nfsd4_read_release when read is done.
> +	 */
> +	if (!status && (read->rd_wd_stid->sc_type != NFS4_DELEG_STID ||
> +			delegstateid(read->rd_wd_stid)->dl_type !=
> +			NFS4_OPEN_DELEGATE_WRITE)) {
> +		nfs4_put_stid(read->rd_wd_stid);
> +		read->rd_wd_stid = NULL;
> +	}
>  	read->rd_rqstp = rqstp;
>  	read->rd_fhp = &cstate->current_fh;
>  	return status;
> @@ -953,6 +963,8 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  static void
>  nfsd4_read_release(union nfsd4_op_u *u)
>  {
> +	if (u->read.rd_wd_stid)
> +		nfs4_put_stid(u->read.rd_wd_stid);
>  	if (u->read.rd_nf)
>  		nfsd_file_put(u->read.rd_nf);
>  	trace_nfsd_read_done(u->read.rd_rqstp, u->read.rd_fhp,
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index b35855c8beb6..833634cdc761 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4125,6 +4125,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	struct file *file;
>  	int starting_len = xdr->buf->len;
>  	__be32 *p;
> +	fmode_t o_fmode = 0;
>  
>  	if (nfserr)
>  		return nfserr;
> @@ -4144,10 +4145,18 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	maxcount = min_t(unsigned long, read->rd_length,
>  			 (xdr->buf->buflen - xdr->buf->len));
>  
> +	if (read->rd_wd_stid) {
> +		/* allow READ using write delegation stateid */
> +		o_fmode = file->f_mode;
> +		file->f_mode |= FMODE_READ;
> +	}
>  	if (file->f_op->splice_read && splice_ok)
>  		nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
>  	else
>  		nfserr = nfsd4_encode_readv(resp, read, file, maxcount);
> +	if (o_fmode)
> +		file->f_mode = o_fmode;
> +
>  	if (nfserr) {
>  		xdr_truncate_encode(xdr, starting_len);
>  		return nfserr;
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 510978e602da..3ccc40f9274a 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -307,6 +307,8 @@ struct nfsd4_read {
>  	struct svc_rqst		*rd_rqstp;          /* response */
>  	struct svc_fh		*rd_fhp;            /* response */
>  	u32			rd_eof;             /* response */
> +
> +	struct nfs4_stid	*rd_wd_stid;		/* internal */
>  };
>  
>  struct nfsd4_readdir {
> -- 
> 2.39.3
> 

Re: [PATCH v6 4/5] NFSD: allow client to use write delegation stateid for READ

[Jeff Layton]

On Thu, 2023-06-29 at 11:02 -0400, Chuck Lever wrote:
> On Wed, Jun 28, 2023 at 07:36:15PM -0700, Dai Ngo wrote:
> > Allow NFSv4 client to use write delegation stateid for READ operation.
> > Per RFC 8881 section 9.1.2. Use of the Stateid and Locking.
> 
> I'm wondering if this fix should precede 2/5 to prevent breakage
> during a bisect. Jeff, what do you think?
> 

Good point. Probably the patch that actually makes it actually hand out
write delegations should be the last one.
 
> 
> > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 16 ++++++++++++++--
> >  fs/nfsd/nfs4xdr.c  |  9 +++++++++
> >  fs/nfsd/xdr4.h     |  2 ++
> >  3 files changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 5ae670807449..3fa66cb38780 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -942,8 +942,18 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	/* check stateid */
> >  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> >  					&read->rd_stateid, RD_STATE,
> > -					&read->rd_nf, NULL);
> > -
> > +					&read->rd_nf, &read->rd_wd_stid);
> > +	/*
> > +	 * rd_wd_stid is needed for nfsd4_encode_read to allow write
> > +	 * delegation stateid used for read. Its refcount is decremented
> > +	 * by nfsd4_read_release when read is done.
> > +	 */
> > +	if (!status && (read->rd_wd_stid->sc_type != NFS4_DELEG_STID ||
> > +			delegstateid(read->rd_wd_stid)->dl_type !=
> > +			NFS4_OPEN_DELEGATE_WRITE)) {
> > +		nfs4_put_stid(read->rd_wd_stid);
> > +		read->rd_wd_stid = NULL;
> > +	}
> >  	read->rd_rqstp = rqstp;
> >  	read->rd_fhp = &cstate->current_fh;
> >  	return status;
> > @@ -953,6 +963,8 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  static void
> >  nfsd4_read_release(union nfsd4_op_u *u)
> >  {
> > +	if (u->read.rd_wd_stid)
> > +		nfs4_put_stid(u->read.rd_wd_stid);
> >  	if (u->read.rd_nf)
> >  		nfsd_file_put(u->read.rd_nf);
> >  	trace_nfsd_read_done(u->read.rd_rqstp, u->read.rd_fhp,
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index b35855c8beb6..833634cdc761 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -4125,6 +4125,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
> >  	struct file *file;
> >  	int starting_len = xdr->buf->len;
> >  	__be32 *p;
> > +	fmode_t o_fmode = 0;
> >  
> >  	if (nfserr)
> >  		return nfserr;
> > @@ -4144,10 +4145,18 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
> >  	maxcount = min_t(unsigned long, read->rd_length,
> >  			 (xdr->buf->buflen - xdr->buf->len));
> >  
> > +	if (read->rd_wd_stid) {
> > +		/* allow READ using write delegation stateid */
> > +		o_fmode = file->f_mode;
> > +		file->f_mode |= FMODE_READ;
> > +	}
> >  	if (file->f_op->splice_read && splice_ok)
> >  		nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
> >  	else
> >  		nfserr = nfsd4_encode_readv(resp, read, file, maxcount);
> > +	if (o_fmode)
> > +		file->f_mode = o_fmode;
> > +
> >  	if (nfserr) {
> >  		xdr_truncate_encode(xdr, starting_len);
> >  		return nfserr;
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index 510978e602da..3ccc40f9274a 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -307,6 +307,8 @@ struct nfsd4_read {
> >  	struct svc_rqst		*rd_rqstp;          /* response */
> >  	struct svc_fh		*rd_fhp;            /* response */
> >  	u32			rd_eof;             /* response */
> > +
> > +	struct nfs4_stid	*rd_wd_stid;		/* internal */
> >  };
> >  
> >  struct nfsd4_readdir {
> > -- 
> > 2.39.3
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v6 4/5] NFSD: allow client to use write delegation stateid for READ

[dai.ngo]

On 6/29/23 8:02 AM, Chuck Lever wrote:
> On Wed, Jun 28, 2023 at 07:36:15PM -0700, Dai Ngo wrote:
>> Allow NFSv4 client to use write delegation stateid for READ operation.
>> Per RFC 8881 section 9.1.2. Use of the Stateid and Locking.
> I'm wondering if this fix should precede 2/5 to prevent breakage
> during a bisect. Jeff, what do you think?

Will make this patch preceed 2/5.

-Dai

>
>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4proc.c | 16 ++++++++++++++--
>>   fs/nfsd/nfs4xdr.c  |  9 +++++++++
>>   fs/nfsd/xdr4.h     |  2 ++
>>   3 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 5ae670807449..3fa66cb38780 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -942,8 +942,18 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>   	/* check stateid */
>>   	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>>   					&read->rd_stateid, RD_STATE,
>> -					&read->rd_nf, NULL);
>> -
>> +					&read->rd_nf, &read->rd_wd_stid);
>> +	/*
>> +	 * rd_wd_stid is needed for nfsd4_encode_read to allow write
>> +	 * delegation stateid used for read. Its refcount is decremented
>> +	 * by nfsd4_read_release when read is done.
>> +	 */
>> +	if (!status && (read->rd_wd_stid->sc_type != NFS4_DELEG_STID ||
>> +			delegstateid(read->rd_wd_stid)->dl_type !=
>> +			NFS4_OPEN_DELEGATE_WRITE)) {
>> +		nfs4_put_stid(read->rd_wd_stid);
>> +		read->rd_wd_stid = NULL;
>> +	}
>>   	read->rd_rqstp = rqstp;
>>   	read->rd_fhp = &cstate->current_fh;
>>   	return status;
>> @@ -953,6 +963,8 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>   static void
>>   nfsd4_read_release(union nfsd4_op_u *u)
>>   {
>> +	if (u->read.rd_wd_stid)
>> +		nfs4_put_stid(u->read.rd_wd_stid);
>>   	if (u->read.rd_nf)
>>   		nfsd_file_put(u->read.rd_nf);
>>   	trace_nfsd_read_done(u->read.rd_rqstp, u->read.rd_fhp,
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index b35855c8beb6..833634cdc761 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -4125,6 +4125,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>>   	struct file *file;
>>   	int starting_len = xdr->buf->len;
>>   	__be32 *p;
>> +	fmode_t o_fmode = 0;
>>   
>>   	if (nfserr)
>>   		return nfserr;
>> @@ -4144,10 +4145,18 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>>   	maxcount = min_t(unsigned long, read->rd_length,
>>   			 (xdr->buf->buflen - xdr->buf->len));
>>   
>> +	if (read->rd_wd_stid) {
>> +		/* allow READ using write delegation stateid */
>> +		o_fmode = file->f_mode;
>> +		file->f_mode |= FMODE_READ;
>> +	}
>>   	if (file->f_op->splice_read && splice_ok)
>>   		nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
>>   	else
>>   		nfserr = nfsd4_encode_readv(resp, read, file, maxcount);
>> +	if (o_fmode)
>> +		file->f_mode = o_fmode;
>> +
>>   	if (nfserr) {
>>   		xdr_truncate_encode(xdr, starting_len);
>>   		return nfserr;
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index 510978e602da..3ccc40f9274a 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -307,6 +307,8 @@ struct nfsd4_read {
>>   	struct svc_rqst		*rd_rqstp;          /* response */
>>   	struct svc_fh		*rd_fhp;            /* response */
>>   	u32			rd_eof;             /* response */
>> +
>> +	struct nfs4_stid	*rd_wd_stid;		/* internal */
>>   };
>>   
>>   struct nfsd4_readdir {
>> -- 
>> 2.39.3
>>

[PATCH v6 5/5] NFSD: add counter for write delegation recall due to conflict GETATTR

[Dai Ngo]

Add counter to keep track of how many times write delegations are
recalled due to conflict with GETATTR.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 1 +
 fs/nfsd/stats.c     | 2 ++
 fs/nfsd/stats.h     | 7 +++++++
 3 files changed, 10 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2d2656c41ffb..6ce95e738359 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8410,6 +8410,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
 			}
 break_lease:
 			spin_unlock(&ctx->flc_lock);
+			nfsd_stats_wdeleg_getattr_inc();
 			status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
 			if (status != nfserr_jukebox ||
 					!nfsd_wait_for_delegreturn(rqstp, inode))
diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c
index 777e24e5da33..63797635e1c3 100644
--- a/fs/nfsd/stats.c
+++ b/fs/nfsd/stats.c
@@ -65,6 +65,8 @@ static int nfsd_show(struct seq_file *seq, void *v)
 		seq_printf(seq, " %lld",
 			   percpu_counter_sum_positive(&nfsdstats.counter[NFSD_STATS_NFS4_OP(i)]));
 	}
+	seq_printf(seq, "\nwdeleg_getattr %lld",
+		percpu_counter_sum_positive(&nfsdstats.counter[NFSD_STATS_WDELEG_GETATTR]));
 
 	seq_putc(seq, '\n');
 #endif
diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h
index 9b43dc3d9991..cf5524e7ca06 100644
--- a/fs/nfsd/stats.h
+++ b/fs/nfsd/stats.h
@@ -22,6 +22,7 @@ enum {
 	NFSD_STATS_FIRST_NFS4_OP,	/* count of individual nfsv4 operations */
 	NFSD_STATS_LAST_NFS4_OP = NFSD_STATS_FIRST_NFS4_OP + LAST_NFS4_OP,
 #define NFSD_STATS_NFS4_OP(op)	(NFSD_STATS_FIRST_NFS4_OP + (op))
+	NFSD_STATS_WDELEG_GETATTR,	/* count of getattr conflict with wdeleg */
 #endif
 	NFSD_STATS_COUNTERS_NUM
 };
@@ -93,4 +94,10 @@ static inline void nfsd_stats_drc_mem_usage_sub(struct nfsd_net *nn, s64 amount)
 	percpu_counter_sub(&nn->counter[NFSD_NET_DRC_MEM_USAGE], amount);
 }
 
+#ifdef CONFIG_NFSD_V4
+static inline void nfsd_stats_wdeleg_getattr_inc(void)
+{
+	percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_WDELEG_GETATTR]);
+}
+#endif
 #endif /* _NFSD_STATS_H */
-- 
2.39.3

Re: [PATCH v6 5/5] NFSD: add counter for write delegation recall due to conflict GETATTR

[Chuck Lever]

On Wed, Jun 28, 2023 at 07:36:16PM -0700, Dai Ngo wrote:
> Add counter to keep track of how many times write delegations are
> recalled due to conflict with GETATTR.

Should this wee patch be squashed into 3/5 ?

The patch description ought to explain /why/ we want to track
GETATTR conflicts. (even if you squash it into 3/5). Mostly I'm
trying to get the important design choices written down so we
can remember them in a year or two.


> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 1 +
>  fs/nfsd/stats.c     | 2 ++
>  fs/nfsd/stats.h     | 7 +++++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2d2656c41ffb..6ce95e738359 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -8410,6 +8410,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
>  			}
>  break_lease:
>  			spin_unlock(&ctx->flc_lock);
> +			nfsd_stats_wdeleg_getattr_inc();
>  			status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
>  			if (status != nfserr_jukebox ||
>  					!nfsd_wait_for_delegreturn(rqstp, inode))
> diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c
> index 777e24e5da33..63797635e1c3 100644
> --- a/fs/nfsd/stats.c
> +++ b/fs/nfsd/stats.c
> @@ -65,6 +65,8 @@ static int nfsd_show(struct seq_file *seq, void *v)
>  		seq_printf(seq, " %lld",
>  			   percpu_counter_sum_positive(&nfsdstats.counter[NFSD_STATS_NFS4_OP(i)]));
>  	}
> +	seq_printf(seq, "\nwdeleg_getattr %lld",
> +		percpu_counter_sum_positive(&nfsdstats.counter[NFSD_STATS_WDELEG_GETATTR]));
>  
>  	seq_putc(seq, '\n');
>  #endif
> diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h
> index 9b43dc3d9991..cf5524e7ca06 100644
> --- a/fs/nfsd/stats.h
> +++ b/fs/nfsd/stats.h
> @@ -22,6 +22,7 @@ enum {
>  	NFSD_STATS_FIRST_NFS4_OP,	/* count of individual nfsv4 operations */
>  	NFSD_STATS_LAST_NFS4_OP = NFSD_STATS_FIRST_NFS4_OP + LAST_NFS4_OP,
>  #define NFSD_STATS_NFS4_OP(op)	(NFSD_STATS_FIRST_NFS4_OP + (op))
> +	NFSD_STATS_WDELEG_GETATTR,	/* count of getattr conflict with wdeleg */
>  #endif
>  	NFSD_STATS_COUNTERS_NUM
>  };
> @@ -93,4 +94,10 @@ static inline void nfsd_stats_drc_mem_usage_sub(struct nfsd_net *nn, s64 amount)
>  	percpu_counter_sub(&nn->counter[NFSD_NET_DRC_MEM_USAGE], amount);
>  }
>  
> +#ifdef CONFIG_NFSD_V4
> +static inline void nfsd_stats_wdeleg_getattr_inc(void)
> +{
> +	percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_WDELEG_GETATTR]);
> +}
> +#endif
>  #endif /* _NFSD_STATS_H */
> -- 
> 2.39.3
> 

Re: [PATCH v6 5/5] NFSD: add counter for write delegation recall due to conflict GETATTR

[dai.ngo]

On 6/29/23 8:07 AM, Chuck Lever wrote:
> On Wed, Jun 28, 2023 at 07:36:16PM -0700, Dai Ngo wrote:
>> Add counter to keep track of how many times write delegations are
>> recalled due to conflict with GETATTR.
> Should this wee patch be squashed into 3/5 ?

Yes, will squash this into 3/5.

>
> The patch description ought to explain /why/ we want to track
> GETATTR conflicts. (even if you squash it into 3/5). Mostly I'm
> trying to get the important design choices written down so we
> can remember them in a year or two.

will do.

-Dai

>
>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 1 +
>>   fs/nfsd/stats.c     | 2 ++
>>   fs/nfsd/stats.h     | 7 +++++++
>>   3 files changed, 10 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 2d2656c41ffb..6ce95e738359 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -8410,6 +8410,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
>>   			}
>>   break_lease:
>>   			spin_unlock(&ctx->flc_lock);
>> +			nfsd_stats_wdeleg_getattr_inc();
>>   			status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
>>   			if (status != nfserr_jukebox ||
>>   					!nfsd_wait_for_delegreturn(rqstp, inode))
>> diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c
>> index 777e24e5da33..63797635e1c3 100644
>> --- a/fs/nfsd/stats.c
>> +++ b/fs/nfsd/stats.c
>> @@ -65,6 +65,8 @@ static int nfsd_show(struct seq_file *seq, void *v)
>>   		seq_printf(seq, " %lld",
>>   			   percpu_counter_sum_positive(&nfsdstats.counter[NFSD_STATS_NFS4_OP(i)]));
>>   	}
>> +	seq_printf(seq, "\nwdeleg_getattr %lld",
>> +		percpu_counter_sum_positive(&nfsdstats.counter[NFSD_STATS_WDELEG_GETATTR]));
>>   
>>   	seq_putc(seq, '\n');
>>   #endif
>> diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h
>> index 9b43dc3d9991..cf5524e7ca06 100644
>> --- a/fs/nfsd/stats.h
>> +++ b/fs/nfsd/stats.h
>> @@ -22,6 +22,7 @@ enum {
>>   	NFSD_STATS_FIRST_NFS4_OP,	/* count of individual nfsv4 operations */
>>   	NFSD_STATS_LAST_NFS4_OP = NFSD_STATS_FIRST_NFS4_OP + LAST_NFS4_OP,
>>   #define NFSD_STATS_NFS4_OP(op)	(NFSD_STATS_FIRST_NFS4_OP + (op))
>> +	NFSD_STATS_WDELEG_GETATTR,	/* count of getattr conflict with wdeleg */
>>   #endif
>>   	NFSD_STATS_COUNTERS_NUM
>>   };
>> @@ -93,4 +94,10 @@ static inline void nfsd_stats_drc_mem_usage_sub(struct nfsd_net *nn, s64 amount)
>>   	percpu_counter_sub(&nn->counter[NFSD_NET_DRC_MEM_USAGE], amount);
>>   }
>>   
>> +#ifdef CONFIG_NFSD_V4
>> +static inline void nfsd_stats_wdeleg_getattr_inc(void)
>> +{
>> +	percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_WDELEG_GETATTR]);
>> +}
>> +#endif
>>   #endif /* _NFSD_STATS_H */
>> -- 
>> 2.39.3
>>

Re: [PATCH v6 0/5] NFSD: add support for NFSv4.1+ write delegation

[Jeff Layton]

On Wed, 2023-06-28 at 19:36 -0700, Dai Ngo wrote:
> The NFSv4 server currently supports read delegation using VFS lease
> which is implemented using file_lock. 
> 
> This patch series add write delegation support for NFSv4.1+ client by:
> 
>     . remove the check for F_WRLCK in generic_add_lease to allow
>       file_lock to be used for write delegation.  
> 
>     . grant write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
>       if there is no conflict with other OPENs.
> 
> Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
> are handled the same as read delegation using notify_change, try_break_deleg.
> 
> The write delegation support is for NFSv4.1+ client only since the NFSv4.0
> Linux client behavior is not compliant with RFC 7530 Section 16.7.5. It
> expects the server to look ahead in the compound to find a stateid in order
> to determine whether the client that sends the GETATTR is the same client
> that holds the write delegation. RFC 7530 spec does not call for the server
> to look ahead in order to service the GETATTR op.
> 
> Changes since v1:
> 
> [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
> - remove WARN_ON_ONCE from encode_bitmap4
> - replace decode_bitmap4 with xdr_stream_decode_uint32_array
> - replace xdr_inline_decode and xdr_decode_hyper in decode_cb_getattr
>    with xdr_stream_decode_u64. Also remove the un-needed likely().
> - modify signature of encode_cb_getattr4args to take pointer to
>    nfs4_cb_fattr
> - replace decode_attr_length with xdr_stream_decode_u32
> - rename decode_cb_getattr to decode_cb_fattr4
> - fold the initialization of cb_cinfo and cb_fsize into decode_cb_fattr4
> - rename ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
>   in fs/nfsd/nfs4xdr.c
> - correct NFS4_dec_cb_getattr_sz and update size description
> 
> [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
> - change nfs4_handle_wrdeleg_conflict returns __be32 to fix test robot
> - change ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
>   in fs/nfsd/nfs4xdr.c
> 
> Changes since v2:
> 
> [PATCH 2/4] NFSD: enable support for write delegation
> - rename 'deleg' to 'dl_type' in nfs4_set_delegation
> - remove 'wdeleg' in nfs4_open_delegation
> 
> - drop [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
>   and [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
>   for futher clarification of the benefits of these patches
> 
> Changes since v3:
> 
> - recall write delegation when there is GETATTR from 2nd client
> - add trace point to track when write delegation is granted 
> 
> Changes since v4:
> - squash 4/4 into 2/4
> - apply 1/4 last instead of first
> - combine nfs4_wrdeleg_filelock and nfs4_handle_wrdeleg_conflict to
>   nfsd4_deleg_getattr_conflict and move it to fs/nfsd/nfs4state.c
> - check for lock belongs to delegation before proceed and do it
>   under the fl_lock
> - check and skip FL_LAYOUT file_locks
> 
> Changes since v5:
> - [patch 2/5] disable write delegation for NFSv4.0 client
> 
> - [patch 4/5] allow client to use write delegation stateid for READ (same
>   behavior as Solaris server)
> 
>   When the server receives a READ request with write delegation stateid
>   the server may returns the NFS4ERR_OPENMODE or allows the READ to proceed
>   to accommodate clients whose WRITE implementation may unavoidably do reads
>   (e.g., due to buffer cache constraints).  Per RFC 8881 section 9.1.2. Use
>   of the Stateid and Locking
> 
>   Returning NFS4ERR_OPENMODE causes the client and server to enter an infinite
>   loop of READ, NFS4ERR_OPENMODE, TEST_STATEID, READs, NFS4ERR_OPENMODEs,
>   TEST_STATEID, READs, NFS4ERR_OPENMODEs. The Linux NFS client can not recover
>   from NFS4ERR_OPENMODE for READ request if the file was opened with
>   OPEN4_SHARE_ACCESS_WRITE. This READ was initiated internally from the NFS
>   client and not from the read(2) system call.
> 
> - pass git regression test with 40 threads
> 

Nice work, Dai! This all looks good to me. You can add:

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