[PATCH 0/4] ceph: new mount device syntax

[PATCH 0/4] ceph: new mount device syntax

[Venky Shankar]

This series introduces changes Ceph File System mount device string.
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.

Venky Shankar (4):
  ceph: new device mount syntax
  ceph: validate cluster FSID for new device syntax
  ceph: record updated mon_addr on remount
  doc: document new CephFS mount device syntax

 Documentation/filesystems/ceph.rst |  23 ++++-
 fs/ceph/super.c                    | 132 ++++++++++++++++++++++++++---
 fs/ceph/super.h                    |   4 +
 include/linux/ceph/libceph.h       |   1 +
 net/ceph/ceph_common.c             |   3 +-
 5 files changed, 149 insertions(+), 14 deletions(-)

-- 
2.27.0

[PATCH 1/4] 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 | 117 +++++++++++++++++++++++++++++++++++++++++++-----
 fs/ceph/super.h |   3 ++
 2 files changed, 110 insertions(+), 10 deletions(-)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 9b1b7f4cfdd4..950a28ad9c59 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),
 	{}
@@ -226,10 +228,68 @@ static void canonicalize_path(char *path)
 	path[j] = '\0';
 }
 
+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);
+	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)
+{
+	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 != '=')
+                return invalfc(fc, "separator '=' missing in source");
+
+	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");
+
+	++fs_name_start; /* start of file system name */
+	fsopt->mds_namespace = kstrndup(fs_name_start,
+					dev_name_end - fs_name_start, 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.  Distinguish the server list from the path.
+ * 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.
  *
- * The source will look like:
+ * 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)
+ *
+ * Old device syntax is:
  *     <server_spec>[,<server_spec>...]:[<path>]
  * where
  *     <server_spec> is <ip>[:<port>]
@@ -262,22 +322,48 @@ 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);
+	dout("trying new device syntax");
+	ret = ceph_parse_new_source(dev_name, dev_name_end, fc);
+	if (ret == 0)
+		goto done;
+
+	dout("trying old device syntax");
+	ret = ceph_parse_old_source(dev_name, dev_name_end, fc);
 	if (ret)
-		return ret;
+		goto out;
 
+ done:
 	fc->source = param->string;
 	param->string = NULL;
-	return 0;
+ out:
+	return ret;
+}
+
+static int ceph_parse_mon_addr(struct fs_parameter *param,
+			       struct fs_context *fc)
+{
+	int r;
+	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
+	struct ceph_mount_options *fsopt = pctx->opts;
+
+	kfree(fsopt->mon_addr);
+	fsopt->mon_addr = kstrdup(param->string, GFP_KERNEL);
+	if (!fsopt->mon_addr)
+		return -ENOMEM;
+
+	strreplace(param->string, '/', ',');
+	r = ceph_parse_mon_ips(param->string, strlen(param->string),
+			       pctx->copts, fc->log.log);
+	param->string = NULL;
+	return r;
 }
 
 static int ceph_parse_mount_param(struct fs_context *fc,
@@ -322,6 +408,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)
@@ -473,6 +561,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);
 }
 
@@ -516,6 +605,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);
 }
 
@@ -571,9 +664,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");
 
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 839e6b0239ee..557348ff3203 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -88,6 +88,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()
@@ -97,6 +99,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 2/4] ceph: validate cluster FSID for new device syntax

[Venky Shankar]

The new device syntax requires the cluster FSID as part
of the device string. Use this FSID to verify if it matches
the cluster FSID we get back from the monitor, failing the
mount on mismatch.

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

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 950a28ad9c59..84bc06e51680 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -266,6 +266,9 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
 	if (!fs_name_start)
 		return invalfc(fc, "missing file system name");
 
+	if (parse_fsid(fsid_start, &fsopt->fsid))
+		return invalfc(fc, "invalid fsid format");
+
 	++fs_name_start; /* start of file system name */
 	fsopt->mds_namespace = kstrndup(fs_name_start,
 					dev_name_end - fs_name_start, GFP_KERNEL);
@@ -748,6 +751,12 @@ static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
 	}
 	opt = NULL; /* fsc->client now owns this */
 
+	/* help learn fsid */
+	if (fsopt->new_dev_syntax) {
+		ceph_check_fsid(fsc->client, &fsopt->fsid);
+		fsc->client->have_fsid = true;
+	}
+
 	fsc->client->extra_mon_dispatch = extra_mon_dispatch;
 	ceph_set_opt(fsc->client, ABORT_ON_FULL);
 
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 557348ff3203..cfd8ec25a9a8 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -100,6 +100,7 @@ struct ceph_mount_options {
 	char *server_path;    /* default NULL (means "/") */
 	char *fscache_uniq;   /* default NULL */
 	char *mon_addr;
+	struct ceph_fsid fsid;
 };
 
 struct ceph_fs_client {
diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index 409d8c29bc4f..24c1f4e9144d 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -296,6 +296,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 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 97d6ea763e32..db21734462a4 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -217,7 +217,7 @@ void *ceph_kvmalloc(size_t size, gfp_t flags)
 	return p;
 }
 
-static int parse_fsid(const char *str, struct ceph_fsid *fsid)
+int parse_fsid(const char *str, struct ceph_fsid *fsid)
 {
 	int i = 0;
 	char tmp[3];
@@ -247,6 +247,7 @@ static int parse_fsid(const char *str, struct ceph_fsid *fsid)
 	dout("parse_fsid ret %d got fsid %pU\n", err, fsid);
 	return err;
 }
+EXPORT_SYMBOL(parse_fsid);
 
 /*
  * ceph options
-- 
2.27.0

[PATCH 3/4] 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 84bc06e51680..48493ac372fa 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -1250,6 +1250,12 @@ 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 = kstrdup(fsopt->mon_addr,
+						       GFP_KERNEL);
+	}
+
 	sync_filesystem(fc->root->d_sb);
 	return 0;
 }
-- 
2.27.0

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

[Venky Shankar]

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

diff --git a/Documentation/filesystems/ceph.rst b/Documentation/filesystems/ceph.rst
index 7d2ef4e27273..e46f9091b851 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_host=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,33 @@ 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_host=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_host=mon-addr
 
+Multiple monitor addresses can be passed by separating each address with a slash (`/`)::
+
+  # mount -t ceph cephuser@cephfs=/ /mnt/ceph -o mon_host=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_host=ip_address[:port][/ip_address[:port]]
+	Monitor address to the cluster
+
+  fsid=cluster-id
+	FSID of the cluster
+
   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 2/4] ceph: validate cluster FSID for new device syntax

[Jeff Layton]

On Mon, 2021-06-28 at 13:25 +0530, Venky Shankar wrote:
> The new device syntax requires the cluster FSID as part
> of the device string. Use this FSID to verify if it matches
> the cluster FSID we get back from the monitor, failing the
> mount on mismatch.
> 
> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> ---
>  fs/ceph/super.c              | 9 +++++++++
>  fs/ceph/super.h              | 1 +
>  include/linux/ceph/libceph.h | 1 +
>  net/ceph/ceph_common.c       | 3 ++-
>  4 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 950a28ad9c59..84bc06e51680 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -266,6 +266,9 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>  	if (!fs_name_start)
>  		return invalfc(fc, "missing file system name");
>  
> +	if (parse_fsid(fsid_start, &fsopt->fsid))
> +		return invalfc(fc, "invalid fsid format");
> +
>  	++fs_name_start; /* start of file system name */
>  	fsopt->mds_namespace = kstrndup(fs_name_start,
>  					dev_name_end - fs_name_start, GFP_KERNEL);
> @@ -748,6 +751,12 @@ static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
>  	}
>  	opt = NULL; /* fsc->client now owns this */
>  
> +	/* help learn fsid */
> +	if (fsopt->new_dev_syntax) {
> +		ceph_check_fsid(fsc->client, &fsopt->fsid);
> +		fsc->client->have_fsid = true;
> +	}
> +
>  	fsc->client->extra_mon_dispatch = extra_mon_dispatch;
>  	ceph_set_opt(fsc->client, ABORT_ON_FULL);
>  
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 557348ff3203..cfd8ec25a9a8 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -100,6 +100,7 @@ struct ceph_mount_options {
>  	char *server_path;    /* default NULL (means "/") */
>  	char *fscache_uniq;   /* default NULL */
>  	char *mon_addr;
> +	struct ceph_fsid fsid;
>  };
>  
>  struct ceph_fs_client {
> diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
> index 409d8c29bc4f..24c1f4e9144d 100644
> --- a/include/linux/ceph/libceph.h
> +++ b/include/linux/ceph/libceph.h
> @@ -296,6 +296,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 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 97d6ea763e32..db21734462a4 100644
> --- a/net/ceph/ceph_common.c
> +++ b/net/ceph/ceph_common.c
> @@ -217,7 +217,7 @@ void *ceph_kvmalloc(size_t size, gfp_t flags)
>  	return p;
>  }
>  
> -static int parse_fsid(const char *str, struct ceph_fsid *fsid)
> +int parse_fsid(const char *str, struct ceph_fsid *fsid)
>  {
>  	int i = 0;
>  	char tmp[3];
> @@ -247,6 +247,7 @@ static int parse_fsid(const char *str, struct ceph_fsid *fsid)
>  	dout("parse_fsid ret %d got fsid %pU\n", err, fsid);
>  	return err;
>  }
> +EXPORT_SYMBOL(parse_fsid);

This function name is too generic. Maybe rename it to "ceph_parse_fsid"?

>  
>  /*
>   * ceph options

-- 
Jeff Layton <jlayton@redhat.com>

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

[Jeff Layton]

On Mon, 2021-06-28 at 13:25 +0530, Venky Shankar wrote:
> Note that the new monitors are just shown in /proc/mounts.
> Ceph does not (re)connect to new monitors yet.
> 

I wasn't sure we'd want to do that anyway, but now I think it might be a
good idea. Being able to re-point a client to a new set of mons manually
seems like a reasonable thing to be able to do in a disaster recovery
situation or the like.

> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> ---
>  fs/ceph/super.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 84bc06e51680..48493ac372fa 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -1250,6 +1250,12 @@ 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 = kstrdup(fsopt->mon_addr,
> +						       GFP_KERNEL);
> +	}
> +

It's probably worth logging a KERN_NOTICE message or something that the
new monitor addresses were ignored. That way, if you implement
connecting to the new mons on remount later you'd have a way to tell.


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

Re: [PATCH 4/4] doc: document new CephFS mount device syntax

[Jeff Layton]

On Mon, 2021-06-28 at 13:25 +0530, Venky Shankar wrote:
> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> ---
>  Documentation/filesystems/ceph.rst | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/filesystems/ceph.rst b/Documentation/filesystems/ceph.rst
> index 7d2ef4e27273..e46f9091b851 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_host=monip1[:port][/monip2[:port]]
>  

The actual code lists the option as "mon_addr".

>  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,33 @@ 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_host=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_host=mon-addr
>  
> +Multiple monitor addresses can be passed by separating each address with a slash (`/`)::
> +
> +  # mount -t ceph cephuser@cephfs=/ /mnt/ceph -o mon_host=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_host=ip_address[:port][/ip_address[:port]]
> +	Monitor address to the cluster
> +

Might want to mention that "mon_addr" is just used to bootstrap the
connection to the cluster, and that it'll follow the monmap after that
point.

> +  fsid=cluster-id
> +	FSID of the cluster
> +
>    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

-- 
Jeff Layton <jlayton@redhat.com>

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

[Jeff Layton]

On Mon, 2021-06-28 at 13:25 +0530, Venky Shankar wrote:
> This series introduces changes Ceph File System mount device string.
> 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.
> 
> Venky Shankar (4):
>   ceph: new device mount syntax
>   ceph: validate cluster FSID for new device syntax
>   ceph: record updated mon_addr on remount
>   doc: document new CephFS mount device syntax
> 
>  Documentation/filesystems/ceph.rst |  23 ++++-
>  fs/ceph/super.c                    | 132 ++++++++++++++++++++++++++---
>  fs/ceph/super.h                    |   4 +
>  include/linux/ceph/libceph.h       |   1 +
>  net/ceph/ceph_common.c             |   3 +-
>  5 files changed, 149 insertions(+), 14 deletions(-)
> 

Nice work, Venky. It needs a few minor changes, but this looks good
overall. Unless anyone has objections or other suggestions for changes,
we ought to aim to get this into the testing branch soon and aim for
merging it in v5.15.

Thoughts?
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/4] ceph: validate cluster FSID for new device syntax

[Venky Shankar]

On Mon, Jun 28, 2021 at 8:34 PM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Mon, 2021-06-28 at 13:25 +0530, Venky Shankar wrote:
> > The new device syntax requires the cluster FSID as part
> > of the device string. Use this FSID to verify if it matches
> > the cluster FSID we get back from the monitor, failing the
> > mount on mismatch.
> >
> > Signed-off-by: Venky Shankar <vshankar@redhat.com>
> > ---
> >  fs/ceph/super.c              | 9 +++++++++
> >  fs/ceph/super.h              | 1 +
> >  include/linux/ceph/libceph.h | 1 +
> >  net/ceph/ceph_common.c       | 3 ++-
> >  4 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 950a28ad9c59..84bc06e51680 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -266,6 +266,9 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
> >       if (!fs_name_start)
> >               return invalfc(fc, "missing file system name");
> >
> > +     if (parse_fsid(fsid_start, &fsopt->fsid))
> > +             return invalfc(fc, "invalid fsid format");
> > +
> >       ++fs_name_start; /* start of file system name */
> >       fsopt->mds_namespace = kstrndup(fs_name_start,
> >                                       dev_name_end - fs_name_start, GFP_KERNEL);
> > @@ -748,6 +751,12 @@ static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
> >       }
> >       opt = NULL; /* fsc->client now owns this */
> >
> > +     /* help learn fsid */
> > +     if (fsopt->new_dev_syntax) {
> > +             ceph_check_fsid(fsc->client, &fsopt->fsid);
> > +             fsc->client->have_fsid = true;
> > +     }
> > +
> >       fsc->client->extra_mon_dispatch = extra_mon_dispatch;
> >       ceph_set_opt(fsc->client, ABORT_ON_FULL);
> >
> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > index 557348ff3203..cfd8ec25a9a8 100644
> > --- a/fs/ceph/super.h
> > +++ b/fs/ceph/super.h
> > @@ -100,6 +100,7 @@ struct ceph_mount_options {
> >       char *server_path;    /* default NULL (means "/") */
> >       char *fscache_uniq;   /* default NULL */
> >       char *mon_addr;
> > +     struct ceph_fsid fsid;
> >  };
> >
> >  struct ceph_fs_client {
> > diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
> > index 409d8c29bc4f..24c1f4e9144d 100644
> > --- a/include/linux/ceph/libceph.h
> > +++ b/include/linux/ceph/libceph.h
> > @@ -296,6 +296,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 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 97d6ea763e32..db21734462a4 100644
> > --- a/net/ceph/ceph_common.c
> > +++ b/net/ceph/ceph_common.c
> > @@ -217,7 +217,7 @@ void *ceph_kvmalloc(size_t size, gfp_t flags)
> >       return p;
> >  }
> >
> > -static int parse_fsid(const char *str, struct ceph_fsid *fsid)
> > +int parse_fsid(const char *str, struct ceph_fsid *fsid)
> >  {
> >       int i = 0;
> >       char tmp[3];
> > @@ -247,6 +247,7 @@ static int parse_fsid(const char *str, struct ceph_fsid *fsid)
> >       dout("parse_fsid ret %d got fsid %pU\n", err, fsid);
> >       return err;
> >  }
> > +EXPORT_SYMBOL(parse_fsid);
>
> This function name is too generic. Maybe rename it to "ceph_parse_fsid"?

Makes sense. ACK.

>
> >
> >  /*
> >   * ceph options
>
> --
> Jeff Layton <jlayton@redhat.com>
>


-- 
Cheers,
Venky

Re: [PATCH 4/4] doc: document new CephFS mount device syntax

[Venky Shankar]

On Mon, Jun 28, 2021 at 8:57 PM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Mon, 2021-06-28 at 13:25 +0530, Venky Shankar wrote:
> > Signed-off-by: Venky Shankar <vshankar@redhat.com>
> > ---
> >  Documentation/filesystems/ceph.rst | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/filesystems/ceph.rst b/Documentation/filesystems/ceph.rst
> > index 7d2ef4e27273..e46f9091b851 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_host=monip1[:port][/monip2[:port]]
> >
>
> The actual code lists the option as "mon_addr".

Good catch. The mount helper uses `mon_host`, however, the option
passed to the kernel is `mon_addr`.

>
> >  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,33 @@ 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_host=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_host=mon-addr
> >
> > +Multiple monitor addresses can be passed by separating each address with a slash (`/`)::
> > +
> > +  # mount -t ceph cephuser@cephfs=/ /mnt/ceph -o mon_host=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_host=ip_address[:port][/ip_address[:port]]
> > +     Monitor address to the cluster
> > +
>
> Might want to mention that "mon_addr" is just used to bootstrap the
> connection to the cluster, and that it'll follow the monmap after that
> point.

ACK

>
> > +  fsid=cluster-id
> > +     FSID of the cluster
> > +
> >    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
>
> --
> Jeff Layton <jlayton@redhat.com>
>


-- 
Cheers,
Venky

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

[Luis Henriques]

On Mon, Jun 28, 2021 at 01:25:41PM +0530, Venky Shankar wrote:
> This series introduces changes Ceph File System mount device string.
> 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.

I haven't fully reviewed this patchset yet.  I've started doing that (I'll
send a few comments in a bit), but stopped when I found some parsing
issues that need fixing.

I gave these patches a quick test (with a not-so-up-to-date mount.ceph)
and saw the splat below.  Does this patchset depends on anything on the
testing branch?  I've tried it on v5.13 mainline.

I also had a segmentation fault on the userspace mount.  I've used
something like:

mount -t ceph admin@ef274016-6131-4936-9277-946b535f5d03.a=/ /mnt/test

Cheers,
--
Luís

[    7.847565] ------------[ cut here ]------------
[    7.849322] kernel BUG at net/ceph/mon_client.c:209!
[    7.851151] invalid opcode: 0000 [#1] SMP PTI
[    7.852651] CPU: 1 PID: 188 Comm: mount Not tainted 5.13.0+ #32
[    7.854698] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a-rebuilt.opensuse.org 04/01/2014
[    7.858555] RIP: 0010:__open_session+0x186/0x210 [libceph]
[    7.860517] Code: 50 01 00 00 e8 db a9 ff ff 48 8b b3 50 01 00 00 48 89 ef 5b 5d 41 5c e9 68 b6 ff ff e8 43 8e 48 e1 31 d2 f7 f5 e9 c9 fe ff ff <0f> 0b 48 8b 43 08 41 b91
[    7.866902] RSP: 0018:ffffc9000085fda0 EFLAGS: 00010246
[    7.868736] RAX: ffff888114396520 RBX: 0000000000003a98 RCX: 0000000000000000
[    7.871260] RDX: 0000000000000000 RSI: ffff88810eeec2a8 RDI: ffff88810eeec298
[    7.873653] RBP: 0000000000000000 R08: 0000000000000008 R09: 0000000000000000
[    7.875923] R10: ffffc9000085fdc0 R11: 0000000000000000 R12: 00000000ffffffff
[    7.878229] R13: ffff88811aef8500 R14: 00000000fffee289 R15: 7fffffffffffffff
[    7.880503] FS:  00007fda2ac9b800(0000) GS:ffff888237d00000(0000) knlGS:0000000000000000
[    7.883088] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    7.884915] CR2: 00007f837ba61e88 CR3: 000000010fefc000 CR4: 00000000000006a0
[    7.887174] Call Trace:
[    7.887920]  ceph_monc_open_session+0x43/0x60 [libceph]
[    7.889502]  __ceph_open_session+0x4b/0x250 [libceph]
[    7.891036]  ceph_get_tree+0x41b/0x880 [ceph]
[    7.892337]  vfs_get_tree+0x23/0x90
[    7.893315]  path_mount+0x73d/0xb20
[    7.894291]  __x64_sys_mount+0x103/0x140
[    7.895387]  do_syscall_64+0x45/0x80
[    7.896324]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[    7.897738] RIP: 0033:0x7fda2aeb617e
[    7.898886] Code: 48 8b 0d f5 1c 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 018
[    7.903441] RSP: 002b:00007fff4b0d3e58 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
[    7.905181] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fda2aeb617e
[    7.906813] RDX: 0000563e07b04c80 RSI: 0000563e07b04d20 RDI: 0000563e07b04ca0
[    7.908506] RBP: 0000563e07b04960 R08: 0000563e07b04be0 R09: 00007fda2af78a60
[    7.910176] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[    7.911830] R13: 0000563e07b04c80 R14: 0000563e07b04ca0 R15: 0000563e07b04960
[    7.913480] Modules linked in: ceph libceph
[    7.914492] ---[ end trace 2798408fec037d5a ]---
[    7.915582] RIP: 0010:__open_session+0x186/0x210 [libceph]
[    7.916853] Code: 50 01 00 00 e8 db a9 ff ff 48 8b b3 50 01 00 00 48 89 ef 5b 5d 41 5c e9 68 b6 ff ff e8 43 8e 48 e1 31 d2 f7 f5 e9 c9 fe ff ff <0f> 0b 48 8b 43 08 41 b91
[    7.921090] RSP: 0018:ffffc9000085fda0 EFLAGS: 00010246
[    7.922288] RAX: ffff888114396520 RBX: 0000000000003a98 RCX: 0000000000000000
[    7.923875] RDX: 0000000000000000 RSI: ffff88810eeec2a8 RDI: ffff88810eeec298
[    7.925323] RBP: 0000000000000000 R08: 0000000000000008 R09: 0000000000000000
[    7.926822] R10: ffffc9000085fdc0 R11: 0000000000000000 R12: 00000000ffffffff
[    7.928320] R13: ffff88811aef8500 R14: 00000000fffee289 R15: 7fffffffffffffff
[    7.929790] FS:  00007fda2ac9b800(0000) GS:ffff888237d00000(0000) knlGS:0000000000000000
[    7.931471] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    7.932579] CR2: 00007f837ba61e88 CR3: 000000010fefc000 CR4: 00000000000006a0

Re: [PATCH 1/4] ceph: new device mount syntax

[Luis Henriques]

[ As I said, I didn't fully reviewed this patch.  Just sending out a few
  comments. ]

On Mon, Jun 28, 2021 at 01:25:42PM +0530, Venky Shankar wrote:
> 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 | 117 +++++++++++++++++++++++++++++++++++++++++++-----
>  fs/ceph/super.h |   3 ++
>  2 files changed, 110 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 9b1b7f4cfdd4..950a28ad9c59 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),
>  	{}
> @@ -226,10 +228,68 @@ static void canonicalize_path(char *path)
>  	path[j] = '\0';
>  }
>  
> +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);
> +	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)
> +{
> +	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 != '=')
> +                return invalfc(fc, "separator '=' missing in source");

An annoying thing is that we'll always see this error message when falling
back to the old_source method.

(Also, is there a good reason for using '=' instead of ':'?  I probably
missed this discussion somewhere else already...)

> +
> +	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");
> +
> +	++fs_name_start; /* start of file system name */
> +	fsopt->mds_namespace = kstrndup(fs_name_start,
> +					dev_name_end - fs_name_start, 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.  Distinguish the server list from the path.
> + * 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.
>   *
> - * The source will look like:
> + * 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)
> + *
> + * Old device syntax is:
>   *     <server_spec>[,<server_spec>...]:[<path>]
>   * where
>   *     <server_spec> is <ip>[:<port>]
> @@ -262,22 +322,48 @@ 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);
> +	dout("trying new device syntax");
> +	ret = ceph_parse_new_source(dev_name, dev_name_end, fc);
> +	if (ret == 0)
> +		goto done;
> +
> +	dout("trying old device syntax");
> +	ret = ceph_parse_old_source(dev_name, dev_name_end, fc);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
> + done:
>  	fc->source = param->string;
>  	param->string = NULL;
> -	return 0;
> + out:
> +	return ret;
> +}
> +
> +static int ceph_parse_mon_addr(struct fs_parameter *param,
> +			       struct fs_context *fc)
> +{
> +	int r;
> +	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
> +	struct ceph_mount_options *fsopt = pctx->opts;
> +
> +	kfree(fsopt->mon_addr);
> +	fsopt->mon_addr = kstrdup(param->string, GFP_KERNEL);
> +	if (!fsopt->mon_addr)
> +		return -ENOMEM;
> +
> +	strreplace(param->string, '/', ',');
> +	r = ceph_parse_mon_ips(param->string, strlen(param->string),
> +			       pctx->copts, fc->log.log);
> +	param->string = NULL;

I think this will result in a memory leak.  Why don't we simply set
fsopt->mon_addr to param->string instead of kstrdup'ing it?

Cheers,
--
Luís

> +	return r;
>  }
>  
>  static int ceph_parse_mount_param(struct fs_context *fc,
> @@ -322,6 +408,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)
> @@ -473,6 +561,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);
>  }
>  
> @@ -516,6 +605,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);
>  }
>  
> @@ -571,9 +664,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");
>  
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 839e6b0239ee..557348ff3203 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -88,6 +88,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()
> @@ -97,6 +99,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
> 

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

[Venky Shankar]

On Tue, Jun 29, 2021 at 4:53 PM Luis Henriques <lhenriques@suse.de> wrote:
>
> On Mon, Jun 28, 2021 at 01:25:41PM +0530, Venky Shankar wrote:
> > This series introduces changes Ceph File System mount device string.
> > 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.
>
> I haven't fully reviewed this patchset yet.  I've started doing that (I'll
> send a few comments in a bit), but stopped when I found some parsing
> issues that need fixing.
>
> I gave these patches a quick test (with a not-so-up-to-date mount.ceph)
> and saw the splat below.  Does this patchset depends on anything on the
> testing branch?  I've tried it on v5.13 mainline.

No.

>
> I also had a segmentation fault on the userspace mount.  I've used
> something like:
>
> mount -t ceph admin@ef274016-6131-4936-9277-946b535f5d03.a=/ /mnt/test

I will check and revert (the trace below seems odd -- somewhere during
opening session with mon)

>
> Cheers,
> --
> Luís
>
> [    7.847565] ------------[ cut here ]------------
> [    7.849322] kernel BUG at net/ceph/mon_client.c:209!
> [    7.851151] invalid opcode: 0000 [#1] SMP PTI
> [    7.852651] CPU: 1 PID: 188 Comm: mount Not tainted 5.13.0+ #32
> [    7.854698] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a-rebuilt.opensuse.org 04/01/2014
> [    7.858555] RIP: 0010:__open_session+0x186/0x210 [libceph]
> [    7.860517] Code: 50 01 00 00 e8 db a9 ff ff 48 8b b3 50 01 00 00 48 89 ef 5b 5d 41 5c e9 68 b6 ff ff e8 43 8e 48 e1 31 d2 f7 f5 e9 c9 fe ff ff <0f> 0b 48 8b 43 08 41 b91
> [    7.866902] RSP: 0018:ffffc9000085fda0 EFLAGS: 00010246
> [    7.868736] RAX: ffff888114396520 RBX: 0000000000003a98 RCX: 0000000000000000
> [    7.871260] RDX: 0000000000000000 RSI: ffff88810eeec2a8 RDI: ffff88810eeec298
> [    7.873653] RBP: 0000000000000000 R08: 0000000000000008 R09: 0000000000000000
> [    7.875923] R10: ffffc9000085fdc0 R11: 0000000000000000 R12: 00000000ffffffff
> [    7.878229] R13: ffff88811aef8500 R14: 00000000fffee289 R15: 7fffffffffffffff
> [    7.880503] FS:  00007fda2ac9b800(0000) GS:ffff888237d00000(0000) knlGS:0000000000000000
> [    7.883088] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    7.884915] CR2: 00007f837ba61e88 CR3: 000000010fefc000 CR4: 00000000000006a0
> [    7.887174] Call Trace:
> [    7.887920]  ceph_monc_open_session+0x43/0x60 [libceph]
> [    7.889502]  __ceph_open_session+0x4b/0x250 [libceph]
> [    7.891036]  ceph_get_tree+0x41b/0x880 [ceph]
> [    7.892337]  vfs_get_tree+0x23/0x90
> [    7.893315]  path_mount+0x73d/0xb20
> [    7.894291]  __x64_sys_mount+0x103/0x140
> [    7.895387]  do_syscall_64+0x45/0x80
> [    7.896324]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [    7.897738] RIP: 0033:0x7fda2aeb617e
> [    7.898886] Code: 48 8b 0d f5 1c 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 018
> [    7.903441] RSP: 002b:00007fff4b0d3e58 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
> [    7.905181] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fda2aeb617e
> [    7.906813] RDX: 0000563e07b04c80 RSI: 0000563e07b04d20 RDI: 0000563e07b04ca0
> [    7.908506] RBP: 0000563e07b04960 R08: 0000563e07b04be0 R09: 00007fda2af78a60
> [    7.910176] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> [    7.911830] R13: 0000563e07b04c80 R14: 0000563e07b04ca0 R15: 0000563e07b04960
> [    7.913480] Modules linked in: ceph libceph
> [    7.914492] ---[ end trace 2798408fec037d5a ]---
> [    7.915582] RIP: 0010:__open_session+0x186/0x210 [libceph]
> [    7.916853] Code: 50 01 00 00 e8 db a9 ff ff 48 8b b3 50 01 00 00 48 89 ef 5b 5d 41 5c e9 68 b6 ff ff e8 43 8e 48 e1 31 d2 f7 f5 e9 c9 fe ff ff <0f> 0b 48 8b 43 08 41 b91
> [    7.921090] RSP: 0018:ffffc9000085fda0 EFLAGS: 00010246
> [    7.922288] RAX: ffff888114396520 RBX: 0000000000003a98 RCX: 0000000000000000
> [    7.923875] RDX: 0000000000000000 RSI: ffff88810eeec2a8 RDI: ffff88810eeec298
> [    7.925323] RBP: 0000000000000000 R08: 0000000000000008 R09: 0000000000000000
> [    7.926822] R10: ffffc9000085fdc0 R11: 0000000000000000 R12: 00000000ffffffff
> [    7.928320] R13: ffff88811aef8500 R14: 00000000fffee289 R15: 7fffffffffffffff
> [    7.929790] FS:  00007fda2ac9b800(0000) GS:ffff888237d00000(0000) knlGS:0000000000000000
> [    7.931471] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    7.932579] CR2: 00007f837ba61e88 CR3: 000000010fefc000 CR4: 00000000000006a0
>


-- 
Cheers,
Venky

Re: [PATCH 1/4] ceph: new device mount syntax

[Venky Shankar]

On Tue, Jun 29, 2021 at 5:02 PM Luis Henriques <lhenriques@suse.de> wrote:
>
> [ As I said, I didn't fully reviewed this patch.  Just sending out a few
>   comments. ]
>
> On Mon, Jun 28, 2021 at 01:25:42PM +0530, Venky Shankar wrote:
> > 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 | 117 +++++++++++++++++++++++++++++++++++++++++++-----
> >  fs/ceph/super.h |   3 ++
> >  2 files changed, 110 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 9b1b7f4cfdd4..950a28ad9c59 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),
> >       {}
> > @@ -226,10 +228,68 @@ static void canonicalize_path(char *path)
> >       path[j] = '\0';
> >  }
> >
> > +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);
> > +     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)
> > +{
> > +     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 != '=')
> > +                return invalfc(fc, "separator '=' missing in source");
>
> An annoying thing is that we'll always see this error message when falling
> back to the old_source method.
>
> (Also, is there a good reason for using '=' instead of ':'?  I probably
> missed this discussion somewhere else already...)
>
> > +
> > +     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");
> > +
> > +     ++fs_name_start; /* start of file system name */
> > +     fsopt->mds_namespace = kstrndup(fs_name_start,
> > +                                     dev_name_end - fs_name_start, 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.  Distinguish the server list from the path.
> > + * 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.
> >   *
> > - * The source will look like:
> > + * 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)
> > + *
> > + * Old device syntax is:
> >   *     <server_spec>[,<server_spec>...]:[<path>]
> >   * where
> >   *     <server_spec> is <ip>[:<port>]
> > @@ -262,22 +322,48 @@ 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);
> > +     dout("trying new device syntax");
> > +     ret = ceph_parse_new_source(dev_name, dev_name_end, fc);
> > +     if (ret == 0)
> > +             goto done;
> > +
> > +     dout("trying old device syntax");
> > +     ret = ceph_parse_old_source(dev_name, dev_name_end, fc);
> >       if (ret)
> > -             return ret;
> > +             goto out;
> >
> > + done:
> >       fc->source = param->string;
> >       param->string = NULL;
> > -     return 0;
> > + out:
> > +     return ret;
> > +}
> > +
> > +static int ceph_parse_mon_addr(struct fs_parameter *param,
> > +                            struct fs_context *fc)
> > +{
> > +     int r;
> > +     struct ceph_parse_opts_ctx *pctx = fc->fs_private;
> > +     struct ceph_mount_options *fsopt = pctx->opts;
> > +
> > +     kfree(fsopt->mon_addr);
> > +     fsopt->mon_addr = kstrdup(param->string, GFP_KERNEL);
> > +     if (!fsopt->mon_addr)
> > +             return -ENOMEM;
> > +
> > +     strreplace(param->string, '/', ',');
> > +     r = ceph_parse_mon_ips(param->string, strlen(param->string),
> > +                            pctx->copts, fc->log.log);
> > +     param->string = NULL;
>
> I think this will result in a memory leak.  Why don't we simply set
> fsopt->mon_addr to param->string instead of kstrdup'ing it?

Sure. As discussed over irc, will do away with the alloc.

>
> Cheers,
> --
> Luís
>
> > +     return r;
> >  }
> >
> >  static int ceph_parse_mount_param(struct fs_context *fc,
> > @@ -322,6 +408,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)
> > @@ -473,6 +561,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);
> >  }
> >
> > @@ -516,6 +605,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);
> >  }
> >
> > @@ -571,9 +664,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");
> >
> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > index 839e6b0239ee..557348ff3203 100644
> > --- a/fs/ceph/super.h
> > +++ b/fs/ceph/super.h
> > @@ -88,6 +88,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()
> > @@ -97,6 +99,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
> >
>


-- 
Cheers,
Venky

Re: [PATCH 1/4] ceph: new device mount syntax

[Jeff Layton]

On Tue, 2021-06-29 at 19:24 +0530, Venky Shankar wrote:
> On Tue, Jun 29, 2021 at 5:02 PM Luis Henriques <lhenriques@suse.de> wrote:
> > 
> > [ As I said, I didn't fully reviewed this patch.  Just sending out a few
> >   comments. ]
> > 
> > On Mon, Jun 28, 2021 at 01:25:42PM +0530, Venky Shankar wrote:
> > > 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 | 117 +++++++++++++++++++++++++++++++++++++++++++-----
> > >  fs/ceph/super.h |   3 ++
> > >  2 files changed, 110 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > index 9b1b7f4cfdd4..950a28ad9c59 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),
> > >       {}
> > > @@ -226,10 +228,68 @@ static void canonicalize_path(char *path)
> > >       path[j] = '\0';
> > >  }
> > > 
> > > +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);
> > > +     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)
> > > +{
> > > +     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 != '=')
> > > +                return invalfc(fc, "separator '=' missing in source");
> > 
> > An annoying thing is that we'll always see this error message when falling
> > back to the old_source method.
> > 

True. I'd rather this fallback be silent.

> > (Also, is there a good reason for using '=' instead of ':'?  I probably
> > missed this discussion somewhere else already...)
> > 

Yes, we needed a way for the kernel to distinguish between the old and
new syntax. Old kernels already reject any mount string without ":/" in
it, so we needed the new format to _not_ have that to ensure a clean
fallback procedure.

It's not as pretty as I would have liked, but it's starting to grow on
me. :)

> > > +
> > > +     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");
> > > +
> > > +     ++fs_name_start; /* start of file system name */
> > > +     fsopt->mds_namespace = kstrndup(fs_name_start,
> > > +                                     dev_name_end - fs_name_start, 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.  Distinguish the server list from the path.
> > > + * 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.
> > >   *
> > > - * The source will look like:
> > > + * 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)
> > > + *
> > > + * Old device syntax is:
> > >   *     <server_spec>[,<server_spec>...]:[<path>]
> > >   * where
> > >   *     <server_spec> is <ip>[:<port>]
> > > @@ -262,22 +322,48 @@ 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);
> > > +     dout("trying new device syntax");
> > > +     ret = ceph_parse_new_source(dev_name, dev_name_end, fc);
> > > +     if (ret == 0)
> > > +             goto done;
> > > +
> > > +     dout("trying old device syntax");
> > > +     ret = ceph_parse_old_source(dev_name, dev_name_end, fc);
> > >       if (ret)
> > > -             return ret;
> > > +             goto out;
> > > 
> > > + done:
> > >       fc->source = param->string;
> > >       param->string = NULL;
> > > -     return 0;
> > > + out:
> > > +     return ret;
> > > +}
> > > +
> > > +static int ceph_parse_mon_addr(struct fs_parameter *param,
> > > +                            struct fs_context *fc)
> > > +{
> > > +     int r;
> > > +     struct ceph_parse_opts_ctx *pctx = fc->fs_private;
> > > +     struct ceph_mount_options *fsopt = pctx->opts;
> > > +
> > > +     kfree(fsopt->mon_addr);
> > > +     fsopt->mon_addr = kstrdup(param->string, GFP_KERNEL);
> > > +     if (!fsopt->mon_addr)
> > > +             return -ENOMEM;
> > > +
> > > +     strreplace(param->string, '/', ',');
> > > +     r = ceph_parse_mon_ips(param->string, strlen(param->string),
> > > +                            pctx->copts, fc->log.log);
> > > +     param->string = NULL;
> > 
> > I think this will result in a memory leak.  Why don't we simply set
> > fsopt->mon_addr to param->string instead of kstrdup'ing it?
> 
> Sure. As discussed over irc, will do away with the alloc.
> 
> > 
> > Cheers,
> > --
> > Luís
> > 
> > > +     return r;
> > >  }
> > > 
> > >  static int ceph_parse_mount_param(struct fs_context *fc,
> > > @@ -322,6 +408,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)
> > > @@ -473,6 +561,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);
> > >  }
> > > 
> > > @@ -516,6 +605,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);
> > >  }
> > > 
> > > @@ -571,9 +664,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");
> > > 
> > > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > > index 839e6b0239ee..557348ff3203 100644
> > > --- a/fs/ceph/super.h
> > > +++ b/fs/ceph/super.h
> > > @@ -88,6 +88,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()
> > > @@ -97,6 +99,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
> > > 
> > 
> 
> 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/4] ceph: new device mount syntax

[Luis Henriques]

On Tue, Jun 29, 2021 at 11:10:02AM -0400, Jeff Layton wrote:
> On Tue, 2021-06-29 at 19:24 +0530, Venky Shankar wrote:
> > On Tue, Jun 29, 2021 at 5:02 PM Luis Henriques <lhenriques@suse.de> wrote:
> > > 
> > > [ As I said, I didn't fully reviewed this patch.  Just sending out a few
> > >   comments. ]
> > > 
> > > On Mon, Jun 28, 2021 at 01:25:42PM +0530, Venky Shankar wrote:
> > > > 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 | 117 +++++++++++++++++++++++++++++++++++++++++++-----
> > > >  fs/ceph/super.h |   3 ++
> > > >  2 files changed, 110 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > > index 9b1b7f4cfdd4..950a28ad9c59 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),
> > > >       {}
> > > > @@ -226,10 +228,68 @@ static void canonicalize_path(char *path)
> > > >       path[j] = '\0';
> > > >  }
> > > > 
> > > > +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);
> > > > +     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)
> > > > +{
> > > > +     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 != '=')
> > > > +                return invalfc(fc, "separator '=' missing in source");
> > > 
> > > An annoying thing is that we'll always see this error message when falling
> > > back to the old_source method.
> > > 
> 
> True. I'd rather this fallback be silent.
> 
> > > (Also, is there a good reason for using '=' instead of ':'?  I probably
> > > missed this discussion somewhere else already...)
> > > 
> 
> Yes, we needed a way for the kernel to distinguish between the old and
> new syntax. Old kernels already reject any mount string without ":/" in
> it, so we needed the new format to _not_ have that to ensure a clean
> fallback procedure.
> 
> It's not as pretty as I would have liked, but it's starting to grow on
> me. :)

Heh.  And gets even worst with using '/' for separating the mons IPs.  (I
understand why it can't be ',' and there's probably a reason for not using
';' either.)  But yeah, that ship has sailed, I'm sure you guys discussed
all this already ;-)

Cheers,
--
Luís