On Fri, 2023-05-19 at 13:08 +0000, Trond Myklebust wrote:

On Fri, 2023-05-19 at 07:17 -0400, Jeff Layton wrote:

quoted

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@kernel.org>
---
 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.

-- 
Jeff Layton [off-list ref]