On Sun, 2021-12-19 at 20:10 +0000, Chuck Lever III wrote:
>
>
> > On Dec 18, 2021, at 8:37 PM, trondmy@xxxxxxxxxx wrote:
> >
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Lance Shelton <lance.shelton@xxxxxxxxxxxxxxx>
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > ---
> > 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.
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.
>
> > /*
> > * Encode weak cache consistency data
> > */
> > @@ -481,7 +496,7 @@ svcxdr_encode_wcc_data(struct svc_rqst *rqstp,
> > struct xdr_stream *xdr,
> > neither:
> > if (xdr_stream_encode_item_absent(xdr) < 0)
> > return false;
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, fhp))
> > + if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > fhp))
> > return false;
> >
> > return true;
> > @@ -854,11 +869,13 @@ nfs3svc_encode_lookupres(struct svc_rqst
> > *rqstp, struct xdr_stream *xdr)
> > return false;
> > if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >fh))
> > return false;
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >dirfh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->dirfh))
> > return false;
> > break;
> > default:
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >dirfh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->dirfh))
> > return false;
> > }
> >
> > @@ -875,13 +892,15 @@ nfs3svc_encode_accessres(struct svc_rqst
> > *rqstp, struct xdr_stream *xdr)
> > return false;
> > switch (resp->status) {
> > case nfs_ok:
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >fh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->fh))
> > return false;
> > if (xdr_stream_encode_u32(xdr, resp->access) < 0)
> > return false;
> > break;
> > default:
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >fh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->fh))
> > return false;
> > }
> >
> > @@ -899,7 +918,8 @@ nfs3svc_encode_readlinkres(struct svc_rqst
> > *rqstp, struct xdr_stream *xdr)
> > return false;
> > switch (resp->status) {
> > case nfs_ok:
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >fh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->fh))
> > return false;
> > if (xdr_stream_encode_u32(xdr, resp->len) < 0)
> > return false;
> > @@ -908,7 +928,8 @@ nfs3svc_encode_readlinkres(struct svc_rqst
> > *rqstp, struct xdr_stream *xdr)
> > return false;
> > break;
> > default:
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >fh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->fh))
> > return false;
> > }
> >
> > @@ -926,7 +947,8 @@ nfs3svc_encode_readres(struct svc_rqst *rqstp,
> > struct xdr_stream *xdr)
> > return false;
> > switch (resp->status) {
> > case nfs_ok:
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >fh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->fh))
> > return false;
> > if (xdr_stream_encode_u32(xdr, resp->count) < 0)
> > return false;
> > @@ -940,7 +962,8 @@ nfs3svc_encode_readres(struct svc_rqst *rqstp,
> > struct xdr_stream *xdr)
> > return false;
> > break;
> > default:
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >fh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->fh))
> > return false;
> > }
> >
> > @@ -1032,7 +1055,8 @@ nfs3svc_encode_readdirres(struct svc_rqst
> > *rqstp, struct xdr_stream *xdr)
> > return false;
> > switch (resp->status) {
> > case nfs_ok:
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >fh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->fh))
> > return false;
> > if (!svcxdr_encode_cookieverf3(xdr, resp->verf))
> > return false;
> > @@ -1044,7 +1068,8 @@ nfs3svc_encode_readdirres(struct svc_rqst
> > *rqstp, struct xdr_stream *xdr)
> > return false;
> > break;
> > default:
> > - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp-
> > >fh))
> > + if
> > (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> > +
> > &resp->fh))
> > return false;
> > }
> >
> > @@ -1188,7 +1213,7 @@ svcxdr_encode_entry3_plus(struct
> > nfsd3_readdirres *resp, const char *name,
> > if (compose_entry_fh(resp, fhp, name, namlen, ino) !=
> > nfs_ok)
> > goto out_noattrs;
> >
> > - if (!svcxdr_encode_post_op_attr(resp->rqstp, xdr, fhp))
> > + if (!svcxdr_encode_post_op_attr_opportunistic(resp->rqstp,
> > xdr, fhp))
> > goto out;
> > if (!svcxdr_encode_post_op_fh3(xdr, fhp))
> > goto out;
> > --
> > 2.33.1
> >
>
> --
> Chuck Lever
>
>
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
