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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
Dai, I can fix the parentheses in my tree, no need for a v5.
--
Chuck Lever
