diff mbox series

[V3,2/2] NFSD: allow client to use write delegation stateid for READ

Message ID 1741059224-4846-3-git-send-email-dai.ngo@oracle.com (mailing list archive)
State Changes Requested
Delegated to: Chuck Lever
Headers show
Series NFSD: offer write delegation for OPEN with OPEN4_SHARE_ACCESS only | expand

Commit Message

Dai Ngo March 4, 2025, 3:33 a.m. UTC 
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).

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.

Suggested-by: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

Comments

Jeff Layton March 4, 2025, 12:27 p.m. UTC | #1 
On Mon, 2025-03-03 at 19:33 -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).
> 
> 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.
> 
> Suggested-by: Chuck Lever <chuck.lever@oracle.com>
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b533225e57cf..d2c6c43b5d0c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6126,6 +6126,30 @@ 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_READ by allocating separate nfsd_file and
> + * struct file to be used for read with delegation stateid.
> + *
> + */
> +static void
> +nfs4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct svc_fh *fh,
> +			     struct nfs4_ol_stateid *stp)

Let's call this nfsd4_add_rdaccess_to_wrdeleg().

> +{
> +	struct nfs4_file *fp;
> +	struct nfsd_file *nf = NULL;
> +
> +	if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, NULL, &nf))
> +		return;


This function should return an error if nfsd_file_acquire_opened()
fails, and the caller should not give out the delegation in that case.

> +	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);
> +}
> +

>  /*
>   * The Linux NFS server does not offer write delegations to NFSv4.0
>   * clients in order to avoid conflicts between write delegations and
> @@ -6151,8 +6175,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);
> @@ -6207,6 +6232,10 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
>  		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
>  		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
> +
> +		if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
> +					NFS4_SHARE_ACCESS_WRITE)
> +			nfs4_add_rdaccess_to_wrdeleg(rqstp, fh, stp);

nit: I'd also move the if statement above into
nfsd4_add_rdaccess_to_wrdeleg().

>  	} else {
>  		open->op_delegate_type = deleg_ts ? OPEN_DELEGATE_READ_ATTRS_DELEG :
>  						    OPEN_DELEGATE_READ;
> @@ -6353,7 +6382,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 +7128,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 +7303,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)

I think this approach looks valid though.

Nice work!
Dai Ngo March 4, 2025, 5:41 p.m. UTC | #2 
On 3/4/25 4:27 AM, Jeff Layton wrote:
> On Mon, 2025-03-03 at 19:33 -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).
>>
>> 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.
>>
>> Suggested-by: Chuck Lever <chuck.lever@oracle.com>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 41 ++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 34 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index b533225e57cf..d2c6c43b5d0c 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -6126,6 +6126,30 @@ 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_READ by allocating separate nfsd_file and
>> + * struct file to be used for read with delegation stateid.
>> + *
>> + */
>> +static void
>> +nfs4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct svc_fh *fh,
>> +			     struct nfs4_ol_stateid *stp)
> Let's call this nfsd4_add_rdaccess_to_wrdeleg().

Okay.

>
>> +{
>> +	struct nfs4_file *fp;
>> +	struct nfsd_file *nf = NULL;
>> +
>> +	if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, NULL, &nf))
>> +		return;
>
> This function should return an error if nfsd_file_acquire_opened()
> fails, and the caller should not give out the delegation in that case.

Okay.

>
>> +	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);
>> +}
>> +
>>   /*
>>    * The Linux NFS server does not offer write delegations to NFSv4.0
>>    * clients in order to avoid conflicts between write delegations and
>> @@ -6151,8 +6175,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);
>> @@ -6207,6 +6232,10 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>   		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
>>   		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
>>   		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
>> +
>> +		if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
>> +					NFS4_SHARE_ACCESS_WRITE)
>> +			nfs4_add_rdaccess_to_wrdeleg(rqstp, fh, stp);
> nit: I'd also move the if statement above into
> nfsd4_add_rdaccess_to_wrdeleg().

Okay.

>
>>   	} else {
>>   		open->op_delegate_type = deleg_ts ? OPEN_DELEGATE_READ_ATTRS_DELEG :
>>   						    OPEN_DELEGATE_READ;
>> @@ -6353,7 +6382,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 +7128,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 +7303,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)
> I think this approach looks valid though.
>
> Nice work!

Thanks for your review.

-Dai
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b533225e57cf..d2c6c43b5d0c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6126,6 +6126,30 @@  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_READ by allocating separate nfsd_file and
+ * struct file to be used for read with delegation stateid.
+ *
+ */
+static void
+nfs4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct svc_fh *fh,
+			     struct nfs4_ol_stateid *stp)
+{
+	struct nfs4_file *fp;
+	struct nfsd_file *nf = NULL;
+
+	if (nfsd_file_acquire_opened(rqstp, fh, NFSD_MAY_READ, NULL, &nf))
+		return;
+	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);
+}
+
 /*
  * The Linux NFS server does not offer write delegations to NFSv4.0
  * clients in order to avoid conflicts between write delegations and
@@ -6151,8 +6175,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);
@@ -6207,6 +6232,10 @@  nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
 		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
 		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
+
+		if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
+					NFS4_SHARE_ACCESS_WRITE)
+			nfs4_add_rdaccess_to_wrdeleg(rqstp, fh, stp);
 	} else {
 		open->op_delegate_type = deleg_ts ? OPEN_DELEGATE_READ_ATTRS_DELEG :
 						    OPEN_DELEGATE_READ;
@@ -6353,7 +6382,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 +7128,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 +7303,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)