[PATCH v4 0/5] ceph: new mount device syntax

[PATCH v4 0/5] ceph: new mount device syntax

[Venky Shankar]

v4:
  - fix delimiter check in ceph_parse_ips()
  - use __func__ in ceph_parse_ips() instead of hardcoded function name
  - KERN_NOTICE that mon_addr is recorded but not reconnected

Venky Shankar (5):
  ceph: generalize addr/ip parsing based on delimiter
  ceph: rename parse_fsid() to ceph_parse_fsid() and export
  ceph: new device mount syntax
  ceph: record updated mon_addr on remount
  doc: document new CephFS mount device syntax

 Documentation/filesystems/ceph.rst |  25 ++++-
 drivers/block/rbd.c                |   3 +-
 fs/ceph/super.c                    | 151 +++++++++++++++++++++++++++--
 fs/ceph/super.h                    |   3 +
 include/linux/ceph/libceph.h       |   5 +-
 include/linux/ceph/messenger.h     |   2 +-
 net/ceph/ceph_common.c             |  17 ++--
 net/ceph/messenger.c               |   8 +-
 8 files changed, 186 insertions(+), 28 deletions(-)

-- 
2.27.0

[PATCH v4 1/5] ceph: generalize addr/ip parsing based on delimiter

[Venky Shankar]

... and remove hardcoded function name in ceph_parse_ips().

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 drivers/block/rbd.c            | 3 ++-
 fs/ceph/super.c                | 3 ++-
 include/linux/ceph/libceph.h   | 4 +++-
 include/linux/ceph/messenger.h | 2 +-
 net/ceph/ceph_common.c         | 8 ++++----
 net/ceph/messenger.c           | 8 ++++----
 6 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index bbb88eb009e0..209a7a128ea3 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -6530,7 +6530,8 @@ static int rbd_add_parse_args(const char *buf,
 	pctx.opts->exclusive = RBD_EXCLUSIVE_DEFAULT;
 	pctx.opts->trim = RBD_TRIM_DEFAULT;
 
-	ret = ceph_parse_mon_ips(mon_addrs, mon_addrs_size, pctx.copts, NULL);
+	ret = ceph_parse_mon_ips(mon_addrs, mon_addrs_size, pctx.copts, NULL,
+				 CEPH_ADDR_PARSE_DEFAULT_DELIM);
 	if (ret)
 		goto out_err;
 
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 9b1b7f4cfdd4..039775553a88 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -271,7 +271,8 @@ static int ceph_parse_source(struct fs_parameter *param, struct fs_context *fc)
 		dout("server path '%s'\n", fsopt->server_path);
 
 	ret = ceph_parse_mon_ips(param->string, dev_name_end - dev_name,
-				 pctx->copts, fc->log.log);
+				 pctx->copts, fc->log.log,
+				 CEPH_ADDR_PARSE_DEFAULT_DELIM);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index 409d8c29bc4f..e50dba429819 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -98,6 +98,8 @@ struct ceph_options {
 
 #define CEPH_AUTH_NAME_DEFAULT   "guest"
 
+#define CEPH_ADDR_PARSE_DEFAULT_DELIM  ','
+
 /* mount state */
 enum {
 	CEPH_MOUNT_MOUNTING,
@@ -301,7 +303,7 @@ struct fs_parameter;
 struct fc_log;
 struct ceph_options *ceph_alloc_options(void);
 int ceph_parse_mon_ips(const char *buf, size_t len, struct ceph_options *opt,
-		       struct fc_log *l);
+		       struct fc_log *l, char delimiter);
 int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt,
 		     struct fc_log *l);
 int ceph_print_client_options(struct seq_file *m, struct ceph_client *client,
diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
index 0e6e9ad3c3bf..c9675ee33f51 100644
--- a/include/linux/ceph/messenger.h
+++ b/include/linux/ceph/messenger.h
@@ -532,7 +532,7 @@ extern const char *ceph_pr_addr(const struct ceph_entity_addr *addr);
 
 extern int ceph_parse_ips(const char *c, const char *end,
 			  struct ceph_entity_addr *addr,
-			  int max_count, int *count);
+			  int max_count, int *count, char delimiter);
 
 extern int ceph_msgr_init(void);
 extern void ceph_msgr_exit(void);
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index 97d6ea763e32..0f74ceeddf48 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -422,14 +422,14 @@ static int get_secret(struct ceph_crypto_key *dst, const char *name,
 }
 
 int ceph_parse_mon_ips(const char *buf, size_t len, struct ceph_options *opt,
-		       struct fc_log *l)
+		       struct fc_log *l, char delimiter)
 {
 	struct p_log log = {.prefix = "libceph", .log = l};
 	int ret;
 
-	/* ip1[:port1][,ip2[:port2]...] */
+	/* ip1[:port1][<delim>ip2[:port2]...] */
 	ret = ceph_parse_ips(buf, buf + len, opt->mon_addr, CEPH_MAX_MON,
-			     &opt->num_mon);
+			     &opt->num_mon, delimiter);
 	if (ret) {
 		error_plog(&log, "Failed to parse monitor IPs: %d", ret);
 		return ret;
@@ -456,7 +456,7 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt,
 		err = ceph_parse_ips(param->string,
 				     param->string + param->size,
 				     &opt->my_addr,
-				     1, NULL);
+				     1, NULL, CEPH_ADDR_PARSE_DEFAULT_DELIM);
 		if (err) {
 			error_plog(&log, "Failed to parse ip: %d", err);
 			return err;
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 57d043b382ed..c93d103fe343 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1267,7 +1267,7 @@ static int ceph_parse_server_name(const char *name, size_t namelen,
  */
 int ceph_parse_ips(const char *c, const char *end,
 		   struct ceph_entity_addr *addr,
-		   int max_count, int *count)
+		   int max_count, int *count, char delimiter)
 {
 	int i, ret = -EINVAL;
 	const char *p = c;
@@ -1276,7 +1276,7 @@ int ceph_parse_ips(const char *c, const char *end,
 	for (i = 0; i < max_count; i++) {
 		const char *ipend;
 		int port;
-		char delim = ',';
+		char delim = delimiter;
 
 		if (*p == '[') {
 			delim = ']';
@@ -1326,11 +1326,11 @@ int ceph_parse_ips(const char *c, const char *end,
 		addr[i].type = CEPH_ENTITY_ADDR_TYPE_LEGACY;
 		addr[i].nonce = 0;
 
-		dout("parse_ips got %s\n", ceph_pr_addr(&addr[i]));
+		dout("%s got %s\n", __func__, ceph_pr_addr(&addr[i]));
 
 		if (p == end)
 			break;
-		if (*p != ',')
+		if (*p != delimiter)
 			goto bad;
 		p++;
 	}
-- 
2.27.0

[PATCH v4 2/5] ceph: rename parse_fsid() to ceph_parse_fsid() and export

[Venky Shankar]

... as it is too generic. also, use __func__ when logging
rather than hardcoding the function name.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 include/linux/ceph/libceph.h | 1 +
 net/ceph/ceph_common.c       | 9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index e50dba429819..37ab639b5012 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -298,6 +298,7 @@ extern bool libceph_compatible(void *data);
 extern const char *ceph_msg_type_name(int type);
 extern int ceph_check_fsid(struct ceph_client *client, struct ceph_fsid *fsid);
 extern void *ceph_kvmalloc(size_t size, gfp_t flags);
+extern int ceph_parse_fsid(const char *str, struct ceph_fsid *fsid);
 
 struct fs_parameter;
 struct fc_log;
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index 0f74ceeddf48..31cbe671121c 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -217,14 +217,14 @@ void *ceph_kvmalloc(size_t size, gfp_t flags)
 	return p;
 }
 
-static int parse_fsid(const char *str, struct ceph_fsid *fsid)
+int ceph_parse_fsid(const char *str, struct ceph_fsid *fsid)
 {
 	int i = 0;
 	char tmp[3];
 	int err = -EINVAL;
 	int d;
 
-	dout("parse_fsid '%s'\n", str);
+	dout("%s '%s'\n", __func__, str);
 	tmp[2] = 0;
 	while (*str && i < 16) {
 		if (ispunct(*str)) {
@@ -244,9 +244,10 @@ static int parse_fsid(const char *str, struct ceph_fsid *fsid)
 
 	if (i == 16)
 		err = 0;
-	dout("parse_fsid ret %d got fsid %pU\n", err, fsid);
+	dout("%s ret %d got fsid %pU\n", __func__, err, fsid);
 	return err;
 }
+EXPORT_SYMBOL(ceph_parse_fsid);
 
 /*
  * ceph options
@@ -465,7 +466,7 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt,
 		break;
 
 	case Opt_fsid:
-		err = parse_fsid(param->string, &opt->fsid);
+		err = ceph_parse_fsid(param->string, &opt->fsid);
 		if (err) {
 			error_plog(&log, "Failed to parse fsid: %d", err);
 			return err;
-- 
2.27.0

[PATCH v4 3/5] ceph: new device mount syntax

[Venky Shankar]

Old mount device syntax (source) has the following problems:

- mounts to the same cluster but with different fsnames
  and/or creds have identical device string which can
  confuse xfstests.

- Userspace mount helper tool resolves monitor addresses
  and fill in mon addrs automatically, but that means the
  device shown in /proc/mounts is different than what was
  used for mounting.

New device syntax is as follows:

  cephuser@fsid.mycephfs2=/path

Note, there is no "monitor address" in the device string.
That gets passed in as mount option. This keeps the device
string same when monitor addresses change (on remounts).

Also note that the userspace mount helper tool is backward
compatible. I.e., the mount helper will fallback to using
old syntax after trying to mount with the new syntax.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 fs/ceph/super.c | 145 ++++++++++++++++++++++++++++++++++++++++++++----
 fs/ceph/super.h |   3 +
 2 files changed, 137 insertions(+), 11 deletions(-)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 039775553a88..d8c6168b7fcd 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -145,6 +145,7 @@ enum {
 	Opt_mds_namespace,
 	Opt_recover_session,
 	Opt_source,
+	Opt_mon_addr,
 	/* string args above */
 	Opt_dirstat,
 	Opt_rbytes,
@@ -196,6 +197,7 @@ static const struct fs_parameter_spec ceph_mount_parameters[] = {
 	fsparam_u32	("rsize",			Opt_rsize),
 	fsparam_string	("snapdirname",			Opt_snapdirname),
 	fsparam_string	("source",			Opt_source),
+	fsparam_string	("mon_addr",			Opt_mon_addr),
 	fsparam_u32	("wsize",			Opt_wsize),
 	fsparam_flag_no	("wsync",			Opt_wsync),
 	{}
@@ -227,9 +229,93 @@ static void canonicalize_path(char *path)
 }
 
 /*
- * Parse the source parameter.  Distinguish the server list from the path.
+ * Check if the mds namespace in ceph_mount_options matches
+ * the passed in namespace string. First time match (when
+ * ->mds_namespace is NULL) is treated specially, since
+ * ->mds_namespace needs to be initialized by the caller.
+ */
+static int namespace_equals(struct ceph_mount_options *fsopt,
+			    const char *namespace, size_t len)
+{
+	return !(fsopt->mds_namespace &&
+		 (strlen(fsopt->mds_namespace) != len ||
+		  strncmp(fsopt->mds_namespace, namespace, len)));
+}
+
+static int ceph_parse_old_source(const char *dev_name, const char *dev_name_end,
+				 struct fs_context *fc)
+{
+	int r;
+	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
+	struct ceph_mount_options *fsopt = pctx->opts;
+
+	if (*dev_name_end != ':')
+		return invalfc(fc, "separator ':' missing in source");
+
+	r = ceph_parse_mon_ips(dev_name, dev_name_end - dev_name,
+			       pctx->copts, fc->log.log,
+			       CEPH_ADDR_PARSE_DEFAULT_DELIM);
+	if (r)
+		return r;
+
+	fsopt->new_dev_syntax = false;
+	return 0;
+}
+
+static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
+				 struct fs_context *fc)
+{
+	size_t len;
+	struct ceph_fsid fsid;
+	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
+	struct ceph_mount_options *fsopt = pctx->opts;
+	char *fsid_start, *fs_name_start;
+
+	if (*dev_name_end != '=') {
+		dout("separator '=' missing in source");
+		return -EINVAL;
+	}
+
+	fsid_start = strchr(dev_name, '@');
+	if (!fsid_start)
+		return invalfc(fc, "missing cluster fsid");
+	++fsid_start; /* start of cluster fsid */
+
+	fs_name_start = strchr(fsid_start, '.');
+	if (!fs_name_start)
+		return invalfc(fc, "missing file system name");
+
+	if (ceph_parse_fsid(fsid_start, &fsid))
+		return invalfc(fc, "Invalid FSID");
+
+	++fs_name_start; /* start of file system name */
+	len = dev_name_end - fs_name_start;
+
+	if (!namespace_equals(fsopt, fs_name_start, len))
+		return invalfc(fc, "Mismatching mds_namespace");
+	kfree(fsopt->mds_namespace);
+	fsopt->mds_namespace = kstrndup(fs_name_start, len, GFP_KERNEL);
+	if (!fsopt->mds_namespace)
+		return -ENOMEM;
+	dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
+
+	fsopt->new_dev_syntax = true;
+	return 0;
+}
+
+/*
+ * Parse the source parameter for new device format. Distinguish the device
+ * spec from the path. Try parsing new device format and fallback to old
+ * format if needed.
+ *
+ * New device syntax will looks like:
+ *     <device_spec>=/<path>
+ * where
+ *     <device_spec> is name@fsid.fsname
+ *     <path> is optional, but if present must begin with '/'
+ * (monitor addresses are passed via mount option)
  *
- * The source will look like:
+ * Old device syntax is:
  *     <server_spec>[,<server_spec>...]:[<path>]
  * where
  *     <server_spec> is <ip>[:<port>]
@@ -262,25 +348,46 @@ static int ceph_parse_source(struct fs_parameter *param, struct fs_context *fc)
 		dev_name_end = dev_name + strlen(dev_name);
 	}
 
-	dev_name_end--;		/* back up to ':' separator */
-	if (dev_name_end < dev_name || *dev_name_end != ':')
-		return invalfc(fc, "No path or : separator in source");
+	dev_name_end--;		/* back up to separator */
+	if (dev_name_end < dev_name)
+		return invalfc(fc, "Path missing in source");
 
 	dout("device name '%.*s'\n", (int)(dev_name_end - dev_name), dev_name);
 	if (fsopt->server_path)
 		dout("server path '%s'\n", fsopt->server_path);
 
-	ret = ceph_parse_mon_ips(param->string, dev_name_end - dev_name,
-				 pctx->copts, fc->log.log,
-				 CEPH_ADDR_PARSE_DEFAULT_DELIM);
-	if (ret)
-		return ret;
+	dout("trying new device syntax");
+	ret = ceph_parse_new_source(dev_name, dev_name_end, fc);
+	if (ret) {
+		if (ret != -EINVAL)
+			return ret;
+		dout("trying old device syntax");
+		ret = ceph_parse_old_source(dev_name, dev_name_end, fc);
+		if (ret)
+			return ret;
+	}
 
 	fc->source = param->string;
 	param->string = NULL;
 	return 0;
 }
 
+#define CEPH_MON_ADDR_MNTOPT_DELIM '/'
+static int ceph_parse_mon_addr(struct fs_parameter *param,
+			       struct fs_context *fc)
+{
+	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
+	struct ceph_mount_options *fsopt = pctx->opts;
+
+	kfree(fsopt->mon_addr);
+	fsopt->mon_addr = param->string;
+	param->string = NULL;
+
+	return ceph_parse_mon_ips(fsopt->mon_addr, strlen(fsopt->mon_addr),
+				  pctx->copts, fc->log.log,
+				  CEPH_MON_ADDR_MNTOPT_DELIM);
+}
+
 static int ceph_parse_mount_param(struct fs_context *fc,
 				  struct fs_parameter *param)
 {
@@ -306,6 +413,8 @@ static int ceph_parse_mount_param(struct fs_context *fc,
 		param->string = NULL;
 		break;
 	case Opt_mds_namespace:
+		if (!namespace_equals(fsopt, param->string, strlen(param->string)))
+			return invalfc(fc, "Mismatching mds_namespace");
 		kfree(fsopt->mds_namespace);
 		fsopt->mds_namespace = param->string;
 		param->string = NULL;
@@ -323,6 +432,8 @@ static int ceph_parse_mount_param(struct fs_context *fc,
 		if (fc->source)
 			return invalfc(fc, "Multiple sources specified");
 		return ceph_parse_source(param, fc);
+	case Opt_mon_addr:
+		return ceph_parse_mon_addr(param, fc);
 	case Opt_wsize:
 		if (result.uint_32 < PAGE_SIZE ||
 		    result.uint_32 > CEPH_MAX_WRITE_SIZE)
@@ -474,6 +585,7 @@ static void destroy_mount_options(struct ceph_mount_options *args)
 	kfree(args->mds_namespace);
 	kfree(args->server_path);
 	kfree(args->fscache_uniq);
+	kfree(args->mon_addr);
 	kfree(args);
 }
 
@@ -517,6 +629,10 @@ static int compare_mount_options(struct ceph_mount_options *new_fsopt,
 	if (ret)
 		return ret;
 
+	ret = strcmp_null(fsopt1->mon_addr, fsopt2->mon_addr);
+	if (ret)
+		return ret;
+
 	return ceph_compare_options(new_opt, fsc->client);
 }
 
@@ -572,9 +688,13 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
 	if ((fsopt->flags & CEPH_MOUNT_OPT_NOCOPYFROM) == 0)
 		seq_puts(m, ",copyfrom");
 
-	if (fsopt->mds_namespace)
+	/* dump mds_namespace when old device syntax is in use */
+	if (fsopt->mds_namespace && !fsopt->new_dev_syntax)
 		seq_show_option(m, "mds_namespace", fsopt->mds_namespace);
 
+	if (fsopt->mon_addr)
+		seq_printf(m, ",mon_addr=%s", fsopt->mon_addr);
+
 	if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
 		seq_show_option(m, "recover_session", "clean");
 
@@ -1049,6 +1169,7 @@ static int ceph_setup_bdi(struct super_block *sb, struct ceph_fs_client *fsc)
 static int ceph_get_tree(struct fs_context *fc)
 {
 	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
+	struct ceph_mount_options *fsopt = pctx->opts;
 	struct super_block *sb;
 	struct ceph_fs_client *fsc;
 	struct dentry *res;
@@ -1060,6 +1181,8 @@ static int ceph_get_tree(struct fs_context *fc)
 
 	if (!fc->source)
 		return invalfc(fc, "No source");
+	if (fsopt->new_dev_syntax && !fsopt->mon_addr)
+		return invalfc(fc, "No monitor address");
 
 	/* create client (which we may/may not use) */
 	fsc = create_fs_client(pctx->opts, pctx->copts);
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index c48bb30c8d70..8f71184b7c85 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -87,6 +87,8 @@ struct ceph_mount_options {
 	unsigned int max_readdir;       /* max readdir result (entries) */
 	unsigned int max_readdir_bytes; /* max readdir result (bytes) */
 
+	bool new_dev_syntax;
+
 	/*
 	 * everything above this point can be memcmp'd; everything below
 	 * is handled in compare_mount_options()
@@ -96,6 +98,7 @@ struct ceph_mount_options {
 	char *mds_namespace;  /* default NULL */
 	char *server_path;    /* default NULL (means "/") */
 	char *fscache_uniq;   /* default NULL */
+	char *mon_addr;
 };
 
 struct ceph_fs_client {
-- 
2.27.0

[PATCH v4 4/5] ceph: record updated mon_addr on remount

[Venky Shankar]

Note that the new monitors are just shown in /proc/mounts.
Ceph does not (re)connect to new monitors yet.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 fs/ceph/super.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index d8c6168b7fcd..d3a5a3729c5b 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -1268,6 +1268,13 @@ static int ceph_reconfigure_fc(struct fs_context *fc)
 	else
 		ceph_clear_mount_opt(fsc, ASYNC_DIROPS);
 
+	if (strcmp(fsc->mount_options->mon_addr, fsopt->mon_addr)) {
+		kfree(fsc->mount_options->mon_addr);
+		fsc->mount_options->mon_addr = fsopt->mon_addr;
+		fsopt->mon_addr = NULL;
+		printk(KERN_NOTICE "ceph: monitor addresses recorded, but not used for reconnection");
+	}
+
 	sync_filesystem(fc->root->d_sb);
 	return 0;
 }
-- 
2.27.0

Re: [PATCH v4 4/5] ceph: record updated mon_addr on remount

[Jeff Layton]

On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> Note that the new monitors are just shown in /proc/mounts.
> Ceph does not (re)connect to new monitors yet.
> 
> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> ---
>  fs/ceph/super.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index d8c6168b7fcd..d3a5a3729c5b 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -1268,6 +1268,13 @@ static int ceph_reconfigure_fc(struct fs_context *fc)
>  	else
>  		ceph_clear_mount_opt(fsc, ASYNC_DIROPS);
>  
> +	if (strcmp(fsc->mount_options->mon_addr, fsopt->mon_addr)) {
> +		kfree(fsc->mount_options->mon_addr);
> +		fsc->mount_options->mon_addr = fsopt->mon_addr;
> +		fsopt->mon_addr = NULL;
> +		printk(KERN_NOTICE "ceph: monitor addresses recorded, but not used for reconnection");

It's currently more in-vogue to use pr_notice() for this. I'll plan to
make that (minor) change before I merge. No need to resend.

> +	}
> +
>  	sync_filesystem(fc->root->d_sb);
>  	return 0;
>  }

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v4 4/5] ceph: record updated mon_addr on remount

[Luis Henriques]

On Wed, Jul 14, 2021 at 12:17:33PM -0400, Jeff Layton wrote:
> On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> > Note that the new monitors are just shown in /proc/mounts.
> > Ceph does not (re)connect to new monitors yet.
> > 
> > Signed-off-by: Venky Shankar <vshankar@redhat.com>
> > ---
> >  fs/ceph/super.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index d8c6168b7fcd..d3a5a3729c5b 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -1268,6 +1268,13 @@ static int ceph_reconfigure_fc(struct fs_context *fc)
> >  	else
> >  		ceph_clear_mount_opt(fsc, ASYNC_DIROPS);
> >  
> > +	if (strcmp(fsc->mount_options->mon_addr, fsopt->mon_addr)) {
> > +		kfree(fsc->mount_options->mon_addr);
> > +		fsc->mount_options->mon_addr = fsopt->mon_addr;
> > +		fsopt->mon_addr = NULL;
> > +		printk(KERN_NOTICE "ceph: monitor addresses recorded, but not used for reconnection");
> 
> It's currently more in-vogue to use pr_notice() for this. I'll plan to
> make that (minor) change before I merge. No need to resend.

Yeah, this was the only comment I had too.  I saw some issues in the
previous revision but the changes to ceph_parse_source() seem to fix it in
this revision.

The other annoying thing I found isn't related with this patchset but with
a change that's been done some time ago by Xiubo (added to CC): it looks
like that if we have an invalid parameter (for example, wrong secret)
we'll always get -EHOSTUNREACH.

See below a possible fix (although I'm not entirely sure that's the correct
one).

Cheers,
--
Luís

From a988d24d8e72fc4933459f3dd5d303cbc9a566ed Mon Sep 17 00:00:00 2001
From: Luis Henriques <lhenriques@suse.de>
Date: Wed, 14 Jul 2021 16:56:36 +0100
Subject: [PATCH] ceph: don't hide error code if we don't have mdsmap

Since commit 97820058fb28 ("ceph: check availability of mds cluster on mount
after wait timeout") we're returning -EHOSTUNREACH, even if the error isn't
related with the MDSs availability.  For example, we'll get it even if we're
trying to mounting a filesystem with an invalid username or secret.

Only return this error if we get -EIO.

Fixes: 97820058fb28 ("ceph: check availability of mds cluster on mount after wait timeout")
Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 fs/ceph/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 086a1ceec9d8..67d70059ce9f 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -1230,7 +1230,8 @@ static int ceph_get_tree(struct fs_context *fc)
 	return 0;
 
 out_splat:
-	if (!ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
+	if ((err == -EIO) &&
+	    !ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
 		pr_info("No mds server is up or the cluster is laggy\n");
 		err = -EHOSTUNREACH;
 	}

Re: [PATCH v4 4/5] ceph: record updated mon_addr on remount

[Jeff Layton]

On Wed, 2021-07-14 at 17:35 +0100, Luis Henriques wrote:
> On Wed, Jul 14, 2021 at 12:17:33PM -0400, Jeff Layton wrote:
> > On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> > > Note that the new monitors are just shown in /proc/mounts.
> > > Ceph does not (re)connect to new monitors yet.
> > > 
> > > Signed-off-by: Venky Shankar <vshankar@redhat.com>
> > > ---
> > >  fs/ceph/super.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > index d8c6168b7fcd..d3a5a3729c5b 100644
> > > --- a/fs/ceph/super.c
> > > +++ b/fs/ceph/super.c
> > > @@ -1268,6 +1268,13 @@ static int ceph_reconfigure_fc(struct fs_context *fc)
> > >  	else
> > >  		ceph_clear_mount_opt(fsc, ASYNC_DIROPS);
> > >  
> > > +	if (strcmp(fsc->mount_options->mon_addr, fsopt->mon_addr)) {
> > > +		kfree(fsc->mount_options->mon_addr);
> > > +		fsc->mount_options->mon_addr = fsopt->mon_addr;
> > > +		fsopt->mon_addr = NULL;
> > > +		printk(KERN_NOTICE "ceph: monitor addresses recorded, but not used for reconnection");
> > 
> > It's currently more in-vogue to use pr_notice() for this. I'll plan to
> > make that (minor) change before I merge. No need to resend.
> 
> Yeah, this was the only comment I had too.  I saw some issues in the
> previous revision but the changes to ceph_parse_source() seem to fix it in
> this revision.
> 
> The other annoying thing I found isn't related with this patchset but with
> a change that's been done some time ago by Xiubo (added to CC): it looks
> like that if we have an invalid parameter (for example, wrong secret)
> we'll always get -EHOSTUNREACH.
> 
> See below a possible fix (although I'm not entirely sure that's the correct
> one).
> 
> Cheers,
> --
> Luís
> 
> From a988d24d8e72fc4933459f3dd5d303cbc9a566ed Mon Sep 17 00:00:00 2001
> From: Luis Henriques <lhenriques@suse.de>
> Date: Wed, 14 Jul 2021 16:56:36 +0100
> Subject: [PATCH] ceph: don't hide error code if we don't have mdsmap
> 
> Since commit 97820058fb28 ("ceph: check availability of mds cluster on mount
> after wait timeout") we're returning -EHOSTUNREACH, even if the error isn't
> related with the MDSs availability.  For example, we'll get it even if we're
> trying to mounting a filesystem with an invalid username or secret.
> 
> Only return this error if we get -EIO.
> 
> Fixes: 97820058fb28 ("ceph: check availability of mds cluster on mount after wait timeout")
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
>  fs/ceph/super.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 086a1ceec9d8..67d70059ce9f 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -1230,7 +1230,8 @@ static int ceph_get_tree(struct fs_context *fc)
>  	return 0;
>  
>  out_splat:
> -	if (!ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
> +	if ((err == -EIO) &&
> +	    !ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
>  		pr_info("No mds server is up or the cluster is laggy\n");
>  		err = -EHOSTUNREACH;
>  	}

Yeah, I've noticed that message pop up under all sorts of circumstances
and it is an annoyance. I'm happy to consider such a patch if you send
it separately.

That said, I'm honestly not sure this message is really helpful, and
overriding errors like this at a high level seems sort of sketchy. Maybe
we should just drop that message, or figure out a way to limit it to
_just_ that situation.

--
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v4 4/5] ceph: record updated mon_addr on remount

[Luis Henriques]

On Wed, Jul 14, 2021 at 02:15:50PM -0400, Jeff Layton wrote:
> On Wed, 2021-07-14 at 17:35 +0100, Luis Henriques wrote:
> > On Wed, Jul 14, 2021 at 12:17:33PM -0400, Jeff Layton wrote:
> > > On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> > > > Note that the new monitors are just shown in /proc/mounts.
> > > > Ceph does not (re)connect to new monitors yet.
> > > > 
> > > > Signed-off-by: Venky Shankar <vshankar@redhat.com>
> > > > ---
> > > >  fs/ceph/super.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > > index d8c6168b7fcd..d3a5a3729c5b 100644
> > > > --- a/fs/ceph/super.c
> > > > +++ b/fs/ceph/super.c
> > > > @@ -1268,6 +1268,13 @@ static int ceph_reconfigure_fc(struct fs_context *fc)
> > > >  	else
> > > >  		ceph_clear_mount_opt(fsc, ASYNC_DIROPS);
> > > >  
> > > > +	if (strcmp(fsc->mount_options->mon_addr, fsopt->mon_addr)) {
> > > > +		kfree(fsc->mount_options->mon_addr);
> > > > +		fsc->mount_options->mon_addr = fsopt->mon_addr;
> > > > +		fsopt->mon_addr = NULL;
> > > > +		printk(KERN_NOTICE "ceph: monitor addresses recorded, but not used for reconnection");
> > > 
> > > It's currently more in-vogue to use pr_notice() for this. I'll plan to
> > > make that (minor) change before I merge. No need to resend.
> > 
> > Yeah, this was the only comment I had too.  I saw some issues in the
> > previous revision but the changes to ceph_parse_source() seem to fix it in
> > this revision.
> > 
> > The other annoying thing I found isn't related with this patchset but with
> > a change that's been done some time ago by Xiubo (added to CC): it looks
> > like that if we have an invalid parameter (for example, wrong secret)
> > we'll always get -EHOSTUNREACH.
> > 
> > See below a possible fix (although I'm not entirely sure that's the correct
> > one).
> > 
> > Cheers,
> > --
> > Luís
> > 
> > From a988d24d8e72fc4933459f3dd5d303cbc9a566ed Mon Sep 17 00:00:00 2001
> > From: Luis Henriques <lhenriques@suse.de>
> > Date: Wed, 14 Jul 2021 16:56:36 +0100
> > Subject: [PATCH] ceph: don't hide error code if we don't have mdsmap
> > 
> > Since commit 97820058fb28 ("ceph: check availability of mds cluster on mount
> > after wait timeout") we're returning -EHOSTUNREACH, even if the error isn't
> > related with the MDSs availability.  For example, we'll get it even if we're
> > trying to mounting a filesystem with an invalid username or secret.
> > 
> > Only return this error if we get -EIO.
> > 
> > Fixes: 97820058fb28 ("ceph: check availability of mds cluster on mount after wait timeout")
> > Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > ---
> >  fs/ceph/super.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 086a1ceec9d8..67d70059ce9f 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -1230,7 +1230,8 @@ static int ceph_get_tree(struct fs_context *fc)
> >  	return 0;
> >  
> >  out_splat:
> > -	if (!ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
> > +	if ((err == -EIO) &&
> > +	    !ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
> >  		pr_info("No mds server is up or the cluster is laggy\n");
> >  		err = -EHOSTUNREACH;
> >  	}
> 
> Yeah, I've noticed that message pop up under all sorts of circumstances
> and it is an annoyance. I'm happy to consider such a patch if you send
> it separately.
> 
> That said, I'm honestly not sure this message is really helpful, and
> overriding errors like this at a high level seems sort of sketchy. Maybe
> we should just drop that message, or figure out a way to limit it to
> _just_ that situation.

I agree that the message isn't really useful but the -EHOSTUNREACH is a
bit more confusing if we get it in the mount command when we simply have a
typo in the parameters.

Anyway, after looking closer, I couldn't find a way to reach this code
and have -EHOSTUNREACH to make sense.  When the MDSs are down/unreachable
we have already err set to this error code.  This would mean that the
correct fix would be to simply drop this 'if' statement.  Xiubo, do you
remember how would this be possible?

Cheers,
--
Luís

Re: [PATCH v4 4/5] ceph: record updated mon_addr on remount

[Venky Shankar]

On Wed, Jul 14, 2021 at 9:47 PM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> > Note that the new monitors are just shown in /proc/mounts.
> > Ceph does not (re)connect to new monitors yet.
> >
> > Signed-off-by: Venky Shankar <vshankar@redhat.com>
> > ---
> >  fs/ceph/super.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index d8c6168b7fcd..d3a5a3729c5b 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -1268,6 +1268,13 @@ static int ceph_reconfigure_fc(struct fs_context *fc)
> >       else
> >               ceph_clear_mount_opt(fsc, ASYNC_DIROPS);
> >
> > +     if (strcmp(fsc->mount_options->mon_addr, fsopt->mon_addr)) {
> > +             kfree(fsc->mount_options->mon_addr);
> > +             fsc->mount_options->mon_addr = fsopt->mon_addr;
> > +             fsopt->mon_addr = NULL;
> > +             printk(KERN_NOTICE "ceph: monitor addresses recorded, but not used for reconnection");
>
> It's currently more in-vogue to use pr_notice() for this. I'll plan to
> make that (minor) change before I merge. No need to resend.

Got it. ACK.

>
> > +     }
> > +
> >       sync_filesystem(fc->root->d_sb);
> >       return 0;
> >  }
>
> --
> Jeff Layton <jlayton@redhat.com>
>


-- 
Cheers,
Venky

Re: [PATCH v4 4/5] ceph: record updated mon_addr on remount

[Jeff Layton]

On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> Note that the new monitors are just shown in /proc/mounts.
> Ceph does not (re)connect to new monitors yet.
> 
> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> ---
>  fs/ceph/super.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index d8c6168b7fcd..d3a5a3729c5b 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -1268,6 +1268,13 @@ static int ceph_reconfigure_fc(struct fs_context *fc)
>  	else
>  		ceph_clear_mount_opt(fsc, ASYNC_DIROPS);
>  
> +	if (strcmp(fsc->mount_options->mon_addr, fsopt->mon_addr)) {

One other problem with this patch...

The above needs to use strcmp_null(). I'll plan to make that change too.

> +		kfree(fsc->mount_options->mon_addr);
> +		fsc->mount_options->mon_addr = fsopt->mon_addr;
> +		fsopt->mon_addr = NULL;
> +		printk(KERN_NOTICE "ceph: monitor addresses recorded, but not used for reconnection");
> +	}
> +
>  	sync_filesystem(fc->root->d_sb);
>  	return 0;
>  }
-- 
Jeff Layton <jlayton@redhat.com>

[PATCH v4 5/5] doc: document new CephFS mount device syntax

[Venky Shankar]

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 Documentation/filesystems/ceph.rst | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/filesystems/ceph.rst b/Documentation/filesystems/ceph.rst
index 7d2ef4e27273..4942e018db85 100644
--- a/Documentation/filesystems/ceph.rst
+++ b/Documentation/filesystems/ceph.rst
@@ -82,7 +82,7 @@ Mount Syntax
 
 The basic mount syntax is::
 
- # mount -t ceph monip[:port][,monip2[:port]...]:/[subdir] mnt
+ # mount -t ceph user@fsid.fs_name=/[subdir] mnt -o mon_addr=monip1[:port][/monip2[:port]]
 
 You only need to specify a single monitor, as the client will get the
 full list when it connects.  (However, if the monitor you specify
@@ -90,16 +90,35 @@ happens to be down, the mount won't succeed.)  The port can be left
 off if the monitor is using the default.  So if the monitor is at
 1.2.3.4::
 
- # mount -t ceph 1.2.3.4:/ /mnt/ceph
+ # mount -t ceph cephuser@07fe3187-00d9-42a3-814b-72a4d5e7d5be.cephfs=/ /mnt/ceph -o mon_addr=1.2.3.4
 
 is sufficient.  If /sbin/mount.ceph is installed, a hostname can be
-used instead of an IP address.
+used instead of an IP address and the cluster FSID can be left out
+(as the mount helper will fill it in by reading the ceph configuration
+file)::
 
+  # mount -t ceph cephuser@cephfs=/ /mnt/ceph -o mon_addr=mon-addr
 
+Multiple monitor addresses can be passed by separating each address with a slash (`/`)::
+
+  # mount -t ceph cephuser@cephfs=/ /mnt/ceph -o mon_addr=192.168.1.100/192.168.1.101
+
+When using the mount helper, monitor address can be read from ceph
+configuration file if available. Note that, the cluster FSID (passed as part
+of the device string) is validated by checking it with the FSID reported by
+the monitor.
 
 Mount Options
 =============
 
+  mon_addr=ip_address[:port][/ip_address[:port]]
+	Monitor address to the cluster. This is used to bootstrap the
+        connection to the cluster. Once connection is established, the
+        monitor addresses in the monitor map are followed.
+
+  fsid=cluster-id
+	FSID of the cluster (from `ceph fsid` command).
+
   ip=A.B.C.D[:N]
 	Specify the IP and/or port the client should bind to locally.
 	There is normally not much reason to do this.  If the IP is not
-- 
2.27.0

Re: [PATCH v4 0/5] ceph: new mount device syntax

[Jeff Layton]

On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> v4:
>   - fix delimiter check in ceph_parse_ips()
>   - use __func__ in ceph_parse_ips() instead of hardcoded function name
>   - KERN_NOTICE that mon_addr is recorded but not reconnected
> 
> Venky Shankar (5):
>   ceph: generalize addr/ip parsing based on delimiter
>   ceph: rename parse_fsid() to ceph_parse_fsid() and export
>   ceph: new device mount syntax
>   ceph: record updated mon_addr on remount
>   doc: document new CephFS mount device syntax
> 
>  Documentation/filesystems/ceph.rst |  25 ++++-
>  drivers/block/rbd.c                |   3 +-
>  fs/ceph/super.c                    | 151 +++++++++++++++++++++++++++--
>  fs/ceph/super.h                    |   3 +
>  include/linux/ceph/libceph.h       |   5 +-
>  include/linux/ceph/messenger.h     |   2 +-
>  net/ceph/ceph_common.c             |  17 ++--
>  net/ceph/messenger.c               |   8 +-
>  8 files changed, 186 insertions(+), 28 deletions(-)
> 

This looks good, Venky. Nice work!

I'm doing a bit of testing with this now, and will plan to merge it into
the testing branch later today.
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v4 0/5] ceph: new mount device syntax

[Jeff Layton]

On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> v4:
>   - fix delimiter check in ceph_parse_ips()
>   - use __func__ in ceph_parse_ips() instead of hardcoded function name
>   - KERN_NOTICE that mon_addr is recorded but not reconnected
> 
> Venky Shankar (5):
>   ceph: generalize addr/ip parsing based on delimiter
>   ceph: rename parse_fsid() to ceph_parse_fsid() and export
>   ceph: new device mount syntax
>   ceph: record updated mon_addr on remount
>   doc: document new CephFS mount device syntax
> 
>  Documentation/filesystems/ceph.rst |  25 ++++-
>  drivers/block/rbd.c                |   3 +-
>  fs/ceph/super.c                    | 151 +++++++++++++++++++++++++++--
>  fs/ceph/super.h                    |   3 +
>  include/linux/ceph/libceph.h       |   5 +-
>  include/linux/ceph/messenger.h     |   2 +-
>  net/ceph/ceph_common.c             |  17 ++--
>  net/ceph/messenger.c               |   8 +-
>  8 files changed, 186 insertions(+), 28 deletions(-)
> 

I've gone ahead and merged these into the testing branch, though I have
not had time to personally do any testing with the new mount helper or
new syntax. It does seem to behave fine with the old mount helper and
syntax.

Venky, I did make a couple of minor changes to this patch:

    ceph: record updated mon_addr on remount

Please take a look when you have time and make sure they're OK.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v4 0/5] ceph: new mount device syntax

[Venky Shankar]

On Sat, Jul 17, 2021 at 12:17 AM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> > v4:
> >   - fix delimiter check in ceph_parse_ips()
> >   - use __func__ in ceph_parse_ips() instead of hardcoded function name
> >   - KERN_NOTICE that mon_addr is recorded but not reconnected
> >
> > Venky Shankar (5):
> >   ceph: generalize addr/ip parsing based on delimiter
> >   ceph: rename parse_fsid() to ceph_parse_fsid() and export
> >   ceph: new device mount syntax
> >   ceph: record updated mon_addr on remount
> >   doc: document new CephFS mount device syntax
> >
> >  Documentation/filesystems/ceph.rst |  25 ++++-
> >  drivers/block/rbd.c                |   3 +-
> >  fs/ceph/super.c                    | 151 +++++++++++++++++++++++++++--
> >  fs/ceph/super.h                    |   3 +
> >  include/linux/ceph/libceph.h       |   5 +-
> >  include/linux/ceph/messenger.h     |   2 +-
> >  net/ceph/ceph_common.c             |  17 ++--
> >  net/ceph/messenger.c               |   8 +-
> >  8 files changed, 186 insertions(+), 28 deletions(-)
> >
>
> I've gone ahead and merged these into the testing branch, though I have
> not had time to personally do any testing with the new mount helper or
> new syntax. It does seem to behave fine with the old mount helper and
> syntax.
>
> Venky, I did make a couple of minor changes to this patch:
>
>     ceph: record updated mon_addr on remount
>
> Please take a look when you have time and make sure they're OK.

Looks good, Thanks.

>
> Thanks,
> --
> Jeff Layton <jlayton@redhat.com>
>


-- 
Cheers,
Venky