[PATCH V4 0/2] NFSD: offer write delegation for OPEN with OPEN4_SHARE_ACCESS only

[PATCH V4 0/2] NFSD: offer write delegation for OPEN with OPEN4_SHARE_ACCESS only

[Dai Ngo]

From RFC8881 does not explicitly state that server must grant write
delegation to OPEN with OPEN4_SHARE_ACCESS_WRITE only. However there
are text in the RFC that implies it is up to the server implementation
to offer write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE only.

Section 9.1.2:

  "In the case of READ, the server may perform the corresponding
   check on the access mode, or it may choose to allow READ for
   OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE
   implementation may unavoidably do (e.g., due to buffer cache
   constraints)."

Also in section 10.4.1

  "Similarly, when closing a file opened for OPEN4_SHARE_ACCESS_WRITE/
   OPEN4_SHARE_ACCESS_BOTH and if an OPEN_DELEGATE_WRITE delegation
   is in effect"

When a write delegation is granted for OPEN with OPEN4_SHARE_ACCESS_WRITE,
a new pair of nfsd_file and struct file are allocated with read access
and added to nfs4_file's fi_fds[]. This is to allow the client to use
the delegation stateid to do reads.

No additional actions is needed when the delegation is returned. The
nfsd_file for read remains attached to the nfs4_file and is freed when
the open stateid is closed.

The use of separate nfsd_file for read was suggested by Chuck.

I also tried the suggestion by Jeff which is to "acquire a R/W open from
the get-go instead when you intend to grant a delegation". To implement
this approach, in nfs4_get_vfs_file OPEN4_SHARE_ACCESS_READ was added to
op_share_access before computing the 'oflag' and 'access' flag. This
forces the rest of the code to process the OPEN as if it was sent with
access mode OPEN4_SHARE_ACCESS_WRITE|OPEN4_SHARE_ACCESS_READ.

This mostly works except a case where the file was created with 0222
permission and the user credential of OPEN is not the same as the owner
of the file. In this case the OPEN fails with NFS4ERR_ACCESS. This is
because nfsd_permission was called with (MAY_READ | MAY_WRITE) mask
and the file permission is 0222.

I don't see any simple fix for this nfsd_permission issue. Basically
the access mode has to be rdwr when creating the nfsd_file but the access
mode passed to nfsd_permission to check should be rdonly (the original
OPEN's op_share_access).

v4: reacted to Jeff's comment.

 fs/nfsd/nfs4state.c | 78 ++++++++++++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 29 deletions(-)

[PATCH V4 1/2] NFSD: Offer write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE

[Dai Ngo]

RFC8881, section 9.1.2 says:

  "In the case of READ, the server may perform the corresponding
   check on the access mode, or it may choose to allow READ for
   OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE
   implementation may unavoidably do (e.g., due to buffer cache
   constraints)."

and in section 10.4.1:
   "Similarly, when closing a file opened for OPEN4_SHARE_ACCESS_WRITE/
   OPEN4_SHARE_ACCESS_BOTH and if an OPEN_DELEGATE_WRITE delegation
   is in effect"

This patch offers write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE
only. Also deleted no longer use find_rw_file().

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0f97f2c62b3a..b533225e57cf 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -633,18 +633,6 @@ find_readable_file(struct nfs4_file *f)
 	return ret;
 }
 
-static struct nfsd_file *
-find_rw_file(struct nfs4_file *f)
-{
-	struct nfsd_file *ret;
-
-	spin_lock(&f->fi_lock);
-	ret = nfsd_file_get(f->fi_fds[O_RDWR]);
-	spin_unlock(&f->fi_lock);
-
-	return ret;
-}
-
 struct nfsd_file *
 find_any_file(struct nfs4_file *f)
 {
@@ -5382,7 +5370,6 @@ static int nfsd4_cb_recall_done(struct nfsd4_callback *cb,
 	if (dp->dl_stid.sc_status)
 		/* CLOSED or REVOKED */
 		return 1;
-
 	switch (task->tk_status) {
 	case 0:
 		return 1;
@@ -5987,14 +5974,19 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	 *  "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
 	 *   on its own, all opens."
 	 *
-	 * Furthermore the client can use a write delegation for most READ
-	 * operations as well, so we require a O_RDWR file here.
+	 * Furthermore, section 9.1.2 says:
+	 *
+	 *  "In the case of READ, the server may perform the corresponding
+	 *  check on the access mode, or it may choose to allow READ for
+	 *  OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE
+	 *  implementation may unavoidably do (e.g., due to buffer cache
+	 *  constraints)."
 	 *
-	 * Offer a write delegation in the case of a BOTH open, and ensure
-	 * we get the O_RDWR descriptor.
+	 *  We choose to offer a write delegation for OPEN with the
+	 *  OPEN4_SHARE_ACCESS_WRITE access mode to accommodate such clients.
 	 */
-	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) {
-		nf = find_rw_file(fp);
+	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
+		nf = find_writeable_file(fp);
 		dl_type = deleg_ts ? OPEN_DELEGATE_WRITE_ATTRS_DELEG : OPEN_DELEGATE_WRITE;
 	}
 
@@ -6116,7 +6108,7 @@ static bool
 nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
 		     struct kstat *stat)
 {
-	struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file);
+	struct nfsd_file *nf = find_writeable_file(dp->dl_stid.sc_file);
 	struct path path;
 	int rc;
 
@@ -7063,7 +7055,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		return_revoked = true;
 	if (typemask & SC_TYPE_DELEG)
 		/* Always allow REVOKED for DELEG so we can
-		 * retturn the appropriate error.
+		 * return the appropriate error.
 		 */
 		statusmask |= SC_STATUS_REVOKED;
 
-- 
2.43.5

[PATCH V4 2/2] NFSD: allow client to use write delegation stateid for READ

[Dai Ngo]

Allow READ using write delegation stateid granted on OPENs with
OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
implementation may unavoidably do (e.g., due to buffer cache
constraints).

For write delegation granted for OPEN with OPEN4_SHARE_ACCESS_WRITE
a new nfsd_file and a struct file are allocated to use for reads.
The nfsd_file is freed when the file is closed by release_all_access.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b533225e57cf..35018af4e7fb 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6126,6 +6126,34 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
 	return rc == 0;
 }
 
+/*
+ * Add NFS4_SHARE_ACCESS_READ to the write delegation granted on OPEN
+ * with NFS4_SHARE_ACCESS_WRITE by allocating separate nfsd_file and
+ * struct file to be used for read with delegation stateid.
+ *
+ */
+static bool
+nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct nfsd4_open *open,
+			      struct svc_fh *fh, struct nfs4_ol_stateid *stp)
+{
+	struct nfs4_file *fp;
+	struct nfsd_file *nf = NULL;
+
+	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
+			NFS4_SHARE_ACCESS_WRITE) {
+		if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, NULL, &nf))
+			return (false);
+		fp = stp->st_stid.sc_file;
+		spin_lock(&fp->fi_lock);
+		__nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);
+		set_access(NFS4_SHARE_ACCESS_READ, stp);
+		fp = stp->st_stid.sc_file;
+		fp->fi_fds[O_RDONLY] = nf;
+		spin_unlock(&fp->fi_lock);
+	}
+	return (true);
+}
+
 /*
  * The Linux NFS server does not offer write delegations to NFSv4.0
  * clients in order to avoid conflicts between write delegations and
@@ -6151,8 +6179,9 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
  * open or lock state.
  */
 static void
-nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
-		     struct svc_fh *currentfh)
+nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
+		     struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
+		     struct svc_fh *fh)
 {
 	bool deleg_ts = open->op_deleg_want & OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS;
 	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
@@ -6197,7 +6226,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
 
 	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
-		if (!nfs4_delegation_stat(dp, currentfh, &stat)) {
+		if ((!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, stp)) ||
+				!nfs4_delegation_stat(dp, currentfh, &stat)) {
 			nfs4_put_stid(&dp->dl_stid);
 			destroy_delegation(dp);
 			goto out_no_deleg;
@@ -6353,7 +6383,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	* Attempt to hand out a delegation. No error return, because the
 	* OPEN succeeds even if we fail.
 	*/
-	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
+	nfs4_open_delegation(rqstp, open, stp,
+		&resp->cstate.current_fh, current_fh);
 
 	/*
 	 * If there is an existing open stateid, it must be updated and
@@ -7098,10 +7129,6 @@ nfs4_find_file(struct nfs4_stid *s, int flags)
 
 	switch (s->sc_type) {
 	case SC_TYPE_DELEG:
-		spin_lock(&s->sc_file->fi_lock);
-		ret = nfsd_file_get(s->sc_file->fi_deleg_file);
-		spin_unlock(&s->sc_file->fi_lock);
-		break;
 	case SC_TYPE_OPEN:
 	case SC_TYPE_LOCK:
 		if (flags & RD_STATE)
@@ -7277,6 +7304,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 		status = find_cpntf_state(nn, stateid, &s);
 	if (status)
 		return status;
+
 	status = nfsd4_stid_check_stateid_generation(stateid, s,
 			nfsd4_has_session(cstate));
 	if (status)
-- 
2.43.5

Re: [PATCH V4 0/2] NFSD: offer write delegation for OPEN with OPEN4_SHARE_ACCESS only

[cel]

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

On Tue, 04 Mar 2025 12:38:11 -0800, Dai Ngo wrote:
> >From RFC8881 does not explicitly state that server must grant write
> delegation to OPEN with OPEN4_SHARE_ACCESS_WRITE only. However there
> are text in the RFC that implies it is up to the server implementation
> to offer write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE only.
> 
> Section 9.1.2:
> 
> [...]

Applied to nfsd-testing, thanks!

Hoping to get some testing experience on this series while review is
ongoing.

[1/2] NFSD: Offer write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE
      commit: a6a3093e768e143d4a6088db2b4ec91097302fe5
[2/2] NFSD: allow client to use write delegation stateid for READ
      commit: 14d6ad10126cf89b64a889467844b4c1ef588f7d

--
Chuck Lever

Re: [PATCH V4 2/2] NFSD: allow client to use write delegation stateid for READ

[Jeff Layton]

On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
> Allow READ using write delegation stateid granted on OPENs with
> OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
> implementation may unavoidably do (e.g., due to buffer cache
> constraints).
> 
> For write delegation granted for OPEN with OPEN4_SHARE_ACCESS_WRITE
> a new nfsd_file and a struct file are allocated to use for reads.
> The nfsd_file is freed when the file is closed by release_all_access.
> 
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 44 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b533225e57cf..35018af4e7fb 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6126,6 +6126,34 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>  	return rc == 0;
>  }
>  
> +/*
> + * Add NFS4_SHARE_ACCESS_READ to the write delegation granted on OPEN
> + * with NFS4_SHARE_ACCESS_WRITE by allocating separate nfsd_file and
> + * struct file to be used for read with delegation stateid.
> + *
> + */
> +static bool
> +nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct nfsd4_open *open,
> +			      struct svc_fh *fh, struct nfs4_ol_stateid *stp)
> +{
> +	struct nfs4_file *fp;
> +	struct nfsd_file *nf = NULL;
> +
> +	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
> +			NFS4_SHARE_ACCESS_WRITE) {
> +		if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, NULL, &nf))
> +			return (false);
> +		fp = stp->st_stid.sc_file;
> +		spin_lock(&fp->fi_lock);
> +		__nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);
> +		set_access(NFS4_SHARE_ACCESS_READ, stp);
> +		fp = stp->st_stid.sc_file;
> +		fp->fi_fds[O_RDONLY] = nf;
> +		spin_unlock(&fp->fi_lock);
> +	}
> +	return (true);

no need for parenthesis here ^^^

> +}
> +
>  /*
>   * The Linux NFS server does not offer write delegations to NFSv4.0
>   * clients in order to avoid conflicts between write delegations and
> @@ -6151,8 +6179,9 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>   * open or lock state.
>   */
>  static void
> -nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> -		     struct svc_fh *currentfh)
> +nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
> +		     struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
> +		     struct svc_fh *fh)
>  {
>  	bool deleg_ts = open->op_deleg_want & OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS;
>  	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
> @@ -6197,7 +6226,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
>  
>  	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> -		if (!nfs4_delegation_stat(dp, currentfh, &stat)) {
> +		if ((!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, stp)) ||

extra set of parens above too ^^^

> +				!nfs4_delegation_stat(dp, currentfh, &stat)) {
>  			nfs4_put_stid(&dp->dl_stid);
>  			destroy_delegation(dp);
>  			goto out_no_deleg;
> @@ -6353,7 +6383,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>  	* Attempt to hand out a delegation. No error return, because the
>  	* OPEN succeeds even if we fail.
>  	*/
> -	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
> +	nfs4_open_delegation(rqstp, open, stp,
> +		&resp->cstate.current_fh, current_fh);
>  
>  	/*
>  	 * If there is an existing open stateid, it must be updated and
> @@ -7098,10 +7129,6 @@ nfs4_find_file(struct nfs4_stid *s, int flags)
>  
>  	switch (s->sc_type) {
>  	case SC_TYPE_DELEG:
> -		spin_lock(&s->sc_file->fi_lock);
> -		ret = nfsd_file_get(s->sc_file->fi_deleg_file);
> -		spin_unlock(&s->sc_file->fi_lock);
> -		break;
>  	case SC_TYPE_OPEN:
>  	case SC_TYPE_LOCK:
>  		if (flags & RD_STATE)
> @@ -7277,6 +7304,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  		status = find_cpntf_state(nn, stateid, &s);
>  	if (status)
>  		return status;
> +
>  	status = nfsd4_stid_check_stateid_generation(stateid, s,
>  			nfsd4_has_session(cstate));
>  	if (status)

Patch itself looks good though, so with the nits fixed up, you can add:

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

Re: [PATCH V4 1/2] NFSD: Offer write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE

[Jeff Layton]

On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
> RFC8881, section 9.1.2 says:
> 
>   "In the case of READ, the server may perform the corresponding
>    check on the access mode, or it may choose to allow READ for
>    OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE
>    implementation may unavoidably do (e.g., due to buffer cache
>    constraints)."
> 
> and in section 10.4.1:
>    "Similarly, when closing a file opened for OPEN4_SHARE_ACCESS_WRITE/
>    OPEN4_SHARE_ACCESS_BOTH and if an OPEN_DELEGATE_WRITE delegation
>    is in effect"
> 
> This patch offers write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE
> only. Also deleted no longer use find_rw_file().
> 
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0f97f2c62b3a..b533225e57cf 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -633,18 +633,6 @@ find_readable_file(struct nfs4_file *f)
>  	return ret;
>  }
>  
> -static struct nfsd_file *
> -find_rw_file(struct nfs4_file *f)
> -{
> -	struct nfsd_file *ret;
> -
> -	spin_lock(&f->fi_lock);
> -	ret = nfsd_file_get(f->fi_fds[O_RDWR]);
> -	spin_unlock(&f->fi_lock);
> -
> -	return ret;
> -}
> -
>  struct nfsd_file *
>  find_any_file(struct nfs4_file *f)
>  {
> @@ -5382,7 +5370,6 @@ static int nfsd4_cb_recall_done(struct nfsd4_callback *cb,
>  	if (dp->dl_stid.sc_status)
>  		/* CLOSED or REVOKED */
>  		return 1;
> -
>  	switch (task->tk_status) {
>  	case 0:
>  		return 1;
> @@ -5987,14 +5974,19 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	 *  "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
>  	 *   on its own, all opens."
>  	 *
> -	 * Furthermore the client can use a write delegation for most READ
> -	 * operations as well, so we require a O_RDWR file here.
> +	 * Furthermore, section 9.1.2 says:
> +	 *
> +	 *  "In the case of READ, the server may perform the corresponding
> +	 *  check on the access mode, or it may choose to allow READ for
> +	 *  OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE
> +	 *  implementation may unavoidably do (e.g., due to buffer cache
> +	 *  constraints)."
>  	 *
> -	 * Offer a write delegation in the case of a BOTH open, and ensure
> -	 * we get the O_RDWR descriptor.
> +	 *  We choose to offer a write delegation for OPEN with the
> +	 *  OPEN4_SHARE_ACCESS_WRITE access mode to accommodate such clients.
>  	 */
> -	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) {
> -		nf = find_rw_file(fp);
> +	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> +		nf = find_writeable_file(fp);
>  		dl_type = deleg_ts ? OPEN_DELEGATE_WRITE_ATTRS_DELEG : OPEN_DELEGATE_WRITE;
>  	}
>  
> @@ -6116,7 +6108,7 @@ static bool
>  nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>  		     struct kstat *stat)
>  {
> -	struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file);
> +	struct nfsd_file *nf = find_writeable_file(dp->dl_stid.sc_file);
>  	struct path path;
>  	int rc;
>  
> @@ -7063,7 +7055,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		return_revoked = true;
>  	if (typemask & SC_TYPE_DELEG)
>  		/* Always allow REVOKED for DELEG so we can
> -		 * retturn the appropriate error.
> +		 * return the appropriate error.
>  		 */
>  		statusmask |= SC_STATUS_REVOKED;
>  

This patch also looks good.

The only other issue I have with this is the patch ordering. If a
bisect lands between these two patches then delegations won't work
quite right. Is there a reason to order the patches this way?

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

Re: [PATCH V4 1/2] NFSD: Offer write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE

[Chuck Lever]

On 3/5/25 9:36 AM, Jeff Layton wrote:
> On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
>> RFC8881, section 9.1.2 says:
>>
>>   "In the case of READ, the server may perform the corresponding
>>    check on the access mode, or it may choose to allow READ for
>>    OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE
>>    implementation may unavoidably do (e.g., due to buffer cache
>>    constraints)."
>>
>> and in section 10.4.1:
>>    "Similarly, when closing a file opened for OPEN4_SHARE_ACCESS_WRITE/
>>    OPEN4_SHARE_ACCESS_BOTH and if an OPEN_DELEGATE_WRITE delegation
>>    is in effect"
>>
>> This patch offers write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE
>> only. Also deleted no longer use find_rw_file().
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>  fs/nfsd/nfs4state.c | 34 +++++++++++++---------------------
>>  1 file changed, 13 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 0f97f2c62b3a..b533225e57cf 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -633,18 +633,6 @@ find_readable_file(struct nfs4_file *f)
>>  	return ret;
>>  }
>>  
>> -static struct nfsd_file *
>> -find_rw_file(struct nfs4_file *f)
>> -{
>> -	struct nfsd_file *ret;
>> -
>> -	spin_lock(&f->fi_lock);
>> -	ret = nfsd_file_get(f->fi_fds[O_RDWR]);
>> -	spin_unlock(&f->fi_lock);
>> -
>> -	return ret;
>> -}
>> -
>>  struct nfsd_file *
>>  find_any_file(struct nfs4_file *f)
>>  {
>> @@ -5382,7 +5370,6 @@ static int nfsd4_cb_recall_done(struct nfsd4_callback *cb,
>>  	if (dp->dl_stid.sc_status)
>>  		/* CLOSED or REVOKED */
>>  		return 1;
>> -
>>  	switch (task->tk_status) {
>>  	case 0:
>>  		return 1;
>> @@ -5987,14 +5974,19 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>  	 *  "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
>>  	 *   on its own, all opens."
>>  	 *
>> -	 * Furthermore the client can use a write delegation for most READ
>> -	 * operations as well, so we require a O_RDWR file here.
>> +	 * Furthermore, section 9.1.2 says:
>> +	 *
>> +	 *  "In the case of READ, the server may perform the corresponding
>> +	 *  check on the access mode, or it may choose to allow READ for
>> +	 *  OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE
>> +	 *  implementation may unavoidably do (e.g., due to buffer cache
>> +	 *  constraints)."
>>  	 *
>> -	 * Offer a write delegation in the case of a BOTH open, and ensure
>> -	 * we get the O_RDWR descriptor.
>> +	 *  We choose to offer a write delegation for OPEN with the
>> +	 *  OPEN4_SHARE_ACCESS_WRITE access mode to accommodate such clients.
>>  	 */
>> -	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) {
>> -		nf = find_rw_file(fp);
>> +	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>> +		nf = find_writeable_file(fp);
>>  		dl_type = deleg_ts ? OPEN_DELEGATE_WRITE_ATTRS_DELEG : OPEN_DELEGATE_WRITE;
>>  	}
>>  
>> @@ -6116,7 +6108,7 @@ static bool
>>  nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>>  		     struct kstat *stat)
>>  {
>> -	struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file);
>> +	struct nfsd_file *nf = find_writeable_file(dp->dl_stid.sc_file);
>>  	struct path path;
>>  	int rc;
>>  
>> @@ -7063,7 +7055,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>>  		return_revoked = true;
>>  	if (typemask & SC_TYPE_DELEG)
>>  		/* Always allow REVOKED for DELEG so we can
>> -		 * retturn the appropriate error.
>> +		 * return the appropriate error.
>>  		 */
>>  		statusmask |= SC_STATUS_REVOKED;
>>  
> 
> This patch also looks good.
> 
> The only other issue I have with this is the patch ordering. If a
> bisect lands between these two patches then delegations won't work
> quite right. Is there a reason to order the patches this way?

I also wondered about the patch order for the same reason.


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


-- 
Chuck Lever

Re: [PATCH V4 2/2] NFSD: allow client to use write delegation stateid for READ

[Chuck Lever]

On 3/5/25 9:34 AM, Jeff Layton wrote:
> On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
>> Allow READ using write delegation stateid granted on OPENs with
>> OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
>> implementation may unavoidably do (e.g., due to buffer cache
>> constraints).
>>
>> For write delegation granted for OPEN with OPEN4_SHARE_ACCESS_WRITE
>> a new nfsd_file and a struct file are allocated to use for reads.
>> The nfsd_file is freed when the file is closed by release_all_access.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>  fs/nfsd/nfs4state.c | 44 ++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index b533225e57cf..35018af4e7fb 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -6126,6 +6126,34 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>>  	return rc == 0;
>>  }
>>  
>> +/*
>> + * Add NFS4_SHARE_ACCESS_READ to the write delegation granted on OPEN
>> + * with NFS4_SHARE_ACCESS_WRITE by allocating separate nfsd_file and
>> + * struct file to be used for read with delegation stateid.
>> + *
>> + */
>> +static bool
>> +nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct nfsd4_open *open,
>> +			      struct svc_fh *fh, struct nfs4_ol_stateid *stp)
>> +{
>> +	struct nfs4_file *fp;
>> +	struct nfsd_file *nf = NULL;
>> +
>> +	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
>> +			NFS4_SHARE_ACCESS_WRITE) {
>> +		if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, NULL, &nf))
>> +			return (false);
>> +		fp = stp->st_stid.sc_file;
>> +		spin_lock(&fp->fi_lock);
>> +		__nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);
>> +		set_access(NFS4_SHARE_ACCESS_READ, stp);
>> +		fp = stp->st_stid.sc_file;
>> +		fp->fi_fds[O_RDONLY] = nf;
>> +		spin_unlock(&fp->fi_lock);
>> +	}
>> +	return (true);
> 
> no need for parenthesis here ^^^
> 
>> +}
>> +
>>  /*
>>   * The Linux NFS server does not offer write delegations to NFSv4.0
>>   * clients in order to avoid conflicts between write delegations and
>> @@ -6151,8 +6179,9 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>>   * open or lock state.
>>   */
>>  static void
>> -nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> -		     struct svc_fh *currentfh)
>> +nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
>> +		     struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
>> +		     struct svc_fh *fh)
>>  {
>>  	bool deleg_ts = open->op_deleg_want & OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS;
>>  	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
>> @@ -6197,7 +6226,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>  	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
>>  
>>  	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>> -		if (!nfs4_delegation_stat(dp, currentfh, &stat)) {
>> +		if ((!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, stp)) ||
> 
> extra set of parens above too ^^^
> 
>> +				!nfs4_delegation_stat(dp, currentfh, &stat)) {
>>  			nfs4_put_stid(&dp->dl_stid);
>>  			destroy_delegation(dp);
>>  			goto out_no_deleg;
>> @@ -6353,7 +6383,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>>  	* Attempt to hand out a delegation. No error return, because the
>>  	* OPEN succeeds even if we fail.
>>  	*/
>> -	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
>> +	nfs4_open_delegation(rqstp, open, stp,
>> +		&resp->cstate.current_fh, current_fh);
>>  
>>  	/*
>>  	 * If there is an existing open stateid, it must be updated and
>> @@ -7098,10 +7129,6 @@ nfs4_find_file(struct nfs4_stid *s, int flags)
>>  
>>  	switch (s->sc_type) {
>>  	case SC_TYPE_DELEG:
>> -		spin_lock(&s->sc_file->fi_lock);
>> -		ret = nfsd_file_get(s->sc_file->fi_deleg_file);
>> -		spin_unlock(&s->sc_file->fi_lock);
>> -		break;
>>  	case SC_TYPE_OPEN:
>>  	case SC_TYPE_LOCK:
>>  		if (flags & RD_STATE)
>> @@ -7277,6 +7304,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>>  		status = find_cpntf_state(nn, stateid, &s);
>>  	if (status)
>>  		return status;
>> +
>>  	status = nfsd4_stid_check_stateid_generation(stateid, s,
>>  			nfsd4_has_session(cstate));
>>  	if (status)
> 
> Patch itself looks good though, so with the nits fixed up, you can add:
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Dai, I can fix the parentheses in my tree, no need for a v5.


-- 
Chuck Lever

Re: [PATCH V4 2/2] NFSD: allow client to use write delegation stateid for READ

[Jeff Layton]

On Wed, 2025-03-05 at 09:46 -0500, Chuck Lever wrote:
> On 3/5/25 9:34 AM, Jeff Layton wrote:
> > On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
> > > Allow READ using write delegation stateid granted on OPENs with
> > > OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
> > > implementation may unavoidably do (e.g., due to buffer cache
> > > constraints).
> > > 
> > > For write delegation granted for OPEN with OPEN4_SHARE_ACCESS_WRITE
> > > a new nfsd_file and a struct file are allocated to use for reads.
> > > The nfsd_file is freed when the file is closed by release_all_access.
> > > 
> > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > > ---
> > >  fs/nfsd/nfs4state.c | 44 ++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 36 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index b533225e57cf..35018af4e7fb 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -6126,6 +6126,34 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
> > >  	return rc == 0;
> > >  }
> > >  
> > > +/*
> > > + * Add NFS4_SHARE_ACCESS_READ to the write delegation granted on OPEN
> > > + * with NFS4_SHARE_ACCESS_WRITE by allocating separate nfsd_file and
> > > + * struct file to be used for read with delegation stateid.
> > > + *
> > > + */
> > > +static bool
> > > +nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct nfsd4_open *open,
> > > +			      struct svc_fh *fh, struct nfs4_ol_stateid *stp)
> > > +{
> > > +	struct nfs4_file *fp;
> > > +	struct nfsd_file *nf = NULL;
> > > +
> > > +	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
> > > +			NFS4_SHARE_ACCESS_WRITE) {
> > > +		if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, NULL, &nf))
> > > +			return (false);
> > > +		fp = stp->st_stid.sc_file;
> > > +		spin_lock(&fp->fi_lock);
> > > +		__nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);
> > > +		set_access(NFS4_SHARE_ACCESS_READ, stp);

The only other (minor) issue is that this might be problematic vs.
DENY_READ modes:

Suppose someone opens the file SHARE_ACCESS_WRITE and gets back a r/w
delegation. Then someone else tries to open the file
SHARE_ACCESS_READ|SHARE_DENY_READ. That should succeed, AFAICT, but I
think with this patch that would fail because we check the deny mode
before doing the open (and revoking the delegation).

It'd be good to test and see if that's the case.


> > > +		fp = stp->st_stid.sc_file;
> > > +		fp->fi_fds[O_RDONLY] = nf;
> > > +		spin_unlock(&fp->fi_lock);
> > > +	}
> > > +	return (true);
> > 
> > no need for parenthesis here ^^^
> > 
> > > +}
> > > +
> > >  /*
> > >   * The Linux NFS server does not offer write delegations to NFSv4.0
> > >   * clients in order to avoid conflicts between write delegations and
> > > @@ -6151,8 +6179,9 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
> > >   * open or lock state.
> > >   */
> > >  static void
> > > -nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > > -		     struct svc_fh *currentfh)
> > > +nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
> > > +		     struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
> > > +		     struct svc_fh *fh)
> > >  {
> > >  	bool deleg_ts = open->op_deleg_want & OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS;
> > >  	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
> > > @@ -6197,7 +6226,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > >  	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
> > >  
> > >  	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> > > -		if (!nfs4_delegation_stat(dp, currentfh, &stat)) {
> > > +		if ((!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, stp)) ||
> > 
> > extra set of parens above too ^^^
> > 
> > > +				!nfs4_delegation_stat(dp, currentfh, &stat)) {
> > >  			nfs4_put_stid(&dp->dl_stid);
> > >  			destroy_delegation(dp);
> > >  			goto out_no_deleg;
> > > @@ -6353,7 +6383,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> > >  	* Attempt to hand out a delegation. No error return, because the
> > >  	* OPEN succeeds even if we fail.
> > >  	*/
> > > -	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
> > > +	nfs4_open_delegation(rqstp, open, stp,
> > > +		&resp->cstate.current_fh, current_fh);
> > >  
> > >  	/*
> > >  	 * If there is an existing open stateid, it must be updated and
> > > @@ -7098,10 +7129,6 @@ nfs4_find_file(struct nfs4_stid *s, int flags)
> > >  
> > >  	switch (s->sc_type) {
> > >  	case SC_TYPE_DELEG:
> > > -		spin_lock(&s->sc_file->fi_lock);
> > > -		ret = nfsd_file_get(s->sc_file->fi_deleg_file);
> > > -		spin_unlock(&s->sc_file->fi_lock);
> > > -		break;
> > >  	case SC_TYPE_OPEN:
> > >  	case SC_TYPE_LOCK:
> > >  		if (flags & RD_STATE)
> > > @@ -7277,6 +7304,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
> > >  		status = find_cpntf_state(nn, stateid, &s);
> > >  	if (status)
> > >  		return status;
> > > +
> > >  	status = nfsd4_stid_check_stateid_generation(stateid, s,
> > >  			nfsd4_has_session(cstate));
> > >  	if (status)
> > 
> > Patch itself looks good though, so with the nits fixed up, you can add:
> > 
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 
> Dai, I can fix the parentheses in my tree, no need for a v5.
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH V4 2/2] NFSD: allow client to use write delegation stateid for READ

[Dai Ngo]

On 3/5/25 8:08 AM, Jeff Layton wrote:
> On Wed, 2025-03-05 at 09:46 -0500, Chuck Lever wrote:
>> On 3/5/25 9:34 AM, Jeff Layton wrote:
>>> On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
>>>> Allow READ using write delegation stateid granted on OPENs with
>>>> OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
>>>> implementation may unavoidably do (e.g., due to buffer cache
>>>> constraints).
>>>>
>>>> For write delegation granted for OPEN with OPEN4_SHARE_ACCESS_WRITE
>>>> a new nfsd_file and a struct file are allocated to use for reads.
>>>> The nfsd_file is freed when the file is closed by release_all_access.
>>>>
>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>> ---
>>>>   fs/nfsd/nfs4state.c | 44 ++++++++++++++++++++++++++++++++++++--------
>>>>   1 file changed, 36 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index b533225e57cf..35018af4e7fb 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -6126,6 +6126,34 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>>>>   	return rc == 0;
>>>>   }
>>>>   
>>>> +/*
>>>> + * Add NFS4_SHARE_ACCESS_READ to the write delegation granted on OPEN
>>>> + * with NFS4_SHARE_ACCESS_WRITE by allocating separate nfsd_file and
>>>> + * struct file to be used for read with delegation stateid.
>>>> + *
>>>> + */
>>>> +static bool
>>>> +nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct nfsd4_open *open,
>>>> +			      struct svc_fh *fh, struct nfs4_ol_stateid *stp)
>>>> +{
>>>> +	struct nfs4_file *fp;
>>>> +	struct nfsd_file *nf = NULL;
>>>> +
>>>> +	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
>>>> +			NFS4_SHARE_ACCESS_WRITE) {
>>>> +		if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, NULL, &nf))
>>>> +			return (false);
>>>> +		fp = stp->st_stid.sc_file;
>>>> +		spin_lock(&fp->fi_lock);
>>>> +		__nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);
>>>> +		set_access(NFS4_SHARE_ACCESS_READ, stp);
> The only other (minor) issue is that this might be problematic vs.
> DENY_READ modes:
>
> Suppose someone opens the file SHARE_ACCESS_WRITE and gets back a r/w
> delegation. Then someone else tries to open the file
> SHARE_ACCESS_READ|SHARE_DENY_READ. That should succeed, AFAICT, but I
> think with this patch that would fail because we check the deny mode
> before doing the open (and revoking the delegation).
>
> It'd be good to test and see if that's the case.

Yes, you're correct. The 2nd OPEN fails due to the read access set
to the file in nfsd4_add_rdaccess_to_wrdeleg().

I think the deny mode is used only by SMB and not Linux client, not
sure though. What should we do about this, any thought?

>
>
>>>> +		fp = stp->st_stid.sc_file;
>>>> +		fp->fi_fds[O_RDONLY] = nf;
>>>> +		spin_unlock(&fp->fi_lock);
>>>> +	}
>>>> +	return (true);
>>> no need for parenthesis here ^^^

Fixed.

>>>
>>>> +}
>>>> +
>>>>   /*
>>>>    * The Linux NFS server does not offer write delegations to NFSv4.0
>>>>    * clients in order to avoid conflicts between write delegations and
>>>> @@ -6151,8 +6179,9 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>>>>    * open or lock state.
>>>>    */
>>>>   static void
>>>> -nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>>> -		     struct svc_fh *currentfh)
>>>> +nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
>>>> +		     struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
>>>> +		     struct svc_fh *fh)
>>>>   {
>>>>   	bool deleg_ts = open->op_deleg_want & OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS;
>>>>   	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
>>>> @@ -6197,7 +6226,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>>>   	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
>>>>   
>>>>   	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>>>> -		if (!nfs4_delegation_stat(dp, currentfh, &stat)) {
>>>> +		if ((!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, stp)) ||
>>> extra set of parens above too ^^^

Fixed.

>>>
>>>> +				!nfs4_delegation_stat(dp, currentfh, &stat)) {
>>>>   			nfs4_put_stid(&dp->dl_stid);
>>>>   			destroy_delegation(dp);
>>>>   			goto out_no_deleg;
>>>> @@ -6353,7 +6383,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>>>>   	* Attempt to hand out a delegation. No error return, because the
>>>>   	* OPEN succeeds even if we fail.
>>>>   	*/
>>>> -	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
>>>> +	nfs4_open_delegation(rqstp, open, stp,
>>>> +		&resp->cstate.current_fh, current_fh);
>>>>   
>>>>   	/*
>>>>   	 * If there is an existing open stateid, it must be updated and
>>>> @@ -7098,10 +7129,6 @@ nfs4_find_file(struct nfs4_stid *s, int flags)
>>>>   
>>>>   	switch (s->sc_type) {
>>>>   	case SC_TYPE_DELEG:
>>>> -		spin_lock(&s->sc_file->fi_lock);
>>>> -		ret = nfsd_file_get(s->sc_file->fi_deleg_file);
>>>> -		spin_unlock(&s->sc_file->fi_lock);
>>>> -		break;
>>>>   	case SC_TYPE_OPEN:
>>>>   	case SC_TYPE_LOCK:
>>>>   		if (flags & RD_STATE)
>>>> @@ -7277,6 +7304,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>>>>   		status = find_cpntf_state(nn, stateid, &s);
>>>>   	if (status)
>>>>   		return status;
>>>> +
>>>>   	status = nfsd4_stid_check_stateid_generation(stateid, s,
>>>>   			nfsd4_has_session(cstate));
>>>>   	if (status)
>>> Patch itself looks good though, so with the nits fixed up, you can add:
>>>
>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>> Dai, I can fix the parentheses in my tree, no need for a v5.

Thanks Chuck, I will fold these patches into one to avoid potential
bisect issue before sending out v5.

-Dai

>>
>>

Re: [PATCH V4 2/2] NFSD: allow client to use write delegation stateid for READ

[Dai Ngo]

On 3/5/25 12:47 PM, Dai Ngo wrote:
>
> On 3/5/25 8:08 AM, Jeff Layton wrote:
>> On Wed, 2025-03-05 at 09:46 -0500, Chuck Lever wrote:
>>> On 3/5/25 9:34 AM, Jeff Layton wrote:
>>>> On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
>>>>> Allow READ using write delegation stateid granted on OPENs with
>>>>> OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
>>>>> implementation may unavoidably do (e.g., due to buffer cache
>>>>> constraints).
>>>>>
>>>>> For write delegation granted for OPEN with OPEN4_SHARE_ACCESS_WRITE
>>>>> a new nfsd_file and a struct file are allocated to use for reads.
>>>>> The nfsd_file is freed when the file is closed by release_all_access.
>>>>>
>>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>>> ---
>>>>>   fs/nfsd/nfs4state.c | 44 
>>>>> ++++++++++++++++++++++++++++++++++++--------
>>>>>   1 file changed, 36 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>> index b533225e57cf..35018af4e7fb 100644
>>>>> --- a/fs/nfsd/nfs4state.c
>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>> @@ -6126,6 +6126,34 @@ nfs4_delegation_stat(struct nfs4_delegation 
>>>>> *dp, struct svc_fh *currentfh,
>>>>>       return rc == 0;
>>>>>   }
>>>>>   +/*
>>>>> + * Add NFS4_SHARE_ACCESS_READ to the write delegation granted on 
>>>>> OPEN
>>>>> + * with NFS4_SHARE_ACCESS_WRITE by allocating separate nfsd_file and
>>>>> + * struct file to be used for read with delegation stateid.
>>>>> + *
>>>>> + */
>>>>> +static bool
>>>>> +nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct 
>>>>> nfsd4_open *open,
>>>>> +                  struct svc_fh *fh, struct nfs4_ol_stateid *stp)
>>>>> +{
>>>>> +    struct nfs4_file *fp;
>>>>> +    struct nfsd_file *nf = NULL;
>>>>> +
>>>>> +    if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
>>>>> +            NFS4_SHARE_ACCESS_WRITE) {
>>>>> +        if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, 
>>>>> NULL, &nf))
>>>>> +            return (false);
>>>>> +        fp = stp->st_stid.sc_file;
>>>>> +        spin_lock(&fp->fi_lock);
>>>>> +        __nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);
>>>>> +        set_access(NFS4_SHARE_ACCESS_READ, stp);
>> The only other (minor) issue is that this might be problematic vs.
>> DENY_READ modes:
>>
>> Suppose someone opens the file SHARE_ACCESS_WRITE and gets back a r/w
>> delegation. Then someone else tries to open the file
>> SHARE_ACCESS_READ|SHARE_DENY_READ. That should succeed, AFAICT, but I
>> think with this patch that would fail because we check the deny mode
>> before doing the open (and revoking the delegation).
>>
>> It'd be good to test and see if that's the case.
>
> Yes, you're correct. The 2nd OPEN fails due to the read access set
> to the file in nfsd4_add_rdaccess_to_wrdeleg().
>
> I think the deny mode is used only by SMB and not Linux client, not
> sure though. What should we do about this, any thought?

Without this patch, nfsd does not hand out the write delegation and don't
set the read access so the 2nd OPEN would work. But is that the correct
behavior because the open stateid of the 1st OPEN is allowed to do read?

-Dai

>
>>
>>
>>>>> +        fp = stp->st_stid.sc_file;
>>>>> +        fp->fi_fds[O_RDONLY] = nf;
>>>>> +        spin_unlock(&fp->fi_lock);
>>>>> +    }
>>>>> +    return (true);
>>>> no need for parenthesis here ^^^
>
> Fixed.
>
>>>>
>>>>> +}
>>>>> +
>>>>>   /*
>>>>>    * The Linux NFS server does not offer write delegations to NFSv4.0
>>>>>    * clients in order to avoid conflicts between write delegations 
>>>>> and
>>>>> @@ -6151,8 +6179,9 @@ nfs4_delegation_stat(struct nfs4_delegation 
>>>>> *dp, struct svc_fh *currentfh,
>>>>>    * open or lock state.
>>>>>    */
>>>>>   static void
>>>>> -nfs4_open_delegation(struct nfsd4_open *open, struct 
>>>>> nfs4_ol_stateid *stp,
>>>>> -             struct svc_fh *currentfh)
>>>>> +nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open 
>>>>> *open,
>>>>> +             struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
>>>>> +             struct svc_fh *fh)
>>>>>   {
>>>>>       bool deleg_ts = open->op_deleg_want & 
>>>>> OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS;
>>>>>       struct nfs4_openowner *oo = openowner(stp->st_stateowner);
>>>>> @@ -6197,7 +6226,8 @@ nfs4_open_delegation(struct nfsd4_open 
>>>>> *open, struct nfs4_ol_stateid *stp,
>>>>>       memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, 
>>>>> sizeof(dp->dl_stid.sc_stateid));
>>>>>         if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>>>>> -        if (!nfs4_delegation_stat(dp, currentfh, &stat)) {
>>>>> +        if ((!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, 
>>>>> stp)) ||
>>>> extra set of parens above too ^^^
>
> Fixed.
>
>>>>
>>>>> + !nfs4_delegation_stat(dp, currentfh, &stat)) {
>>>>>               nfs4_put_stid(&dp->dl_stid);
>>>>>               destroy_delegation(dp);
>>>>>               goto out_no_deleg;
>>>>> @@ -6353,7 +6383,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, 
>>>>> struct svc_fh *current_fh, struct nf
>>>>>       * Attempt to hand out a delegation. No error return, because 
>>>>> the
>>>>>       * OPEN succeeds even if we fail.
>>>>>       */
>>>>> -    nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
>>>>> +    nfs4_open_delegation(rqstp, open, stp,
>>>>> +        &resp->cstate.current_fh, current_fh);
>>>>>         /*
>>>>>        * If there is an existing open stateid, it must be updated and
>>>>> @@ -7098,10 +7129,6 @@ nfs4_find_file(struct nfs4_stid *s, int flags)
>>>>>         switch (s->sc_type) {
>>>>>       case SC_TYPE_DELEG:
>>>>> -        spin_lock(&s->sc_file->fi_lock);
>>>>> -        ret = nfsd_file_get(s->sc_file->fi_deleg_file);
>>>>> -        spin_unlock(&s->sc_file->fi_lock);
>>>>> -        break;
>>>>>       case SC_TYPE_OPEN:
>>>>>       case SC_TYPE_LOCK:
>>>>>           if (flags & RD_STATE)
>>>>> @@ -7277,6 +7304,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst 
>>>>> *rqstp,
>>>>>           status = find_cpntf_state(nn, stateid, &s);
>>>>>       if (status)
>>>>>           return status;
>>>>> +
>>>>>       status = nfsd4_stid_check_stateid_generation(stateid, s,
>>>>>               nfsd4_has_session(cstate));
>>>>>       if (status)
>>>> Patch itself looks good though, so with the nits fixed up, you can 
>>>> add:
>>>>
>>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>> Dai, I can fix the parentheses in my tree, no need for a v5.
>
> Thanks Chuck, I will fold these patches into one to avoid potential
> bisect issue before sending out v5.
>
> -Dai
>
>>>
>>>
>

Re: [PATCH V4 2/2] NFSD: allow client to use write delegation stateid for READ

[Jeff Layton]

On Wed, 2025-03-05 at 12:59 -0800, Dai Ngo wrote:
> On 3/5/25 12:47 PM, Dai Ngo wrote:
> > 
> > On 3/5/25 8:08 AM, Jeff Layton wrote:
> > > On Wed, 2025-03-05 at 09:46 -0500, Chuck Lever wrote:
> > > > On 3/5/25 9:34 AM, Jeff Layton wrote:
> > > > > On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
> > > > > > Allow READ using write delegation stateid granted on OPENs with
> > > > > > OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
> > > > > > implementation may unavoidably do (e.g., due to buffer cache
> > > > > > constraints).
> > > > > > 
> > > > > > For write delegation granted for OPEN with OPEN4_SHARE_ACCESS_WRITE
> > > > > > a new nfsd_file and a struct file are allocated to use for reads.
> > > > > > The nfsd_file is freed when the file is closed by release_all_access.
> > > > > > 
> > > > > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > > > > > ---
> > > > > >   fs/nfsd/nfs4state.c | 44 
> > > > > > ++++++++++++++++++++++++++++++++++++--------
> > > > > >   1 file changed, 36 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > > index b533225e57cf..35018af4e7fb 100644
> > > > > > --- a/fs/nfsd/nfs4state.c
> > > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > > @@ -6126,6 +6126,34 @@ nfs4_delegation_stat(struct nfs4_delegation 
> > > > > > *dp, struct svc_fh *currentfh,
> > > > > >       return rc == 0;
> > > > > >   }
> > > > > >   +/*
> > > > > > + * Add NFS4_SHARE_ACCESS_READ to the write delegation granted on 
> > > > > > OPEN
> > > > > > + * with NFS4_SHARE_ACCESS_WRITE by allocating separate nfsd_file and
> > > > > > + * struct file to be used for read with delegation stateid.
> > > > > > + *
> > > > > > + */
> > > > > > +static bool
> > > > > > +nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct 
> > > > > > nfsd4_open *open,
> > > > > > +                  struct svc_fh *fh, struct nfs4_ol_stateid *stp)
> > > > > > +{
> > > > > > +    struct nfs4_file *fp;
> > > > > > +    struct nfsd_file *nf = NULL;
> > > > > > +
> > > > > > +    if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
> > > > > > +            NFS4_SHARE_ACCESS_WRITE) {
> > > > > > +        if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, 
> > > > > > NULL, &nf))
> > > > > > +            return (false);
> > > > > > +        fp = stp->st_stid.sc_file;
> > > > > > +        spin_lock(&fp->fi_lock);
> > > > > > +        __nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);
> > > > > > +        set_access(NFS4_SHARE_ACCESS_READ, stp);
> > > The only other (minor) issue is that this might be problematic vs.
> > > DENY_READ modes:
> > > 
> > > Suppose someone opens the file SHARE_ACCESS_WRITE and gets back a r/w
> > > delegation. Then someone else tries to open the file
> > > SHARE_ACCESS_READ|SHARE_DENY_READ. That should succeed, AFAICT, but I
> > > think with this patch that would fail because we check the deny mode
> > > before doing the open (and revoking the delegation).
> > > 
> > > It'd be good to test and see if that's the case.
> > 
> > Yes, you're correct. The 2nd OPEN fails due to the read access set
> > to the file in nfsd4_add_rdaccess_to_wrdeleg().
> > 
> > I think the deny mode is used only by SMB and not Linux client, not
> > sure though. What should we do about this, any thought?

Deny modes are a Windows/DOS thing, but they are part of the NFSv4 spec
too. Linux doesn't have a userland interface that allows you to set
them, and they aren't plumbed through the VFS layer, so you can still
do an open locally on the box, even if a deny mode is set. I _think_
BSD might also have support at the VFS layer for share/deny locking but
I don't know for sure.

> 
> Without this patch, nfsd does not hand out the write delegation and don't
> set the read access so the 2nd OPEN would work. But is that the correct
> behavior because the open stateid of the 1st OPEN is allowed to do read?
> 

That's a good question.

The main reason the server might allow reads on an O_WRONLY open is
because the client may need to do a RMW cycle if it's doing page-
aligned buffered I/Os. The client really shouldn't allow userland to do
an O_WRONLY open and start issuing read() calls on it, however. So,
from that standpoint I think the original behavior of knfsd does
conform to the spec.

To fix this the right way, we probably need to make the implicit
O_WRONLY -> O_RDRW upgrade for a delegation take some sort of "shadow"
reference. IOW, we need to be able to use the O_RDONLY file internally
and put its reference when the file is closed, but we don't want to
count that reference toward share/deny mode enforcement. 

> 
> > 
> > > 
> > > 
> > > > > > +        fp = stp->st_stid.sc_file;
> > > > > > +        fp->fi_fds[O_RDONLY] = nf;
> > > > > > +        spin_unlock(&fp->fi_lock);
> > > > > > +    }
> > > > > > +    return (true);
> > > > > no need for parenthesis here ^^^
> > 
> > Fixed.
> > 
> > > > > 
> > > > > > +}
> > > > > > +
> > > > > >   /*
> > > > > >    * The Linux NFS server does not offer write delegations to NFSv4.0
> > > > > >    * clients in order to avoid conflicts between write delegations 
> > > > > > and
> > > > > > @@ -6151,8 +6179,9 @@ nfs4_delegation_stat(struct nfs4_delegation 
> > > > > > *dp, struct svc_fh *currentfh,
> > > > > >    * open or lock state.
> > > > > >    */
> > > > > >   static void
> > > > > > -nfs4_open_delegation(struct nfsd4_open *open, struct 
> > > > > > nfs4_ol_stateid *stp,
> > > > > > -             struct svc_fh *currentfh)
> > > > > > +nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open 
> > > > > > *open,
> > > > > > +             struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
> > > > > > +             struct svc_fh *fh)
> > > > > >   {
> > > > > >       bool deleg_ts = open->op_deleg_want & 
> > > > > > OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS;
> > > > > >       struct nfs4_openowner *oo = openowner(stp->st_stateowner);
> > > > > > @@ -6197,7 +6226,8 @@ nfs4_open_delegation(struct nfsd4_open 
> > > > > > *open, struct nfs4_ol_stateid *stp,
> > > > > >       memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, 
> > > > > > sizeof(dp->dl_stid.sc_stateid));
> > > > > >         if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> > > > > > -        if (!nfs4_delegation_stat(dp, currentfh, &stat)) {
> > > > > > +        if ((!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, 
> > > > > > stp)) ||
> > > > > extra set of parens above too ^^^
> > 
> > Fixed.
> > 
> > > > > 
> > > > > > + !nfs4_delegation_stat(dp, currentfh, &stat)) {
> > > > > >               nfs4_put_stid(&dp->dl_stid);
> > > > > >               destroy_delegation(dp);
> > > > > >               goto out_no_deleg;
> > > > > > @@ -6353,7 +6383,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, 
> > > > > > struct svc_fh *current_fh, struct nf
> > > > > >       * Attempt to hand out a delegation. No error return, because 
> > > > > > the
> > > > > >       * OPEN succeeds even if we fail.
> > > > > >       */
> > > > > > -    nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
> > > > > > +    nfs4_open_delegation(rqstp, open, stp,
> > > > > > +        &resp->cstate.current_fh, current_fh);
> > > > > >         /*
> > > > > >        * If there is an existing open stateid, it must be updated and
> > > > > > @@ -7098,10 +7129,6 @@ nfs4_find_file(struct nfs4_stid *s, int flags)
> > > > > >         switch (s->sc_type) {
> > > > > >       case SC_TYPE_DELEG:
> > > > > > -        spin_lock(&s->sc_file->fi_lock);
> > > > > > -        ret = nfsd_file_get(s->sc_file->fi_deleg_file);
> > > > > > -        spin_unlock(&s->sc_file->fi_lock);
> > > > > > -        break;
> > > > > >       case SC_TYPE_OPEN:
> > > > > >       case SC_TYPE_LOCK:
> > > > > >           if (flags & RD_STATE)
> > > > > > @@ -7277,6 +7304,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst 
> > > > > > *rqstp,
> > > > > >           status = find_cpntf_state(nn, stateid, &s);
> > > > > >       if (status)
> > > > > >           return status;
> > > > > > +
> > > > > >       status = nfsd4_stid_check_stateid_generation(stateid, s,
> > > > > >               nfsd4_has_session(cstate));
> > > > > >       if (status)
> > > > > Patch itself looks good though, so with the nits fixed up, you can 
> > > > > add:
> > > > > 
> > > > > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > > > Dai, I can fix the parentheses in my tree, no need for a v5.
> > 
> > Thanks Chuck, I will fold these patches into one to avoid potential
> > bisect issue before sending out v5.
> > 
> > -Dai
> > 
> > > > 
> > > > 
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH V4 2/2] NFSD: allow client to use write delegation stateid for READ

[Tom Talpey]

On 3/6/2025 6:52 AM, Jeff Layton wrote:
> On Wed, 2025-03-05 at 12:59 -0800, Dai Ngo wrote:
>> On 3/5/25 12:47 PM, Dai Ngo wrote:
>>>
>>> On 3/5/25 8:08 AM, Jeff Layton wrote:
>>>> On Wed, 2025-03-05 at 09:46 -0500, Chuck Lever wrote:
>>>>> On 3/5/25 9:34 AM, Jeff Layton wrote:
>>>>>> On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
>>>>>>> Allow READ using write delegation stateid granted on OPENs with
>>>>>>> OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
>>>>>>> implementation may unavoidably do (e.g., due to buffer cache
>>>>>>> constraints).
>>>>>>>
>>>>>>> For write delegation granted for OPEN with OPEN4_SHARE_ACCESS_WRITE
>>>>>>> a new nfsd_file and a struct file are allocated to use for reads.
>>>>>>> The nfsd_file is freed when the file is closed by release_all_access.
>>>>>>>
>>>>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>>>>> ---
>>>>>>>    fs/nfsd/nfs4state.c | 44
>>>>>>> ++++++++++++++++++++++++++++++++++++--------
>>>>>>>    1 file changed, 36 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>>>> index b533225e57cf..35018af4e7fb 100644
>>>>>>> --- a/fs/nfsd/nfs4state.c
>>>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>>>> @@ -6126,6 +6126,34 @@ nfs4_delegation_stat(struct nfs4_delegation
>>>>>>> *dp, struct svc_fh *currentfh,
>>>>>>>        return rc == 0;
>>>>>>>    }
>>>>>>>    +/*
>>>>>>> + * Add NFS4_SHARE_ACCESS_READ to the write delegation granted on
>>>>>>> OPEN
>>>>>>> + * with NFS4_SHARE_ACCESS_WRITE by allocating separate nfsd_file and
>>>>>>> + * struct file to be used for read with delegation stateid.
>>>>>>> + *
>>>>>>> + */
>>>>>>> +static bool
>>>>>>> +nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct
>>>>>>> nfsd4_open *open,
>>>>>>> +                  struct svc_fh *fh, struct nfs4_ol_stateid *stp)
>>>>>>> +{
>>>>>>> +    struct nfs4_file *fp;
>>>>>>> +    struct nfsd_file *nf = NULL;
>>>>>>> +
>>>>>>> +    if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
>>>>>>> +            NFS4_SHARE_ACCESS_WRITE) {
>>>>>>> +        if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ,
>>>>>>> NULL, &nf))
>>>>>>> +            return (false);
>>>>>>> +        fp = stp->st_stid.sc_file;
>>>>>>> +        spin_lock(&fp->fi_lock);
>>>>>>> +        __nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);
>>>>>>> +        set_access(NFS4_SHARE_ACCESS_READ, stp);
>>>> The only other (minor) issue is that this might be problematic vs.
>>>> DENY_READ modes:
>>>>
>>>> Suppose someone opens the file SHARE_ACCESS_WRITE and gets back a r/w
>>>> delegation. Then someone else tries to open the file
>>>> SHARE_ACCESS_READ|SHARE_DENY_READ. That should succeed, AFAICT, but I
>>>> think with this patch that would fail because we check the deny mode
>>>> before doing the open (and revoking the delegation).
>>>>
>>>> It'd be good to test and see if that's the case.
>>>
>>> Yes, you're correct. The 2nd OPEN fails due to the read access set
>>> to the file in nfsd4_add_rdaccess_to_wrdeleg().
>>>
>>> I think the deny mode is used only by SMB and not Linux client, not
>>> sure though. What should we do about this, any thought?
> 
> Deny modes are a Windows/DOS thing, but they are part of the NFSv4 spec

Windows NFSv4 clients certainly might use these modes, so it's
important that knfsd supports them, or at least, not reject them.

Tom.

> too. Linux doesn't have a userland interface that allows you to set
> them, and they aren't plumbed through the VFS layer, so you can still
> do an open locally on the box, even if a deny mode is set. I _think_
> BSD might also have support at the VFS layer for share/deny locking but
> I don't know for sure.
> 
>>
>> Without this patch, nfsd does not hand out the write delegation and don't
>> set the read access so the 2nd OPEN would work. But is that the correct
>> behavior because the open stateid of the 1st OPEN is allowed to do read?
>>
> 
> That's a good question.
> 
> The main reason the server might allow reads on an O_WRONLY open is
> because the client may need to do a RMW cycle if it's doing page-
> aligned buffered I/Os. The client really shouldn't allow userland to do
> an O_WRONLY open and start issuing read() calls on it, however. So,
> from that standpoint I think the original behavior of knfsd does
> conform to the spec.
> 
> To fix this the right way, we probably need to make the implicit
> O_WRONLY -> O_RDRW upgrade for a delegation take some sort of "shadow"
> reference. IOW, we need to be able to use the O_RDONLY file internally
> and put its reference when the file is closed, but we don't want to
> count that reference toward share/deny mode enforcement.
> 
>>
>>>
>>>>
>>>>
>>>>>>> +        fp = stp->st_stid.sc_file;
>>>>>>> +        fp->fi_fds[O_RDONLY] = nf;
>>>>>>> +        spin_unlock(&fp->fi_lock);
>>>>>>> +    }
>>>>>>> +    return (true);
>>>>>> no need for parenthesis here ^^^
>>>
>>> Fixed.
>>>
>>>>>>
>>>>>>> +}
>>>>>>> +
>>>>>>>    /*
>>>>>>>     * The Linux NFS server does not offer write delegations to NFSv4.0
>>>>>>>     * clients in order to avoid conflicts between write delegations
>>>>>>> and
>>>>>>> @@ -6151,8 +6179,9 @@ nfs4_delegation_stat(struct nfs4_delegation
>>>>>>> *dp, struct svc_fh *currentfh,
>>>>>>>     * open or lock state.
>>>>>>>     */
>>>>>>>    static void
>>>>>>> -nfs4_open_delegation(struct nfsd4_open *open, struct
>>>>>>> nfs4_ol_stateid *stp,
>>>>>>> -             struct svc_fh *currentfh)
>>>>>>> +nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open
>>>>>>> *open,
>>>>>>> +             struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
>>>>>>> +             struct svc_fh *fh)
>>>>>>>    {
>>>>>>>        bool deleg_ts = open->op_deleg_want &
>>>>>>> OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS;
>>>>>>>        struct nfs4_openowner *oo = openowner(stp->st_stateowner);
>>>>>>> @@ -6197,7 +6226,8 @@ nfs4_open_delegation(struct nfsd4_open
>>>>>>> *open, struct nfs4_ol_stateid *stp,
>>>>>>>        memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid,
>>>>>>> sizeof(dp->dl_stid.sc_stateid));
>>>>>>>          if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>>>>>>> -        if (!nfs4_delegation_stat(dp, currentfh, &stat)) {
>>>>>>> +        if ((!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh,
>>>>>>> stp)) ||
>>>>>> extra set of parens above too ^^^
>>>
>>> Fixed.
>>>
>>>>>>
>>>>>>> + !nfs4_delegation_stat(dp, currentfh, &stat)) {
>>>>>>>                nfs4_put_stid(&dp->dl_stid);
>>>>>>>                destroy_delegation(dp);
>>>>>>>                goto out_no_deleg;
>>>>>>> @@ -6353,7 +6383,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp,
>>>>>>> struct svc_fh *current_fh, struct nf
>>>>>>>        * Attempt to hand out a delegation. No error return, because
>>>>>>> the
>>>>>>>        * OPEN succeeds even if we fail.
>>>>>>>        */
>>>>>>> -    nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
>>>>>>> +    nfs4_open_delegation(rqstp, open, stp,
>>>>>>> +        &resp->cstate.current_fh, current_fh);
>>>>>>>          /*
>>>>>>>         * If there is an existing open stateid, it must be updated and
>>>>>>> @@ -7098,10 +7129,6 @@ nfs4_find_file(struct nfs4_stid *s, int flags)
>>>>>>>          switch (s->sc_type) {
>>>>>>>        case SC_TYPE_DELEG:
>>>>>>> -        spin_lock(&s->sc_file->fi_lock);
>>>>>>> -        ret = nfsd_file_get(s->sc_file->fi_deleg_file);
>>>>>>> -        spin_unlock(&s->sc_file->fi_lock);
>>>>>>> -        break;
>>>>>>>        case SC_TYPE_OPEN:
>>>>>>>        case SC_TYPE_LOCK:
>>>>>>>            if (flags & RD_STATE)
>>>>>>> @@ -7277,6 +7304,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst
>>>>>>> *rqstp,
>>>>>>>            status = find_cpntf_state(nn, stateid, &s);
>>>>>>>        if (status)
>>>>>>>            return status;
>>>>>>> +
>>>>>>>        status = nfsd4_stid_check_stateid_generation(stateid, s,
>>>>>>>                nfsd4_has_session(cstate));
>>>>>>>        if (status)
>>>>>> Patch itself looks good though, so with the nits fixed up, you can
>>>>>> add:
>>>>>>
>>>>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>>>> Dai, I can fix the parentheses in my tree, no need for a v5.
>>>
>>> Thanks Chuck, I will fold these patches into one to avoid potential
>>> bisect issue before sending out v5.
>>>
>>> -Dai
>>>
>>>>>
>>>>>
>>>
> 

Re: [PATCH V4 2/2] NFSD: allow client to use write delegation stateid for READ

[Dai Ngo]

On 3/6/25 3:52 AM, Jeff Layton wrote:
> On Wed, 2025-03-05 at 12:59 -0800, Dai Ngo wrote:
>> On 3/5/25 12:47 PM, Dai Ngo wrote:
>>> On 3/5/25 8:08 AM, Jeff Layton wrote:
>>>> On Wed, 2025-03-05 at 09:46 -0500, Chuck Lever wrote:
>>>>> On 3/5/25 9:34 AM, Jeff Layton wrote:
>>>>>> On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote:
>>>>>>> Allow READ using write delegation stateid granted on OPENs with
>>>>>>> OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
>>>>>>> implementation may unavoidably do (e.g., due to buffer cache
>>>>>>> constraints).
>>>>>>>
>>>>>>> For write delegation granted for OPEN with OPEN4_SHARE_ACCESS_WRITE
>>>>>>> a new nfsd_file and a struct file are allocated to use for reads.
>>>>>>> The nfsd_file is freed when the file is closed by release_all_access.
>>>>>>>
>>>>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>>>>> ---
>>>>>>>    fs/nfsd/nfs4state.c | 44
>>>>>>> ++++++++++++++++++++++++++++++++++++--------
>>>>>>>    1 file changed, 36 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>>>> index b533225e57cf..35018af4e7fb 100644
>>>>>>> --- a/fs/nfsd/nfs4state.c
>>>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>>>> @@ -6126,6 +6126,34 @@ nfs4_delegation_stat(struct nfs4_delegation
>>>>>>> *dp, struct svc_fh *currentfh,
>>>>>>>        return rc == 0;
>>>>>>>    }
>>>>>>>    +/*
>>>>>>> + * Add NFS4_SHARE_ACCESS_READ to the write delegation granted on
>>>>>>> OPEN
>>>>>>> + * with NFS4_SHARE_ACCESS_WRITE by allocating separate nfsd_file and
>>>>>>> + * struct file to be used for read with delegation stateid.
>>>>>>> + *
>>>>>>> + */
>>>>>>> +static bool
>>>>>>> +nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct
>>>>>>> nfsd4_open *open,
>>>>>>> +                  struct svc_fh *fh, struct nfs4_ol_stateid *stp)
>>>>>>> +{
>>>>>>> +    struct nfs4_file *fp;
>>>>>>> +    struct nfsd_file *nf = NULL;
>>>>>>> +
>>>>>>> +    if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
>>>>>>> +            NFS4_SHARE_ACCESS_WRITE) {
>>>>>>> +        if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ,
>>>>>>> NULL, &nf))
>>>>>>> +            return (false);
>>>>>>> +        fp = stp->st_stid.sc_file;
>>>>>>> +        spin_lock(&fp->fi_lock);
>>>>>>> +        __nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);
>>>>>>> +        set_access(NFS4_SHARE_ACCESS_READ, stp);
>>>> The only other (minor) issue is that this might be problematic vs.
>>>> DENY_READ modes:
>>>>
>>>> Suppose someone opens the file SHARE_ACCESS_WRITE and gets back a r/w
>>>> delegation. Then someone else tries to open the file
>>>> SHARE_ACCESS_READ|SHARE_DENY_READ. That should succeed, AFAICT, but I
>>>> think with this patch that would fail because we check the deny mode
>>>> before doing the open (and revoking the delegation).
>>>>
>>>> It'd be good to test and see if that's the case.
>>> Yes, you're correct. The 2nd OPEN fails due to the read access set
>>> to the file in nfsd4_add_rdaccess_to_wrdeleg().
>>>
>>> I think the deny mode is used only by SMB and not Linux client, not
>>> sure though. What should we do about this, any thought?
> Deny modes are a Windows/DOS thing, but they are part of the NFSv4 spec
> too. Linux doesn't have a userland interface that allows you to set
> them, and they aren't plumbed through the VFS layer, so you can still
> do an open locally on the box, even if a deny mode is set. I _think_
> BSD might also have support at the VFS layer for share/deny locking but
> I don't know for sure.
>
>> Without this patch, nfsd does not hand out the write delegation and don't
>> set the read access so the 2nd OPEN would work. But is that the correct
>> behavior because the open stateid of the 1st OPEN is allowed to do read?
>>
> That's a good question.
>
> The main reason the server might allow reads on an O_WRONLY open is
> because the client may need to do a RMW cycle if it's doing page-
> aligned buffered I/Os. The client really shouldn't allow userland to do
> an O_WRONLY open and start issuing read() calls on it, however. So,
> from that standpoint I think the original behavior of knfsd does
> conform to the spec.
>
> To fix this the right way, we probably need to make the implicit
> O_WRONLY -> O_RDRW upgrade for a delegation take some sort of "shadow"
> reference. IOW, we need to be able to use the O_RDONLY file internally
> and put its reference when the file is closed, but we don't want to
> count that reference toward share/deny mode enforcement.

The fix to support deny mode turns out to be simple. When we upgrade
the write delegation from WRONLY to RDWR, don't set the read access in
st_access_bmap, don't need it. The nfsd always allow file opened with
write to do read anyway, this is done in access_permit_read().

-Dai

>   
>
>>>>
>>>>>>> +        fp = stp->st_stid.sc_file;
>>>>>>> +        fp->fi_fds[O_RDONLY] = nf;
>>>>>>> +        spin_unlock(&fp->fi_lock);
>>>>>>> +    }
>>>>>>> +    return (true);
>>>>>> no need for parenthesis here ^^^
>>> Fixed.
>>>
>>>>>>> +}
>>>>>>> +
>>>>>>>    /*
>>>>>>>     * The Linux NFS server does not offer write delegations to NFSv4.0
>>>>>>>     * clients in order to avoid conflicts between write delegations
>>>>>>> and
>>>>>>> @@ -6151,8 +6179,9 @@ nfs4_delegation_stat(struct nfs4_delegation
>>>>>>> *dp, struct svc_fh *currentfh,
>>>>>>>     * open or lock state.
>>>>>>>     */
>>>>>>>    static void
>>>>>>> -nfs4_open_delegation(struct nfsd4_open *open, struct
>>>>>>> nfs4_ol_stateid *stp,
>>>>>>> -             struct svc_fh *currentfh)
>>>>>>> +nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open
>>>>>>> *open,
>>>>>>> +             struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
>>>>>>> +             struct svc_fh *fh)
>>>>>>>    {
>>>>>>>        bool deleg_ts = open->op_deleg_want &
>>>>>>> OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS;
>>>>>>>        struct nfs4_openowner *oo = openowner(stp->st_stateowner);
>>>>>>> @@ -6197,7 +6226,8 @@ nfs4_open_delegation(struct nfsd4_open
>>>>>>> *open, struct nfs4_ol_stateid *stp,
>>>>>>>        memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid,
>>>>>>> sizeof(dp->dl_stid.sc_stateid));
>>>>>>>          if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>>>>>>> -        if (!nfs4_delegation_stat(dp, currentfh, &stat)) {
>>>>>>> +        if ((!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh,
>>>>>>> stp)) ||
>>>>>> extra set of parens above too ^^^
>>> Fixed.
>>>
>>>>>>> + !nfs4_delegation_stat(dp, currentfh, &stat)) {
>>>>>>>                nfs4_put_stid(&dp->dl_stid);
>>>>>>>                destroy_delegation(dp);
>>>>>>>                goto out_no_deleg;
>>>>>>> @@ -6353,7 +6383,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp,
>>>>>>> struct svc_fh *current_fh, struct nf
>>>>>>>        * Attempt to hand out a delegation. No error return, because
>>>>>>> the
>>>>>>>        * OPEN succeeds even if we fail.
>>>>>>>        */
>>>>>>> -    nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
>>>>>>> +    nfs4_open_delegation(rqstp, open, stp,
>>>>>>> +        &resp->cstate.current_fh, current_fh);
>>>>>>>          /*
>>>>>>>         * If there is an existing open stateid, it must be updated and
>>>>>>> @@ -7098,10 +7129,6 @@ nfs4_find_file(struct nfs4_stid *s, int flags)
>>>>>>>          switch (s->sc_type) {
>>>>>>>        case SC_TYPE_DELEG:
>>>>>>> -        spin_lock(&s->sc_file->fi_lock);
>>>>>>> -        ret = nfsd_file_get(s->sc_file->fi_deleg_file);
>>>>>>> -        spin_unlock(&s->sc_file->fi_lock);
>>>>>>> -        break;
>>>>>>>        case SC_TYPE_OPEN:
>>>>>>>        case SC_TYPE_LOCK:
>>>>>>>            if (flags & RD_STATE)
>>>>>>> @@ -7277,6 +7304,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst
>>>>>>> *rqstp,
>>>>>>>            status = find_cpntf_state(nn, stateid, &s);
>>>>>>>        if (status)
>>>>>>>            return status;
>>>>>>> +
>>>>>>>        status = nfsd4_stid_check_stateid_generation(stateid, s,
>>>>>>>                nfsd4_has_session(cstate));
>>>>>>>        if (status)
>>>>>> Patch itself looks good though, so with the nits fixed up, you can
>>>>>> add:
>>>>>>
>>>>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>>>> Dai, I can fix the parentheses in my tree, no need for a v5.
>>> Thanks Chuck, I will fold these patches into one to avoid potential
>>> bisect issue before sending out v5.
>>>
>>> -Dai
>>>
>>>>>