All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chuck Lever III <chuck.lever@oracle.com>
To: "trondmy@kernel.org" <trondmy@kernel.org>
Cc: Bruce Fields <bfields@redhat.com>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 04/10] nfsd: Distinguish between required and optional NFSv3 post-op attributes
Date: Sun, 19 Dec 2021 20:10:35 +0000	[thread overview]
Message-ID: <B6B9027D-D7EB-4324-9C26-9FDADCCCEFCC@oracle.com> (raw)
In-Reply-To: <20211219013803.324724-5-trondmy@kernel.org>



> On Dec 18, 2021, at 8:37 PM, trondmy@kernel.org wrote:
> 
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> 
> The fhp->fh_no_wcc flag is automatically set in nfsd_set_fh_dentry()
> when the EXPORT_OP_NOWCC flag is set. In svcxdr_encode_post_op_attr(),
> this then causes nfsd to skip returning the post-op attributes.
> 
> The problem is that some of these post-op attributes are not really
> optional. In particular, we do want LOOKUP to always return post-op
> attributes for the file that is being looked up.
> 
> The solution is therefore to explicitly label the attributes that we can
> safely opt out from, and only apply the 'fhp->fh_no_wcc' test in that
> case.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: Lance Shelton <lance.shelton@hammerspace.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
> fs/nfsd/nfs3xdr.c | 77 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 51 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index c3ac1b6aa3aa..6adfc40722fa 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -415,19 +415,9 @@ svcxdr_encode_pre_op_attr(struct xdr_stream *xdr, const struct svc_fh *fhp)
> 	return svcxdr_encode_wcc_attr(xdr, fhp);
> }
> 
> -/**
> - * svcxdr_encode_post_op_attr - Encode NFSv3 post-op attributes
> - * @rqstp: Context of a completed RPC transaction
> - * @xdr: XDR stream
> - * @fhp: File handle to encode
> - *
> - * Return values:
> - *   %false: Send buffer space was exhausted
> - *   %true: Success
> - */
> -bool
> -svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
> -			   const struct svc_fh *fhp)
> +static bool
> +__svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
> +			     const struct svc_fh *fhp, bool force)
> {
> 	struct dentry *dentry = fhp->fh_dentry;
> 	struct kstat stat;
> @@ -437,7 +427,7 @@ svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
> 	 * stale file handle. In this case, no attributes are
> 	 * returned.
> 	 */
> -	if (fhp->fh_no_wcc || !dentry || !d_really_is_positive(dentry))
> +	if (!force || !dentry || !d_really_is_positive(dentry))
> 		goto no_post_op_attrs;
> 	if (fh_getattr(fhp, &stat) != nfs_ok)
> 		goto no_post_op_attrs;
> @@ -454,6 +444,31 @@ svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
> 	return xdr_stream_encode_item_absent(xdr) > 0;
> }
> 
> +/**
> + * svcxdr_encode_post_op_attr - Encode NFSv3 post-op attributes
> + * @rqstp: Context of a completed RPC transaction
> + * @xdr: XDR stream
> + * @fhp: File handle to encode
> + *
> + * Return values:
> + *   %false: Send buffer space was exhausted
> + *   %true: Success
> + */
> +bool
> +svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
> +			   const struct svc_fh *fhp)
> +{
> +	return __svcxdr_encode_post_op_attr(rqstp, xdr, fhp, true);
> +}
> +
> +static bool
> +svcxdr_encode_post_op_attr_opportunistic(struct svc_rqst *rqstp,
> +					 struct xdr_stream *xdr,
> +					 const struct svc_fh *fhp)
> +{
> +	return __svcxdr_encode_post_op_attr(rqstp, xdr, fhp, !fhp->fh_no_wcc);
> +}
> +

Thanks for splitting this out: the "why" is much clearer.

Wouldn't it be simpler to explicitly set fh_no_wcc to false
in each of the cases where you want to ensure the server
emits WCC? And perhaps that should be done in nfs3proc.c
instead of in nfs3xdr.c.


> /*
>  * Encode weak cache consistency data
>  */
> @@ -481,7 +496,7 @@ svcxdr_encode_wcc_data(struct svc_rqst *rqstp, struct xdr_stream *xdr,
> neither:
> 	if (xdr_stream_encode_item_absent(xdr) < 0)
> 		return false;
> -	if (!svcxdr_encode_post_op_attr(rqstp, xdr, fhp))
> +	if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr, fhp))
> 		return false;
> 
> 	return true;
> @@ -854,11 +869,13 @@ nfs3svc_encode_lookupres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
> 			return false;
> 		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> 			return false;
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->dirfh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->dirfh))
> 			return false;
> 		break;
> 	default:
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->dirfh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->dirfh))
> 			return false;
> 	}
> 
> @@ -875,13 +892,15 @@ nfs3svc_encode_accessres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
> 		return false;
> 	switch (resp->status) {
> 	case nfs_ok:
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->fh))
> 			return false;
> 		if (xdr_stream_encode_u32(xdr, resp->access) < 0)
> 			return false;
> 		break;
> 	default:
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->fh))
> 			return false;
> 	}
> 
> @@ -899,7 +918,8 @@ nfs3svc_encode_readlinkres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
> 		return false;
> 	switch (resp->status) {
> 	case nfs_ok:
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->fh))
> 			return false;
> 		if (xdr_stream_encode_u32(xdr, resp->len) < 0)
> 			return false;
> @@ -908,7 +928,8 @@ nfs3svc_encode_readlinkres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
> 			return false;
> 		break;
> 	default:
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->fh))
> 			return false;
> 	}
> 
> @@ -926,7 +947,8 @@ nfs3svc_encode_readres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
> 		return false;
> 	switch (resp->status) {
> 	case nfs_ok:
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->fh))
> 			return false;
> 		if (xdr_stream_encode_u32(xdr, resp->count) < 0)
> 			return false;
> @@ -940,7 +962,8 @@ nfs3svc_encode_readres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
> 			return false;
> 		break;
> 	default:
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->fh))
> 			return false;
> 	}
> 
> @@ -1032,7 +1055,8 @@ nfs3svc_encode_readdirres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
> 		return false;
> 	switch (resp->status) {
> 	case nfs_ok:
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->fh))
> 			return false;
> 		if (!svcxdr_encode_cookieverf3(xdr, resp->verf))
> 			return false;
> @@ -1044,7 +1068,8 @@ nfs3svc_encode_readdirres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
> 			return false;
> 		break;
> 	default:
> -		if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> +		if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> +							      &resp->fh))
> 			return false;
> 	}
> 
> @@ -1188,7 +1213,7 @@ svcxdr_encode_entry3_plus(struct nfsd3_readdirres *resp, const char *name,
> 	if (compose_entry_fh(resp, fhp, name, namlen, ino) != nfs_ok)
> 		goto out_noattrs;
> 
> -	if (!svcxdr_encode_post_op_attr(resp->rqstp, xdr, fhp))
> +	if (!svcxdr_encode_post_op_attr_opportunistic(resp->rqstp, xdr, fhp))
> 		goto out;
> 	if (!svcxdr_encode_post_op_fh3(xdr, fhp))
> 		goto out;
> -- 
> 2.33.1
> 

--
Chuck Lever




  parent reply	other threads:[~2021-12-19 20:10 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-19  1:37 [PATCH v2 00/10] Assorted patches for knfsd trondmy
2021-12-19  1:37 ` [PATCH v2 01/10] nfsd: map EBADF trondmy
2021-12-19  1:37   ` [PATCH v2 02/10] nfsd: Add errno mapping for EREMOTEIO trondmy
2021-12-19  1:37     ` [PATCH v2 03/10] nfsd: Retry once in nfsd_open on an -EOPENSTALE return trondmy
2021-12-19  1:37       ` [PATCH v2 04/10] nfsd: Distinguish between required and optional NFSv3 post-op attributes trondmy
2021-12-19  1:37         ` [PATCH v2 05/10] nfs: Add export support for weak cache consistency attributes trondmy
2021-12-19  1:37           ` [PATCH v2 06/10] nfsd: NFSv3 should allow zero length writes trondmy
2021-12-19  1:38             ` [PATCH v2 07/10] nfsd: Add a tracepoint for errors in nfsd4_clone_file_range() trondmy
2021-12-19  1:38               ` [PATCH v2 08/10] nfsd: Replace use of rwsem with errseq_t trondmy
2021-12-19  1:38                 ` [PATCH v2 09/10] nfsd: allow lockd to be forcibly disabled trondmy
2021-12-19  1:38                   ` [PATCH v2 10/10] nfsd: Ignore rpcbind errors on nfsd startup trondmy
2021-12-19 18:15                     ` Chuck Lever III
2021-12-19 20:49                       ` Trond Myklebust
2021-12-20 15:51                         ` Chuck Lever III
2021-12-20 18:35                           ` Trond Myklebust
2021-12-20 19:02                             ` Chuck Lever III
2021-12-20 19:52                               ` Trond Myklebust
2021-12-20 20:12                                 ` Chuck Lever III
2021-12-19 18:34                   ` [PATCH v2 09/10] nfsd: allow lockd to be forcibly disabled Chuck Lever III
2021-12-21 18:14               ` [PATCH v2 07/10] nfsd: Add a tracepoint for errors in nfsd4_clone_file_range() Chuck Lever III
2021-12-19 20:11             ` [PATCH v2 06/10] nfsd: NFSv3 should allow zero length writes Chuck Lever III
2022-01-05 16:10           ` [PATCH v2 05/10] nfs: Add export support for weak cache consistency attributes Daire Byrne
2021-12-19 20:10         ` Chuck Lever III [this message]
2021-12-19 21:09           ` [PATCH v2 04/10] nfsd: Distinguish between required and optional NFSv3 post-op attributes Trond Myklebust
2021-12-20 16:02             ` Chuck Lever III
2021-12-20 18:38               ` Trond Myklebust
2021-12-20 19:22                 ` Chuck Lever III
2021-12-21 18:10 ` [PATCH v2 00/10] Assorted patches for knfsd Chuck Lever III

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=B6B9027D-D7EB-4324-9C26-9FDADCCCEFCC@oracle.com \
    --to=chuck.lever@oracle.com \
    --cc=bfields@redhat.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=trondmy@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.