> On May 19, 2023, at 9:26 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Fri, 2023-05-19 at 13:08 +0000, Trond Myklebust wrote:
>> On Fri, 2023-05-19 at 07:17 -0400, 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;
>>> +
>>> 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);
>>
>> Unfortunately, I did recently find a corner case where this behaviour
>> will break the Linux NFSv3 client. In the case where READ sometimes
>> returns post-op attributes, and sometimes not, we can end up triggering
>> the 'out_overflow' in xdr_get_next_encode_buffer(), resulting in an EIO
>> error.
>>
>> The problem is ultimately due to the attempt by the client to align the
>> pages to where it expects the READ reply to occur. When the behaviour
>> is unpredictable, then it sometimes ends up allocating too little
>> memory for the attributes, and ends up getting confused.
>>
>> This bug does need to be fixed in the client, but just a warning that
>> the above server patch would be capable of triggering it.
>>
>
> Thanks for the heads up. This is not a critical issue, so I'm OK with
> delaying this change if it's going to cause undue pain in the field. We
> could also consider providing a module option or something in the
> meantime, to give people a workaround if they hit it.
>
> OTOH, this should only rarely happen. getattr doesn't often fail unless
> you're exporting something like NFS or Ceph and someone does something
> nefarious on the backend server/cluster.
Right, we expect that the fh_getattr() operation will fail only very
rarely, usually due to a file getting removed or renamed over at the
wrong instant.
If you can provide a fix for the client for v6.5, IMO it would be a
low risk to apply both this patch and your fix at the same time.
--
Chuck Lever
