On May 23, 2023, at 6:03 PM, NeilBrown [off-list ref] wrote:

On Tue, 23 May 2023, Chuck Lever III wrote:

quoted

quoted

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.

That leaves the curious reader, who isn't completely familiar with the
code, wondering what "0" would mean.
It's not a big deal, but it looked odd so I thought I would mention it.

Code readability is always on point.

The return values are a feature of all the xdr_stream_encode_* helpers.
For _item_absent() in particular, we could go with "!= XDR_UNIT" but
that's more verbose and still not especially more clear.

Probably the one spot that is confusing is the "_item_absent() > 0"
call site. That's meant to mean "true if it worked, false if not".
There was again no real better alternative.

quoted

quoted

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.

quoted

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.

quoted

Patch itself looks good.

May I add "Reviewed-by: Neil Brown <neilb@suse.de <mailto:neilb@suse.de>>" ?

Yes please. (though maybe without the "mailto:" :-)

Thanks.

I typed the address correctly, but oddly, Mail.app helpfully added the
mailto: tag when it sent the mail. Ho hum.


--
Chuck Lever