* [PATCH] ceph: add 'fs' mount option support
@ 2020-02-23 2:14 xiubli
2020-02-23 14:56 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: xiubli @ 2020-02-23 2:14 UTC (permalink / raw)
To: jlayton, idryomov; +Cc: sage, zyan, pdonnell, ceph-devel, Xiubo Li
From: Xiubo Li <xiubli@redhat.com>
The 'fs' here will be cleaner when specifying the ceph fs name,
and we can easily get the corresponding name from the `ceph fs
dump`:
[...]
Filesystem 'a' (1)
fs_name a
epoch 12
flags 12
[...]
The 'fs' here just an alias name for 'mds_namespace' mount options,
and we will keep 'mds_namespace' for backwards compatibility.
URL: https://tracker.ceph.com/issues/44214
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
fs/ceph/mds_client.c | 8 ++++----
fs/ceph/super.c | 21 +++++++++++----------
fs/ceph/super.h | 2 +-
3 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 3e792eca6af7..82f63ef2694c 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -4590,7 +4590,7 @@ void ceph_mdsc_destroy(struct ceph_fs_client *fsc)
void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
{
struct ceph_fs_client *fsc = mdsc->fsc;
- const char *mds_namespace = fsc->mount_options->mds_namespace;
+ const char *fs_name = fsc->mount_options->fs_name;
void *p = msg->front.iov_base;
void *end = p + msg->front.iov_len;
u32 epoch;
@@ -4634,9 +4634,9 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
namelen = ceph_decode_32(&info_p);
ceph_decode_need(&info_p, info_end, namelen, bad);
- if (mds_namespace &&
- strlen(mds_namespace) == namelen &&
- !strncmp(mds_namespace, (char *)info_p, namelen)) {
+ if (fs_name &&
+ strlen(fs_name) == namelen &&
+ !strncmp(fs_name, (char *)info_p, namelen)) {
mount_fscid = fscid;
break;
}
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index c7f150686a53..31acb4fe1f2c 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -140,7 +140,7 @@ enum {
Opt_congestion_kb,
/* int args above */
Opt_snapdirname,
- Opt_mds_namespace,
+ Opt_fs,
Opt_recover_session,
Opt_source,
/* string args above */
@@ -181,7 +181,8 @@ static const struct fs_parameter_spec ceph_mount_parameters[] = {
fsparam_flag_no ("fsc", Opt_fscache), // fsc|nofsc
fsparam_string ("fsc", Opt_fscache), // fsc=...
fsparam_flag_no ("ino32", Opt_ino32),
- fsparam_string ("mds_namespace", Opt_mds_namespace),
+ fsparam_string ("mds_namespace", Opt_fs), // backwards compatibility
+ fsparam_string ("fs", Opt_fs), // new alias for mds_namespace
fsparam_flag_no ("poolperm", Opt_poolperm),
fsparam_flag_no ("quotadf", Opt_quotadf),
fsparam_u32 ("rasize", Opt_rasize),
@@ -300,9 +301,9 @@ static int ceph_parse_mount_param(struct fs_context *fc,
fsopt->snapdir_name = param->string;
param->string = NULL;
break;
- case Opt_mds_namespace:
- kfree(fsopt->mds_namespace);
- fsopt->mds_namespace = param->string;
+ case Opt_fs:
+ kfree(fsopt->fs_name);
+ fsopt->fs_name = param->string;
param->string = NULL;
break;
case Opt_recover_session:
@@ -460,7 +461,7 @@ static void destroy_mount_options(struct ceph_mount_options *args)
return;
kfree(args->snapdir_name);
- kfree(args->mds_namespace);
+ kfree(args->fs_name);
kfree(args->server_path);
kfree(args->fscache_uniq);
kfree(args);
@@ -494,7 +495,7 @@ static int compare_mount_options(struct ceph_mount_options *new_fsopt,
if (ret)
return ret;
- ret = strcmp_null(fsopt1->mds_namespace, fsopt2->mds_namespace);
+ ret = strcmp_null(fsopt1->fs_name, fsopt2->fs_name);
if (ret)
return ret;
@@ -561,8 +562,8 @@ 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)
- seq_show_option(m, "mds_namespace", fsopt->mds_namespace);
+ if (fsopt->fs_name)
+ seq_show_option(m, "fs", fsopt->fs_name);
if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
seq_show_option(m, "recover_session", "clean");
@@ -643,7 +644,7 @@ static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
fsc->client->extra_mon_dispatch = extra_mon_dispatch;
ceph_set_opt(fsc->client, ABORT_ON_FULL);
- if (!fsopt->mds_namespace) {
+ if (!fsopt->fs_name) {
ceph_monc_want_map(&fsc->client->monc, CEPH_SUB_MDSMAP,
0, true);
} else {
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 4b269dc845bb..fc4c125b42fb 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -90,7 +90,7 @@ struct ceph_mount_options {
*/
char *snapdir_name; /* default ".snap" */
- char *mds_namespace; /* default NULL */
+ char *fs_name; /* default NULL */
char *server_path; /* default NULL (means "/") */
char *fscache_uniq; /* default NULL */
};
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: add 'fs' mount option support
2020-02-23 2:14 [PATCH] ceph: add 'fs' mount option support xiubli
@ 2020-02-23 14:56 ` Jeff Layton
2020-02-24 2:41 ` Xiubo Li
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2020-02-23 14:56 UTC (permalink / raw)
To: xiubli, idryomov; +Cc: sage, zyan, pdonnell, ceph-devel
On Sat, 2020-02-22 at 21:14 -0500, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
>
> The 'fs' here will be cleaner when specifying the ceph fs name,
> and we can easily get the corresponding name from the `ceph fs
> dump`:
>
> [...]
> Filesystem 'a' (1)
> fs_name a
> epoch 12
> flags 12
> [...]
>
> The 'fs' here just an alias name for 'mds_namespace' mount options,
> and we will keep 'mds_namespace' for backwards compatibility.
>
> URL: https://tracker.ceph.com/issues/44214
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
It looks like mds_namespace feature went into the kernel in 2016 (in
v4.7). We're at v5.5 today, so that's a large swath of kernels in the
field that only support the old option.
While I agree that 'fs=' would have been cleaner and more user-friendly,
I've found that it's just not worth it to add mount option aliases like
this unless you have a really good reason. It all ends up being a huge
amount of churn for little benefit.
The problem with changing it after the fact like this is that you still
have to support both options forever. Removing support isn't worth the
pain as you can break working environments. When working environments
upgrade they won't change to use the new option (why bother?)
Maybe it would be good to start this change by doing a "fs=" to
"mds_namespace=" translation in the mount helper? That would make the
new option work across older kernel releases too, and make it simpler to
document what options are supported.
> ---
> fs/ceph/mds_client.c | 8 ++++----
> fs/ceph/super.c | 21 +++++++++++----------
> fs/ceph/super.h | 2 +-
> 3 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 3e792eca6af7..82f63ef2694c 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -4590,7 +4590,7 @@ void ceph_mdsc_destroy(struct ceph_fs_client *fsc)
> void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
> {
> struct ceph_fs_client *fsc = mdsc->fsc;
> - const char *mds_namespace = fsc->mount_options->mds_namespace;
> + const char *fs_name = fsc->mount_options->fs_name;
> void *p = msg->front.iov_base;
> void *end = p + msg->front.iov_len;
> u32 epoch;
> @@ -4634,9 +4634,9 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
> namelen = ceph_decode_32(&info_p);
> ceph_decode_need(&info_p, info_end, namelen, bad);
>
> - if (mds_namespace &&
> - strlen(mds_namespace) == namelen &&
> - !strncmp(mds_namespace, (char *)info_p, namelen)) {
> + if (fs_name &&
> + strlen(fs_name) == namelen &&
> + !strncmp(fs_name, (char *)info_p, namelen)) {
> mount_fscid = fscid;
> break;
> }
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index c7f150686a53..31acb4fe1f2c 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -140,7 +140,7 @@ enum {
> Opt_congestion_kb,
> /* int args above */
> Opt_snapdirname,
> - Opt_mds_namespace,
> + Opt_fs,
> Opt_recover_session,
> Opt_source,
> /* string args above */
> @@ -181,7 +181,8 @@ static const struct fs_parameter_spec ceph_mount_parameters[] = {
> fsparam_flag_no ("fsc", Opt_fscache), // fsc|nofsc
> fsparam_string ("fsc", Opt_fscache), // fsc=...
> fsparam_flag_no ("ino32", Opt_ino32),
> - fsparam_string ("mds_namespace", Opt_mds_namespace),
> + fsparam_string ("mds_namespace", Opt_fs), // backwards compatibility
> + fsparam_string ("fs", Opt_fs), // new alias for mds_namespace
> fsparam_flag_no ("poolperm", Opt_poolperm),
> fsparam_flag_no ("quotadf", Opt_quotadf),
> fsparam_u32 ("rasize", Opt_rasize),
> @@ -300,9 +301,9 @@ static int ceph_parse_mount_param(struct fs_context *fc,
> fsopt->snapdir_name = param->string;
> param->string = NULL;
> break;
> - case Opt_mds_namespace:
> - kfree(fsopt->mds_namespace);
> - fsopt->mds_namespace = param->string;
> + case Opt_fs:
> + kfree(fsopt->fs_name);
> + fsopt->fs_name = param->string;
> param->string = NULL;
> break;
> case Opt_recover_session:
> @@ -460,7 +461,7 @@ static void destroy_mount_options(struct ceph_mount_options *args)
> return;
>
> kfree(args->snapdir_name);
> - kfree(args->mds_namespace);
> + kfree(args->fs_name);
> kfree(args->server_path);
> kfree(args->fscache_uniq);
> kfree(args);
> @@ -494,7 +495,7 @@ static int compare_mount_options(struct ceph_mount_options *new_fsopt,
> if (ret)
> return ret;
>
> - ret = strcmp_null(fsopt1->mds_namespace, fsopt2->mds_namespace);
> + ret = strcmp_null(fsopt1->fs_name, fsopt2->fs_name);
> if (ret)
> return ret;
>
> @@ -561,8 +562,8 @@ 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)
> - seq_show_option(m, "mds_namespace", fsopt->mds_namespace);
> + if (fsopt->fs_name)
> + seq_show_option(m, "fs", fsopt->fs_name);
Someone will mount with mds_namespace= but then that will be converted
to fs= when displaying options here. It's not necessarily a problem but
it may be noticed by some users.
>
> if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
> seq_show_option(m, "recover_session", "clean");
> @@ -643,7 +644,7 @@ static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
> fsc->client->extra_mon_dispatch = extra_mon_dispatch;
> ceph_set_opt(fsc->client, ABORT_ON_FULL);
>
> - if (!fsopt->mds_namespace) {
> + if (!fsopt->fs_name) {
> ceph_monc_want_map(&fsc->client->monc, CEPH_SUB_MDSMAP,
> 0, true);
> } else {
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 4b269dc845bb..fc4c125b42fb 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -90,7 +90,7 @@ struct ceph_mount_options {
> */
>
> char *snapdir_name; /* default ".snap" */
> - char *mds_namespace; /* default NULL */
> + char *fs_name; /* default NULL */
> char *server_path; /* default NULL (means "/") */
> char *fscache_uniq; /* default NULL */
> };
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: add 'fs' mount option support
2020-02-23 14:56 ` Jeff Layton
@ 2020-02-24 2:41 ` Xiubo Li
2020-02-24 22:20 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Xiubo Li @ 2020-02-24 2:41 UTC (permalink / raw)
To: Jeff Layton, idryomov; +Cc: sage, zyan, pdonnell, ceph-devel
On 2020/2/23 22:56, Jeff Layton wrote:
> On Sat, 2020-02-22 at 21:14 -0500, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The 'fs' here will be cleaner when specifying the ceph fs name,
>> and we can easily get the corresponding name from the `ceph fs
>> dump`:
>>
>> [...]
>> Filesystem 'a' (1)
>> fs_name a
>> epoch 12
>> flags 12
>> [...]
>>
>> The 'fs' here just an alias name for 'mds_namespace' mount options,
>> and we will keep 'mds_namespace' for backwards compatibility.
>>
>> URL: https://tracker.ceph.com/issues/44214
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> It looks like mds_namespace feature went into the kernel in 2016 (in
> v4.7). We're at v5.5 today, so that's a large swath of kernels in the
> field that only support the old option.
>
> While I agree that 'fs=' would have been cleaner and more user-friendly,
> I've found that it's just not worth it to add mount option aliases like
> this unless you have a really good reason. It all ends up being a huge
> amount of churn for little benefit.
>
> The problem with changing it after the fact like this is that you still
> have to support both options forever. Removing support isn't worth the
> pain as you can break working environments. When working environments
> upgrade they won't change to use the new option (why bother?)
>
> Maybe it would be good to start this change by doing a "fs=" to
> "mds_namespace=" translation in the mount helper? That would make the
> new option work across older kernel releases too, and make it simpler to
> document what options are supported.
This sounds a pretty good idea for me.
>> @@ -561,8 +562,8 @@ 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)
>> - seq_show_option(m, "mds_namespace", fsopt->mds_namespace);
>> + if (fsopt->fs_name)
>> + seq_show_option(m, "fs", fsopt->fs_name);
> Someone will mount with mds_namespace= but then that will be converted
> to fs= when displaying options here. It's not necessarily a problem but
> it may be noticed by some users.
Yeah, but if we convert 'fs=' to 'mds_namespace=' in userland and here
it will always showing with 'mds_namespace=', won't it be the same issue?
Or should we covert it to "fs/mds_namespace=" here ? Will it make sense ?
Thanks
BRs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: add 'fs' mount option support
2020-02-24 2:41 ` Xiubo Li
@ 2020-02-24 22:20 ` Jeff Layton
2020-02-24 22:24 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2020-02-24 22:20 UTC (permalink / raw)
To: Xiubo Li, idryomov; +Cc: sage, zyan, pdonnell, ceph-devel
On Mon, 2020-02-24 at 10:41 +0800, Xiubo Li wrote:
> On 2020/2/23 22:56, Jeff Layton wrote:
> > On Sat, 2020-02-22 at 21:14 -0500, xiubli@redhat.com wrote:
> > > From: Xiubo Li <xiubli@redhat.com>
> > >
> > > The 'fs' here will be cleaner when specifying the ceph fs name,
> > > and we can easily get the corresponding name from the `ceph fs
> > > dump`:
> > >
> > > [...]
> > > Filesystem 'a' (1)
> > > fs_name a
> > > epoch 12
> > > flags 12
> > > [...]
> > >
> > > The 'fs' here just an alias name for 'mds_namespace' mount options,
> > > and we will keep 'mds_namespace' for backwards compatibility.
> > >
> > > URL: https://tracker.ceph.com/issues/44214
> > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > It looks like mds_namespace feature went into the kernel in 2016 (in
> > v4.7). We're at v5.5 today, so that's a large swath of kernels in the
> > field that only support the old option.
> >
> > While I agree that 'fs=' would have been cleaner and more user-friendly,
> > I've found that it's just not worth it to add mount option aliases like
> > this unless you have a really good reason. It all ends up being a huge
> > amount of churn for little benefit.
> >
> > The problem with changing it after the fact like this is that you still
> > have to support both options forever. Removing support isn't worth the
> > pain as you can break working environments. When working environments
> > upgrade they won't change to use the new option (why bother?)
> >
> > Maybe it would be good to start this change by doing a "fs=" to
> > "mds_namespace=" translation in the mount helper? That would make the
> > new option work across older kernel releases too, and make it simpler to
> > document what options are supported.
>
> This sounds a pretty good idea for me.
>
>
> > > @@ -561,8 +562,8 @@ 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)
> > > - seq_show_option(m, "mds_namespace", fsopt->mds_namespace);
> > > + if (fsopt->fs_name)
> > > + seq_show_option(m, "fs", fsopt->fs_name);
> > Someone will mount with mds_namespace= but then that will be converted
> > to fs= when displaying options here. It's not necessarily a problem but
> > it may be noticed by some users.
>
> Yeah, but if we convert 'fs=' to 'mds_namespace=' in userland and here
> it will always showing with 'mds_namespace=', won't it be the same issue?
>
> Or should we covert it to "fs/mds_namespace=" here ? Will it make sense ?
>
Now that I think about it more, it's probably not a problem either way,
but we should probably convert it to read 'fs=' here since that's what
we're planning to encourage people to use long-term.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: add 'fs' mount option support
2020-02-24 22:20 ` Jeff Layton
@ 2020-02-24 22:24 ` Jeff Layton
2020-02-25 4:02 ` Xiubo Li
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2020-02-24 22:24 UTC (permalink / raw)
To: Xiubo Li, idryomov; +Cc: sage, zyan, pdonnell, ceph-devel
On Mon, 2020-02-24 at 14:20 -0800, Jeff Layton wrote:
> On Mon, 2020-02-24 at 10:41 +0800, Xiubo Li wrote:
> > On 2020/2/23 22:56, Jeff Layton wrote:
> > > On Sat, 2020-02-22 at 21:14 -0500, xiubli@redhat.com wrote:
> > > > From: Xiubo Li <xiubli@redhat.com>
> > > >
> > > > The 'fs' here will be cleaner when specifying the ceph fs name,
> > > > and we can easily get the corresponding name from the `ceph fs
> > > > dump`:
> > > >
> > > > [...]
> > > > Filesystem 'a' (1)
> > > > fs_name a
> > > > epoch 12
> > > > flags 12
> > > > [...]
> > > >
> > > > The 'fs' here just an alias name for 'mds_namespace' mount options,
> > > > and we will keep 'mds_namespace' for backwards compatibility.
> > > >
> > > > URL: https://tracker.ceph.com/issues/44214
> > > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > It looks like mds_namespace feature went into the kernel in 2016 (in
> > > v4.7). We're at v5.5 today, so that's a large swath of kernels in the
> > > field that only support the old option.
> > >
> > > While I agree that 'fs=' would have been cleaner and more user-friendly,
> > > I've found that it's just not worth it to add mount option aliases like
> > > this unless you have a really good reason. It all ends up being a huge
> > > amount of churn for little benefit.
> > >
> > > The problem with changing it after the fact like this is that you still
> > > have to support both options forever. Removing support isn't worth the
> > > pain as you can break working environments. When working environments
> > > upgrade they won't change to use the new option (why bother?)
> > >
> > > Maybe it would be good to start this change by doing a "fs=" to
> > > "mds_namespace=" translation in the mount helper? That would make the
> > > new option work across older kernel releases too, and make it simpler to
> > > document what options are supported.
> >
> > This sounds a pretty good idea for me.
> >
> >
> > > > @@ -561,8 +562,8 @@ 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)
> > > > - seq_show_option(m, "mds_namespace", fsopt->mds_namespace);
> > > > + if (fsopt->fs_name)
> > > > + seq_show_option(m, "fs", fsopt->fs_name);
> > > Someone will mount with mds_namespace= but then that will be converted
> > > to fs= when displaying options here. It's not necessarily a problem but
> > > it may be noticed by some users.
> >
> > Yeah, but if we convert 'fs=' to 'mds_namespace=' in userland and here
> > it will always showing with 'mds_namespace=', won't it be the same issue?
> >
> > Or should we covert it to "fs/mds_namespace=" here ? Will it make sense ?
> >
>
> Now that I think about it more, it's probably not a problem either way,
> but we should probably convert it to read 'fs=' here since that's what
> we're planning to encourage people to use long-term.
>
To be even more clear:
We should probably take your initial patch (or something like it) into
the kernel as well, but also change the mount helper to smooth over the
difference in the option handling there. Once your userland changes
are merged, let me know and I'll plan to re-review and merge this.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: add 'fs' mount option support
2020-02-24 22:24 ` Jeff Layton
@ 2020-02-25 4:02 ` Xiubo Li
0 siblings, 0 replies; 6+ messages in thread
From: Xiubo Li @ 2020-02-25 4:02 UTC (permalink / raw)
To: Jeff Layton, idryomov; +Cc: sage, zyan, pdonnell, ceph-devel
On 2020/2/25 6:24, Jeff Layton wrote:
> On Mon, 2020-02-24 at 14:20 -0800, Jeff Layton wrote:
>> On Mon, 2020-02-24 at 10:41 +0800, Xiubo Li wrote:
>>> On 2020/2/23 22:56, Jeff Layton wrote:
>>>> On Sat, 2020-02-22 at 21:14 -0500, xiubli@redhat.com wrote:
>>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>>
>>>>> The 'fs' here will be cleaner when specifying the ceph fs name,
>>>>> and we can easily get the corresponding name from the `ceph fs
>>>>> dump`:
>>>>>
>>>>> [...]
>>>>> Filesystem 'a' (1)
>>>>> fs_name a
>>>>> epoch 12
>>>>> flags 12
>>>>> [...]
>>>>>
>>>>> The 'fs' here just an alias name for 'mds_namespace' mount options,
>>>>> and we will keep 'mds_namespace' for backwards compatibility.
>>>>>
>>>>> URL: https://tracker.ceph.com/issues/44214
>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>> It looks like mds_namespace feature went into the kernel in 2016 (in
>>>> v4.7). We're at v5.5 today, so that's a large swath of kernels in the
>>>> field that only support the old option.
>>>>
>>>> While I agree that 'fs=' would have been cleaner and more user-friendly,
>>>> I've found that it's just not worth it to add mount option aliases like
>>>> this unless you have a really good reason. It all ends up being a huge
>>>> amount of churn for little benefit.
>>>>
>>>> The problem with changing it after the fact like this is that you still
>>>> have to support both options forever. Removing support isn't worth the
>>>> pain as you can break working environments. When working environments
>>>> upgrade they won't change to use the new option (why bother?)
>>>>
>>>> Maybe it would be good to start this change by doing a "fs=" to
>>>> "mds_namespace=" translation in the mount helper? That would make the
>>>> new option work across older kernel releases too, and make it simpler to
>>>> document what options are supported.
>>> This sounds a pretty good idea for me.
>>>
>>>
>>>>> @@ -561,8 +562,8 @@ 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)
>>>>> - seq_show_option(m, "mds_namespace", fsopt->mds_namespace);
>>>>> + if (fsopt->fs_name)
>>>>> + seq_show_option(m, "fs", fsopt->fs_name);
>>>> Someone will mount with mds_namespace= but then that will be converted
>>>> to fs= when displaying options here. It's not necessarily a problem but
>>>> it may be noticed by some users.
>>> Yeah, but if we convert 'fs=' to 'mds_namespace=' in userland and here
>>> it will always showing with 'mds_namespace=', won't it be the same issue?
>>>
>>> Or should we covert it to "fs/mds_namespace=" here ? Will it make sense ?
>>>
>> Now that I think about it more, it's probably not a problem either way,
>> but we should probably convert it to read 'fs=' here since that's what
>> we're planning to encourage people to use long-term.
>>
> To be even more clear:
>
> We should probably take your initial patch (or something like it) into
> the kernel as well, but also change the mount helper to smooth over the
> difference in the option handling there. Once your userland changes
> are merged, let me know and I'll plan to re-review and merge this.
>
Sure.
Thanks.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-25 4:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-23 2:14 [PATCH] ceph: add 'fs' mount option support xiubli
2020-02-23 14:56 ` Jeff Layton
2020-02-24 2:41 ` Xiubo Li
2020-02-24 22:20 ` Jeff Layton
2020-02-24 22:24 ` Jeff Layton
2020-02-25 4:02 ` Xiubo Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.