From: Trond Myklebust <[email protected]>
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 <[email protected]>
Signed-off-by: Lance Shelton <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
---
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);
+}
+
/*
* 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
From: Trond Myklebust <[email protected]>
Allow knfsd to request weak cache consistency attributes on files that
have delegations and/or have up to date attribute caches by propagating
the information to NFS that the attributes being requested are optional.
Signed-off-by: Trond Myklebust <[email protected]>
Signed-off-by: Lance Shelton <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/export.c | 24 ++++++++++++++++++++++++
fs/nfsd/nfs3xdr.c | 8 ++++++--
fs/nfsd/nfs4xdr.c | 6 +++---
fs/nfsd/vfs.c | 14 ++++++++++++++
fs/nfsd/vfs.h | 5 +++--
include/linux/exportfs.h | 3 +++
6 files changed, 53 insertions(+), 7 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 6adfc40722fa..df6e29796494 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -420,6 +420,9 @@ __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;
/*
@@ -427,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 (!force || !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)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 5a93a5db4fb0..8026925c121f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2862,9 +2862,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 0faa3839ea6c..eb9818432149 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -2411,3 +2411,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;
+}
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
> On Dec 18, 2021, at 8:37 PM, [email protected] wrote:
>
> From: Trond Myklebust <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Lance Shelton <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> 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
On Sun, 2021-12-19 at 20:10 +0000, Chuck Lever III wrote:
>
>
> > On Dec 18, 2021, at 8:37 PM, [email protected] wrote:
> >
> > From: Trond Myklebust <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Lance Shelton <[email protected]>
> > Signed-off-by: Trond Myklebust <[email protected]>
> > ---
> > 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
[email protected]
> On Dec 19, 2021, at 4:09 PM, Trond Myklebust <[email protected]> wrote:
>
> On Sun, 2021-12-19 at 20:10 +0000, Chuck Lever III wrote:
>>
>>
>>> On Dec 18, 2021, at 8:37 PM, [email protected] wrote:
>>>
>>> From: Trond Myklebust <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Lance Shelton <[email protected]>
>>> Signed-off-by: Trond Myklebust <[email protected]>
>>> ---
>>> 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.
That suggests that a Fixes: tag is appropriate. Can you recommend one?
> 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.
I get all that, but I don't see how this is cumbersome at all:
82 static __be32
83 nfsd3_proc_lookup(struct svc_rqst *rqstp)
84 {
...
96 resp->status = nfsd_lookup(rqstp, &resp->dirfh,
97 argp->name, argp->len,
98 &resp->fh);
+ resp->fh.fh_no_wcc = false;
99 return rpc_success;
100 }
Then in 5/10, pass !fhp->fh_no_wcc to nfsd_getattr().
>>> /*
>>> * 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
> [email protected]
>
>
--
Chuck Lever
On Mon, 2021-12-20 at 16:02 +0000, Chuck Lever III wrote:
>
>
> > On Dec 19, 2021, at 4:09 PM, Trond Myklebust
> > <[email protected]> wrote:
> >
> > On Sun, 2021-12-19 at 20:10 +0000, Chuck Lever III wrote:
> > >
> > >
> > > > On Dec 18, 2021, at 8:37 PM, [email protected] wrote:
> > > >
> > > > From: Trond Myklebust <[email protected]>
> > > >
> > > > 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
> > > > <[email protected]>
> > > > Signed-off-by: Lance Shelton <[email protected]>
> > > > Signed-off-by: Trond Myklebust
> > > > <[email protected]>
> > > > ---
> > > > 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.
>
> That suggests that a Fixes: tag is appropriate. Can you recommend
> one?
>
>
> > 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.
>
> I get all that, but I don't see how this is cumbersome at all:
>
> 82 static __be32
> 83 nfsd3_proc_lookup(struct svc_rqst *rqstp)
> 84 {
> ...
> 96 resp->status = nfsd_lookup(rqstp, &resp->dirfh,
> 97 argp->name, argp->len,
> 98 &resp->fh);
> + resp->fh.fh_no_wcc = false;
> 99 return rpc_success;
> 100 }
>
> Then in 5/10, pass !fhp->fh_no_wcc to nfsd_getattr().
>
>
That's not equivalent. That will force the NFS client to retrieve the
lookup object attributes AND the directory attributes.
As I said above, the latter is optional. The former is not.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
> On Dec 20, 2021, at 1:38 PM, Trond Myklebust <[email protected]> wrote:
>
> On Mon, 2021-12-20 at 16:02 +0000, Chuck Lever III wrote:
>>
>>
>>> On Dec 19, 2021, at 4:09 PM, Trond Myklebust
>>> <[email protected]> wrote:
>>>
>>> On Sun, 2021-12-19 at 20:10 +0000, Chuck Lever III wrote:
>>>>
>>>>
>>>>> On Dec 18, 2021, at 8:37 PM, [email protected] wrote:
>>>>>
>>>>> From: Trond Myklebust <[email protected]>
>>>>>
>>>>> 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
>>>>> <[email protected]>
>>>>> Signed-off-by: Lance Shelton <[email protected]>
>>>>> Signed-off-by: Trond Myklebust
>>>>> <[email protected]>
>>>>> ---
>>>>> 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.
>>
>> That suggests that a Fixes: tag is appropriate. Can you recommend
>> one?
>>
>>
>>> 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.
>>
>> I get all that, but I don't see how this is cumbersome at all:
>>
>> 82 static __be32
>> 83 nfsd3_proc_lookup(struct svc_rqst *rqstp)
>> 84 {
>> ...
>> 96 resp->status = nfsd_lookup(rqstp, &resp->dirfh,
>> 97 argp->name, argp->len,
>> 98 &resp->fh);
>> + resp->fh.fh_no_wcc = false;
>> 99 return rpc_success;
>> 100 }
>>
>> Then in 5/10, pass !fhp->fh_no_wcc to nfsd_getattr().
>
> That's not equivalent. That will force the NFS client to retrieve the
> lookup object attributes AND the directory attributes.
Does it? Your patch does this:
418 static bool
419 __svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
420 const struct svc_fh *fhp, bool force)
421 {
...
436 if (nfsd_getattr(&path, &stat, force) != nfs_ok)
437 goto no_post_op_attrs;
...
461 bool
462 svcxdr_encode_post_op_attr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
463 const struct svc_fh *fhp)
464 {
465 return __svcxdr_encode_post_op_attr(rqstp, xdr, fhp, true);
466 }
467
468 static bool
469 svcxdr_encode_post_op_attr_opportunistic(struct svc_rqst *rqstp,
470 struct xdr_stream *xdr,
471 const struct svc_fh *fhp)
472 {
473 return __svcxdr_encode_post_op_attr(rqstp, xdr, fhp, !fhp->fh_no_wcc);
474 }
...
863 bool
864 nfs3svc_encode_lookupres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
865 {
...
871 case nfs_ok:
872 if (!svcxdr_encode_nfs_fh3(xdr, &resp->fh))
873 return false;
874 if (!svcxdr_encode_post_op_attr(rqstp, xdr, &resp->fh))
875 return false;
876 if (!svcxdr_encode_post_op_attr_opportunistic(rqstp, xdr,
877 &resp->dirfh))
878 return false;
879 break;
So for resp->fh, this is equivalent to resp->fh.fh_no_wcc = false
and for resp->dirfh, this is equivalent to passing in
resp->dirfh.fh_no_wcc unchanged.
I don't see how that's different from what my suggestion does --
it forces WCC for the looked-up object, and leaves WCC for the
parent directory optional.
--
Chuck Lever
Hi,
I was interested in testing these patches with our re-export
workloads. However, it looks like this specific patch and the previous
one (nfsd: Distinguish between required and optional NFSv3 post-op
attributes) are breaking re-exports in such a way as to cause
applications to randomly crash out with sigbus errors.
At a guess, loading applications and memory mapping data files are
particularly good at triggering this. I can see no errors logged on
either the re-export server or the eventual client (e.g. stale
filehandles). We mostly re-export NFSv4.2 servers to NFSv3 clients but
we do also have a few NFSv3 servers re-exported as NFSv3 too
(Netapps).
This only happens with patches 4 + 5 and vanilla 5.16-rc6 does not
have any issues. I haven't had a chance to dig much deeper yet, but
thought I'd flag it anyway.
Daire
On Sun, 19 Dec 2021 at 01:44, <[email protected]> wrote:
>
> From: Trond Myklebust <[email protected]>
>
> Allow knfsd to request weak cache consistency attributes on files that
> have delegations and/or have up to date attribute caches by propagating
> the information to NFS that the attributes being requested are optional.