On Fri, 2023-05-19 at 07:17 -0400, 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;
> +
> 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);
Unfortunately, I did recently find a corner case where this behaviour
will break the Linux NFSv3 client. In the case where READ sometimes
returns post-op attributes, and sometimes not, we can end up triggering
the 'out_overflow' in xdr_get_next_encode_buffer(), resulting in an EIO
error.
The problem is ultimately due to the attempt by the client to align the
pages to where it expects the READ reply to occur. When the behaviour
is unpredictable, then it sometimes ends up allocating too little
memory for the attributes, and ends up getting confused.
This bug does need to be fixed in the client, but just a warning that
the above server patch would be capable of triggering it.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
