> On May 23, 2023, at 6:03 PM, NeilBrown <neilb@xxxxxxx> wrote:
>
> On Tue, 23 May 2023, Chuck Lever III wrote:
>>
>>> On May 21, 2023, at 9:24 PM, NeilBrown <neilb@xxxxxxx> wrote:
>>>
>>> On Fri, 19 May 2023, Jeff Layton wrote:
>>>> nfsd calls fh_getattr to get the latest inode attrs for pre/post-op
>>>> info. In the event that fh_getattr fails, it resorts to scraping cached
>>>> values out of the inode directly.
>>>>
>>>> Since these attributes are optional, we can just skip providing them
>>>> altogether when this happens.
>>>>
>>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>>> ---
>>>> fs/nfsd/nfsfh.c | 26 +++++++-------------------
>>>> 1 file changed, 7 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
>>>> index ccd8485fee04..e8e13ae72e3c 100644
>>>> --- a/fs/nfsd/nfsfh.c
>>>> +++ b/fs/nfsd/nfsfh.c
>>>> @@ -623,16 +623,9 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
>>>>
>>>> inode = d_inode(fhp->fh_dentry);
>>>> err = fh_getattr(fhp, &stat);
>>>> - if (err) {
>>>> - /* Grab the times from inode anyway */
>>>> - stat.mtime = inode->i_mtime;
>>>> - stat.ctime = inode->i_ctime;
>>>> - stat.size = inode->i_size;
>>>> - if (v4 && IS_I_VERSION(inode)) {
>>>> - stat.change_cookie = inode_query_iversion(inode);
>>>> - stat.result_mask |= STATX_CHANGE_COOKIE;
>>>> - }
>>>> - }
>>>> + if (err)
>>>> + return;
>>>> +
>>>
>>> I wondered if this might exercise error paths which had not previously
>>> been tested. Before this change fh_pre_saved is always set, now it is
>>> not.
>>>
>>> The code looks OK, but I was amused by xdr_stream_encode_item_absent().
>>> Various places in the code test for "< 0" or "> 0" which seems to
>>> suggest that "0" is not being handled consistently.
>>
>> You can read those as "returns positive" and "returns negative" tests.
>
> That leaves the curious reader, who isn't completely familiar with the
> code, wondering what "0" would mean.
> It's not a big deal, but it looked odd so I thought I would mention it.
Code readability is always on point.
The return values are a feature of all the xdr_stream_encode_* helpers.
For _item_absent() in particular, we could go with "!= XDR_UNIT" but
that's more verbose and still not especially more clear.
Probably the one spot that is confusing is the "_item_absent() > 0"
call site. That's meant to mean "true if it worked, false if not".
There was again no real better alternative.
>>> But of course xdr_stream_encode_item_absent() can never return 0. It
>>> returns either XDR_UNIT or -EMSGSIZE.
>>
>> I don't see any tests for it returning exactly zero.
>>
>>
>>> I wonder if we should be consistent in how we test for an error .... or
>>> if it it really matters.
>>
>> The xdr_stream_encode_* functions conventionally return a negative errno
>> or a positive number of bytes encoded. The "< 0" and "> 0" tests convert
>> that return value into a boolean.
>>
>> I reviewed the call sites just now and do not see an evident problem.
>>
>>
>>> Patch itself looks good.
>>
>> May I add "Reviewed-by: Neil Brown <neilb@xxxxxxx <mailto:neilb@xxxxxxx>>" ?
>
> Yes please. (though maybe without the "mailto:"; :-)
Thanks.
I typed the address correctly, but oddly, Mail.app helpfully added the
mailto: tag when it sent the mail. Ho hum.
--
Chuck Lever
