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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
--
Chuck Lever
