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

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2024-07-24 at 14:47 -0700, Sagi Grimberg wrote:
> 
> 
> 
> On 24/07/2024 22:29, Olga Kornievskaia wrote:
> > On Wed, Jul 24, 2024 at 1:01 PM Sagi Grimberg <sagi@grimberg.me>
> > wrote:
> > > Based on a patch fom Dai Ngo, allow NFSv4 client to use write
> > > delegation
> > > stateid for READ operation. Per RFC 8881 section 9.1.2. Use of
> > > the
> > > Stateid and Locking.
> > > 
> > > In addition, for anonymous stateids, check for pending
> > > delegations by
> > > the filehandle and client_id, and if a conflict found, recall the
> > > delegation
> > > before allowing the read to take place.
> > > 
> > > Suggested-by: Dai Ngo <dai.ngo@oracle.com>
> > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > > ---
> > >   fs/nfsd/nfs4proc.c  | 22 +++++++++++++++++++--
> > >   fs/nfsd/nfs4state.c | 47
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > >   fs/nfsd/nfs4xdr.c   |  9 +++++++++
> > >   fs/nfsd/state.h     |  2 ++
> > >   fs/nfsd/xdr4.h      |  2 ++
> > >   5 files changed, 80 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index 7b70309ad8fb..324984ec70c6 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -979,8 +979,24 @@ 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);
> > Now I can see that this patch wants to leverage the "returned
> > stateid
> > of the nfs4_preprocess_stateid_op() but the logic in the previous
> > patch was in the way because it distinguished the copy_notify by
> > the
> > non-null passed in stateid. So I would suggest that in order to not
> > break the copy_notify and help with this functionality something
> > else
> > is sent into nfs4_proprocess_staetid_op() to allow for the stateid
> > to
> > be passed and then distinguish between copy_notify and read.
> 
> My thinking is that instead having the generic stateid pre-processing
> be aware which proc may accept a special stateid and which may not,
> we'd want to have the call-sites enforce that...
> 
> But I am not the expert here, and will be happy to change based on
> your preference...

I tend to agree with Sagi here. Keeping this sort of operation-specific
check in the operation-specific function makes more sense to me too.
-- 
Jeff Layton <jlayton@kernel.org>