> On Dec 20, 2021, at 1:38 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> 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@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().
>
> 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
