From: Trond Myklebust <trondmy@hammerspace.com>
To: "chuck.lever@oracle.com" <chuck.lever@oracle.com>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
"bfields@redhat.com" <bfields@redhat.com>
Subject: Re: [PATCH v2 04/10] nfsd: Distinguish between required and optional NFSv3 post-op attributes
Date: Sun, 19 Dec 2021 21:09:07 +0000 [thread overview]
Message-ID: <6adbcc44f48cdb0a6a96ed488d5a1959c09e13cd.camel@hammerspace.com> (raw)
In-Reply-To: <B6B9027D-D7EB-4324-9C26-9FDADCCCEFCC@oracle.com>
On Sun, 2021-12-19 at 20:10 +0000, Chuck Lever III wrote:
>
>
> > 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.
>
It can't be done in nfs3proc.c, and toggling the value of fh_no_wcc is
a lot more cumbersome than this approach.
The current code is broken for NFSv3 exports because it is unable to
distinguish between what is and isn't mandatory to return for in the
same NFS operation. That's the problem this patch fixes.
LOOKUP has to return the attributes for the object being looked up in
order to be useful. If the attributes are not up to date then we should
ask the NFS client that is being re-exported to go to the server to
revalidate its attributes.
The same is not true of the directory post-op attributes. LOOKUP does
not even change the contents of the directory, and so while it is
beneficial to have the NFS client return those attributes if they are
up to date, forcing it to go to the server to retrieve them is less
than optimal for system performance.
>
> > /*
> > * 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
>
>
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
next prev parent reply other threads:[~2021-12-19 21:09 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 ` [PATCH v2 04/10] nfsd: Distinguish between required and optional NFSv3 post-op attributes Chuck Lever III
2021-12-19 21:09 ` Trond Myklebust [this message]
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=6adbcc44f48cdb0a6a96ed488d5a1959c09e13cd.camel@hammerspace.com \
--to=trondmy@hammerspace.com \
--cc=bfields@redhat.com \
--cc=chuck.lever@oracle.com \
--cc=linux-nfs@vger.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox