| Message ID | 20211219013803.324724-5-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Assorted patches for knfsd | expand |
> 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. > /* > * 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
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. 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 > > >
> 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(). >>> /* >>> * 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@hammerspace.com > > -- Chuck Lever
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.
> On Dec 20, 2021, at 1:38 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > 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. Does it? Your patch does this: 418 static bool 419 __svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct xdr_stream *xdr, 420 const struct svc_fh *fhp, bool force) 421 { ... 436 if (nfsd_getattr(&path, &stat, force) != nfs_ok) 437 goto no_post_op_attrs; ... 461 bool 462 svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct xdr_stream *xdr, 463 const struct svc_fh *fhp) 464 { 465 return __svcxdr_encode_post_op_attr(rqstp, xdr, fhp, true); 466 } 467 468 static bool 469 svcxdr_encode_post_op_attr_opportunistic(struct svc_rqst *rqstp, 470 struct xdr_stream *xdr, 471 const struct svc_fh *fhp) 472 { 473 return __svcxdr_encode_post_op_attr(rqstp, xdr, fhp, !fhp->fh_no_wcc); 474 } ... 863 bool 864 nfs3svc_encode_lookupres(struct svc_rqst *rqstp, struct xdr_stream *xdr) 865 { ... 871 case nfs_ok: 872 if (!svcxdr_encode_nfs_fh3(xdr, &resp->fh)) 873 return false; 874 if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh)) 875 return false; 876 if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr, 877 &resp->dirfh)) 878 return false; 879 break; So for resp->fh, this is equivalent to resp->fh.fh_no_wcc = false and for resp->dirfh, this is equivalent to passing in resp->dirfh.fh_no_wcc unchanged. I don't see how that's different from what my suggestion does -- it forces WCC for the looked-up object, and leaves WCC for the parent directory optional. -- Chuck Lever
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); +} + /* * 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;