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@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);
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.
> > > + 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.
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
