On 2/13/25 6:29 PM, Jeff Layton wrote:
> On Thu, 2025-02-13 at 16:07 -0500, Jeff Layton wrote:
>> On Thu, 2025-02-13 at 11:37 -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 this condition is detected in nfsd4_encode_read the access
>>> mode FMODE_READ is temporarily added to the file's f_mode and is
>>> removed when the read is done.
>>>
>>> Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
>>> ---
>>> fs/nfsd/nfs4proc.c | 15 ++++++++++++++-
>>> fs/nfsd/nfs4xdr.c | 8 ++++++++
>>> fs/nfsd/xdr4.h | 1 +
>>> 3 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index f6e06c779d09..be43627bbf78 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -973,7 +973,18 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> /* check stateid */
>>> status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>>> &read->rd_stateid, RD_STATE,
>>> - &read->rd_nf, NULL);
>>> + &read->rd_nf, &read->rd_wd_stid);
>>> + /*
>>> + * rd_wd_stid is needed for nfsd4_encode_read to allow write
>>> + * delegation stateid used for read. Its refcount is decremented
>>> + * by nfsd4_read_release when read is done.
>>> + */
>>> + if (!status && read->rd_wd_stid &&
>>> + (read->rd_wd_stid->sc_type != SC_TYPE_DELEG ||
>>> + delegstateid(read->rd_wd_stid)->dl_type != NFS4_OPEN_DELEGATE_WRITE)) {
>>> + nfs4_put_stid(read->rd_wd_stid);
>>> + read->rd_wd_stid = NULL;
>>> + }
>>>
>>> read->rd_rqstp = rqstp;
>>> read->rd_fhp = &cstate->current_fh;
>>> @@ -984,6 +995,8 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> static void
>>> nfsd4_read_release(union nfsd4_op_u *u)
>>> {
>>> + if (u->read.rd_wd_stid)
>>> + nfs4_put_stid(u->read.rd_wd_stid);
>>> if (u->read.rd_nf)
>>> nfsd_file_put(u->read.rd_nf);
>>> trace_nfsd_read_done(u->read.rd_rqstp, u->read.rd_fhp,
>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>> index e67420729ecd..3996678bab3f 100644
>>> --- a/fs/nfsd/nfs4xdr.c
>>> +++ b/fs/nfsd/nfs4xdr.c
>>> @@ -4498,6 +4498,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>>> unsigned long maxcount;
>>> __be32 wire_data[2];
>>> struct file *file;
>>> + bool wronly = false;
>>>
>>> if (nfserr)
>>> return nfserr;
>>> @@ -4515,10 +4516,17 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>>> maxcount = min_t(unsigned long, read->rd_length,
>>> (xdr->buf->buflen - xdr->buf->len));
>>>
>>> + if (!(file->f_mode & FMODE_READ) && read->rd_wd_stid) {
>>> + /* allow READ using write delegation stateid */
>>> + wronly = true;
>>> + file->f_mode |= FMODE_READ;
>>> + }
>>
>> Is that really OK? Can we just upgrade the f_mode like that?
>>
>> Also, what happens with more exotic exported filesystems like NFS?
>>
>> For example, if I'm reexporting NFS, the backend NFS server may not
>> allow you to do a READ operation using a OPEN4_SHARE_ACCESS_WRITE only
>> stateid. Won't this break in that case?
>>
>
> Hmm...bad example since we don't allow delegations on reexported NFS
> these days. Reexporting Ceph or SMB might be a better example. They'll
> likely both have problems if you try to issue a read on the result from
> a O_WRONLY open. I think you will probably need to rework the way
> nfs4_file's track their struct files.
>
> IOW, when the client does a OPEN4_SHARE_ACCESS_WRITE-only open, you
> need to get a struct file that is FMODE_READ|FMODE_WRITE to hang off
> the delegation. But, you'll also need to fix up the accounting for the
> share/deny mode locking to ignore that you _actually_ have it open for
> read too in that case.
For the record, I agree with Jeff's suggested approach.
> Smoke and mirrors...
>
>>> if (file->f_op->splice_read && splice_ok)
>>> nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
>>> else
>>> nfserr = nfsd4_encode_readv(resp, read, file, maxcount);
>>> + if (wronly)
>>> + file->f_mode &= ~FMODE_READ;
>>> if (nfserr) {
>>> xdr_truncate_encode(xdr, eof_offset);
>>> return nfserr;
>>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>>> index c26ba86dbdfd..2f053beed899 100644
>>> --- a/fs/nfsd/xdr4.h
>>> +++ b/fs/nfsd/xdr4.h
>>> @@ -426,6 +426,7 @@ struct nfsd4_read {
>>> struct svc_rqst *rd_rqstp; /* response */
>>> struct svc_fh *rd_fhp; /* response */
>>> u32 rd_eof; /* response */
>>> + struct nfs4_stid *rd_wd_stid; /* internal */
>>> };
>>>
>>> struct nfsd4_readdir {
>>
>
--
Chuck Lever
