Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Jeff Layton" <jlayton@kernel.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] nfsd: don't provide pre/post-op attrs if fh_getattr fails
In-reply-to: <143D2797-B071-43FF-AA85-E4C4A7218691@oracle.com>
References: <20230519111723.20612-1-jlayton@kernel.org>,
 <168471866230.5298.3283829268036917998@noble.neil.brown.name>,
 <143D2797-B071-43FF-AA85-E4C4A7218691@oracle.com>
Date:   Wed, 24 May 2023 08:03:48 +1000
Message-id: <168487942834.21808.17784288972950301860@noble.neil.brown.name>
Precedence: bulk

On Tue, 23 May 2023, Chuck Lever III wrote:
> 
> > On May 21, 2023, at 9:24 PM, NeilBrown <neilb@suse.de> wrote:
> > 
> > On Fri, 19 May 2023, Jeff Layton wrote:
> >> 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.

> 
> 
> > 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>>" ?

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

NeilBrown