> 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.
> 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>>" ?
> Thanks,
> NeilBrown
>
>
>> if (v4)
>> fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
>>
>> @@ -660,15 +653,10 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
>> printk("nfsd: inode locked twice during operation.\n");
>>
>> err = fh_getattr(fhp, &fhp->fh_post_attr);
>> - if (err) {
>> - fhp->fh_post_saved = false;
>> - fhp->fh_post_attr.ctime = inode->i_ctime;
>> - if (v4 && IS_I_VERSION(inode)) {
>> - fhp->fh_post_attr.change_cookie = inode_query_iversion(inode);
>> - fhp->fh_post_attr.result_mask |= STATX_CHANGE_COOKIE;
>> - }
>> - } else
>> - fhp->fh_post_saved = true;
>> + if (err)
>> + return;
>> +
>> + fhp->fh_post_saved = true;
>> if (v4)
>> fhp->fh_post_change =
>> nfsd4_change_attribute(&fhp->fh_post_attr, inode);
>> --
>> 2.40.1
--
Chuck Lever
