2020-10-19 10:09:10

by Dai Ngo

Subject: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
build errors and some configs with NFSD=m to get NFS4ERR_STALE
error when doing inter server copy.

Added ops table in nfs_common for knfsd to access NFS client modules.

Fixes: 3ac3711adb88 ("NFSD: Fix NFS server build errors")
Signed-off-by: Dai Ngo <[email protected]>
---
changes from v2: fix 0-day build issues.
changes from v3: reacted to Bruce's comments, removed paranoid error checking.
---
fs/nfs/nfs4file.c | 38 ++++++++++++++++----
fs/nfs/nfs4super.c | 5 +++
fs/nfs/super.c | 17 +++++++++
fs/nfs_common/Makefile | 1 +
fs/nfs_common/nfs_ssc.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++
fs/nfsd/Kconfig | 2 +-
fs/nfsd/nfs4proc.c | 3 +-
include/linux/nfs_ssc.h | 67 +++++++++++++++++++++++++++++++++++
8 files changed, 219 insertions(+), 8 deletions(-)
create mode 100644 fs/nfs_common/nfs_ssc.c
create mode 100644 include/linux/nfs_ssc.h

diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index fdfc77486ace..984938024011 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -9,6 +9,7 @@
#include <linux/falloc.h>
#include <linux/mount.h>
#include <linux/nfs_fs.h>
+#include <linux/nfs_ssc.h>
#include "delegation.h"
#include "internal.h"
#include "iostat.h"
@@ -314,9 +315,8 @@ static loff_t nfs42_remap_file_range(struct file *src_file, loff_t src_off,
static int read_name_gen = 1;
#define SSC_READ_NAME_BODY "ssc_read_%d"

-struct file *
-nfs42_ssc_open(struct vfsmount *ss_mnt, struct nfs_fh *src_fh,
- nfs4_stateid *stateid)
+static struct file *__nfs42_ssc_open(struct vfsmount *ss_mnt,
+ struct nfs_fh *src_fh, nfs4_stateid *stateid)
{
struct nfs_fattr fattr;
struct file *filep, *res;
@@ -398,14 +398,40 @@ struct file *
fput(filep);
goto out_free_name;
}
-EXPORT_SYMBOL_GPL(nfs42_ssc_open);
-void nfs42_ssc_close(struct file *filep)
+
+static void __nfs42_ssc_close(struct file *filep)
{
struct nfs_open_context *ctx = nfs_file_open_context(filep);

ctx->state->flags = 0;
}
-EXPORT_SYMBOL_GPL(nfs42_ssc_close);
+
+static const struct nfs4_ssc_client_ops nfs4_ssc_clnt_ops_tbl = {
+ .sco_open = __nfs42_ssc_open,
+ .sco_close = __nfs42_ssc_close,
+};
+
+/**
+ * nfs42_ssc_register_ops - Wrapper to register NFS_V4 ops in nfs_common
+ *
+ * Return values:
+ * None
+ */
+void nfs42_ssc_register_ops(void)
+{
+ nfs42_ssc_register(&nfs4_ssc_clnt_ops_tbl);
+}
+
+/**
+ * nfs42_ssc_unregister_ops - wrapper to un-register NFS_V4 ops in nfs_common
+ *
+ * Return values:
+ * None.
+ */
+void nfs42_ssc_unregister_ops(void)
+{
+ nfs42_ssc_unregister(&nfs4_ssc_clnt_ops_tbl);
+}
#endif /* CONFIG_NFS_V4_2 */

const struct file_operations nfs4_file_operations = {
diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
index 0c1ab846b83d..93f5c1678ec2 100644
--- a/fs/nfs/nfs4super.c
+++ b/fs/nfs/nfs4super.c
@@ -7,6 +7,7 @@
#include <linux/mount.h>
#include <linux/nfs4_mount.h>
#include <linux/nfs_fs.h>
+#include <linux/nfs_ssc.h>
#include "delegation.h"
#include "internal.h"
#include "nfs4_fs.h"
@@ -279,6 +280,9 @@ static int __init init_nfs_v4(void)
if (err)
goto out2;

+#ifdef CONFIG_NFS_V4_2
+ nfs42_ssc_register_ops();
+#endif
register_nfs_version(&nfs_v4);
return 0;
out2:
@@ -297,6 +301,7 @@ static void __exit exit_nfs_v4(void)
unregister_nfs_version(&nfs_v4);
#ifdef CONFIG_NFS_V4_2
nfs4_xattr_cache_exit();
+ nfs42_ssc_unregister_ops();
#endif
nfs4_unregister_sysctl();
nfs_idmap_quit();
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 7a70287f21a2..f7dad8227a5f 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -57,6 +57,7 @@
#include <linux/rcupdate.h>

#include <linux/uaccess.h>
+#include <linux/nfs_ssc.h>

#include "nfs4_fs.h"
#include "callback.h"
@@ -85,6 +86,10 @@
};
EXPORT_SYMBOL_GPL(nfs_sops);

+static const struct nfs_ssc_client_ops nfs_ssc_clnt_ops_tbl = {
+ .sco_sb_deactive = nfs_sb_deactive,
+};
+
#if IS_ENABLED(CONFIG_NFS_V4)
static int __init register_nfs4_fs(void)
{
@@ -106,6 +111,16 @@ static void unregister_nfs4_fs(void)
}
#endif

+static void nfs_ssc_register_ops(void)
+{
+ nfs_ssc_register(&nfs_ssc_clnt_ops_tbl);
+}
+
+static void nfs_ssc_unregister_ops(void)
+{
+ nfs_ssc_unregister(&nfs_ssc_clnt_ops_tbl);
+}
+
static struct shrinker acl_shrinker = {
.count_objects = nfs_access_cache_count,
.scan_objects = nfs_access_cache_scan,
@@ -133,6 +148,7 @@ int __init register_nfs_fs(void)
ret = register_shrinker(&acl_shrinker);
if (ret < 0)
goto error_3;
+ nfs_ssc_register_ops();
return 0;
error_3:
nfs_unregister_sysctl();
@@ -152,6 +168,7 @@ void __exit unregister_nfs_fs(void)
unregister_shrinker(&acl_shrinker);
nfs_unregister_sysctl();
unregister_nfs4_fs();
+ nfs_ssc_unregister_ops();
unregister_filesystem(&nfs_fs_type);
}

diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
index 4bebe834c009..fa82f5aaa6d9 100644
--- a/fs/nfs_common/Makefile
+++ b/fs/nfs_common/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
nfs_acl-objs := nfsacl.o

obj-$(CONFIG_GRACE_PERIOD) += grace.o
+obj-$(CONFIG_GRACE_PERIOD) += nfs_ssc.o
diff --git a/fs/nfs_common/nfs_ssc.c b/fs/nfs_common/nfs_ssc.c
new file mode 100644
index 000000000000..f43bbb373913
--- /dev/null
+++ b/fs/nfs_common/nfs_ssc.c
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * fs/nfs_common/nfs_ssc_comm.c
+ *
+ * Helper for knfsd's SSC to access ops in NFS client modules
+ *
+ * Author: Dai Ngo <[email protected]>
+ *
+ * Copyright (c) 2020, Oracle and/or its affiliates.
+ */
+
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/nfs_ssc.h>
+#include "../nfs/nfs4_fs.h"
+
+MODULE_LICENSE("GPL");
+
+struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
+EXPORT_SYMBOL_GPL(nfs_ssc_client_tbl);
+
+#ifdef CONFIG_NFS_V4_2
+/**
+ * nfs42_ssc_register - install the NFS_V4 client ops in the nfs_ssc_client_tbl
+ * @ops: NFS_V4 ops to be installed
+ *
+ * Return values:
+ * None
+ */
+void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops)
+{
+ nfs_ssc_client_tbl.ssc_nfs4_ops = ops;
+}
+EXPORT_SYMBOL_GPL(nfs42_ssc_register);
+
+/**
+ * nfs42_ssc_unregister - uninstall the NFS_V4 client ops from
+ * the nfs_ssc_client_tbl
+ * @ops: ops to be uninstalled
+ *
+ * Return values:
+ * None
+ */
+void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops)
+{
+ if (nfs_ssc_client_tbl.ssc_nfs4_ops != ops)
+ return;
+
+ nfs_ssc_client_tbl.ssc_nfs4_ops = NULL;
+}
+EXPORT_SYMBOL_GPL(nfs42_ssc_unregister);
+#endif /* CONFIG_NFS_V4_2 */
+
+#ifdef CONFIG_NFS_V4_2
+/**
+ * nfs_ssc_register - install the NFS_FS client ops in the nfs_ssc_client_tbl
+ * @ops: NFS_FS ops to be installed
+ *
+ * Return values:
+ * None
+ */
+void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
+{
+ nfs_ssc_client_tbl.ssc_nfs_ops = ops;
+}
+EXPORT_SYMBOL_GPL(nfs_ssc_register);
+
+/**
+ * nfs_ssc_unregister - uninstall the NFS_FS client ops from
+ * the nfs_ssc_client_tbl
+ * @ops: ops to be uninstalled
+ *
+ * Return values:
+ * None
+ */
+void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
+{
+ if (nfs_ssc_client_tbl.ssc_nfs_ops != ops)
+ return;
+ nfs_ssc_client_tbl.ssc_nfs_ops = NULL;
+}
+EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
+
+#else
+void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
+{
+}
+EXPORT_SYMBOL_GPL(nfs_ssc_register);
+
+void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
+{
+}
+EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
+#endif /* CONFIG_NFS_V4_2 */
diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
index 99d2cae91bd6..f368f3215f88 100644
--- a/fs/nfsd/Kconfig
+++ b/fs/nfsd/Kconfig
@@ -136,7 +136,7 @@ config NFSD_FLEXFILELAYOUT

config NFSD_V4_2_INTER_SSC
bool "NFSv4.2 inter server to server COPY"
- depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2 && NFS_FS=y
+ depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
help
This option enables support for NFSv4.2 inter server to
server copy where the destination server calls the NFSv4.2
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index eaf50eafa935..84e10aef1417 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -38,6 +38,7 @@
#include <linux/slab.h>
#include <linux/kthread.h>
#include <linux/sunrpc/addr.h>
+#include <linux/nfs_ssc.h>

#include "idmap.h"
#include "cache.h"
@@ -1247,7 +1248,7 @@ extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
static void
nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
{
- nfs_sb_deactive(ss_mnt->mnt_sb);
+ nfs_do_sb_deactive(ss_mnt->mnt_sb);
mntput(ss_mnt);
}

diff --git a/include/linux/nfs_ssc.h b/include/linux/nfs_ssc.h
new file mode 100644
index 000000000000..f5ba0fbff72f
--- /dev/null
+++ b/include/linux/nfs_ssc.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * include/linux/nfs_ssc.h
+ *
+ * Author: Dai Ngo <[email protected]>
+ *
+ * Copyright (c) 2020, Oracle and/or its affiliates.
+ */
+
+#include <linux/nfs_fs.h>
+
+extern struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
+
+/*
+ * NFS_V4
+ */
+struct nfs4_ssc_client_ops {
+ struct file *(*sco_open)(struct vfsmount *ss_mnt,
+ struct nfs_fh *src_fh, nfs4_stateid *stateid);
+ void (*sco_close)(struct file *filep);
+};
+
+/*
+ * NFS_FS
+ */
+struct nfs_ssc_client_ops {
+ void (*sco_sb_deactive)(struct super_block *sb);
+};
+
+struct nfs_ssc_client_ops_tbl {
+ const struct nfs4_ssc_client_ops *ssc_nfs4_ops;
+ const struct nfs_ssc_client_ops *ssc_nfs_ops;
+};
+
+extern void nfs42_ssc_register_ops(void);
+extern void nfs42_ssc_unregister_ops(void);
+
+extern void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops);
+extern void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops);
+
+#ifdef CONFIG_NFSD_V4_2_INTER_SSC
+static inline struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
+ struct nfs_fh *src_fh, nfs4_stateid *stateid)
+{
+ if (nfs_ssc_client_tbl.ssc_nfs4_ops)
+ return (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_open)(ss_mnt, src_fh, stateid);
+ return ERR_PTR(-EIO);
+}
+
+static inline void nfs42_ssc_close(struct file *filep)
+{
+ if (nfs_ssc_client_tbl.ssc_nfs4_ops)
+ (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_close)(filep);
+}
+#endif
+
+/*
+ * NFS_FS
+ */
+extern void nfs_ssc_register(const struct nfs_ssc_client_ops *ops);
+extern void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops);
+
+static inline void nfs_do_sb_deactive(struct super_block *sb)
+{
+ if (nfs_ssc_client_tbl.ssc_nfs_ops)
+ (*nfs_ssc_client_tbl.ssc_nfs_ops->sco_sb_deactive)(sb);
+}
--
1.8.3.1

2020-10-21 06:42:34

by J. Bruce Fields

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> build errors and some configs with NFSD=m to get NFS4ERR_STALE
> error when doing inter server copy.
>
> Added ops table in nfs_common for knfsd to access NFS client modules.

OK, looks reasonable to me, applying. Does this resolve all the
problems you've seen, or is there any bad case left?

--b.

>
> Fixes: 3ac3711adb88 ("NFSD: Fix NFS server build errors")
> Signed-off-by: Dai Ngo <[email protected]>
> ---
> changes from v2: fix 0-day build issues.
> changes from v3: reacted to Bruce's comments, removed paranoid error checking.
> ---
> fs/nfs/nfs4file.c | 38 ++++++++++++++++----
> fs/nfs/nfs4super.c | 5 +++
> fs/nfs/super.c | 17 +++++++++
> fs/nfs_common/Makefile | 1 +
> fs/nfs_common/nfs_ssc.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/Kconfig | 2 +-
> fs/nfsd/nfs4proc.c | 3 +-
> include/linux/nfs_ssc.h | 67 +++++++++++++++++++++++++++++++++++
> 8 files changed, 219 insertions(+), 8 deletions(-)
> create mode 100644 fs/nfs_common/nfs_ssc.c
> create mode 100644 include/linux/nfs_ssc.h
>
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index fdfc77486ace..984938024011 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -9,6 +9,7 @@
> #include <linux/falloc.h>
> #include <linux/mount.h>
> #include <linux/nfs_fs.h>
> +#include <linux/nfs_ssc.h>
> #include "delegation.h"
> #include "internal.h"
> #include "iostat.h"
> @@ -314,9 +315,8 @@ static loff_t nfs42_remap_file_range(struct file *src_file, loff_t src_off,
> static int read_name_gen = 1;
> #define SSC_READ_NAME_BODY "ssc_read_%d"
>
> -struct file *
> -nfs42_ssc_open(struct vfsmount *ss_mnt, struct nfs_fh *src_fh,
> - nfs4_stateid *stateid)
> +static struct file *__nfs42_ssc_open(struct vfsmount *ss_mnt,
> + struct nfs_fh *src_fh, nfs4_stateid *stateid)
> {
> struct nfs_fattr fattr;
> struct file *filep, *res;
> @@ -398,14 +398,40 @@ struct file *
> fput(filep);
> goto out_free_name;
> }
> -EXPORT_SYMBOL_GPL(nfs42_ssc_open);
> -void nfs42_ssc_close(struct file *filep)
> +
> +static void __nfs42_ssc_close(struct file *filep)
> {
> struct nfs_open_context *ctx = nfs_file_open_context(filep);
>
> ctx->state->flags = 0;
> }
> -EXPORT_SYMBOL_GPL(nfs42_ssc_close);
> +
> +static const struct nfs4_ssc_client_ops nfs4_ssc_clnt_ops_tbl = {
> + .sco_open = __nfs42_ssc_open,
> + .sco_close = __nfs42_ssc_close,
> +};
> +
> +/**
> + * nfs42_ssc_register_ops - Wrapper to register NFS_V4 ops in nfs_common
> + *
> + * Return values:
> + * None
> + */
> +void nfs42_ssc_register_ops(void)
> +{
> + nfs42_ssc_register(&nfs4_ssc_clnt_ops_tbl);
> +}
> +
> +/**
> + * nfs42_ssc_unregister_ops - wrapper to un-register NFS_V4 ops in nfs_common
> + *
> + * Return values:
> + * None.
> + */
> +void nfs42_ssc_unregister_ops(void)
> +{
> + nfs42_ssc_unregister(&nfs4_ssc_clnt_ops_tbl);
> +}
> #endif /* CONFIG_NFS_V4_2 */
>
> const struct file_operations nfs4_file_operations = {
> diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
> index 0c1ab846b83d..93f5c1678ec2 100644
> --- a/fs/nfs/nfs4super.c
> +++ b/fs/nfs/nfs4super.c
> @@ -7,6 +7,7 @@
> #include <linux/mount.h>
> #include <linux/nfs4_mount.h>
> #include <linux/nfs_fs.h>
> +#include <linux/nfs_ssc.h>
> #include "delegation.h"
> #include "internal.h"
> #include "nfs4_fs.h"
> @@ -279,6 +280,9 @@ static int __init init_nfs_v4(void)
> if (err)
> goto out2;
>
> +#ifdef CONFIG_NFS_V4_2
> + nfs42_ssc_register_ops();
> +#endif
> register_nfs_version(&nfs_v4);
> return 0;
> out2:
> @@ -297,6 +301,7 @@ static void __exit exit_nfs_v4(void)
> unregister_nfs_version(&nfs_v4);
> #ifdef CONFIG_NFS_V4_2
> nfs4_xattr_cache_exit();
> + nfs42_ssc_unregister_ops();
> #endif
> nfs4_unregister_sysctl();
> nfs_idmap_quit();
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 7a70287f21a2..f7dad8227a5f 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -57,6 +57,7 @@
> #include <linux/rcupdate.h>
>
> #include <linux/uaccess.h>
> +#include <linux/nfs_ssc.h>
>
> #include "nfs4_fs.h"
> #include "callback.h"
> @@ -85,6 +86,10 @@
> };
> EXPORT_SYMBOL_GPL(nfs_sops);
>
> +static const struct nfs_ssc_client_ops nfs_ssc_clnt_ops_tbl = {
> + .sco_sb_deactive = nfs_sb_deactive,
> +};
> +
> #if IS_ENABLED(CONFIG_NFS_V4)
> static int __init register_nfs4_fs(void)
> {
> @@ -106,6 +111,16 @@ static void unregister_nfs4_fs(void)
> }
> #endif
>
> +static void nfs_ssc_register_ops(void)
> +{
> + nfs_ssc_register(&nfs_ssc_clnt_ops_tbl);
> +}
> +
> +static void nfs_ssc_unregister_ops(void)
> +{
> + nfs_ssc_unregister(&nfs_ssc_clnt_ops_tbl);
> +}
> +
> static struct shrinker acl_shrinker = {
> .count_objects = nfs_access_cache_count,
> .scan_objects = nfs_access_cache_scan,
> @@ -133,6 +148,7 @@ int __init register_nfs_fs(void)
> ret = register_shrinker(&acl_shrinker);
> if (ret < 0)
> goto error_3;
> + nfs_ssc_register_ops();
> return 0;
> error_3:
> nfs_unregister_sysctl();
> @@ -152,6 +168,7 @@ void __exit unregister_nfs_fs(void)
> unregister_shrinker(&acl_shrinker);
> nfs_unregister_sysctl();
> unregister_nfs4_fs();
> + nfs_ssc_unregister_ops();
> unregister_filesystem(&nfs_fs_type);
> }
>
> diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
> index 4bebe834c009..fa82f5aaa6d9 100644
> --- a/fs/nfs_common/Makefile
> +++ b/fs/nfs_common/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
> nfs_acl-objs := nfsacl.o
>
> obj-$(CONFIG_GRACE_PERIOD) += grace.o
> +obj-$(CONFIG_GRACE_PERIOD) += nfs_ssc.o
> diff --git a/fs/nfs_common/nfs_ssc.c b/fs/nfs_common/nfs_ssc.c
> new file mode 100644
> index 000000000000..f43bbb373913
> --- /dev/null
> +++ b/fs/nfs_common/nfs_ssc.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * fs/nfs_common/nfs_ssc_comm.c
> + *
> + * Helper for knfsd's SSC to access ops in NFS client modules
> + *
> + * Author: Dai Ngo <[email protected]>
> + *
> + * Copyright (c) 2020, Oracle and/or its affiliates.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/nfs_ssc.h>
> +#include "../nfs/nfs4_fs.h"
> +
> +MODULE_LICENSE("GPL");
> +
> +struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
> +EXPORT_SYMBOL_GPL(nfs_ssc_client_tbl);
> +
> +#ifdef CONFIG_NFS_V4_2
> +/**
> + * nfs42_ssc_register - install the NFS_V4 client ops in the nfs_ssc_client_tbl
> + * @ops: NFS_V4 ops to be installed
> + *
> + * Return values:
> + * None
> + */
> +void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops)
> +{
> + nfs_ssc_client_tbl.ssc_nfs4_ops = ops;
> +}
> +EXPORT_SYMBOL_GPL(nfs42_ssc_register);
> +
> +/**
> + * nfs42_ssc_unregister - uninstall the NFS_V4 client ops from
> + * the nfs_ssc_client_tbl
> + * @ops: ops to be uninstalled
> + *
> + * Return values:
> + * None
> + */
> +void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops)
> +{
> + if (nfs_ssc_client_tbl.ssc_nfs4_ops != ops)
> + return;
> +
> + nfs_ssc_client_tbl.ssc_nfs4_ops = NULL;
> +}
> +EXPORT_SYMBOL_GPL(nfs42_ssc_unregister);
> +#endif /* CONFIG_NFS_V4_2 */
> +
> +#ifdef CONFIG_NFS_V4_2
> +/**
> + * nfs_ssc_register - install the NFS_FS client ops in the nfs_ssc_client_tbl
> + * @ops: NFS_FS ops to be installed
> + *
> + * Return values:
> + * None
> + */
> +void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
> +{
> + nfs_ssc_client_tbl.ssc_nfs_ops = ops;
> +}
> +EXPORT_SYMBOL_GPL(nfs_ssc_register);
> +
> +/**
> + * nfs_ssc_unregister - uninstall the NFS_FS client ops from
> + * the nfs_ssc_client_tbl
> + * @ops: ops to be uninstalled
> + *
> + * Return values:
> + * None
> + */
> +void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
> +{
> + if (nfs_ssc_client_tbl.ssc_nfs_ops != ops)
> + return;
> + nfs_ssc_client_tbl.ssc_nfs_ops = NULL;
> +}
> +EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
> +
> +#else
> +void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
> +{
> +}
> +EXPORT_SYMBOL_GPL(nfs_ssc_register);
> +
> +void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
> +{
> +}
> +EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
> +#endif /* CONFIG_NFS_V4_2 */
> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> index 99d2cae91bd6..f368f3215f88 100644
> --- a/fs/nfsd/Kconfig
> +++ b/fs/nfsd/Kconfig
> @@ -136,7 +136,7 @@ config NFSD_FLEXFILELAYOUT
>
> config NFSD_V4_2_INTER_SSC
> bool "NFSv4.2 inter server to server COPY"
> - depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2 && NFS_FS=y
> + depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
> help
> This option enables support for NFSv4.2 inter server to
> server copy where the destination server calls the NFSv4.2
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index eaf50eafa935..84e10aef1417 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -38,6 +38,7 @@
> #include <linux/slab.h>
> #include <linux/kthread.h>
> #include <linux/sunrpc/addr.h>
> +#include <linux/nfs_ssc.h>
>
> #include "idmap.h"
> #include "cache.h"
> @@ -1247,7 +1248,7 @@ extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
> static void
> nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
> {
> - nfs_sb_deactive(ss_mnt->mnt_sb);
> + nfs_do_sb_deactive(ss_mnt->mnt_sb);
> mntput(ss_mnt);
> }
>
> diff --git a/include/linux/nfs_ssc.h b/include/linux/nfs_ssc.h
> new file mode 100644
> index 000000000000..f5ba0fbff72f
> --- /dev/null
> +++ b/include/linux/nfs_ssc.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * include/linux/nfs_ssc.h
> + *
> + * Author: Dai Ngo <[email protected]>
> + *
> + * Copyright (c) 2020, Oracle and/or its affiliates.
> + */
> +
> +#include <linux/nfs_fs.h>
> +
> +extern struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
> +
> +/*
> + * NFS_V4
> + */
> +struct nfs4_ssc_client_ops {
> + struct file *(*sco_open)(struct vfsmount *ss_mnt,
> + struct nfs_fh *src_fh, nfs4_stateid *stateid);
> + void (*sco_close)(struct file *filep);
> +};
> +
> +/*
> + * NFS_FS
> + */
> +struct nfs_ssc_client_ops {
> + void (*sco_sb_deactive)(struct super_block *sb);
> +};
> +
> +struct nfs_ssc_client_ops_tbl {
> + const struct nfs4_ssc_client_ops *ssc_nfs4_ops;
> + const struct nfs_ssc_client_ops *ssc_nfs_ops;
> +};
> +
> +extern void nfs42_ssc_register_ops(void);
> +extern void nfs42_ssc_unregister_ops(void);
> +
> +extern void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops);
> +extern void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops);
> +
> +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> +static inline struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
> + struct nfs_fh *src_fh, nfs4_stateid *stateid)
> +{
> + if (nfs_ssc_client_tbl.ssc_nfs4_ops)
> + return (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_open)(ss_mnt, src_fh, stateid);
> + return ERR_PTR(-EIO);
> +}
> +
> +static inline void nfs42_ssc_close(struct file *filep)
> +{
> + if (nfs_ssc_client_tbl.ssc_nfs4_ops)
> + (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_close)(filep);
> +}
> +#endif
> +
> +/*
> + * NFS_FS
> + */
> +extern void nfs_ssc_register(const struct nfs_ssc_client_ops *ops);
> +extern void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops);
> +
> +static inline void nfs_do_sb_deactive(struct super_block *sb)
> +{
> + if (nfs_ssc_client_tbl.ssc_nfs_ops)
> + (*nfs_ssc_client_tbl.ssc_nfs_ops->sco_sb_deactive)(sb);
> +}
> --
> 1.8.3.1

2020-10-21 09:09:56

by Dai Ngo

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
>> NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
>> build errors and some configs with NFSD=m to get NFS4ERR_STALE
>> error when doing inter server copy.
>>
>> Added ops table in nfs_common for knfsd to access NFS client modules.
> OK, looks reasonable to me, applying. Does this resolve all the
> problems you've seen, or is there any bad case left?

Thanks Bruce.

With this patch, I no longer see the NFS4ERR_STALE in any config.

The problem with NFS4ERR_STALE was because of a bug in nfs42_ssc_open.
When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
returns NULL which is incorrect allowing the operation to continue
until nfsd4_putfh which does not have the code to handle nfserr_stale.

With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
to switch over to the split copying (read src and write to dst).

The other problem that I see was the "use-after-free" which I think
happens only on the 1st inter server copy. I will look into this:


 Sep 23 01:08:10 nfsvmf24 kernel: ------------[ cut here ]------------
Sep 23 01:08:10 nfsvmf24 kernel: refcount_t: underflow; use-after-free.
Sep 23 01:08:10 nfsvmf24 kernel: WARNING: CPU: 0 PID: 4217 at lib/refcount.c:28 refcount_warn_saturate+0xae/0xf0
Sep 23 01:08:10 nfsvmf24 kernel: Modules linked in: cts rpcsec_gss_krb5 xt_REDIRECT xt_nat ip6table_nat ip6_tables iptable_nat nf_nat nf_conntrack btrfs nf_defrag_ipv6 nf_defrag_ipv4 blake2b_generic xor zstd_compress rfkill raid6_pq sb_edac intel_powerclamp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel crypto_simd cryptd glue_helper sg pcspkr i2c_piix4 video ip_tables xfs libcrc32c sd_mod t10_pi ahci libahci libata e1000 crc32c_intel serio_raw dm_mirror dm_region_hash dm_log dm_mod
Sep 23 01:08:10 nfsvmf24 kernel: CPU: 0 PID: 4217 Comm: copy thread Not tainted 5.9.0-rc5+ #14
Sep 23 01:08:10 nfsvmf24 kernel: Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
Sep 23 01:08:10 nfsvmf24 kernel: RIP: 0010:refcount_warn_saturate+0xae/0xf0
Sep 23 01:08:10 nfsvmf24 kernel: Code: 99 83 31 01 01 e8 27 ba b6 ff 0f 0b 5d c3 80 3d 86 83 31 01 00 75 91 48 c7 c7 20 d3 3a 84 c6 05 76 83 31 01 01 e8 07 ba b6 ff <0f> 0b 5d c3 80 3d 64 83 31 01 00 0f 85 6d ff ff ff 48 c7 c7 78 d3
Sep 23 01:08:10 nfsvmf24 kernel: RSP: 0000:ffffc20300403e68 EFLAGS: 00010286
Sep 23 01:08:10 nfsvmf24 kernel: RAX: 0000000000000000 RBX: 0000000000000010 RCX: 0000000000000027
Sep 23 01:08:10 nfsvmf24 kernel: RDX: 0000000000000027 RSI: 0000000000000086 RDI: ffff9e8c97c18c48
Sep 23 01:08:10 nfsvmf24 kernel: RBP: ffffc20300403e68 R08: ffff9e8c97c18c40 R09: 0000000000000004
Sep 23 01:08:10 nfsvmf24 kernel: R10: 0000000000000000 R11: 0000000000000001 R12: ffff9e8c96693300
Sep 23 01:08:10 nfsvmf24 kernel: R13: ffff9e8c96693300 R14: ffff9e8c8a6e3000 R15: ffff9e8c959c5520
Sep 23 01:08:10 nfsvmf24 kernel: FS: 0000000000000000(0000) GS:ffff9e8c97c00000(0000) knlGS:0000000000000000
Sep 23 01:08:10 nfsvmf24 kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Sep 23 01:08:10 nfsvmf24 kernel: CR2: 000055aedfd031a8 CR3: 000000020ee6e000 CR4: 00000000000406f0
Sep 23 01:08:10 nfsvmf24 kernel: Call Trace:
Sep 23 01:08:10 nfsvmf24 kernel: nfsd_file_put_noref+0x8f/0xa0
Sep 23 01:08:10 nfsvmf24 kernel: nfsd_file_put+0x3e/0x90
Sep 23 01:08:10 nfsvmf24 kernel: nfsd4_do_copy+0xe5/0x150
Sep 23 01:08:10 nfsvmf24 kernel: nfsd4_do_async_copy+0x84/0x200
Sep 23 01:08:10 nfsvmf24 kernel: kthread+0x114/0x150
Sep 23 01:08:10 nfsvmf24 kernel: ? nfsd4_copy+0x4e0/0x4e0
Sep 23 01:08:10 nfsvmf24 kernel: ? kthread_park+0x90/0x90
Sep 23 01:08:10 nfsvmf24 kernel: ret_from_fork+0x22/0x30

-Dai

>
> --b.
>
>> Fixes: 3ac3711adb88 ("NFSD: Fix NFS server build errors")
>> Signed-off-by: Dai Ngo <[email protected]>
>> ---
>> changes from v2: fix 0-day build issues.
>> changes from v3: reacted to Bruce's comments, removed paranoid error checking.
>> ---
>> fs/nfs/nfs4file.c | 38 ++++++++++++++++----
>> fs/nfs/nfs4super.c | 5 +++
>> fs/nfs/super.c | 17 +++++++++
>> fs/nfs_common/Makefile | 1 +
>> fs/nfs_common/nfs_ssc.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++
>> fs/nfsd/Kconfig | 2 +-
>> fs/nfsd/nfs4proc.c | 3 +-
>> include/linux/nfs_ssc.h | 67 +++++++++++++++++++++++++++++++++++
>> 8 files changed, 219 insertions(+), 8 deletions(-)
>> create mode 100644 fs/nfs_common/nfs_ssc.c
>> create mode 100644 include/linux/nfs_ssc.h
>>
>> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
>> index fdfc77486ace..984938024011 100644
>> --- a/fs/nfs/nfs4file.c
>> +++ b/fs/nfs/nfs4file.c
>> @@ -9,6 +9,7 @@
>> #include <linux/falloc.h>
>> #include <linux/mount.h>
>> #include <linux/nfs_fs.h>
>> +#include <linux/nfs_ssc.h>
>> #include "delegation.h"
>> #include "internal.h"
>> #include "iostat.h"
>> @@ -314,9 +315,8 @@ static loff_t nfs42_remap_file_range(struct file *src_file, loff_t src_off,
>> static int read_name_gen = 1;
>> #define SSC_READ_NAME_BODY "ssc_read_%d"
>>
>> -struct file *
>> -nfs42_ssc_open(struct vfsmount *ss_mnt, struct nfs_fh *src_fh,
>> - nfs4_stateid *stateid)
>> +static struct file *__nfs42_ssc_open(struct vfsmount *ss_mnt,
>> + struct nfs_fh *src_fh, nfs4_stateid *stateid)
>> {
>> struct nfs_fattr fattr;
>> struct file *filep, *res;
>> @@ -398,14 +398,40 @@ struct file *
>> fput(filep);
>> goto out_free_name;
>> }
>> -EXPORT_SYMBOL_GPL(nfs42_ssc_open);
>> -void nfs42_ssc_close(struct file *filep)
>> +
>> +static void __nfs42_ssc_close(struct file *filep)
>> {
>> struct nfs_open_context *ctx = nfs_file_open_context(filep);
>>
>> ctx->state->flags = 0;
>> }
>> -EXPORT_SYMBOL_GPL(nfs42_ssc_close);
>> +
>> +static const struct nfs4_ssc_client_ops nfs4_ssc_clnt_ops_tbl = {
>> + .sco_open = __nfs42_ssc_open,
>> + .sco_close = __nfs42_ssc_close,
>> +};
>> +
>> +/**
>> + * nfs42_ssc_register_ops - Wrapper to register NFS_V4 ops in nfs_common
>> + *
>> + * Return values:
>> + * None
>> + */
>> +void nfs42_ssc_register_ops(void)
>> +{
>> + nfs42_ssc_register(&nfs4_ssc_clnt_ops_tbl);
>> +}
>> +
>> +/**
>> + * nfs42_ssc_unregister_ops - wrapper to un-register NFS_V4 ops in nfs_common
>> + *
>> + * Return values:
>> + * None.
>> + */
>> +void nfs42_ssc_unregister_ops(void)
>> +{
>> + nfs42_ssc_unregister(&nfs4_ssc_clnt_ops_tbl);
>> +}
>> #endif /* CONFIG_NFS_V4_2 */
>>
>> const struct file_operations nfs4_file_operations = {
>> diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
>> index 0c1ab846b83d..93f5c1678ec2 100644
>> --- a/fs/nfs/nfs4super.c
>> +++ b/fs/nfs/nfs4super.c
>> @@ -7,6 +7,7 @@
>> #include <linux/mount.h>
>> #include <linux/nfs4_mount.h>
>> #include <linux/nfs_fs.h>
>> +#include <linux/nfs_ssc.h>
>> #include "delegation.h"
>> #include "internal.h"
>> #include "nfs4_fs.h"
>> @@ -279,6 +280,9 @@ static int __init init_nfs_v4(void)
>> if (err)
>> goto out2;
>>
>> +#ifdef CONFIG_NFS_V4_2
>> + nfs42_ssc_register_ops();
>> +#endif
>> register_nfs_version(&nfs_v4);
>> return 0;
>> out2:
>> @@ -297,6 +301,7 @@ static void __exit exit_nfs_v4(void)
>> unregister_nfs_version(&nfs_v4);
>> #ifdef CONFIG_NFS_V4_2
>> nfs4_xattr_cache_exit();
>> + nfs42_ssc_unregister_ops();
>> #endif
>> nfs4_unregister_sysctl();
>> nfs_idmap_quit();
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index 7a70287f21a2..f7dad8227a5f 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -57,6 +57,7 @@
>> #include <linux/rcupdate.h>
>>
>> #include <linux/uaccess.h>
>> +#include <linux/nfs_ssc.h>
>>
>> #include "nfs4_fs.h"
>> #include "callback.h"
>> @@ -85,6 +86,10 @@
>> };
>> EXPORT_SYMBOL_GPL(nfs_sops);
>>
>> +static const struct nfs_ssc_client_ops nfs_ssc_clnt_ops_tbl = {
>> + .sco_sb_deactive = nfs_sb_deactive,
>> +};
>> +
>> #if IS_ENABLED(CONFIG_NFS_V4)
>> static int __init register_nfs4_fs(void)
>> {
>> @@ -106,6 +111,16 @@ static void unregister_nfs4_fs(void)
>> }
>> #endif
>>
>> +static void nfs_ssc_register_ops(void)
>> +{
>> + nfs_ssc_register(&nfs_ssc_clnt_ops_tbl);
>> +}
>> +
>> +static void nfs_ssc_unregister_ops(void)
>> +{
>> + nfs_ssc_unregister(&nfs_ssc_clnt_ops_tbl);
>> +}
>> +
>> static struct shrinker acl_shrinker = {
>> .count_objects = nfs_access_cache_count,
>> .scan_objects = nfs_access_cache_scan,
>> @@ -133,6 +148,7 @@ int __init register_nfs_fs(void)
>> ret = register_shrinker(&acl_shrinker);
>> if (ret < 0)
>> goto error_3;
>> + nfs_ssc_register_ops();
>> return 0;
>> error_3:
>> nfs_unregister_sysctl();
>> @@ -152,6 +168,7 @@ void __exit unregister_nfs_fs(void)
>> unregister_shrinker(&acl_shrinker);
>> nfs_unregister_sysctl();
>> unregister_nfs4_fs();
>> + nfs_ssc_unregister_ops();
>> unregister_filesystem(&nfs_fs_type);
>> }
>>
>> diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
>> index 4bebe834c009..fa82f5aaa6d9 100644
>> --- a/fs/nfs_common/Makefile
>> +++ b/fs/nfs_common/Makefile
>> @@ -7,3 +7,4 @@ obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
>> nfs_acl-objs := nfsacl.o
>>
>> obj-$(CONFIG_GRACE_PERIOD) += grace.o
>> +obj-$(CONFIG_GRACE_PERIOD) += nfs_ssc.o
>> diff --git a/fs/nfs_common/nfs_ssc.c b/fs/nfs_common/nfs_ssc.c
>> new file mode 100644
>> index 000000000000..f43bbb373913
>> --- /dev/null
>> +++ b/fs/nfs_common/nfs_ssc.c
>> @@ -0,0 +1,94 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * fs/nfs_common/nfs_ssc_comm.c
>> + *
>> + * Helper for knfsd's SSC to access ops in NFS client modules
>> + *
>> + * Author: Dai Ngo <[email protected]>
>> + *
>> + * Copyright (c) 2020, Oracle and/or its affiliates.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/fs.h>
>> +#include <linux/nfs_ssc.h>
>> +#include "../nfs/nfs4_fs.h"
>> +
>> +MODULE_LICENSE("GPL");
>> +
>> +struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
>> +EXPORT_SYMBOL_GPL(nfs_ssc_client_tbl);
>> +
>> +#ifdef CONFIG_NFS_V4_2
>> +/**
>> + * nfs42_ssc_register - install the NFS_V4 client ops in the nfs_ssc_client_tbl
>> + * @ops: NFS_V4 ops to be installed
>> + *
>> + * Return values:
>> + * None
>> + */
>> +void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops)
>> +{
>> + nfs_ssc_client_tbl.ssc_nfs4_ops = ops;
>> +}
>> +EXPORT_SYMBOL_GPL(nfs42_ssc_register);
>> +
>> +/**
>> + * nfs42_ssc_unregister - uninstall the NFS_V4 client ops from
>> + * the nfs_ssc_client_tbl
>> + * @ops: ops to be uninstalled
>> + *
>> + * Return values:
>> + * None
>> + */
>> +void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops)
>> +{
>> + if (nfs_ssc_client_tbl.ssc_nfs4_ops != ops)
>> + return;
>> +
>> + nfs_ssc_client_tbl.ssc_nfs4_ops = NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(nfs42_ssc_unregister);
>> +#endif /* CONFIG_NFS_V4_2 */
>> +
>> +#ifdef CONFIG_NFS_V4_2
>> +/**
>> + * nfs_ssc_register - install the NFS_FS client ops in the nfs_ssc_client_tbl
>> + * @ops: NFS_FS ops to be installed
>> + *
>> + * Return values:
>> + * None
>> + */
>> +void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
>> +{
>> + nfs_ssc_client_tbl.ssc_nfs_ops = ops;
>> +}
>> +EXPORT_SYMBOL_GPL(nfs_ssc_register);
>> +
>> +/**
>> + * nfs_ssc_unregister - uninstall the NFS_FS client ops from
>> + * the nfs_ssc_client_tbl
>> + * @ops: ops to be uninstalled
>> + *
>> + * Return values:
>> + * None
>> + */
>> +void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
>> +{
>> + if (nfs_ssc_client_tbl.ssc_nfs_ops != ops)
>> + return;
>> + nfs_ssc_client_tbl.ssc_nfs_ops = NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
>> +
>> +#else
>> +void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
>> +{
>> +}
>> +EXPORT_SYMBOL_GPL(nfs_ssc_register);
>> +
>> +void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
>> +{
>> +}
>> +EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
>> +#endif /* CONFIG_NFS_V4_2 */
>> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
>> index 99d2cae91bd6..f368f3215f88 100644
>> --- a/fs/nfsd/Kconfig
>> +++ b/fs/nfsd/Kconfig
>> @@ -136,7 +136,7 @@ config NFSD_FLEXFILELAYOUT
>>
>> config NFSD_V4_2_INTER_SSC
>> bool "NFSv4.2 inter server to server COPY"
>> - depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2 && NFS_FS=y
>> + depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
>> help
>> This option enables support for NFSv4.2 inter server to
>> server copy where the destination server calls the NFSv4.2
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index eaf50eafa935..84e10aef1417 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -38,6 +38,7 @@
>> #include <linux/slab.h>
>> #include <linux/kthread.h>
>> #include <linux/sunrpc/addr.h>
>> +#include <linux/nfs_ssc.h>
>>
>> #include "idmap.h"
>> #include "cache.h"
>> @@ -1247,7 +1248,7 @@ extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
>> static void
>> nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
>> {
>> - nfs_sb_deactive(ss_mnt->mnt_sb);
>> + nfs_do_sb_deactive(ss_mnt->mnt_sb);
>> mntput(ss_mnt);
>> }
>>
>> diff --git a/include/linux/nfs_ssc.h b/include/linux/nfs_ssc.h
>> new file mode 100644
>> index 000000000000..f5ba0fbff72f
>> --- /dev/null
>> +++ b/include/linux/nfs_ssc.h
>> @@ -0,0 +1,67 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * include/linux/nfs_ssc.h
>> + *
>> + * Author: Dai Ngo <[email protected]>
>> + *
>> + * Copyright (c) 2020, Oracle and/or its affiliates.
>> + */
>> +
>> +#include <linux/nfs_fs.h>
>> +
>> +extern struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
>> +
>> +/*
>> + * NFS_V4
>> + */
>> +struct nfs4_ssc_client_ops {
>> + struct file *(*sco_open)(struct vfsmount *ss_mnt,
>> + struct nfs_fh *src_fh, nfs4_stateid *stateid);
>> + void (*sco_close)(struct file *filep);
>> +};
>> +
>> +/*
>> + * NFS_FS
>> + */
>> +struct nfs_ssc_client_ops {
>> + void (*sco_sb_deactive)(struct super_block *sb);
>> +};
>> +
>> +struct nfs_ssc_client_ops_tbl {
>> + const struct nfs4_ssc_client_ops *ssc_nfs4_ops;
>> + const struct nfs_ssc_client_ops *ssc_nfs_ops;
>> +};
>> +
>> +extern void nfs42_ssc_register_ops(void);
>> +extern void nfs42_ssc_unregister_ops(void);
>> +
>> +extern void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops);
>> +extern void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops);
>> +
>> +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
>> +static inline struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
>> + struct nfs_fh *src_fh, nfs4_stateid *stateid)
>> +{
>> + if (nfs_ssc_client_tbl.ssc_nfs4_ops)
>> + return (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_open)(ss_mnt, src_fh, stateid);
>> + return ERR_PTR(-EIO);
>> +}
>> +
>> +static inline void nfs42_ssc_close(struct file *filep)
>> +{
>> + if (nfs_ssc_client_tbl.ssc_nfs4_ops)
>> + (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_close)(filep);
>> +}
>> +#endif
>> +
>> +/*
>> + * NFS_FS
>> + */
>> +extern void nfs_ssc_register(const struct nfs_ssc_client_ops *ops);
>> +extern void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops);
>> +
>> +static inline void nfs_do_sb_deactive(struct super_block *sb)
>> +{
>> + if (nfs_ssc_client_tbl.ssc_nfs_ops)
>> + (*nfs_ssc_client_tbl.ssc_nfs_ops->sco_sb_deactive)(sb);
>> +}
>> --
>> 1.8.3.1

2020-11-09 18:31:30

by J. Bruce Fields

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
>
> On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> >On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> >>NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> >>build errors and some configs with NFSD=m to get NFS4ERR_STALE
> >>error when doing inter server copy.
> >>
> >>Added ops table in nfs_common for knfsd to access NFS client modules.
> >OK, looks reasonable to me, applying. Does this resolve all the
> >problems you've seen, or is there any bad case left?
>
> Thanks Bruce.
>
> With this patch, I no longer see the NFS4ERR_STALE in any config.
>
> The problem with NFS4ERR_STALE was because of a bug in nfs42_ssc_open.
> When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
> returns NULL which is incorrect allowing the operation to continue
> until nfsd4_putfh which does not have the code to handle nfserr_stale.
>
> With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
> new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
> to switch over to the split copying (read src and write to dst).

That sounds reasonable, but I don't see any of the patches you've sent
changing that error return. Did I overlook something, or did you mean
to append a patch to this message?

--b.

>
> The other problem that I see was the "use-after-free" which I think
> happens only on the 1st inter server copy. I will look into this:
>
>
>  Sep 23 01:08:10 nfsvmf24 kernel: ------------[ cut here ]------------
> Sep 23 01:08:10 nfsvmf24 kernel: refcount_t: underflow; use-after-free.
> Sep 23 01:08:10 nfsvmf24 kernel: WARNING: CPU: 0 PID: 4217 at lib/refcount.c:28 refcount_warn_saturate+0xae/0xf0
> Sep 23 01:08:10 nfsvmf24 kernel: Modules linked in: cts rpcsec_gss_krb5 xt_REDIRECT xt_nat ip6table_nat ip6_tables iptable_nat nf_nat nf_conntrack btrfs nf_defrag_ipv6 nf_defrag_ipv4 blake2b_generic xor zstd_compress rfkill raid6_pq sb_edac intel_powerclamp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel crypto_simd cryptd glue_helper sg pcspkr i2c_piix4 video ip_tables xfs libcrc32c sd_mod t10_pi ahci libahci libata e1000 crc32c_intel serio_raw dm_mirror dm_region_hash dm_log dm_mod
> Sep 23 01:08:10 nfsvmf24 kernel: CPU: 0 PID: 4217 Comm: copy thread Not tainted 5.9.0-rc5+ #14
> Sep 23 01:08:10 nfsvmf24 kernel: Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
> Sep 23 01:08:10 nfsvmf24 kernel: RIP: 0010:refcount_warn_saturate+0xae/0xf0
> Sep 23 01:08:10 nfsvmf24 kernel: Code: 99 83 31 01 01 e8 27 ba b6 ff 0f 0b 5d c3 80 3d 86 83 31 01 00 75 91 48 c7 c7 20 d3 3a 84 c6 05 76 83 31 01 01 e8 07 ba b6 ff <0f> 0b 5d c3 80 3d 64 83 31 01 00 0f 85 6d ff ff ff 48 c7 c7 78 d3
> Sep 23 01:08:10 nfsvmf24 kernel: RSP: 0000:ffffc20300403e68 EFLAGS: 00010286
> Sep 23 01:08:10 nfsvmf24 kernel: RAX: 0000000000000000 RBX: 0000000000000010 RCX: 0000000000000027
> Sep 23 01:08:10 nfsvmf24 kernel: RDX: 0000000000000027 RSI: 0000000000000086 RDI: ffff9e8c97c18c48
> Sep 23 01:08:10 nfsvmf24 kernel: RBP: ffffc20300403e68 R08: ffff9e8c97c18c40 R09: 0000000000000004
> Sep 23 01:08:10 nfsvmf24 kernel: R10: 0000000000000000 R11: 0000000000000001 R12: ffff9e8c96693300
> Sep 23 01:08:10 nfsvmf24 kernel: R13: ffff9e8c96693300 R14: ffff9e8c8a6e3000 R15: ffff9e8c959c5520
> Sep 23 01:08:10 nfsvmf24 kernel: FS: 0000000000000000(0000) GS:ffff9e8c97c00000(0000) knlGS:0000000000000000
> Sep 23 01:08:10 nfsvmf24 kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> Sep 23 01:08:10 nfsvmf24 kernel: CR2: 000055aedfd031a8 CR3: 000000020ee6e000 CR4: 00000000000406f0
> Sep 23 01:08:10 nfsvmf24 kernel: Call Trace:
> Sep 23 01:08:10 nfsvmf24 kernel: nfsd_file_put_noref+0x8f/0xa0
> Sep 23 01:08:10 nfsvmf24 kernel: nfsd_file_put+0x3e/0x90
> Sep 23 01:08:10 nfsvmf24 kernel: nfsd4_do_copy+0xe5/0x150
> Sep 23 01:08:10 nfsvmf24 kernel: nfsd4_do_async_copy+0x84/0x200
> Sep 23 01:08:10 nfsvmf24 kernel: kthread+0x114/0x150
> Sep 23 01:08:10 nfsvmf24 kernel: ? nfsd4_copy+0x4e0/0x4e0
> Sep 23 01:08:10 nfsvmf24 kernel: ? kthread_park+0x90/0x90
> Sep 23 01:08:10 nfsvmf24 kernel: ret_from_fork+0x22/0x30
>
> -Dai
>
> >
> >--b.
> >
> >>Fixes: 3ac3711adb88 ("NFSD: Fix NFS server build errors")
> >>Signed-off-by: Dai Ngo <[email protected]>
> >>---
> >>changes from v2: fix 0-day build issues.
> >>changes from v3: reacted to Bruce's comments, removed paranoid error checking.
> >>---
> >> fs/nfs/nfs4file.c | 38 ++++++++++++++++----
> >> fs/nfs/nfs4super.c | 5 +++
> >> fs/nfs/super.c | 17 +++++++++
> >> fs/nfs_common/Makefile | 1 +
> >> fs/nfs_common/nfs_ssc.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> fs/nfsd/Kconfig | 2 +-
> >> fs/nfsd/nfs4proc.c | 3 +-
> >> include/linux/nfs_ssc.h | 67 +++++++++++++++++++++++++++++++++++
> >> 8 files changed, 219 insertions(+), 8 deletions(-)
> >> create mode 100644 fs/nfs_common/nfs_ssc.c
> >> create mode 100644 include/linux/nfs_ssc.h
> >>
> >>diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> >>index fdfc77486ace..984938024011 100644
> >>--- a/fs/nfs/nfs4file.c
> >>+++ b/fs/nfs/nfs4file.c
> >>@@ -9,6 +9,7 @@
> >> #include <linux/falloc.h>
> >> #include <linux/mount.h>
> >> #include <linux/nfs_fs.h>
> >>+#include <linux/nfs_ssc.h>
> >> #include "delegation.h"
> >> #include "internal.h"
> >> #include "iostat.h"
> >>@@ -314,9 +315,8 @@ static loff_t nfs42_remap_file_range(struct file *src_file, loff_t src_off,
> >> static int read_name_gen = 1;
> >> #define SSC_READ_NAME_BODY "ssc_read_%d"
> >>-struct file *
> >>-nfs42_ssc_open(struct vfsmount *ss_mnt, struct nfs_fh *src_fh,
> >>- nfs4_stateid *stateid)
> >>+static struct file *__nfs42_ssc_open(struct vfsmount *ss_mnt,
> >>+ struct nfs_fh *src_fh, nfs4_stateid *stateid)
> >> {
> >> struct nfs_fattr fattr;
> >> struct file *filep, *res;
> >>@@ -398,14 +398,40 @@ struct file *
> >> fput(filep);
> >> goto out_free_name;
> >> }
> >>-EXPORT_SYMBOL_GPL(nfs42_ssc_open);
> >>-void nfs42_ssc_close(struct file *filep)
> >>+
> >>+static void __nfs42_ssc_close(struct file *filep)
> >> {
> >> struct nfs_open_context *ctx = nfs_file_open_context(filep);
> >> ctx->state->flags = 0;
> >> }
> >>-EXPORT_SYMBOL_GPL(nfs42_ssc_close);
> >>+
> >>+static const struct nfs4_ssc_client_ops nfs4_ssc_clnt_ops_tbl = {
> >>+ .sco_open = __nfs42_ssc_open,
> >>+ .sco_close = __nfs42_ssc_close,
> >>+};
> >>+
> >>+/**
> >>+ * nfs42_ssc_register_ops - Wrapper to register NFS_V4 ops in nfs_common
> >>+ *
> >>+ * Return values:
> >>+ * None
> >>+ */
> >>+void nfs42_ssc_register_ops(void)
> >>+{
> >>+ nfs42_ssc_register(&nfs4_ssc_clnt_ops_tbl);
> >>+}
> >>+
> >>+/**
> >>+ * nfs42_ssc_unregister_ops - wrapper to un-register NFS_V4 ops in nfs_common
> >>+ *
> >>+ * Return values:
> >>+ * None.
> >>+ */
> >>+void nfs42_ssc_unregister_ops(void)
> >>+{
> >>+ nfs42_ssc_unregister(&nfs4_ssc_clnt_ops_tbl);
> >>+}
> >> #endif /* CONFIG_NFS_V4_2 */
> >> const struct file_operations nfs4_file_operations = {
> >>diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
> >>index 0c1ab846b83d..93f5c1678ec2 100644
> >>--- a/fs/nfs/nfs4super.c
> >>+++ b/fs/nfs/nfs4super.c
> >>@@ -7,6 +7,7 @@
> >> #include <linux/mount.h>
> >> #include <linux/nfs4_mount.h>
> >> #include <linux/nfs_fs.h>
> >>+#include <linux/nfs_ssc.h>
> >> #include "delegation.h"
> >> #include "internal.h"
> >> #include "nfs4_fs.h"
> >>@@ -279,6 +280,9 @@ static int __init init_nfs_v4(void)
> >> if (err)
> >> goto out2;
> >>+#ifdef CONFIG_NFS_V4_2
> >>+ nfs42_ssc_register_ops();
> >>+#endif
> >> register_nfs_version(&nfs_v4);
> >> return 0;
> >> out2:
> >>@@ -297,6 +301,7 @@ static void __exit exit_nfs_v4(void)
> >> unregister_nfs_version(&nfs_v4);
> >> #ifdef CONFIG_NFS_V4_2
> >> nfs4_xattr_cache_exit();
> >>+ nfs42_ssc_unregister_ops();
> >> #endif
> >> nfs4_unregister_sysctl();
> >> nfs_idmap_quit();
> >>diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> >>index 7a70287f21a2..f7dad8227a5f 100644
> >>--- a/fs/nfs/super.c
> >>+++ b/fs/nfs/super.c
> >>@@ -57,6 +57,7 @@
> >> #include <linux/rcupdate.h>
> >> #include <linux/uaccess.h>
> >>+#include <linux/nfs_ssc.h>
> >> #include "nfs4_fs.h"
> >> #include "callback.h"
> >>@@ -85,6 +86,10 @@
> >> };
> >> EXPORT_SYMBOL_GPL(nfs_sops);
> >>+static const struct nfs_ssc_client_ops nfs_ssc_clnt_ops_tbl = {
> >>+ .sco_sb_deactive = nfs_sb_deactive,
> >>+};
> >>+
> >> #if IS_ENABLED(CONFIG_NFS_V4)
> >> static int __init register_nfs4_fs(void)
> >> {
> >>@@ -106,6 +111,16 @@ static void unregister_nfs4_fs(void)
> >> }
> >> #endif
> >>+static void nfs_ssc_register_ops(void)
> >>+{
> >>+ nfs_ssc_register(&nfs_ssc_clnt_ops_tbl);
> >>+}
> >>+
> >>+static void nfs_ssc_unregister_ops(void)
> >>+{
> >>+ nfs_ssc_unregister(&nfs_ssc_clnt_ops_tbl);
> >>+}
> >>+
> >> static struct shrinker acl_shrinker = {
> >> .count_objects = nfs_access_cache_count,
> >> .scan_objects = nfs_access_cache_scan,
> >>@@ -133,6 +148,7 @@ int __init register_nfs_fs(void)
> >> ret = register_shrinker(&acl_shrinker);
> >> if (ret < 0)
> >> goto error_3;
> >>+ nfs_ssc_register_ops();
> >> return 0;
> >> error_3:
> >> nfs_unregister_sysctl();
> >>@@ -152,6 +168,7 @@ void __exit unregister_nfs_fs(void)
> >> unregister_shrinker(&acl_shrinker);
> >> nfs_unregister_sysctl();
> >> unregister_nfs4_fs();
> >>+ nfs_ssc_unregister_ops();
> >> unregister_filesystem(&nfs_fs_type);
> >> }
> >>diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
> >>index 4bebe834c009..fa82f5aaa6d9 100644
> >>--- a/fs/nfs_common/Makefile
> >>+++ b/fs/nfs_common/Makefile
> >>@@ -7,3 +7,4 @@ obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
> >> nfs_acl-objs := nfsacl.o
> >> obj-$(CONFIG_GRACE_PERIOD) += grace.o
> >>+obj-$(CONFIG_GRACE_PERIOD) += nfs_ssc.o
> >>diff --git a/fs/nfs_common/nfs_ssc.c b/fs/nfs_common/nfs_ssc.c
> >>new file mode 100644
> >>index 000000000000..f43bbb373913
> >>--- /dev/null
> >>+++ b/fs/nfs_common/nfs_ssc.c
> >>@@ -0,0 +1,94 @@
> >>+// SPDX-License-Identifier: GPL-2.0-only
> >>+/*
> >>+ * fs/nfs_common/nfs_ssc_comm.c
> >>+ *
> >>+ * Helper for knfsd's SSC to access ops in NFS client modules
> >>+ *
> >>+ * Author: Dai Ngo <[email protected]>
> >>+ *
> >>+ * Copyright (c) 2020, Oracle and/or its affiliates.
> >>+ */
> >>+
> >>+#include <linux/module.h>
> >>+#include <linux/fs.h>
> >>+#include <linux/nfs_ssc.h>
> >>+#include "../nfs/nfs4_fs.h"
> >>+
> >>+MODULE_LICENSE("GPL");
> >>+
> >>+struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
> >>+EXPORT_SYMBOL_GPL(nfs_ssc_client_tbl);
> >>+
> >>+#ifdef CONFIG_NFS_V4_2
> >>+/**
> >>+ * nfs42_ssc_register - install the NFS_V4 client ops in the nfs_ssc_client_tbl
> >>+ * @ops: NFS_V4 ops to be installed
> >>+ *
> >>+ * Return values:
> >>+ * None
> >>+ */
> >>+void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops)
> >>+{
> >>+ nfs_ssc_client_tbl.ssc_nfs4_ops = ops;
> >>+}
> >>+EXPORT_SYMBOL_GPL(nfs42_ssc_register);
> >>+
> >>+/**
> >>+ * nfs42_ssc_unregister - uninstall the NFS_V4 client ops from
> >>+ * the nfs_ssc_client_tbl
> >>+ * @ops: ops to be uninstalled
> >>+ *
> >>+ * Return values:
> >>+ * None
> >>+ */
> >>+void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops)
> >>+{
> >>+ if (nfs_ssc_client_tbl.ssc_nfs4_ops != ops)
> >>+ return;
> >>+
> >>+ nfs_ssc_client_tbl.ssc_nfs4_ops = NULL;
> >>+}
> >>+EXPORT_SYMBOL_GPL(nfs42_ssc_unregister);
> >>+#endif /* CONFIG_NFS_V4_2 */
> >>+
> >>+#ifdef CONFIG_NFS_V4_2
> >>+/**
> >>+ * nfs_ssc_register - install the NFS_FS client ops in the nfs_ssc_client_tbl
> >>+ * @ops: NFS_FS ops to be installed
> >>+ *
> >>+ * Return values:
> >>+ * None
> >>+ */
> >>+void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
> >>+{
> >>+ nfs_ssc_client_tbl.ssc_nfs_ops = ops;
> >>+}
> >>+EXPORT_SYMBOL_GPL(nfs_ssc_register);
> >>+
> >>+/**
> >>+ * nfs_ssc_unregister - uninstall the NFS_FS client ops from
> >>+ * the nfs_ssc_client_tbl
> >>+ * @ops: ops to be uninstalled
> >>+ *
> >>+ * Return values:
> >>+ * None
> >>+ */
> >>+void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
> >>+{
> >>+ if (nfs_ssc_client_tbl.ssc_nfs_ops != ops)
> >>+ return;
> >>+ nfs_ssc_client_tbl.ssc_nfs_ops = NULL;
> >>+}
> >>+EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
> >>+
> >>+#else
> >>+void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
> >>+{
> >>+}
> >>+EXPORT_SYMBOL_GPL(nfs_ssc_register);
> >>+
> >>+void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
> >>+{
> >>+}
> >>+EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
> >>+#endif /* CONFIG_NFS_V4_2 */
> >>diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> >>index 99d2cae91bd6..f368f3215f88 100644
> >>--- a/fs/nfsd/Kconfig
> >>+++ b/fs/nfsd/Kconfig
> >>@@ -136,7 +136,7 @@ config NFSD_FLEXFILELAYOUT
> >> config NFSD_V4_2_INTER_SSC
> >> bool "NFSv4.2 inter server to server COPY"
> >>- depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2 && NFS_FS=y
> >>+ depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
> >> help
> >> This option enables support for NFSv4.2 inter server to
> >> server copy where the destination server calls the NFSv4.2
> >>diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>index eaf50eafa935..84e10aef1417 100644
> >>--- a/fs/nfsd/nfs4proc.c
> >>+++ b/fs/nfsd/nfs4proc.c
> >>@@ -38,6 +38,7 @@
> >> #include <linux/slab.h>
> >> #include <linux/kthread.h>
> >> #include <linux/sunrpc/addr.h>
> >>+#include <linux/nfs_ssc.h>
> >> #include "idmap.h"
> >> #include "cache.h"
> >>@@ -1247,7 +1248,7 @@ extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
> >> static void
> >> nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
> >> {
> >>- nfs_sb_deactive(ss_mnt->mnt_sb);
> >>+ nfs_do_sb_deactive(ss_mnt->mnt_sb);
> >> mntput(ss_mnt);
> >> }
> >>diff --git a/include/linux/nfs_ssc.h b/include/linux/nfs_ssc.h
> >>new file mode 100644
> >>index 000000000000..f5ba0fbff72f
> >>--- /dev/null
> >>+++ b/include/linux/nfs_ssc.h
> >>@@ -0,0 +1,67 @@
> >>+/* SPDX-License-Identifier: GPL-2.0 */
> >>+/*
> >>+ * include/linux/nfs_ssc.h
> >>+ *
> >>+ * Author: Dai Ngo <[email protected]>
> >>+ *
> >>+ * Copyright (c) 2020, Oracle and/or its affiliates.
> >>+ */
> >>+
> >>+#include <linux/nfs_fs.h>
> >>+
> >>+extern struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
> >>+
> >>+/*
> >>+ * NFS_V4
> >>+ */
> >>+struct nfs4_ssc_client_ops {
> >>+ struct file *(*sco_open)(struct vfsmount *ss_mnt,
> >>+ struct nfs_fh *src_fh, nfs4_stateid *stateid);
> >>+ void (*sco_close)(struct file *filep);
> >>+};
> >>+
> >>+/*
> >>+ * NFS_FS
> >>+ */
> >>+struct nfs_ssc_client_ops {
> >>+ void (*sco_sb_deactive)(struct super_block *sb);
> >>+};
> >>+
> >>+struct nfs_ssc_client_ops_tbl {
> >>+ const struct nfs4_ssc_client_ops *ssc_nfs4_ops;
> >>+ const struct nfs_ssc_client_ops *ssc_nfs_ops;
> >>+};
> >>+
> >>+extern void nfs42_ssc_register_ops(void);
> >>+extern void nfs42_ssc_unregister_ops(void);
> >>+
> >>+extern void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops);
> >>+extern void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops);
> >>+
> >>+#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> >>+static inline struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
> >>+ struct nfs_fh *src_fh, nfs4_stateid *stateid)
> >>+{
> >>+ if (nfs_ssc_client_tbl.ssc_nfs4_ops)
> >>+ return (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_open)(ss_mnt, src_fh, stateid);
> >>+ return ERR_PTR(-EIO);
> >>+}
> >>+
> >>+static inline void nfs42_ssc_close(struct file *filep)
> >>+{
> >>+ if (nfs_ssc_client_tbl.ssc_nfs4_ops)
> >>+ (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_close)(filep);
> >>+}
> >>+#endif
> >>+
> >>+/*
> >>+ * NFS_FS
> >>+ */
> >>+extern void nfs_ssc_register(const struct nfs_ssc_client_ops *ops);
> >>+extern void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops);
> >>+
> >>+static inline void nfs_do_sb_deactive(struct super_block *sb)
> >>+{
> >>+ if (nfs_ssc_client_tbl.ssc_nfs_ops)
> >>+ (*nfs_ssc_client_tbl.ssc_nfs_ops->sco_sb_deactive)(sb);
> >>+}
> >>--
> >>1.8.3.1

2020-11-09 19:37:45

by Dai Ngo

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On 11/9/20 10:30 AM, J. Bruce Fields wrote:
> On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
>> On 10/20/20 10:01 AM, J. Bruce Fields wrote:
>>> On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
>>>> NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
>>>> build errors and some configs with NFSD=m to get NFS4ERR_STALE
>>>> error when doing inter server copy.
>>>>
>>>> Added ops table in nfs_common for knfsd to access NFS client modules.
>>> OK, looks reasonable to me, applying. Does this resolve all the
>>> problems you've seen, or is there any bad case left?
>> Thanks Bruce.
>>
>> With this patch, I no longer see the NFS4ERR_STALE in any config.
>>
>> The problem with NFS4ERR_STALE was because of a bug in nfs42_ssc_open.
>> When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
>> returns NULL which is incorrect allowing the operation to continue
>> until nfsd4_putfh which does not have the code to handle nfserr_stale.
>>
>> With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
>> new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
>> to switch over to the split copying (read src and write to dst).
> That sounds reasonable, but I don't see any of the patches you've sent
> changing that error return. Did I overlook something, or did you mean
> to append a patch to this message?

Since with the patch, I did not run into the condition where NFS4ERR_STALE
is returned so I did not fix this return error code. Do you want me to
submit another patch to change the returned error code from NFS4ERR_STALE
to NFS4ERR_NOTSUPP if it ever runs into that condition?

-Dai

>
> --b.
>
>> The other problem that I see was the "use-after-free" which I think
>> happens only on the 1st inter server copy. I will look into this:
>>
>>
>>  Sep 23 01:08:10 nfsvmf24 kernel: ------------[ cut here ]------------
>> Sep 23 01:08:10 nfsvmf24 kernel: refcount_t: underflow; use-after-free.
>> Sep 23 01:08:10 nfsvmf24 kernel: WARNING: CPU: 0 PID: 4217 at lib/refcount.c:28 refcount_warn_saturate+0xae/0xf0
>> Sep 23 01:08:10 nfsvmf24 kernel: Modules linked in: cts rpcsec_gss_krb5 xt_REDIRECT xt_nat ip6table_nat ip6_tables iptable_nat nf_nat nf_conntrack btrfs nf_defrag_ipv6 nf_defrag_ipv4 blake2b_generic xor zstd_compress rfkill raid6_pq sb_edac intel_powerclamp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel crypto_simd cryptd glue_helper sg pcspkr i2c_piix4 video ip_tables xfs libcrc32c sd_mod t10_pi ahci libahci libata e1000 crc32c_intel serio_raw dm_mirror dm_region_hash dm_log dm_mod
>> Sep 23 01:08:10 nfsvmf24 kernel: CPU: 0 PID: 4217 Comm: copy thread Not tainted 5.9.0-rc5+ #14
>> Sep 23 01:08:10 nfsvmf24 kernel: Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
>> Sep 23 01:08:10 nfsvmf24 kernel: RIP: 0010:refcount_warn_saturate+0xae/0xf0
>> Sep 23 01:08:10 nfsvmf24 kernel: Code: 99 83 31 01 01 e8 27 ba b6 ff 0f 0b 5d c3 80 3d 86 83 31 01 00 75 91 48 c7 c7 20 d3 3a 84 c6 05 76 83 31 01 01 e8 07 ba b6 ff <0f> 0b 5d c3 80 3d 64 83 31 01 00 0f 85 6d ff ff ff 48 c7 c7 78 d3
>> Sep 23 01:08:10 nfsvmf24 kernel: RSP: 0000:ffffc20300403e68 EFLAGS: 00010286
>> Sep 23 01:08:10 nfsvmf24 kernel: RAX: 0000000000000000 RBX: 0000000000000010 RCX: 0000000000000027
>> Sep 23 01:08:10 nfsvmf24 kernel: RDX: 0000000000000027 RSI: 0000000000000086 RDI: ffff9e8c97c18c48
>> Sep 23 01:08:10 nfsvmf24 kernel: RBP: ffffc20300403e68 R08: ffff9e8c97c18c40 R09: 0000000000000004
>> Sep 23 01:08:10 nfsvmf24 kernel: R10: 0000000000000000 R11: 0000000000000001 R12: ffff9e8c96693300
>> Sep 23 01:08:10 nfsvmf24 kernel: R13: ffff9e8c96693300 R14: ffff9e8c8a6e3000 R15: ffff9e8c959c5520
>> Sep 23 01:08:10 nfsvmf24 kernel: FS: 0000000000000000(0000) GS:ffff9e8c97c00000(0000) knlGS:0000000000000000
>> Sep 23 01:08:10 nfsvmf24 kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> Sep 23 01:08:10 nfsvmf24 kernel: CR2: 000055aedfd031a8 CR3: 000000020ee6e000 CR4: 00000000000406f0
>> Sep 23 01:08:10 nfsvmf24 kernel: Call Trace:
>> Sep 23 01:08:10 nfsvmf24 kernel: nfsd_file_put_noref+0x8f/0xa0
>> Sep 23 01:08:10 nfsvmf24 kernel: nfsd_file_put+0x3e/0x90
>> Sep 23 01:08:10 nfsvmf24 kernel: nfsd4_do_copy+0xe5/0x150
>> Sep 23 01:08:10 nfsvmf24 kernel: nfsd4_do_async_copy+0x84/0x200
>> Sep 23 01:08:10 nfsvmf24 kernel: kthread+0x114/0x150
>> Sep 23 01:08:10 nfsvmf24 kernel: ? nfsd4_copy+0x4e0/0x4e0
>> Sep 23 01:08:10 nfsvmf24 kernel: ? kthread_park+0x90/0x90
>> Sep 23 01:08:10 nfsvmf24 kernel: ret_from_fork+0x22/0x30
>>
>> -Dai
>>
>>> --b.
>>>
>>>> Fixes: 3ac3711adb88 ("NFSD: Fix NFS server build errors")
>>>> Signed-off-by: Dai Ngo <[email protected]>
>>>> ---
>>>> changes from v2: fix 0-day build issues.
>>>> changes from v3: reacted to Bruce's comments, removed paranoid error checking.
>>>> ---
>>>> fs/nfs/nfs4file.c | 38 ++++++++++++++++----
>>>> fs/nfs/nfs4super.c | 5 +++
>>>> fs/nfs/super.c | 17 +++++++++
>>>> fs/nfs_common/Makefile | 1 +
>>>> fs/nfs_common/nfs_ssc.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>> fs/nfsd/Kconfig | 2 +-
>>>> fs/nfsd/nfs4proc.c | 3 +-
>>>> include/linux/nfs_ssc.h | 67 +++++++++++++++++++++++++++++++++++
>>>> 8 files changed, 219 insertions(+), 8 deletions(-)
>>>> create mode 100644 fs/nfs_common/nfs_ssc.c
>>>> create mode 100644 include/linux/nfs_ssc.h
>>>>
>>>> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
>>>> index fdfc77486ace..984938024011 100644
>>>> --- a/fs/nfs/nfs4file.c
>>>> +++ b/fs/nfs/nfs4file.c
>>>> @@ -9,6 +9,7 @@
>>>> #include <linux/falloc.h>
>>>> #include <linux/mount.h>
>>>> #include <linux/nfs_fs.h>
>>>> +#include <linux/nfs_ssc.h>
>>>> #include "delegation.h"
>>>> #include "internal.h"
>>>> #include "iostat.h"
>>>> @@ -314,9 +315,8 @@ static loff_t nfs42_remap_file_range(struct file *src_file, loff_t src_off,
>>>> static int read_name_gen = 1;
>>>> #define SSC_READ_NAME_BODY "ssc_read_%d"
>>>> -struct file *
>>>> -nfs42_ssc_open(struct vfsmount *ss_mnt, struct nfs_fh *src_fh,
>>>> - nfs4_stateid *stateid)
>>>> +static struct file *__nfs42_ssc_open(struct vfsmount *ss_mnt,
>>>> + struct nfs_fh *src_fh, nfs4_stateid *stateid)
>>>> {
>>>> struct nfs_fattr fattr;
>>>> struct file *filep, *res;
>>>> @@ -398,14 +398,40 @@ struct file *
>>>> fput(filep);
>>>> goto out_free_name;
>>>> }
>>>> -EXPORT_SYMBOL_GPL(nfs42_ssc_open);
>>>> -void nfs42_ssc_close(struct file *filep)
>>>> +
>>>> +static void __nfs42_ssc_close(struct file *filep)
>>>> {
>>>> struct nfs_open_context *ctx = nfs_file_open_context(filep);
>>>> ctx->state->flags = 0;
>>>> }
>>>> -EXPORT_SYMBOL_GPL(nfs42_ssc_close);
>>>> +
>>>> +static const struct nfs4_ssc_client_ops nfs4_ssc_clnt_ops_tbl = {
>>>> + .sco_open = __nfs42_ssc_open,
>>>> + .sco_close = __nfs42_ssc_close,
>>>> +};
>>>> +
>>>> +/**
>>>> + * nfs42_ssc_register_ops - Wrapper to register NFS_V4 ops in nfs_common
>>>> + *
>>>> + * Return values:
>>>> + * None
>>>> + */
>>>> +void nfs42_ssc_register_ops(void)
>>>> +{
>>>> + nfs42_ssc_register(&nfs4_ssc_clnt_ops_tbl);
>>>> +}
>>>> +
>>>> +/**
>>>> + * nfs42_ssc_unregister_ops - wrapper to un-register NFS_V4 ops in nfs_common
>>>> + *
>>>> + * Return values:
>>>> + * None.
>>>> + */
>>>> +void nfs42_ssc_unregister_ops(void)
>>>> +{
>>>> + nfs42_ssc_unregister(&nfs4_ssc_clnt_ops_tbl);
>>>> +}
>>>> #endif /* CONFIG_NFS_V4_2 */
>>>> const struct file_operations nfs4_file_operations = {
>>>> diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
>>>> index 0c1ab846b83d..93f5c1678ec2 100644
>>>> --- a/fs/nfs/nfs4super.c
>>>> +++ b/fs/nfs/nfs4super.c
>>>> @@ -7,6 +7,7 @@
>>>> #include <linux/mount.h>
>>>> #include <linux/nfs4_mount.h>
>>>> #include <linux/nfs_fs.h>
>>>> +#include <linux/nfs_ssc.h>
>>>> #include "delegation.h"
>>>> #include "internal.h"
>>>> #include "nfs4_fs.h"
>>>> @@ -279,6 +280,9 @@ static int __init init_nfs_v4(void)
>>>> if (err)
>>>> goto out2;
>>>> +#ifdef CONFIG_NFS_V4_2
>>>> + nfs42_ssc_register_ops();
>>>> +#endif
>>>> register_nfs_version(&nfs_v4);
>>>> return 0;
>>>> out2:
>>>> @@ -297,6 +301,7 @@ static void __exit exit_nfs_v4(void)
>>>> unregister_nfs_version(&nfs_v4);
>>>> #ifdef CONFIG_NFS_V4_2
>>>> nfs4_xattr_cache_exit();
>>>> + nfs42_ssc_unregister_ops();
>>>> #endif
>>>> nfs4_unregister_sysctl();
>>>> nfs_idmap_quit();
>>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>>> index 7a70287f21a2..f7dad8227a5f 100644
>>>> --- a/fs/nfs/super.c
>>>> +++ b/fs/nfs/super.c
>>>> @@ -57,6 +57,7 @@
>>>> #include <linux/rcupdate.h>
>>>> #include <linux/uaccess.h>
>>>> +#include <linux/nfs_ssc.h>
>>>> #include "nfs4_fs.h"
>>>> #include "callback.h"
>>>> @@ -85,6 +86,10 @@
>>>> };
>>>> EXPORT_SYMBOL_GPL(nfs_sops);
>>>> +static const struct nfs_ssc_client_ops nfs_ssc_clnt_ops_tbl = {
>>>> + .sco_sb_deactive = nfs_sb_deactive,
>>>> +};
>>>> +
>>>> #if IS_ENABLED(CONFIG_NFS_V4)
>>>> static int __init register_nfs4_fs(void)
>>>> {
>>>> @@ -106,6 +111,16 @@ static void unregister_nfs4_fs(void)
>>>> }
>>>> #endif
>>>> +static void nfs_ssc_register_ops(void)
>>>> +{
>>>> + nfs_ssc_register(&nfs_ssc_clnt_ops_tbl);
>>>> +}
>>>> +
>>>> +static void nfs_ssc_unregister_ops(void)
>>>> +{
>>>> + nfs_ssc_unregister(&nfs_ssc_clnt_ops_tbl);
>>>> +}
>>>> +
>>>> static struct shrinker acl_shrinker = {
>>>> .count_objects = nfs_access_cache_count,
>>>> .scan_objects = nfs_access_cache_scan,
>>>> @@ -133,6 +148,7 @@ int __init register_nfs_fs(void)
>>>> ret = register_shrinker(&acl_shrinker);
>>>> if (ret < 0)
>>>> goto error_3;
>>>> + nfs_ssc_register_ops();
>>>> return 0;
>>>> error_3:
>>>> nfs_unregister_sysctl();
>>>> @@ -152,6 +168,7 @@ void __exit unregister_nfs_fs(void)
>>>> unregister_shrinker(&acl_shrinker);
>>>> nfs_unregister_sysctl();
>>>> unregister_nfs4_fs();
>>>> + nfs_ssc_unregister_ops();
>>>> unregister_filesystem(&nfs_fs_type);
>>>> }
>>>> diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
>>>> index 4bebe834c009..fa82f5aaa6d9 100644
>>>> --- a/fs/nfs_common/Makefile
>>>> +++ b/fs/nfs_common/Makefile
>>>> @@ -7,3 +7,4 @@ obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
>>>> nfs_acl-objs := nfsacl.o
>>>> obj-$(CONFIG_GRACE_PERIOD) += grace.o
>>>> +obj-$(CONFIG_GRACE_PERIOD) += nfs_ssc.o
>>>> diff --git a/fs/nfs_common/nfs_ssc.c b/fs/nfs_common/nfs_ssc.c
>>>> new file mode 100644
>>>> index 000000000000..f43bbb373913
>>>> --- /dev/null
>>>> +++ b/fs/nfs_common/nfs_ssc.c
>>>> @@ -0,0 +1,94 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * fs/nfs_common/nfs_ssc_comm.c
>>>> + *
>>>> + * Helper for knfsd's SSC to access ops in NFS client modules
>>>> + *
>>>> + * Author: Dai Ngo <[email protected]>
>>>> + *
>>>> + * Copyright (c) 2020, Oracle and/or its affiliates.
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/fs.h>
>>>> +#include <linux/nfs_ssc.h>
>>>> +#include "../nfs/nfs4_fs.h"
>>>> +
>>>> +MODULE_LICENSE("GPL");
>>>> +
>>>> +struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
>>>> +EXPORT_SYMBOL_GPL(nfs_ssc_client_tbl);
>>>> +
>>>> +#ifdef CONFIG_NFS_V4_2
>>>> +/**
>>>> + * nfs42_ssc_register - install the NFS_V4 client ops in the nfs_ssc_client_tbl
>>>> + * @ops: NFS_V4 ops to be installed
>>>> + *
>>>> + * Return values:
>>>> + * None
>>>> + */
>>>> +void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops)
>>>> +{
>>>> + nfs_ssc_client_tbl.ssc_nfs4_ops = ops;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(nfs42_ssc_register);
>>>> +
>>>> +/**
>>>> + * nfs42_ssc_unregister - uninstall the NFS_V4 client ops from
>>>> + * the nfs_ssc_client_tbl
>>>> + * @ops: ops to be uninstalled
>>>> + *
>>>> + * Return values:
>>>> + * None
>>>> + */
>>>> +void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops)
>>>> +{
>>>> + if (nfs_ssc_client_tbl.ssc_nfs4_ops != ops)
>>>> + return;
>>>> +
>>>> + nfs_ssc_client_tbl.ssc_nfs4_ops = NULL;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(nfs42_ssc_unregister);
>>>> +#endif /* CONFIG_NFS_V4_2 */
>>>> +
>>>> +#ifdef CONFIG_NFS_V4_2
>>>> +/**
>>>> + * nfs_ssc_register - install the NFS_FS client ops in the nfs_ssc_client_tbl
>>>> + * @ops: NFS_FS ops to be installed
>>>> + *
>>>> + * Return values:
>>>> + * None
>>>> + */
>>>> +void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
>>>> +{
>>>> + nfs_ssc_client_tbl.ssc_nfs_ops = ops;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(nfs_ssc_register);
>>>> +
>>>> +/**
>>>> + * nfs_ssc_unregister - uninstall the NFS_FS client ops from
>>>> + * the nfs_ssc_client_tbl
>>>> + * @ops: ops to be uninstalled
>>>> + *
>>>> + * Return values:
>>>> + * None
>>>> + */
>>>> +void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
>>>> +{
>>>> + if (nfs_ssc_client_tbl.ssc_nfs_ops != ops)
>>>> + return;
>>>> + nfs_ssc_client_tbl.ssc_nfs_ops = NULL;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
>>>> +
>>>> +#else
>>>> +void nfs_ssc_register(const struct nfs_ssc_client_ops *ops)
>>>> +{
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(nfs_ssc_register);
>>>> +
>>>> +void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops)
>>>> +{
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(nfs_ssc_unregister);
>>>> +#endif /* CONFIG_NFS_V4_2 */
>>>> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
>>>> index 99d2cae91bd6..f368f3215f88 100644
>>>> --- a/fs/nfsd/Kconfig
>>>> +++ b/fs/nfsd/Kconfig
>>>> @@ -136,7 +136,7 @@ config NFSD_FLEXFILELAYOUT
>>>> config NFSD_V4_2_INTER_SSC
>>>> bool "NFSv4.2 inter server to server COPY"
>>>> - depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2 && NFS_FS=y
>>>> + depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
>>>> help
>>>> This option enables support for NFSv4.2 inter server to
>>>> server copy where the destination server calls the NFSv4.2
>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>> index eaf50eafa935..84e10aef1417 100644
>>>> --- a/fs/nfsd/nfs4proc.c
>>>> +++ b/fs/nfsd/nfs4proc.c
>>>> @@ -38,6 +38,7 @@
>>>> #include <linux/slab.h>
>>>> #include <linux/kthread.h>
>>>> #include <linux/sunrpc/addr.h>
>>>> +#include <linux/nfs_ssc.h>
>>>> #include "idmap.h"
>>>> #include "cache.h"
>>>> @@ -1247,7 +1248,7 @@ extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
>>>> static void
>>>> nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
>>>> {
>>>> - nfs_sb_deactive(ss_mnt->mnt_sb);
>>>> + nfs_do_sb_deactive(ss_mnt->mnt_sb);
>>>> mntput(ss_mnt);
>>>> }
>>>> diff --git a/include/linux/nfs_ssc.h b/include/linux/nfs_ssc.h
>>>> new file mode 100644
>>>> index 000000000000..f5ba0fbff72f
>>>> --- /dev/null
>>>> +++ b/include/linux/nfs_ssc.h
>>>> @@ -0,0 +1,67 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * include/linux/nfs_ssc.h
>>>> + *
>>>> + * Author: Dai Ngo <[email protected]>
>>>> + *
>>>> + * Copyright (c) 2020, Oracle and/or its affiliates.
>>>> + */
>>>> +
>>>> +#include <linux/nfs_fs.h>
>>>> +
>>>> +extern struct nfs_ssc_client_ops_tbl nfs_ssc_client_tbl;
>>>> +
>>>> +/*
>>>> + * NFS_V4
>>>> + */
>>>> +struct nfs4_ssc_client_ops {
>>>> + struct file *(*sco_open)(struct vfsmount *ss_mnt,
>>>> + struct nfs_fh *src_fh, nfs4_stateid *stateid);
>>>> + void (*sco_close)(struct file *filep);
>>>> +};
>>>> +
>>>> +/*
>>>> + * NFS_FS
>>>> + */
>>>> +struct nfs_ssc_client_ops {
>>>> + void (*sco_sb_deactive)(struct super_block *sb);
>>>> +};
>>>> +
>>>> +struct nfs_ssc_client_ops_tbl {
>>>> + const struct nfs4_ssc_client_ops *ssc_nfs4_ops;
>>>> + const struct nfs_ssc_client_ops *ssc_nfs_ops;
>>>> +};
>>>> +
>>>> +extern void nfs42_ssc_register_ops(void);
>>>> +extern void nfs42_ssc_unregister_ops(void);
>>>> +
>>>> +extern void nfs42_ssc_register(const struct nfs4_ssc_client_ops *ops);
>>>> +extern void nfs42_ssc_unregister(const struct nfs4_ssc_client_ops *ops);
>>>> +
>>>> +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
>>>> +static inline struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
>>>> + struct nfs_fh *src_fh, nfs4_stateid *stateid)
>>>> +{
>>>> + if (nfs_ssc_client_tbl.ssc_nfs4_ops)
>>>> + return (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_open)(ss_mnt, src_fh, stateid);
>>>> + return ERR_PTR(-EIO);
>>>> +}
>>>> +
>>>> +static inline void nfs42_ssc_close(struct file *filep)
>>>> +{
>>>> + if (nfs_ssc_client_tbl.ssc_nfs4_ops)
>>>> + (*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_close)(filep);
>>>> +}
>>>> +#endif
>>>> +
>>>> +/*
>>>> + * NFS_FS
>>>> + */
>>>> +extern void nfs_ssc_register(const struct nfs_ssc_client_ops *ops);
>>>> +extern void nfs_ssc_unregister(const struct nfs_ssc_client_ops *ops);
>>>> +
>>>> +static inline void nfs_do_sb_deactive(struct super_block *sb)
>>>> +{
>>>> + if (nfs_ssc_client_tbl.ssc_nfs_ops)
>>>> + (*nfs_ssc_client_tbl.ssc_nfs_ops->sco_sb_deactive)(sb);
>>>> +}
>>>> --
>>>> 1.8.3.1

2020-11-09 20:45:01

by J. Bruce Fields

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
>
> On 11/9/20 10:30 AM, J. Bruce Fields wrote:
> >On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
> >>On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> >>>On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> >>>>NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> >>>>build errors and some configs with NFSD=m to get NFS4ERR_STALE
> >>>>error when doing inter server copy.
> >>>>
> >>>>Added ops table in nfs_common for knfsd to access NFS client modules.
> >>>OK, looks reasonable to me, applying. Does this resolve all the
> >>>problems you've seen, or is there any bad case left?
> >>Thanks Bruce.
> >>
> >>With this patch, I no longer see the NFS4ERR_STALE in any config.
> >>
> >>The problem with NFS4ERR_STALE was because of a bug in nfs42_ssc_open.
> >>When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
> >>returns NULL which is incorrect allowing the operation to continue
> >>until nfsd4_putfh which does not have the code to handle nfserr_stale.
> >>
> >>With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
> >>new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
> >>to switch over to the split copying (read src and write to dst).
> >That sounds reasonable, but I don't see any of the patches you've sent
> >changing that error return. Did I overlook something, or did you mean
> >to append a patch to this message?
>
> Since with the patch, I did not run into the condition where NFS4ERR_STALE
> is returned so I did not fix this return error code. Do you want me to
> submit another patch to change the returned error code from NFS4ERR_STALE
> to NFS4ERR_NOTSUPP if it ever runs into that condition?

That would be great, thanks. (I mean, it is still possible to hit that
case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)

--b.

2020-11-09 22:27:50

by Dai Ngo

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On 11/9/20 12:42 PM, J. Bruce Fields wrote:
> On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
>> On 11/9/20 10:30 AM, J. Bruce Fields wrote:
>>> On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
>>>> On 10/20/20 10:01 AM, J. Bruce Fields wrote:
>>>>> On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
>>>>>> NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
>>>>>> build errors and some configs with NFSD=m to get NFS4ERR_STALE
>>>>>> error when doing inter server copy.
>>>>>>
>>>>>> Added ops table in nfs_common for knfsd to access NFS client modules.
>>>>> OK, looks reasonable to me, applying. Does this resolve all the
>>>>> problems you've seen, or is there any bad case left?
>>>> Thanks Bruce.
>>>>
>>>> With this patch, I no longer see the NFS4ERR_STALE in any config.
>>>>
>>>> The problem with NFS4ERR_STALE was because of a bug in nfs42_ssc_open.
>>>> When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
>>>> returns NULL which is incorrect allowing the operation to continue
>>>> until nfsd4_putfh which does not have the code to handle nfserr_stale.
>>>>
>>>> With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
>>>> new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
>>>> to switch over to the split copying (read src and write to dst).
>>> That sounds reasonable, but I don't see any of the patches you've sent
>>> changing that error return. Did I overlook something, or did you mean
>>> to append a patch to this message?
>> Since with the patch, I did not run into the condition where NFS4ERR_STALE
>> is returned so I did not fix this return error code. Do you want me to
>> submit another patch to change the returned error code from NFS4ERR_STALE
>> to NFS4ERR_NOTSUPP if it ever runs into that condition?
> That would be great, thanks. (I mean, it is still possible to hit that
> case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)

will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
this case. Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
the old code, which causes the NFS client to switch over to the split
copying (read src and write to dst).
Returning NULL in the old nfs42_ssc_open is not correct, it allows the copy
operation to proceed and hits the NFS4ERR_STALE case in the COPY operation.

-Dai

>
> --b.

2020-11-10 06:47:02

by Dai Ngo

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On 11/9/20 2:26 PM, Dai Ngo wrote:
>
> On 11/9/20 12:42 PM, J. Bruce Fields wrote:
>> On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
>>> On 11/9/20 10:30 AM, J. Bruce Fields wrote:
>>>> On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
>>>>> On 10/20/20 10:01 AM, J. Bruce Fields wrote:
>>>>>> On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
>>>>>>> NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
>>>>>>> build errors and some configs with NFSD=m to get NFS4ERR_STALE
>>>>>>> error when doing inter server copy.
>>>>>>>
>>>>>>> Added ops table in nfs_common for knfsd to access NFS client
>>>>>>> modules.
>>>>>> OK, looks reasonable to me, applying.  Does this resolve all the
>>>>>> problems you've seen, or is there any bad case left?
>>>>> Thanks Bruce.
>>>>>
>>>>> With this patch, I no longer see the NFS4ERR_STALE in any config.
>>>>>
>>>>> The problem with NFS4ERR_STALE was because of a bug in
>>>>> nfs42_ssc_open.
>>>>> When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
>>>>> returns NULL which is incorrect allowing the operation to continue
>>>>> until nfsd4_putfh which does not have the code to handle
>>>>> nfserr_stale.
>>>>>
>>>>> With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
>>>>> new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
>>>>> to switch over to the split copying (read src and write to dst).
>>>> That sounds reasonable, but I don't see any of the patches you've sent
>>>> changing that error return.  Did I overlook something, or did you mean
>>>> to append a patch to this message?
>>> Since with the patch, I did not run into the condition where
>>> NFS4ERR_STALE
>>> is returned so I did not fix this return error code. Do you want me to
>>> submit another patch to change the returned error code from
>>> NFS4ERR_STALE
>>> to NFS4ERR_NOTSUPP if it ever runs into that condition?
>> That would be great, thanks.  (I mean, it is still possible to hit that
>> case, right?  You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
>
> will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
> this case.

I need to qualify this, the copy_file_range syscall did not return
ESTALE in the test.

> Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
> defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
> the old code, which causes the NFS client to switch over to the split
> copying (read src and write to dst).

This is not the reason why the client switches to generic_copy_file_range.

> Returning NULL in the old nfs42_ssc_open is not correct, it allows the
> copy
> operation to proceed and hits the NFS4ERR_STALE case in the COPY
> operation.

I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
returned for the PUTFH of the SRC in the COPY compound. However on the
client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
to do the copy.

The ESTALE error is only returned by copy_file_range if the client
does not have commit 7e350197a1c10. So I think there is no need to
make any change on the source server for the NFS4ERR_STALE error.

-Dai

>
>
> -Dai
>
>>
>> --b.

2020-11-10 20:14:38

by J. Bruce Fields

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
>
> On 11/9/20 2:26 PM, Dai Ngo wrote:
> >
> >On 11/9/20 12:42 PM, J. Bruce Fields wrote:
> >>On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
> >>>On 11/9/20 10:30 AM, J. Bruce Fields wrote:
> >>>>On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
> >>>>>On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> >>>>>>On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> >>>>>>>NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> >>>>>>>build errors and some configs with NFSD=m to get NFS4ERR_STALE
> >>>>>>>error when doing inter server copy.
> >>>>>>>
> >>>>>>>Added ops table in nfs_common for knfsd to access NFS
> >>>>>>>client modules.
> >>>>>>OK, looks reasonable to me, applying.  Does this resolve all the
> >>>>>>problems you've seen, or is there any bad case left?
> >>>>>Thanks Bruce.
> >>>>>
> >>>>>With this patch, I no longer see the NFS4ERR_STALE in any config.
> >>>>>
> >>>>>The problem with NFS4ERR_STALE was because of a bug in
> >>>>>nfs42_ssc_open.
> >>>>>When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
> >>>>>returns NULL which is incorrect allowing the operation to continue
> >>>>>until nfsd4_putfh which does not have the code to handle
> >>>>>nfserr_stale.
> >>>>>
> >>>>>With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
> >>>>>new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
> >>>>>to switch over to the split copying (read src and write to dst).
> >>>>That sounds reasonable, but I don't see any of the patches you've sent
> >>>>changing that error return.  Did I overlook something, or did you mean
> >>>>to append a patch to this message?
> >>>Since with the patch, I did not run into the condition where
> >>>NFS4ERR_STALE
> >>>is returned so I did not fix this return error code. Do you want me to
> >>>submit another patch to change the returned error code from
> >>>NFS4ERR_STALE
> >>>to NFS4ERR_NOTSUPP if it ever runs into that condition?
> >>That would be great, thanks.  (I mean, it is still possible to hit that
> >>case, right?  You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
> >
> >will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
> >this case.
>
> I need to qualify this, the copy_file_range syscall did not return
> ESTALE in the test.
>
> >Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
> >defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
> >the old code, which causes the NFS client to switch over to the split
> >copying (read src and write to dst).
>
> This is not the reason why the client switches to generic_copy_file_range.
>
> >Returning NULL in the old nfs42_ssc_open is not correct, it allows
> >the copy
> >operation to proceed and hits the NFS4ERR_STALE case in the COPY
> >operation.
>
> I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
> returned for the PUTFH of the SRC in the COPY compound. However on the
> client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
> with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
> to do the copy.
>
> The ESTALE error is only returned by copy_file_range if the client
> does not have commit 7e350197a1c10. So I think there is no need to
> make any change on the source server for the NFS4ERR_STALE error.

I don't believe NFS4ERR_STALE is the correct error for the server to
return. It's nice that the client is able to do the right thing despite
the server returning the wrong error, but we should still try to get
this right on the server.

--b.

2020-11-10 21:08:32

by Olga Kornievskaia

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <[email protected]> wrote:
>
> On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
> >
> > On 11/9/20 2:26 PM, Dai Ngo wrote:
> > >
> > >On 11/9/20 12:42 PM, J. Bruce Fields wrote:
> > >>On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
> > >>>On 11/9/20 10:30 AM, J. Bruce Fields wrote:
> > >>>>On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
> > >>>>>On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> > >>>>>>On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> > >>>>>>>NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> > >>>>>>>build errors and some configs with NFSD=m to get NFS4ERR_STALE
> > >>>>>>>error when doing inter server copy.
> > >>>>>>>
> > >>>>>>>Added ops table in nfs_common for knfsd to access NFS
> > >>>>>>>client modules.
> > >>>>>>OK, looks reasonable to me, applying. Does this resolve all the
> > >>>>>>problems you've seen, or is there any bad case left?
> > >>>>>Thanks Bruce.
> > >>>>>
> > >>>>>With this patch, I no longer see the NFS4ERR_STALE in any config.
> > >>>>>
> > >>>>>The problem with NFS4ERR_STALE was because of a bug in
> > >>>>>nfs42_ssc_open.
> > >>>>>When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
> > >>>>>returns NULL which is incorrect allowing the operation to continue
> > >>>>>until nfsd4_putfh which does not have the code to handle
> > >>>>>nfserr_stale.
> > >>>>>
> > >>>>>With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
> > >>>>>new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
> > >>>>>to switch over to the split copying (read src and write to dst).
> > >>>>That sounds reasonable, but I don't see any of the patches you've sent
> > >>>>changing that error return. Did I overlook something, or did you mean
> > >>>>to append a patch to this message?
> > >>>Since with the patch, I did not run into the condition where
> > >>>NFS4ERR_STALE
> > >>>is returned so I did not fix this return error code. Do you want me to
> > >>>submit another patch to change the returned error code from
> > >>>NFS4ERR_STALE
> > >>>to NFS4ERR_NOTSUPP if it ever runs into that condition?
> > >>That would be great, thanks. (I mean, it is still possible to hit that
> > >>case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
> > >
> > >will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
> > >this case.
> >
> > I need to qualify this, the copy_file_range syscall did not return
> > ESTALE in the test.
> >
> > >Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
> > >defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
> > >the old code, which causes the NFS client to switch over to the split
> > >copying (read src and write to dst).
> >
> > This is not the reason why the client switches to generic_copy_file_range.
> >
> > >Returning NULL in the old nfs42_ssc_open is not correct, it allows
> > >the copy
> > >operation to proceed and hits the NFS4ERR_STALE case in the COPY
> > >operation.
> >
> > I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
> > returned for the PUTFH of the SRC in the COPY compound. However on the
> > client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
> > with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
> > to do the copy.
> >
> > The ESTALE error is only returned by copy_file_range if the client
> > does not have commit 7e350197a1c10. So I think there is no need to
> > make any change on the source server for the NFS4ERR_STALE error.
>
> I don't believe NFS4ERR_STALE is the correct error for the server to
> return. It's nice that the client is able to do the right thing despite
> the server returning the wrong error, but we should still try to get
> this right on the server.

Hi Bruce,

ERR_STALE is the appropriate error to be returned by the server that
gets a COPY compound when it doesn't support COPY. Since server can't
understand the filehandle so it can't process it so we can't get to
processing COPY opcode where the server could have returned
EOPNOTSUPP. Thus a client side patch is needed and the server is doing
everything it can in the situation.

I'm confused about the title of this patch. I thought what it does is
removes NFSD dependency on the NFS and instead loads the needed
function dynamically.

Honestly, I don't understand why that allows removal of the NFS_FS
from the dependencies I don't understand. nfs4_ssc_open calls nfs
client functions that are built when NFS_FS is compiled but I'm
assuming will not be otherwise.

>
> --b.

2020-11-10 21:52:56

by J. Bruce Fields

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Tue, Nov 10, 2020 at 04:07:41PM -0500, Olga Kornievskaia wrote:
> On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <[email protected]> wrote:
> >
> > On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
> > >
> > > On 11/9/20 2:26 PM, Dai Ngo wrote:
> > > >
> > > >On 11/9/20 12:42 PM, J. Bruce Fields wrote:
> > > >>On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
> > > >>>On 11/9/20 10:30 AM, J. Bruce Fields wrote:
> > > >>>>On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
> > > >>>>>On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> > > >>>>>>On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> > > >>>>>>>NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> > > >>>>>>>build errors and some configs with NFSD=m to get NFS4ERR_STALE
> > > >>>>>>>error when doing inter server copy.
> > > >>>>>>>
> > > >>>>>>>Added ops table in nfs_common for knfsd to access NFS
> > > >>>>>>>client modules.
> > > >>>>>>OK, looks reasonable to me, applying. Does this resolve all the
> > > >>>>>>problems you've seen, or is there any bad case left?
> > > >>>>>Thanks Bruce.
> > > >>>>>
> > > >>>>>With this patch, I no longer see the NFS4ERR_STALE in any config.
> > > >>>>>
> > > >>>>>The problem with NFS4ERR_STALE was because of a bug in
> > > >>>>>nfs42_ssc_open.
> > > >>>>>When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
> > > >>>>>returns NULL which is incorrect allowing the operation to continue
> > > >>>>>until nfsd4_putfh which does not have the code to handle
> > > >>>>>nfserr_stale.
> > > >>>>>
> > > >>>>>With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
> > > >>>>>new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
> > > >>>>>to switch over to the split copying (read src and write to dst).
> > > >>>>That sounds reasonable, but I don't see any of the patches you've sent
> > > >>>>changing that error return. Did I overlook something, or did you mean
> > > >>>>to append a patch to this message?
> > > >>>Since with the patch, I did not run into the condition where
> > > >>>NFS4ERR_STALE
> > > >>>is returned so I did not fix this return error code. Do you want me to
> > > >>>submit another patch to change the returned error code from
> > > >>>NFS4ERR_STALE
> > > >>>to NFS4ERR_NOTSUPP if it ever runs into that condition?
> > > >>That would be great, thanks. (I mean, it is still possible to hit that
> > > >>case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
> > > >
> > > >will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
> > > >this case.
> > >
> > > I need to qualify this, the copy_file_range syscall did not return
> > > ESTALE in the test.
> > >
> > > >Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
> > > >defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
> > > >the old code, which causes the NFS client to switch over to the split
> > > >copying (read src and write to dst).
> > >
> > > This is not the reason why the client switches to generic_copy_file_range.
> > >
> > > >Returning NULL in the old nfs42_ssc_open is not correct, it allows
> > > >the copy
> > > >operation to proceed and hits the NFS4ERR_STALE case in the COPY
> > > >operation.
> > >
> > > I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
> > > returned for the PUTFH of the SRC in the COPY compound. However on the
> > > client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
> > > with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
> > > to do the copy.
> > >
> > > The ESTALE error is only returned by copy_file_range if the client
> > > does not have commit 7e350197a1c10. So I think there is no need to
> > > make any change on the source server for the NFS4ERR_STALE error.
> >
> > I don't believe NFS4ERR_STALE is the correct error for the server to
> > return. It's nice that the client is able to do the right thing despite
> > the server returning the wrong error, but we should still try to get
> > this right on the server.
>
> Hi Bruce,
>
> ERR_STALE is the appropriate error to be returned by the server that
> gets a COPY compound when it doesn't support COPY. Since server can't
> understand the filehandle so it can't process it so we can't get to
> processing COPY opcode where the server could have returned
> EOPNOTSUPP.

The case we're discussing is the case where we support COPY but not
server-to-server copy.

--b.

> Thus a client side patch is needed and the server is doing
> everything it can in the situation.
>
> I'm confused about the title of this patch. I thought what it does is
> removes NFSD dependency on the NFS and instead loads the needed
> function dynamically.
>
> Honestly, I don't understand why that allows removal of the NFS_FS
> from the dependencies I don't understand. nfs4_ssc_open calls nfs
> client functions that are built when NFS_FS is compiled but I'm
> assuming will not be otherwise.

2020-11-10 21:56:47

by Dai Ngo

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On 11/10/20 1:07 PM, Olga Kornievskaia wrote:
> On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <[email protected]> wrote:
>> On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
>>> On 11/9/20 2:26 PM, Dai Ngo wrote:
>>>> On 11/9/20 12:42 PM, J. Bruce Fields wrote:
>>>>> On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
>>>>>> On 11/9/20 10:30 AM, J. Bruce Fields wrote:
>>>>>>> On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
>>>>>>>> On 10/20/20 10:01 AM, J. Bruce Fields wrote:
>>>>>>>>> On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
>>>>>>>>>> NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
>>>>>>>>>> build errors and some configs with NFSD=m to get NFS4ERR_STALE
>>>>>>>>>> error when doing inter server copy.
>>>>>>>>>>
>>>>>>>>>> Added ops table in nfs_common for knfsd to access NFS
>>>>>>>>>> client modules.
>>>>>>>>> OK, looks reasonable to me, applying. Does this resolve all the
>>>>>>>>> problems you've seen, or is there any bad case left?
>>>>>>>> Thanks Bruce.
>>>>>>>>
>>>>>>>> With this patch, I no longer see the NFS4ERR_STALE in any config.
>>>>>>>>
>>>>>>>> The problem with NFS4ERR_STALE was because of a bug in
>>>>>>>> nfs42_ssc_open.
>>>>>>>> When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
>>>>>>>> returns NULL which is incorrect allowing the operation to continue
>>>>>>>> until nfsd4_putfh which does not have the code to handle
>>>>>>>> nfserr_stale.
>>>>>>>>
>>>>>>>> With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
>>>>>>>> new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
>>>>>>>> to switch over to the split copying (read src and write to dst).
>>>>>>> That sounds reasonable, but I don't see any of the patches you've sent
>>>>>>> changing that error return. Did I overlook something, or did you mean
>>>>>>> to append a patch to this message?
>>>>>> Since with the patch, I did not run into the condition where
>>>>>> NFS4ERR_STALE
>>>>>> is returned so I did not fix this return error code. Do you want me to
>>>>>> submit another patch to change the returned error code from
>>>>>> NFS4ERR_STALE
>>>>>> to NFS4ERR_NOTSUPP if it ever runs into that condition?
>>>>> That would be great, thanks. (I mean, it is still possible to hit that
>>>>> case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
>>>> will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
>>>> this case.
>>> I need to qualify this, the copy_file_range syscall did not return
>>> ESTALE in the test.
>>>
>>>> Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
>>>> defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
>>>> the old code, which causes the NFS client to switch over to the split
>>>> copying (read src and write to dst).
>>> This is not the reason why the client switches to generic_copy_file_range.
>>>
>>>> Returning NULL in the old nfs42_ssc_open is not correct, it allows
>>>> the copy
>>>> operation to proceed and hits the NFS4ERR_STALE case in the COPY
>>>> operation.
>>> I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
>>> returned for the PUTFH of the SRC in the COPY compound. However on the
>>> client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
>>> with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
>>> to do the copy.
>>>
>>> The ESTALE error is only returned by copy_file_range if the client
>>> does not have commit 7e350197a1c10. So I think there is no need to
>>> make any change on the source server for the NFS4ERR_STALE error.
>> I don't believe NFS4ERR_STALE is the correct error for the server to
>> return. It's nice that the client is able to do the right thing despite
>> the server returning the wrong error, but we should still try to get
>> this right on the server.

Hi Olga,

> Hi Bruce,
>
> ERR_STALE is the appropriate error to be returned by the server that
> gets a COPY compound when it doesn't support COPY. Since server can't
> understand the filehandle so it can't process it so we can't get to
> processing COPY opcode where the server could have returned
> EOPNOTSUPP. Thus a client side patch is needed and the server is doing
> everything it can in the situation.
>
> I'm confused about the title of this patch. I thought what it does is
> removes NFSD dependency on the NFS and instead loads the needed
> function dynamically.

Yes, the title of the patch is misleading. The main goal of the patch
is to add the ops table in nfs_common for knfsd to access NFS client
modules without calling these functions directly. This also prevents
build errors in config such as when NFSD=y and NFS_FS=m.

-Dai

>
> Honestly, I don't understand why that allows removal of the NFS_FS
> from the dependencies I don't understand. nfs4_ssc_open calls nfs
> client functions that are built when NFS_FS is compiled but I'm
> assuming will not be otherwise.
>
>
>> --b.

2020-11-10 22:12:07

by Olga Kornievskaia

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Tue, Nov 10, 2020 at 4:52 PM J. Bruce Fields <[email protected]> wrote:
>
> On Tue, Nov 10, 2020 at 04:07:41PM -0500, Olga Kornievskaia wrote:
> > On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <[email protected]> wrote:
> > >
> > > On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
> > > >
> > > > On 11/9/20 2:26 PM, Dai Ngo wrote:
> > > > >
> > > > >On 11/9/20 12:42 PM, J. Bruce Fields wrote:
> > > > >>On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
> > > > >>>On 11/9/20 10:30 AM, J. Bruce Fields wrote:
> > > > >>>>On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
> > > > >>>>>On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> > > > >>>>>>On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> > > > >>>>>>>NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> > > > >>>>>>>build errors and some configs with NFSD=m to get NFS4ERR_STALE
> > > > >>>>>>>error when doing inter server copy.
> > > > >>>>>>>
> > > > >>>>>>>Added ops table in nfs_common for knfsd to access NFS
> > > > >>>>>>>client modules.
> > > > >>>>>>OK, looks reasonable to me, applying. Does this resolve all the
> > > > >>>>>>problems you've seen, or is there any bad case left?
> > > > >>>>>Thanks Bruce.
> > > > >>>>>
> > > > >>>>>With this patch, I no longer see the NFS4ERR_STALE in any config.
> > > > >>>>>
> > > > >>>>>The problem with NFS4ERR_STALE was because of a bug in
> > > > >>>>>nfs42_ssc_open.
> > > > >>>>>When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
> > > > >>>>>returns NULL which is incorrect allowing the operation to continue
> > > > >>>>>until nfsd4_putfh which does not have the code to handle
> > > > >>>>>nfserr_stale.
> > > > >>>>>
> > > > >>>>>With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
> > > > >>>>>new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
> > > > >>>>>to switch over to the split copying (read src and write to dst).
> > > > >>>>That sounds reasonable, but I don't see any of the patches you've sent
> > > > >>>>changing that error return. Did I overlook something, or did you mean
> > > > >>>>to append a patch to this message?
> > > > >>>Since with the patch, I did not run into the condition where
> > > > >>>NFS4ERR_STALE
> > > > >>>is returned so I did not fix this return error code. Do you want me to
> > > > >>>submit another patch to change the returned error code from
> > > > >>>NFS4ERR_STALE
> > > > >>>to NFS4ERR_NOTSUPP if it ever runs into that condition?
> > > > >>That would be great, thanks. (I mean, it is still possible to hit that
> > > > >>case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
> > > > >
> > > > >will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
> > > > >this case.
> > > >
> > > > I need to qualify this, the copy_file_range syscall did not return
> > > > ESTALE in the test.
> > > >
> > > > >Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
> > > > >defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
> > > > >the old code, which causes the NFS client to switch over to the split
> > > > >copying (read src and write to dst).
> > > >
> > > > This is not the reason why the client switches to generic_copy_file_range.
> > > >
> > > > >Returning NULL in the old nfs42_ssc_open is not correct, it allows
> > > > >the copy
> > > > >operation to proceed and hits the NFS4ERR_STALE case in the COPY
> > > > >operation.
> > > >
> > > > I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
> > > > returned for the PUTFH of the SRC in the COPY compound. However on the
> > > > client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
> > > > with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
> > > > to do the copy.
> > > >
> > > > The ESTALE error is only returned by copy_file_range if the client
> > > > does not have commit 7e350197a1c10. So I think there is no need to
> > > > make any change on the source server for the NFS4ERR_STALE error.
> > >
> > > I don't believe NFS4ERR_STALE is the correct error for the server to
> > > return. It's nice that the client is able to do the right thing despite
> > > the server returning the wrong error, but we should still try to get
> > > this right on the server.
> >
> > Hi Bruce,
> >
> > ERR_STALE is the appropriate error to be returned by the server that
> > gets a COPY compound when it doesn't support COPY. Since server can't
> > understand the filehandle so it can't process it so we can't get to
> > processing COPY opcode where the server could have returned
> > EOPNOTSUPP.
>
> The case we're discussing is the case where we support COPY but not
> server-to-server copy.

My point is still the same, that's an appropriate error for when
server-to-server copy is not supported.

> --b.
>
> > Thus a client side patch is needed and the server is doing
> > everything it can in the situation.
> >
> > I'm confused about the title of this patch. I thought what it does is
> > removes NFSD dependency on the NFS and instead loads the needed
> > function dynamically.
> >
> > Honestly, I don't understand why that allows removal of the NFS_FS
> > from the dependencies I don't understand. nfs4_ssc_open calls nfs
> > client functions that are built when NFS_FS is compiled but I'm
> > assuming will not be otherwise.

2020-11-10 22:25:02

by J. Bruce Fields

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Tue, Nov 10, 2020 at 05:08:59PM -0500, Olga Kornievskaia wrote:
> On Tue, Nov 10, 2020 at 4:52 PM J. Bruce Fields <[email protected]> wrote:
> >
> > On Tue, Nov 10, 2020 at 04:07:41PM -0500, Olga Kornievskaia wrote:
> > > On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <[email protected]> wrote:
> > > >
> > > > On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
> > > > >
> > > > > On 11/9/20 2:26 PM, Dai Ngo wrote:
> > > > > >
> > > > > >On 11/9/20 12:42 PM, J. Bruce Fields wrote:
> > > > > >>On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
> > > > > >>>On 11/9/20 10:30 AM, J. Bruce Fields wrote:
> > > > > >>>>On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
> > > > > >>>>>On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> > > > > >>>>>>On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> > > > > >>>>>>>NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> > > > > >>>>>>>build errors and some configs with NFSD=m to get NFS4ERR_STALE
> > > > > >>>>>>>error when doing inter server copy.
> > > > > >>>>>>>
> > > > > >>>>>>>Added ops table in nfs_common for knfsd to access NFS
> > > > > >>>>>>>client modules.
> > > > > >>>>>>OK, looks reasonable to me, applying. Does this resolve all the
> > > > > >>>>>>problems you've seen, or is there any bad case left?
> > > > > >>>>>Thanks Bruce.
> > > > > >>>>>
> > > > > >>>>>With this patch, I no longer see the NFS4ERR_STALE in any config.
> > > > > >>>>>
> > > > > >>>>>The problem with NFS4ERR_STALE was because of a bug in
> > > > > >>>>>nfs42_ssc_open.
> > > > > >>>>>When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
> > > > > >>>>>returns NULL which is incorrect allowing the operation to continue
> > > > > >>>>>until nfsd4_putfh which does not have the code to handle
> > > > > >>>>>nfserr_stale.
> > > > > >>>>>
> > > > > >>>>>With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
> > > > > >>>>>new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
> > > > > >>>>>to switch over to the split copying (read src and write to dst).
> > > > > >>>>That sounds reasonable, but I don't see any of the patches you've sent
> > > > > >>>>changing that error return. Did I overlook something, or did you mean
> > > > > >>>>to append a patch to this message?
> > > > > >>>Since with the patch, I did not run into the condition where
> > > > > >>>NFS4ERR_STALE
> > > > > >>>is returned so I did not fix this return error code. Do you want me to
> > > > > >>>submit another patch to change the returned error code from
> > > > > >>>NFS4ERR_STALE
> > > > > >>>to NFS4ERR_NOTSUPP if it ever runs into that condition?
> > > > > >>That would be great, thanks. (I mean, it is still possible to hit that
> > > > > >>case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
> > > > > >
> > > > > >will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
> > > > > >this case.
> > > > >
> > > > > I need to qualify this, the copy_file_range syscall did not return
> > > > > ESTALE in the test.
> > > > >
> > > > > >Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
> > > > > >defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
> > > > > >the old code, which causes the NFS client to switch over to the split
> > > > > >copying (read src and write to dst).
> > > > >
> > > > > This is not the reason why the client switches to generic_copy_file_range.
> > > > >
> > > > > >Returning NULL in the old nfs42_ssc_open is not correct, it allows
> > > > > >the copy
> > > > > >operation to proceed and hits the NFS4ERR_STALE case in the COPY
> > > > > >operation.
> > > > >
> > > > > I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
> > > > > returned for the PUTFH of the SRC in the COPY compound. However on the
> > > > > client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
> > > > > with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
> > > > > to do the copy.
> > > > >
> > > > > The ESTALE error is only returned by copy_file_range if the client
> > > > > does not have commit 7e350197a1c10. So I think there is no need to
> > > > > make any change on the source server for the NFS4ERR_STALE error.
> > > >
> > > > I don't believe NFS4ERR_STALE is the correct error for the server to
> > > > return. It's nice that the client is able to do the right thing despite
> > > > the server returning the wrong error, but we should still try to get
> > > > this right on the server.
> > >
> > > Hi Bruce,
> > >
> > > ERR_STALE is the appropriate error to be returned by the server that
> > > gets a COPY compound when it doesn't support COPY. Since server can't
> > > understand the filehandle so it can't process it so we can't get to
> > > processing COPY opcode where the server could have returned
> > > EOPNOTSUPP.
> >
> > The case we're discussing is the case where we support COPY but not
> > server-to-server copy.
>
> My point is still the same, that's an appropriate error for when
> server-to-server copy is not supported.

Uh, OK, if it backs up and returns it to the PUTFH, I guess?

Was it really the intention of nfsd4_do_async_copy() that it return
STALE in the case NFS42_ssc_open() returns NULL? That's pretty
confusing.

--b.

>
> > --b.
> >
> > > Thus a client side patch is needed and the server is doing
> > > everything it can in the situation.
> > >
> > > I'm confused about the title of this patch. I thought what it does is
> > > removes NFSD dependency on the NFS and instead loads the needed
> > > function dynamically.
> > >
> > > Honestly, I don't understand why that allows removal of the NFS_FS
> > > from the dependencies I don't understand. nfs4_ssc_open calls nfs
> > > client functions that are built when NFS_FS is compiled but I'm
> > > assuming will not be otherwise.

2020-11-12 01:51:27

by Dai Ngo

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On 11/10/20 2:21 PM, J. Bruce Fields wrote:
> On Tue, Nov 10, 2020 at 05:08:59PM -0500, Olga Kornievskaia wrote:
>> On Tue, Nov 10, 2020 at 4:52 PM J. Bruce Fields <[email protected]> wrote:
>>> On Tue, Nov 10, 2020 at 04:07:41PM -0500, Olga Kornievskaia wrote:
>>>> On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <[email protected]> wrote:
>>>>> On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
>>>>>> On 11/9/20 2:26 PM, Dai Ngo wrote:
>>>>>>> On 11/9/20 12:42 PM, J. Bruce Fields wrote:
>>>>>>>> On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
>>>>>>>>> On 11/9/20 10:30 AM, J. Bruce Fields wrote:
>>>>>>>>>> On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
>>>>>>>>>>> On 10/20/20 10:01 AM, J. Bruce Fields wrote:
>>>>>>>>>>>> On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
>>>>>>>>>>>>> NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
>>>>>>>>>>>>> build errors and some configs with NFSD=m to get NFS4ERR_STALE
>>>>>>>>>>>>> error when doing inter server copy.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Added ops table in nfs_common for knfsd to access NFS
>>>>>>>>>>>>> client modules.
>>>>>>>>>>>> OK, looks reasonable to me, applying. Does this resolve all the
>>>>>>>>>>>> problems you've seen, or is there any bad case left?
>>>>>>>>>>> Thanks Bruce.
>>>>>>>>>>>
>>>>>>>>>>> With this patch, I no longer see the NFS4ERR_STALE in any config.
>>>>>>>>>>>
>>>>>>>>>>> The problem with NFS4ERR_STALE was because of a bug in
>>>>>>>>>>> nfs42_ssc_open.
>>>>>>>>>>> When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
>>>>>>>>>>> returns NULL which is incorrect allowing the operation to continue
>>>>>>>>>>> until nfsd4_putfh which does not have the code to handle
>>>>>>>>>>> nfserr_stale.
>>>>>>>>>>>
>>>>>>>>>>> With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
>>>>>>>>>>> new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
>>>>>>>>>>> to switch over to the split copying (read src and write to dst).
>>>>>>>>>> That sounds reasonable, but I don't see any of the patches you've sent
>>>>>>>>>> changing that error return. Did I overlook something, or did you mean
>>>>>>>>>> to append a patch to this message?
>>>>>>>>> Since with the patch, I did not run into the condition where
>>>>>>>>> NFS4ERR_STALE
>>>>>>>>> is returned so I did not fix this return error code. Do you want me to
>>>>>>>>> submit another patch to change the returned error code from
>>>>>>>>> NFS4ERR_STALE
>>>>>>>>> to NFS4ERR_NOTSUPP if it ever runs into that condition?
>>>>>>>> That would be great, thanks. (I mean, it is still possible to hit that
>>>>>>>> case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
>>>>>>> will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
>>>>>>> this case.
>>>>>> I need to qualify this, the copy_file_range syscall did not return
>>>>>> ESTALE in the test.
>>>>>>
>>>>>>> Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
>>>>>>> defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
>>>>>>> the old code, which causes the NFS client to switch over to the split
>>>>>>> copying (read src and write to dst).
>>>>>> This is not the reason why the client switches to generic_copy_file_range.
>>>>>>
>>>>>>> Returning NULL in the old nfs42_ssc_open is not correct, it allows
>>>>>>> the copy
>>>>>>> operation to proceed and hits the NFS4ERR_STALE case in the COPY
>>>>>>> operation.
>>>>>> I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
>>>>>> returned for the PUTFH of the SRC in the COPY compound. However on the
>>>>>> client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
>>>>>> with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
>>>>>> to do the copy.
>>>>>>
>>>>>> The ESTALE error is only returned by copy_file_range if the client
>>>>>> does not have commit 7e350197a1c10. So I think there is no need to
>>>>>> make any change on the source server for the NFS4ERR_STALE error.
>>>>> I don't believe NFS4ERR_STALE is the correct error for the server to
>>>>> return. It's nice that the client is able to do the right thing despite
>>>>> the server returning the wrong error, but we should still try to get
>>>>> this right on the server.
>>>> Hi Bruce,
>>>>
>>>> ERR_STALE is the appropriate error to be returned by the server that
>>>> gets a COPY compound when it doesn't support COPY. Since server can't
>>>> understand the filehandle so it can't process it so we can't get to
>>>> processing COPY opcode where the server could have returned
>>>> EOPNOTSUPP.
>>> The case we're discussing is the case where we support COPY but not
>>> server-to-server copy.
>> My point is still the same, that's an appropriate error for when
>> server-to-server copy is not supported.
> Uh, OK, if it backs up and returns it to the PUTFH, I guess?
>
> Was it really the intention of nfsd4_do_async_copy() that it return
> STALE in the case NFS42_ssc_open() returns NULL? That's pretty
> confusing.

In this scenario, the COPY compound fails at the PUTFH op and
NFS4ERR_NOTSUPP is not a valid error code for PUTFH, NFS4ERR_STALE is.

From section 15.2 of RFC 8881:

> | PUTFH | NFS4ERR_BADHANDLE, NFS4ERR_BADXDR, |
> | | NFS4ERR_DEADSESSION, NFS4ERR_DELAY, |
> | | NFS4ERR_MOVED, |
> | | NFS4ERR_OP_NOT_IN_SESSION, |
> | | NFS4ERR_REP_TOO_BIG, |
> | | NFS4ERR_REP_TOO_BIG_TO_CACHE, |
> | | NFS4ERR_REQ_TOO_BIG, |
> | | NFS4ERR_RETRY_UNCACHED_REP, |
> | | NFS4ERR_SERVERFAULT, NFS4ERR_STALE, |
> | | NFS4ERR_TOO_MANY_OPS, NFS4ERR_WRONGSEC |

Regarding fh_verify returns NFS4ERR_STALE, I think the code works as the spec
describes in 15.23 of RFC 7862:
> If the request is for an inter-server copy, the source-fh is a
> filehandle from the source server and the COMPOUND procedure is being
> executed on the destination server. In this case, the source-fh is a
> foreign filehandle on the server receiving the COPY request. If
> either PUTFH or SAVEFH checked the validity of the filehandle, the
> operation would likely fail and return NFS4ERR_STALE.

-Dai

>
> --b.
>
>>> --b.
>>>
>>>> Thus a client side patch is needed and the server is doing
>>>> everything it can in the situation.
>>>>
>>>> I'm confused about the title of this patch. I thought what it does is
>>>> removes NFSD dependency on the NFS and instead loads the needed
>>>> function dynamically.
>>>>
>>>> Honestly, I don't understand why that allows removal of the NFS_FS
>>>> from the dependencies I don't understand. nfs4_ssc_open calls nfs
>>>> client functions that are built when NFS_FS is compiled but I'm
>>>> assuming will not be otherwise.

2020-11-23 16:27:44

by J. Bruce Fields

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Wed, Nov 11, 2020 at 03:02:19PM -0800, Dai Ngo wrote:
>
> On 11/10/20 2:21 PM, J. Bruce Fields wrote:
> >On Tue, Nov 10, 2020 at 05:08:59PM -0500, Olga Kornievskaia wrote:
> >>On Tue, Nov 10, 2020 at 4:52 PM J. Bruce Fields <[email protected]> wrote:
> >>>On Tue, Nov 10, 2020 at 04:07:41PM -0500, Olga Kornievskaia wrote:
> >>>>On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <[email protected]> wrote:
> >>>>>On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
> >>>>>>On 11/9/20 2:26 PM, Dai Ngo wrote:
> >>>>>>>On 11/9/20 12:42 PM, J. Bruce Fields wrote:
> >>>>>>>>On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
> >>>>>>>>>On 11/9/20 10:30 AM, J. Bruce Fields wrote:
> >>>>>>>>>>On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
> >>>>>>>>>>>On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> >>>>>>>>>>>>On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> >>>>>>>>>>>>>NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> >>>>>>>>>>>>>build errors and some configs with NFSD=m to get NFS4ERR_STALE
> >>>>>>>>>>>>>error when doing inter server copy.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>Added ops table in nfs_common for knfsd to access NFS
> >>>>>>>>>>>>>client modules.
> >>>>>>>>>>>>OK, looks reasonable to me, applying. Does this resolve all the
> >>>>>>>>>>>>problems you've seen, or is there any bad case left?
> >>>>>>>>>>>Thanks Bruce.
> >>>>>>>>>>>
> >>>>>>>>>>>With this patch, I no longer see the NFS4ERR_STALE in any config.
> >>>>>>>>>>>
> >>>>>>>>>>>The problem with NFS4ERR_STALE was because of a bug in
> >>>>>>>>>>>nfs42_ssc_open.
> >>>>>>>>>>>When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
> >>>>>>>>>>>returns NULL which is incorrect allowing the operation to continue
> >>>>>>>>>>>until nfsd4_putfh which does not have the code to handle
> >>>>>>>>>>>nfserr_stale.
> >>>>>>>>>>>
> >>>>>>>>>>>With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
> >>>>>>>>>>>new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
> >>>>>>>>>>>to switch over to the split copying (read src and write to dst).
> >>>>>>>>>>That sounds reasonable, but I don't see any of the patches you've sent
> >>>>>>>>>>changing that error return. Did I overlook something, or did you mean
> >>>>>>>>>>to append a patch to this message?
> >>>>>>>>>Since with the patch, I did not run into the condition where
> >>>>>>>>>NFS4ERR_STALE
> >>>>>>>>>is returned so I did not fix this return error code. Do you want me to
> >>>>>>>>>submit another patch to change the returned error code from
> >>>>>>>>>NFS4ERR_STALE
> >>>>>>>>>to NFS4ERR_NOTSUPP if it ever runs into that condition?
> >>>>>>>>That would be great, thanks. (I mean, it is still possible to hit that
> >>>>>>>>case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
> >>>>>>>will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
> >>>>>>>this case.
> >>>>>>I need to qualify this, the copy_file_range syscall did not return
> >>>>>>ESTALE in the test.
> >>>>>>
> >>>>>>>Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
> >>>>>>>defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
> >>>>>>>the old code, which causes the NFS client to switch over to the split
> >>>>>>>copying (read src and write to dst).
> >>>>>>This is not the reason why the client switches to generic_copy_file_range.
> >>>>>>
> >>>>>>>Returning NULL in the old nfs42_ssc_open is not correct, it allows
> >>>>>>>the copy
> >>>>>>>operation to proceed and hits the NFS4ERR_STALE case in the COPY
> >>>>>>>operation.
> >>>>>>I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
> >>>>>>returned for the PUTFH of the SRC in the COPY compound. However on the
> >>>>>>client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
> >>>>>>with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
> >>>>>>to do the copy.
> >>>>>>
> >>>>>>The ESTALE error is only returned by copy_file_range if the client
> >>>>>>does not have commit 7e350197a1c10. So I think there is no need to
> >>>>>>make any change on the source server for the NFS4ERR_STALE error.
> >>>>>I don't believe NFS4ERR_STALE is the correct error for the server to
> >>>>>return. It's nice that the client is able to do the right thing despite
> >>>>>the server returning the wrong error, but we should still try to get
> >>>>>this right on the server.
> >>>>Hi Bruce,
> >>>>
> >>>>ERR_STALE is the appropriate error to be returned by the server that
> >>>>gets a COPY compound when it doesn't support COPY. Since server can't
> >>>>understand the filehandle so it can't process it so we can't get to
> >>>>processing COPY opcode where the server could have returned
> >>>>EOPNOTSUPP.
> >>>The case we're discussing is the case where we support COPY but not
> >>>server-to-server copy.
> >>My point is still the same, that's an appropriate error for when
> >>server-to-server copy is not supported.
> >Uh, OK, if it backs up and returns it to the PUTFH, I guess?
> >
> >Was it really the intention of nfsd4_do_async_copy() that it return
> >STALE in the case NFS42_ssc_open() returns NULL? That's pretty
> >confusing.
>
> In this scenario, the COPY compound fails at the PUTFH op and
> NFS4ERR_NOTSUPP is not a valid error code for PUTFH, NFS4ERR_STALE is.

OK, makes sense.

I've lost track of what's left to apply.

--b.

2020-11-23 18:20:45

by Dai Ngo

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On 11/23/20 8:25 AM, J. Bruce Fields wrote:
> On Wed, Nov 11, 2020 at 03:02:19PM -0800, Dai Ngo wrote:
>> On 11/10/20 2:21 PM, J. Bruce Fields wrote:
>>> On Tue, Nov 10, 2020 at 05:08:59PM -0500, Olga Kornievskaia wrote:
>>>> On Tue, Nov 10, 2020 at 4:52 PM J. Bruce Fields <[email protected]> wrote:
>>>>> On Tue, Nov 10, 2020 at 04:07:41PM -0500, Olga Kornievskaia wrote:
>>>>>> On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <[email protected]> wrote:
>>>>>>> On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
>>>>>>>> On 11/9/20 2:26 PM, Dai Ngo wrote:
>>>>>>>>> On 11/9/20 12:42 PM, J. Bruce Fields wrote:
>>>>>>>>>> On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
>>>>>>>>>>> On 11/9/20 10:30 AM, J. Bruce Fields wrote:
>>>>>>>>>>>> On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
>>>>>>>>>>>>> On 10/20/20 10:01 AM, J. Bruce Fields wrote:
>>>>>>>>>>>>>> On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
>>>>>>>>>>>>>>> NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
>>>>>>>>>>>>>>> build errors and some configs with NFSD=m to get NFS4ERR_STALE
>>>>>>>>>>>>>>> error when doing inter server copy.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Added ops table in nfs_common for knfsd to access NFS
>>>>>>>>>>>>>>> client modules.
>>>>>>>>>>>>>> OK, looks reasonable to me, applying. Does this resolve all the
>>>>>>>>>>>>>> problems you've seen, or is there any bad case left?
>>>>>>>>>>>>> Thanks Bruce.
>>>>>>>>>>>>>
>>>>>>>>>>>>> With this patch, I no longer see the NFS4ERR_STALE in any config.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The problem with NFS4ERR_STALE was because of a bug in
>>>>>>>>>>>>> nfs42_ssc_open.
>>>>>>>>>>>>> When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
>>>>>>>>>>>>> returns NULL which is incorrect allowing the operation to continue
>>>>>>>>>>>>> until nfsd4_putfh which does not have the code to handle
>>>>>>>>>>>>> nfserr_stale.
>>>>>>>>>>>>>
>>>>>>>>>>>>> With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
>>>>>>>>>>>>> new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
>>>>>>>>>>>>> to switch over to the split copying (read src and write to dst).
>>>>>>>>>>>> That sounds reasonable, but I don't see any of the patches you've sent
>>>>>>>>>>>> changing that error return. Did I overlook something, or did you mean
>>>>>>>>>>>> to append a patch to this message?
>>>>>>>>>>> Since with the patch, I did not run into the condition where
>>>>>>>>>>> NFS4ERR_STALE
>>>>>>>>>>> is returned so I did not fix this return error code. Do you want me to
>>>>>>>>>>> submit another patch to change the returned error code from
>>>>>>>>>>> NFS4ERR_STALE
>>>>>>>>>>> to NFS4ERR_NOTSUPP if it ever runs into that condition?
>>>>>>>>>> That would be great, thanks. (I mean, it is still possible to hit that
>>>>>>>>>> case, right? You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
>>>>>>>>> will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
>>>>>>>>> this case.
>>>>>>>> I need to qualify this, the copy_file_range syscall did not return
>>>>>>>> ESTALE in the test.
>>>>>>>>
>>>>>>>>> Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
>>>>>>>>> defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
>>>>>>>>> the old code, which causes the NFS client to switch over to the split
>>>>>>>>> copying (read src and write to dst).
>>>>>>>> This is not the reason why the client switches to generic_copy_file_range.
>>>>>>>>
>>>>>>>>> Returning NULL in the old nfs42_ssc_open is not correct, it allows
>>>>>>>>> the copy
>>>>>>>>> operation to proceed and hits the NFS4ERR_STALE case in the COPY
>>>>>>>>> operation.
>>>>>>>> I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
>>>>>>>> returned for the PUTFH of the SRC in the COPY compound. However on the
>>>>>>>> client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
>>>>>>>> with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
>>>>>>>> to do the copy.
>>>>>>>>
>>>>>>>> The ESTALE error is only returned by copy_file_range if the client
>>>>>>>> does not have commit 7e350197a1c10. So I think there is no need to
>>>>>>>> make any change on the source server for the NFS4ERR_STALE error.
>>>>>>> I don't believe NFS4ERR_STALE is the correct error for the server to
>>>>>>> return. It's nice that the client is able to do the right thing despite
>>>>>>> the server returning the wrong error, but we should still try to get
>>>>>>> this right on the server.
>>>>>> Hi Bruce,
>>>>>>
>>>>>> ERR_STALE is the appropriate error to be returned by the server that
>>>>>> gets a COPY compound when it doesn't support COPY. Since server can't
>>>>>> understand the filehandle so it can't process it so we can't get to
>>>>>> processing COPY opcode where the server could have returned
>>>>>> EOPNOTSUPP.
>>>>> The case we're discussing is the case where we support COPY but not
>>>>> server-to-server copy.
>>>> My point is still the same, that's an appropriate error for when
>>>> server-to-server copy is not supported.
>>> Uh, OK, if it backs up and returns it to the PUTFH, I guess?
>>>
>>> Was it really the intention of nfsd4_do_async_copy() that it return
>>> STALE in the case NFS42_ssc_open() returns NULL? That's pretty
>>> confusing.
>> In this scenario, the COPY compound fails at the PUTFH op and
>> NFS4ERR_NOTSUPP is not a valid error code for PUTFH, NFS4ERR_STALE is.
> OK, makes sense.
>
> I've lost track of what's left to apply.

I think we're good with this issue.
There is still one inter server copy related patch waiting for
your review:

PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

Thanks,
-Dai

>
> --b.

2020-11-23 22:10:16

by J. Bruce Fields

Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

On Mon, Nov 23, 2020 at 10:14:51AM -0800, Dai Ngo wrote:
> On 11/23/20 8:25 AM, J. Bruce Fields wrote:
> >I've lost track of what's left to apply.
>
> I think we're good with this issue.
> There is still one inter server copy related patch waiting for
> your review:
>
> PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

Looking... Could you break that up into two fixes, one fof the client,
one for the server?

Also note the RFC is 7862, not 7682.

--b.