Re: [PATCH] nfsd: don't provide pre/post-op attrs if fh_getattr fails
From: NeilBrown <hidden>
Date: 2023-05-22 23:08:15
On Fri, 19 May 2023, 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; +
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. But of course xdr_stream_encode_item_absent() can never return 0. It returns either XDR_UNIT or -EMSGSIZE. I wonder if we should be consistent in how we test for an error .... or if it it really matters. Patch itself looks good. Thanks, NeilBrown
quoted hunk ↗ jump to hunk
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