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@xxxxxxxxxx>
> ---
> 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
> 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
>
>
