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:     "Jeff Layton" <jlayton@kernel.org>
Cc:     chuck.lever@oracle.com, linux-nfs@vger.kernel.org
Subject: Re: [PATCH] nfsd: don't provide pre/post-op attrs if fh_getattr fails
In-reply-to: <20230519111723.20612-1-jlayton@kernel.org>
References: <20230519111723.20612-1-jlayton@kernel.org>
Date:   Mon, 22 May 2023 11:24:22 +1000
Message-id: <168471866230.5298.3283829268036917998@noble.neil.brown.name>
Precedence: bulk

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.
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
> 
>