Thread (8 messages) 8 messages, 4 authors, 2023-05-23

Re: [PATCH] nfsd: don't provide pre/post-op attrs if fh_getattr fails

From: Trond Myklebust <hidden>
Date: 2023-05-19 13:08:11

On Fri, 2023-05-19 at 07:17 -0400, Jeff Layton wrote:
quoted hunk ↗ jump to hunk
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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help