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 4/9] nfs: Add export support for weak cache consistency attributes
Date: Sat, 18 Dec 2021 21:16:47 +0000 [thread overview]
Message-ID: <01D9E7C4-25F2-4C99-A4FE-2741A5722F58@oracle.com> (raw)
In-Reply-To: <20211217215046.40316-5-trondmy@kernel.org>
> On Dec 17, 2021, at 4:50 PM, trondmy@kernel.org wrote:
>
> From: Trond Myklebust <trond.myklebust@primarydata.com>
>
> Allow knfsd to request weak cache consistency attributes on files that
> have delegations and/or have up to date attribute caches.
>
> 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>
This one does not apply to v5.16-rc. Please rebase this series
and resend.
More below:
> ---
> fs/nfs/export.c | 24 ++++++++++++
> fs/nfsd/nfs3xdr.c | 83 +++++++++++++++++++++++++++-------------
> fs/nfsd/nfs4xdr.c | 6 +--
> fs/nfsd/vfs.c | 14 +++++++
> fs/nfsd/vfs.h | 5 ++-
> include/linux/exportfs.h | 3 ++
> 6 files changed, 103 insertions(+), 32 deletions(-)
>
> diff --git a/fs/nfs/export.c b/fs/nfs/export.c
> index 171c424cb6d5..967f8902c49b 100644
> --- a/fs/nfs/export.c
> +++ b/fs/nfs/export.c
> @@ -151,10 +151,34 @@ static u64 nfs_fetch_iversion(struct inode *inode)
> return inode_peek_iversion_raw(inode);
> }
>
> +static int nfs_exp_getattr(struct path *path, struct kstat *stat, bool force)
> +{
> + const unsigned long check_valid =
> + NFS_INO_INVALID_CHANGE | NFS_INO_INVALID_ATIME |
> + NFS_INO_INVALID_CTIME | NFS_INO_INVALID_MTIME |
> + NFS_INO_INVALID_SIZE | /* NFS_INO_INVALID_BLOCKS | */
> + NFS_INO_INVALID_OTHER | NFS_INO_INVALID_NLINK;
> + struct inode *inode = d_inode(path->dentry);
> + int flags = force ? AT_STATX_SYNC_AS_STAT : AT_STATX_DONT_SYNC;
> + int ret, ret2 = 0;
> +
> + if (!force && nfs_check_cache_invalid(inode, check_valid))
> + ret2 = -EAGAIN;
> + ret = vfs_getattr(path, stat, STATX_BASIC_STATS & ~STATX_BLOCKS, flags);
> + if (ret < 0)
> + return ret;
> + stat->blocks = nfs_calc_block_size(stat->size);
> + if (S_ISDIR(inode->i_mode))
> + stat->blksize = NFS_SERVER(inode)->dtsize;
> + stat->result_mask |= STATX_BLOCKS;
> + return ret2;
> +}
> +
> const struct export_operations nfs_export_ops = {
> .encode_fh = nfs_encode_fh,
> .fh_to_dentry = nfs_fh_to_dentry,
> .get_parent = nfs_get_parent,
> + .getattr = nfs_exp_getattr,
> .fetch_iversion = nfs_fetch_iversion,
> .flags = EXPORT_OP_NOWCC|EXPORT_OP_NOSUBTREECHK|
> EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS|
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index 0a5ebc52e6a9..81b6cf228517 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -415,21 +415,14 @@ 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 path path = {
> + .dentry = dentry,
> + };
> struct kstat stat;
>
> /*
> @@ -437,9 +430,10 @@ 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 (!dentry || !d_really_is_positive(dentry))
> goto no_post_op_attrs;
> - if (fh_getattr(fhp, &stat) != nfs_ok)
> + path.mnt = fhp->fh_export->ex_path.mnt;
> + if (nfsd_getattr(&path, &stat, force) != nfs_ok)
> goto no_post_op_attrs;
>
> if (xdr_stream_encode_item_present(xdr) < 0)
> @@ -454,6 +448,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);
> +}
I don't quite understand the need for splitting encode_post_op_attr.
At the least, more commentary in the patch description would help.
For now, I will study this and get back to you.
> +
> /*
> * Encode weak cache consistency data
> */
> @@ -481,7 +500,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;
> @@ -879,11 +898,13 @@ int nfs3svc_encode_lookupres(struct svc_rqst *rqstp, __be32 *p)
> return 0;
> if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> return 0;
> - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->dirfh))
> + if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> + &resp->dirfh))
> return 0;
> break;
> default:
> - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->dirfh))
> + if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> + &resp->dirfh))
> return 0;
> }
>
> @@ -901,13 +922,15 @@ nfs3svc_encode_accessres(struct svc_rqst *rqstp, __be32 *p)
> return 0;
> 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 0;
> if (xdr_stream_encode_u32(xdr, resp->access) < 0)
> return 0;
> break;
> default:
> - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> + if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> + &resp->fh))
> return 0;
> }
>
> @@ -926,7 +949,8 @@ nfs3svc_encode_readlinkres(struct svc_rqst *rqstp, __be32 *p)
> return 0;
> 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 0;
> if (xdr_stream_encode_u32(xdr, resp->len) < 0)
> return 0;
> @@ -935,7 +959,8 @@ nfs3svc_encode_readlinkres(struct svc_rqst *rqstp, __be32 *p)
> return 0;
> break;
> default:
> - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> + if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> + &resp->fh))
> return 0;
> }
>
> @@ -954,7 +979,8 @@ nfs3svc_encode_readres(struct svc_rqst *rqstp, __be32 *p)
> return 0;
> 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 0;
> if (xdr_stream_encode_u32(xdr, resp->count) < 0)
> return 0;
> @@ -968,7 +994,8 @@ nfs3svc_encode_readres(struct svc_rqst *rqstp, __be32 *p)
> return 0;
> break;
> default:
> - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> + if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> + &resp->fh))
> return 0;
> }
>
> @@ -1065,7 +1092,8 @@ nfs3svc_encode_readdirres(struct svc_rqst *rqstp, __be32 *p)
> return 0;
> 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 0;
> if (!svcxdr_encode_cookieverf3(xdr, resp->verf))
> return 0;
> @@ -1077,7 +1105,8 @@ nfs3svc_encode_readdirres(struct svc_rqst *rqstp, __be32 *p)
> return 0;
> break;
> default:
> - if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
> + if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
> + &resp->fh))
> return 0;
> }
>
> @@ -1221,7 +1250,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;
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 7abeccb975b2..e14af0383b70 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2865,9 +2865,9 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> goto out;
> }
>
> - err = vfs_getattr(&path, &stat, STATX_BASIC_STATS, AT_STATX_SYNC_AS_STAT);
> - if (err)
> - goto out_nfserr;
> + status = nfsd_getattr(&path, &stat, true);
> + if (status)
> + goto out;
> if ((bmval0 & (FATTR4_WORD0_FILES_AVAIL | FATTR4_WORD0_FILES_FREE |
> FATTR4_WORD0_FILES_TOTAL | FATTR4_WORD0_MAXNAME)) ||
> (bmval1 & (FATTR4_WORD1_SPACE_AVAIL | FATTR4_WORD1_SPACE_FREE |
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index a90cc08211a3..4d57befdac6e 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -2412,3 +2412,17 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
>
> return err? nfserrno(err) : 0;
> }
> +
> +
> +__be32
> +nfsd_getattr(struct path *p, struct kstat *stat, bool force)
> +{
> + const struct export_operations *ops = p->dentry->d_sb->s_export_op;
> + int err;
> +
> + if (ops->getattr)
> + err = ops->getattr(p, stat, force);
> + else
> + err = vfs_getattr(p, stat, STATX_BASIC_STATS, AT_STATX_SYNC_AS_STAT);
> + return err ? nfserrno(err) : 0;
> +}
Please add nfsd_getattr() in a separate patch ("Refactor:").
And please include a kerneldoc comment for it.
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index b21b76e6b9a8..6edae1b9a96e 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -132,6 +132,8 @@ __be32 nfsd_statfs(struct svc_rqst *, struct svc_fh *,
> __be32 nfsd_permission(struct svc_rqst *, struct svc_export *,
> struct dentry *, int);
>
> +__be32 nfsd_getattr(struct path *p, struct kstat *, bool);
> +
> static inline int fh_want_write(struct svc_fh *fh)
> {
> int ret;
> @@ -156,8 +158,7 @@ static inline __be32 fh_getattr(const struct svc_fh *fh, struct kstat *stat)
> {
> struct path p = {.mnt = fh->fh_export->ex_path.mnt,
> .dentry = fh->fh_dentry};
> - return nfserrno(vfs_getattr(&p, stat, STATX_BASIC_STATS,
> - AT_STATX_SYNC_AS_STAT));
> + return nfsd_getattr(&p, stat, true);
> }
>
> static inline int nfsd_create_is_exclusive(int createmode)
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 3260fe714846..58f36022787e 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -10,6 +10,8 @@ struct inode;
> struct iomap;
> struct super_block;
> struct vfsmount;
> +struct path;
> +struct kstat;
>
> /* limit the handle size to NFSv4 handle size now */
> #define MAX_HANDLE_SZ 128
> @@ -224,6 +226,7 @@ struct export_operations {
> #define EXPORT_OP_SYNC_LOCKS (0x20) /* Filesystem can't do
> asychronous blocking locks */
> unsigned long flags;
> + int (*getattr)(struct path *, struct kstat *, bool);
> };
>
> extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> --
> 2.33.1
>
--
Chuck Lever
prev parent reply other threads:[~2021-12-18 21:16 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-17 21:50 [PATCH 0/9] Assorted patches for knfsd trondmy
2021-12-17 21:50 ` [PATCH 1/9] nfsd: map EBADF trondmy
2021-12-17 21:50 ` [PATCH 2/9] nfsd: Add errno mapping for EREMOTEIO trondmy
2021-12-17 21:50 ` [PATCH 3/9] nfsd: Retry once in nfsd_open on an -EOPENSTALE return trondmy
2021-12-17 21:50 ` [PATCH 4/9] nfs: Add export support for weak cache consistency attributes trondmy
2021-12-17 21:50 ` [PATCH 5/9] nfsd: NFSv3 should allow zero length writes trondmy
2021-12-17 21:50 ` [PATCH 6/9] nfsd: Add a tracepoint for errors in nfsd4_clone_file_range() trondmy
2021-12-17 21:50 ` [PATCH 7/9] nfsd: Replace use of rwsem with errseq_t trondmy
2021-12-17 21:50 ` [PATCH 8/9] nfsd: allow lockd to be forcibly disabled trondmy
2021-12-17 21:50 ` [PATCH 9/9] nfsd: Ignore rpcbind errors on nfsd startup trondmy
2021-12-18 18:07 ` [PATCH 8/9] nfsd: allow lockd to be forcibly disabled Chuck Lever III
2021-12-19 22:21 ` Bruce Fields
2021-12-17 22:23 ` [PATCH 5/9] nfsd: NFSv3 should allow zero length writes Bruce Fields
2021-12-18 18:41 ` Chuck Lever III
2021-12-19 22:25 ` Bruce Fields
2021-12-18 21:16 ` Chuck Lever III [this message]
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=01D9E7C4-25F2-4C99-A4FE-2741A5722F58@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.