Re: [PATCH v2 04/10] nfsd: Distinguish between required and optional NFSv3 post-op attributes

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2021-12-20 at 16:02 +0000, Chuck Lever III wrote:
> 
> 
> > On Dec 19, 2021, at 4:09 PM, Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > 
> > On Sun, 2021-12-19 at 20:10 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Dec 18, 2021, at 8:37 PM, trondmy@kernel.org wrote:
> > > > 
> > > > From: Trond Myklebust <trond.myklebust@primarydata.com>
> > > > 
> > > > The fhp->fh_no_wcc flag is automatically set in
> > > > nfsd_set_fh_dentry()
> > > > when the EXPORT_OP_NOWCC flag is set. In
> > > > svcxdr_encode_post_op_attr(),
> > > > this then causes nfsd to skip returning the post-op attributes.
> > > > 
> > > > The problem is that some of these post-op attributes are not
> > > > really
> > > > optional. In particular, we do want LOOKUP to always return
> > > > post-op
> > > > attributes for the file that is being looked up.
> > > > 
> > > > The solution is therefore to explicitly label the attributes
> > > > that
> > > > we can
> > > > safely opt out from, and only apply the 'fhp->fh_no_wcc' test
> > > > in
> > > > that
> > > > case.
> > > > 
> > > > Signed-off-by: Trond Myklebust
> > > > <trond.myklebust@primarydata.com>
> > > > Signed-off-by: Lance Shelton <lance.shelton@hammerspace.com>
> > > > Signed-off-by: Trond Myklebust
> > > > <trond.myklebust@hammerspace.com>
> > > > ---
> > > > fs/nfsd/nfs3xdr.c | 77 +++++++++++++++++++++++++++++++---------
> > > > ----
> > > > ---
> > > > 1 file changed, 51 insertions(+), 26 deletions(-)
> > > > 
> > > > diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> > > > index c3ac1b6aa3aa..6adfc40722fa 100644
> > > > --- a/fs/nfsd/nfs3xdr.c
> > > > +++ b/fs/nfsd/nfs3xdr.c
> > > > @@ -415,19 +415,9 @@ svcxdr_encode_pre_op_attr(struct
> > > > xdr_stream
> > > > *xdr, const struct svc_fh *fhp)
> > > >         return svcxdr_encode_wcc_attr(xdr, fhp);
> > > > }
> > > > 
> > > > -/**
> > > > - * svcxdr_encode_post_op_attr - Encode NFSv3 post-op
> > > > attributes
> > > > - * @rqstp: Context of a completed RPC transaction
> > > > - * @xdr: XDR stream
> > > > - * @fhp: File handle to encode
> > > > - *
> > > > - * Return values:
> > > > - *   %false: Send buffer space was exhausted
> > > > - *   %true: Success
> > > > - */
> > > > -bool
> > > > -svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct
> > > > xdr_stream *xdr,
> > > > -                          const struct svc_fh *fhp)
> > > > +static bool
> > > > +__svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct
> > > > xdr_stream *xdr,
> > > > +                            const struct svc_fh *fhp, bool
> > > > force)
> > > > {
> > > >         struct dentry *dentry = fhp->fh_dentry;
> > > >         struct kstat stat;
> > > > @@ -437,7 +427,7 @@ svcxdr_encode_post_op_attr(struct svc_rqst
> > > > *rqstp, struct xdr_stream *xdr,
> > > >          * stale file handle. In this case, no attributes are
> > > >          * returned.
> > > >          */
> > > > -       if (fhp->fh_no_wcc || !dentry ||
> > > > !d_really_is_positive(dentry))
> > > > +       if (!force || !dentry || !d_really_is_positive(dentry))
> > > >                 goto no_post_op_attrs;
> > > >         if (fh_getattr(fhp, &stat) != nfs_ok)
> > > >                 goto no_post_op_attrs;
> > > > @@ -454,6 +444,31 @@ svcxdr_encode_post_op_attr(struct svc_rqst
> > > > *rqstp, struct xdr_stream *xdr,
> > > >         return xdr_stream_encode_item_absent(xdr) > 0;
> > > > }
> > > > 
> > > > +/**
> > > > + * svcxdr_encode_post_op_attr - Encode NFSv3 post-op
> > > > attributes
> > > > + * @rqstp: Context of a completed RPC transaction
> > > > + * @xdr: XDR stream
> > > > + * @fhp: File handle to encode
> > > > + *
> > > > + * Return values:
> > > > + *   %false: Send buffer space was exhausted
> > > > + *   %true: Success
> > > > + */
> > > > +bool
> > > > +svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct
> > > > xdr_stream *xdr,
> > > > +                          const struct svc_fh *fhp)
> > > > +{
> > > > +       return __svcxdr_encode_post_op_attr(rqstp, xdr, fhp,
> > > > true);
> > > > +}
> > > > +
> > > > +static bool
> > > > +svcxdr_encode_post_op_attr_opportunistic(struct svc_rqst
> > > > *rqstp,
> > > > +                                        struct xdr_stream
> > > > *xdr,
> > > > +                                        const struct svc_fh
> > > > *fhp)
> > > > +{
> > > > +       return __svcxdr_encode_post_op_attr(rqstp, xdr, fhp,
> > > > !fhp-
> > > > > fh_no_wcc);
> > > > +}
> > > > +
> > > 
> > > Thanks for splitting this out: the "why" is much clearer.
> > > 
> > > Wouldn't it be simpler to explicitly set fh_no_wcc to false
> > > in each of the cases where you want to ensure the server
> > > emits WCC? And perhaps that should be done in nfs3proc.c
> > > instead of in nfs3xdr.c.
> > > 
> > 
> > It can't be done in nfs3proc.c, and toggling the value of fh_no_wcc
> > is
> > a lot more cumbersome than this approach.
> > 
> > The current code is broken for NFSv3 exports because it is unable
> > to
> > distinguish between what is and isn't mandatory to return for in
> > the
> > same NFS operation. That's the problem this patch fixes.
> 
> That suggests that a Fixes: tag is appropriate. Can you recommend
> one?
> 
> 
> > LOOKUP has to return the attributes for the object being looked up
> > in
> > order to be useful. If the attributes are not up to date then we
> > should
> > ask the NFS client that is being re-exported to go to the server to
> > revalidate its attributes.
> > The same is not true of the directory post-op attributes. LOOKUP
> > does
> > not even change the contents of the directory, and so while it is
> > beneficial to have the NFS client return those attributes if they
> > are
> > up to date, forcing it to go to the server to retrieve them is less
> > than optimal for system performance.
> 
> I get all that, but I don't see how this is cumbersome at all:
> 
>  82 static __be32
>  83 nfsd3_proc_lookup(struct svc_rqst *rqstp)
>  84 {
> ...
>  96         resp->status = nfsd_lookup(rqstp, &resp->dirfh,
>  97                                    argp->name, argp->len,
>  98                                    &resp->fh);
>     +       resp->fh.fh_no_wcc = false;
>  99         return rpc_success;
> 100 }
> 
> Then in 5/10, pass !fhp->fh_no_wcc to nfsd_getattr().
> 
> 

That's not equivalent. That will force the NFS client to retrieve the
lookup object attributes AND the directory attributes.

As I said above, the latter is optional. The former is not.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com