On May 21, 2023, at 9:24 PM, NeilBrown [off-list ref] wrote:

On Fri, 19 May 2023, 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;
+

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@suse.de <mailto:neilb@suse.de>>" ?

Thanks,
NeilBrown

quoted

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