> On Dec 19, 2021, at 4:09 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> 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.
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@xxxxxxxxxxxxxxx
>
>
--
Chuck Lever
