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

[Jeff Layton <jlayton@kernel.org>]

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>