diff mbox series

[2/3] ceph: save name and fsid in mount source

Message ID TYCP286MB206604655F7CAB7C50C6218FC0709@TYCP286MB2066.JPNP286.PROD.OUTLOOK.COM (mailing list archive)
State New, archived
Headers show
Series ceph: account for name and fsid in new device spec | expand

Commit Message

胡玮文 May 7, 2023, 5:55 p.m. UTC
From: Hu Weiwen <sehuww@mail.scut.edu.cn>

We have name and fsid in the new device syntax.  It is confusing that
the kernel accept these info but do not take them into account when
connecting to the cluster.

Although the mount.ceph helper program will extract the name from device
spec and pass it as name options, these changes are still useful if we
don't have that program installed, or if we want to call `mount()'
directly.

Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
---
 fs/ceph/super.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Xiubo Li May 9, 2023, 1:36 a.m. UTC | #1
On 5/8/23 01:55, Hu Weiwen wrote:
> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
>
> We have name and fsid in the new device syntax.  It is confusing that
> the kernel accept these info but do not take them into account when
> connecting to the cluster.
>
> Although the mount.ceph helper program will extract the name from device
> spec and pass it as name options, these changes are still useful if we
> don't have that program installed, or if we want to call `mount()'
> directly.
>
> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> ---
>   fs/ceph/super.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 4e1f4031e888..74636b9383b8 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -267,6 +267,7 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>   	struct ceph_fsid fsid;
>   	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
>   	struct ceph_mount_options *fsopt = pctx->opts;
> +	struct ceph_options *copts = pctx->copts;
>   	char *fsid_start, *fs_name_start;
>   
>   	if (*dev_name_end != '=') {
> @@ -285,6 +286,12 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>   
>   	if (ceph_parse_fsid(fsid_start, &fsid))
>   		return invalfc(fc, "Invalid FSID");
> +	if (!(copts->flags & CEPH_OPT_FSID)) {
> +		copts->fsid = fsid;
> +		copts->flags |= CEPH_OPT_FSID;
> +	} else if (ceph_fsid_compare(&fsid, &copts->fsid)) {
> +		return invalfc(fc, "Mismatching cluster FSID");
> +	}
>   
>   	++fs_name_start; /* start of file system name */
>   	len = dev_name_end - fs_name_start;
> @@ -298,6 +305,16 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>   	}
>   	dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
>   
> +	len = fsid_start - dev_name - 1;
> +	if (!copts->name) {
> +		copts->name = kstrndup(dev_name, len, GFP_KERNEL);
> +		if (!copts->name)
> +			return -ENOMEM;

Shouldn't we kfree the 'copts->mds_namespace' here ?


> +	} else if (!strstrn_equals(copts->name, dev_name, len)) {
> +		return invalfc(fc, "Mismatching cephx name");
> +	}
> +	dout("cephx name '%s'\n", copts->name);
> +
>   	fsopt->new_dev_syntax = true;
>   	return 0;
>   }
胡玮文 May 9, 2023, 1:55 p.m. UTC | #2
On Tue, May 09, 2023 at 09:36:16AM +0800, Xiubo Li wrote:
> 
> On 5/8/23 01:55, Hu Weiwen wrote:
> > From: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > 
> > We have name and fsid in the new device syntax.  It is confusing that
> > the kernel accept these info but do not take them into account when
> > connecting to the cluster.
> > 
> > Although the mount.ceph helper program will extract the name from device
> > spec and pass it as name options, these changes are still useful if we
> > don't have that program installed, or if we want to call `mount()'
> > directly.
> > 
> > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > ---
> >   fs/ceph/super.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> > 
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 4e1f4031e888..74636b9383b8 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -267,6 +267,7 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
> >   	struct ceph_fsid fsid;
> >   	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
> >   	struct ceph_mount_options *fsopt = pctx->opts;
> > +	struct ceph_options *copts = pctx->copts;
> >   	char *fsid_start, *fs_name_start;
> >   	if (*dev_name_end != '=') {
> > @@ -285,6 +286,12 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
> >   	if (ceph_parse_fsid(fsid_start, &fsid))
> >   		return invalfc(fc, "Invalid FSID");
> > +	if (!(copts->flags & CEPH_OPT_FSID)) {
> > +		copts->fsid = fsid;
> > +		copts->flags |= CEPH_OPT_FSID;
> > +	} else if (ceph_fsid_compare(&fsid, &copts->fsid)) {
> > +		return invalfc(fc, "Mismatching cluster FSID");
> > +	}
> >   	++fs_name_start; /* start of file system name */
> >   	len = dev_name_end - fs_name_start;
> > @@ -298,6 +305,16 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
> >   	}
> >   	dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
> > +	len = fsid_start - dev_name - 1;
> > +	if (!copts->name) {
> > +		copts->name = kstrndup(dev_name, len, GFP_KERNEL);
> > +		if (!copts->name)
> > +			return -ENOMEM;
> 
> Shouldn't we kfree the 'copts->mds_namespace' here ?

Seems not necessary.  ceph_free_fc() will take care of releasing the
whole 'struct ceph_parse_opts_ctx', including 'copts->mds_namespace'.
Besides, the mds_namespace may already be set before we parse the source
here.

ceph_free_fc() is called from:
put_fs_context
do_new_mount
do_mount

> > +	} else if (!strstrn_equals(copts->name, dev_name, len)) {
> > +		return invalfc(fc, "Mismatching cephx name");
> > +	}
> > +	dout("cephx name '%s'\n", copts->name);
> > +
> >   	fsopt->new_dev_syntax = true;
> >   	return 0;
> >   }
>
Xiubo Li May 10, 2023, 12:42 a.m. UTC | #3
On 5/9/23 21:55, 胡玮文 wrote:
> On Tue, May 09, 2023 at 09:36:16AM +0800, Xiubo Li wrote:
>> On 5/8/23 01:55, Hu Weiwen wrote:
>>> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
>>>
>>> We have name and fsid in the new device syntax.  It is confusing that
>>> the kernel accept these info but do not take them into account when
>>> connecting to the cluster.
>>>
>>> Although the mount.ceph helper program will extract the name from device
>>> spec and pass it as name options, these changes are still useful if we
>>> don't have that program installed, or if we want to call `mount()'
>>> directly.
>>>
>>> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
>>> ---
>>>    fs/ceph/super.c | 17 +++++++++++++++++
>>>    1 file changed, 17 insertions(+)
>>>
>>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>>> index 4e1f4031e888..74636b9383b8 100644
>>> --- a/fs/ceph/super.c
>>> +++ b/fs/ceph/super.c
>>> @@ -267,6 +267,7 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>>>    	struct ceph_fsid fsid;
>>>    	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
>>>    	struct ceph_mount_options *fsopt = pctx->opts;
>>> +	struct ceph_options *copts = pctx->copts;
>>>    	char *fsid_start, *fs_name_start;
>>>    	if (*dev_name_end != '=') {
>>> @@ -285,6 +286,12 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>>>    	if (ceph_parse_fsid(fsid_start, &fsid))
>>>    		return invalfc(fc, "Invalid FSID");
>>> +	if (!(copts->flags & CEPH_OPT_FSID)) {
>>> +		copts->fsid = fsid;
>>> +		copts->flags |= CEPH_OPT_FSID;
>>> +	} else if (ceph_fsid_compare(&fsid, &copts->fsid)) {
>>> +		return invalfc(fc, "Mismatching cluster FSID");
>>> +	}
>>>    	++fs_name_start; /* start of file system name */
>>>    	len = dev_name_end - fs_name_start;
>>> @@ -298,6 +305,16 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>>>    	}
>>>    	dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
>>> +	len = fsid_start - dev_name - 1;
>>> +	if (!copts->name) {
>>> +		copts->name = kstrndup(dev_name, len, GFP_KERNEL);
>>> +		if (!copts->name)
>>> +			return -ENOMEM;
>> Shouldn't we kfree the 'copts->mds_namespace' here ?
> Seems not necessary.  ceph_free_fc() will take care of releasing the
> whole 'struct ceph_parse_opts_ctx', including 'copts->mds_namespace'.
> Besides, the mds_namespace may already be set before we parse the source
> here.
>
> ceph_free_fc() is called from:
> put_fs_context

457 void put_fs_context(struct fs_context *fc)
458 {
459         struct super_block *sb;
460
461         if (fc->root) {
462                 sb = fc->root->d_sb;
463                 dput(fc->root);
464                 fc->root = NULL;
465                 deactivate_super(sb);
466         }
467
468         if (fc->need_free && fc->ops && fc->ops->free)
469                 fc->ops->free(fc);

But are u sure the 'fc->need_free' is correctly set ?

It seems not from my reading if I didn't miss something.

Thanks

- Xiubo

> do_new_mount
> do_mount
>
>>> +	} else if (!strstrn_equals(copts->name, dev_name, len)) {
>>> +		return invalfc(fc, "Mismatching cephx name");
>>> +	}
>>> +	dout("cephx name '%s'\n", copts->name);
>>> +
>>>    	fsopt->new_dev_syntax = true;
>>>    	return 0;
>>>    }
胡玮文 May 10, 2023, 1:50 a.m. UTC | #4
On Wed, May 10, 2023 at 08:42:10AM +0800, Xiubo Li wrote:
> 
> On 5/9/23 21:55, 胡玮文 wrote:
> > On Tue, May 09, 2023 at 09:36:16AM +0800, Xiubo Li wrote:
> > > On 5/8/23 01:55, Hu Weiwen wrote:
> > > > From: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > > > 
> > > > We have name and fsid in the new device syntax.  It is confusing that
> > > > the kernel accept these info but do not take them into account when
> > > > connecting to the cluster.
> > > > 
> > > > Although the mount.ceph helper program will extract the name from device
> > > > spec and pass it as name options, these changes are still useful if we
> > > > don't have that program installed, or if we want to call `mount()'
> > > > directly.
> > > > 
> > > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > > > ---
> > > >    fs/ceph/super.c | 17 +++++++++++++++++
> > > >    1 file changed, 17 insertions(+)
> > > > 
> > > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > > index 4e1f4031e888..74636b9383b8 100644
> > > > --- a/fs/ceph/super.c
> > > > +++ b/fs/ceph/super.c
> > > > @@ -267,6 +267,7 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
> > > >    	struct ceph_fsid fsid;
> > > >    	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
> > > >    	struct ceph_mount_options *fsopt = pctx->opts;
> > > > +	struct ceph_options *copts = pctx->copts;
> > > >    	char *fsid_start, *fs_name_start;
> > > >    	if (*dev_name_end != '=') {
> > > > @@ -285,6 +286,12 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
> > > >    	if (ceph_parse_fsid(fsid_start, &fsid))
> > > >    		return invalfc(fc, "Invalid FSID");
> > > > +	if (!(copts->flags & CEPH_OPT_FSID)) {
> > > > +		copts->fsid = fsid;
> > > > +		copts->flags |= CEPH_OPT_FSID;
> > > > +	} else if (ceph_fsid_compare(&fsid, &copts->fsid)) {
> > > > +		return invalfc(fc, "Mismatching cluster FSID");
> > > > +	}
> > > >    	++fs_name_start; /* start of file system name */
> > > >    	len = dev_name_end - fs_name_start;
> > > > @@ -298,6 +305,16 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
> > > >    	}
> > > >    	dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
> > > > +	len = fsid_start - dev_name - 1;
> > > > +	if (!copts->name) {
> > > > +		copts->name = kstrndup(dev_name, len, GFP_KERNEL);
> > > > +		if (!copts->name)
> > > > +			return -ENOMEM;
> > > Shouldn't we kfree the 'copts->mds_namespace' here ?
> > Seems not necessary.  ceph_free_fc() will take care of releasing the
> > whole 'struct ceph_parse_opts_ctx', including 'copts->mds_namespace'.
> > Besides, the mds_namespace may already be set before we parse the source
> > here.
> > 
> > ceph_free_fc() is called from:
> > put_fs_context
> 
> 457 void put_fs_context(struct fs_context *fc)
> 458 {
> 459         struct super_block *sb;
> 460
> 461         if (fc->root) {
> 462                 sb = fc->root->d_sb;
> 463                 dput(fc->root);
> 464                 fc->root = NULL;
> 465                 deactivate_super(sb);
> 466         }
> 467
> 468         if (fc->need_free && fc->ops && fc->ops->free)
> 469                 fc->ops->free(fc);
> 
> But are u sure the 'fc->need_free' is correctly set ?
> 
> It seems not from my reading if I didn't miss something.

'fc->need_free' is initialized to true just after init_fs_context() is
called, see 'alloc_fs_context()'.  And it is only reset to false after
calling free().

I've verified with gdb that ceph_free_fc() got called if
ceph_parse_new_source() returns an error.

Anyway, if ceph_free_fc() is not invoked, then we are leaking a lot of
things, not just mds_namespace.  The whole fs_context need to be freed
unconditionally after the mount is finished.

> Thanks
> 
> - Xiubo
> 
> > do_new_mount
> > do_mount
> > 
> > > > +	} else if (!strstrn_equals(copts->name, dev_name, len)) {
> > > > +		return invalfc(fc, "Mismatching cephx name");
> > > > +	}
> > > > +	dout("cephx name '%s'\n", copts->name);
> > > > +
> > > >    	fsopt->new_dev_syntax = true;
> > > >    	return 0;
> > > >    }
>
Xiubo Li May 10, 2023, 2 a.m. UTC | #5
On 5/10/23 09:50, 胡玮文 wrote:
> On Wed, May 10, 2023 at 08:42:10AM +0800, Xiubo Li wrote:
>> On 5/9/23 21:55, 胡玮文 wrote:
>>> On Tue, May 09, 2023 at 09:36:16AM +0800, Xiubo Li wrote:
>>>> On 5/8/23 01:55, Hu Weiwen wrote:
>>>>> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
>>>>>
>>>>> We have name and fsid in the new device syntax.  It is confusing that
>>>>> the kernel accept these info but do not take them into account when
>>>>> connecting to the cluster.
>>>>>
>>>>> Although the mount.ceph helper program will extract the name from device
>>>>> spec and pass it as name options, these changes are still useful if we
>>>>> don't have that program installed, or if we want to call `mount()'
>>>>> directly.
>>>>>
>>>>> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
>>>>> ---
>>>>>     fs/ceph/super.c | 17 +++++++++++++++++
>>>>>     1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>>>>> index 4e1f4031e888..74636b9383b8 100644
>>>>> --- a/fs/ceph/super.c
>>>>> +++ b/fs/ceph/super.c
>>>>> @@ -267,6 +267,7 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>>>>>     	struct ceph_fsid fsid;
>>>>>     	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
>>>>>     	struct ceph_mount_options *fsopt = pctx->opts;
>>>>> +	struct ceph_options *copts = pctx->copts;
>>>>>     	char *fsid_start, *fs_name_start;
>>>>>     	if (*dev_name_end != '=') {
>>>>> @@ -285,6 +286,12 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>>>>>     	if (ceph_parse_fsid(fsid_start, &fsid))
>>>>>     		return invalfc(fc, "Invalid FSID");
>>>>> +	if (!(copts->flags & CEPH_OPT_FSID)) {
>>>>> +		copts->fsid = fsid;
>>>>> +		copts->flags |= CEPH_OPT_FSID;
>>>>> +	} else if (ceph_fsid_compare(&fsid, &copts->fsid)) {
>>>>> +		return invalfc(fc, "Mismatching cluster FSID");
>>>>> +	}
>>>>>     	++fs_name_start; /* start of file system name */
>>>>>     	len = dev_name_end - fs_name_start;
>>>>> @@ -298,6 +305,16 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>>>>>     	}
>>>>>     	dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
>>>>> +	len = fsid_start - dev_name - 1;
>>>>> +	if (!copts->name) {
>>>>> +		copts->name = kstrndup(dev_name, len, GFP_KERNEL);
>>>>> +		if (!copts->name)
>>>>> +			return -ENOMEM;
>>>> Shouldn't we kfree the 'copts->mds_namespace' here ?
>>> Seems not necessary.  ceph_free_fc() will take care of releasing the
>>> whole 'struct ceph_parse_opts_ctx', including 'copts->mds_namespace'.
>>> Besides, the mds_namespace may already be set before we parse the source
>>> here.
>>>
>>> ceph_free_fc() is called from:
>>> put_fs_context
>> 457 void put_fs_context(struct fs_context *fc)
>> 458 {
>> 459         struct super_block *sb;
>> 460
>> 461         if (fc->root) {
>> 462                 sb = fc->root->d_sb;
>> 463                 dput(fc->root);
>> 464                 fc->root = NULL;
>> 465                 deactivate_super(sb);
>> 466         }
>> 467
>> 468         if (fc->need_free && fc->ops && fc->ops->free)
>> 469                 fc->ops->free(fc);
>>
>> But are u sure the 'fc->need_free' is correctly set ?
>>
>> It seems not from my reading if I didn't miss something.
> 'fc->need_free' is initialized to true just after init_fs_context() is
> called, see 'alloc_fs_context()'.  And it is only reset to false after
> calling free().
>
> I've verified with gdb that ceph_free_fc() got called if
> ceph_parse_new_source() returns an error.
>
> Anyway, if ceph_free_fc() is not invoked, then we are leaking a lot of
> things, not just mds_namespace.  The whole fs_context need to be freed
> unconditionally after the mount is finished.

Okay, you are right. I just missed that.

Thanks


>> Thanks
>>
>> - Xiubo
>>
>>> do_new_mount
>>> do_mount
>>>
>>>>> +	} else if (!strstrn_equals(copts->name, dev_name, len)) {
>>>>> +		return invalfc(fc, "Mismatching cephx name");
>>>>> +	}
>>>>> +	dout("cephx name '%s'\n", copts->name);
>>>>> +
>>>>>     	fsopt->new_dev_syntax = true;
>>>>>     	return 0;
>>>>>     }
diff mbox series

Patch

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 4e1f4031e888..74636b9383b8 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -267,6 +267,7 @@  static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
 	struct ceph_fsid fsid;
 	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
 	struct ceph_mount_options *fsopt = pctx->opts;
+	struct ceph_options *copts = pctx->copts;
 	char *fsid_start, *fs_name_start;
 
 	if (*dev_name_end != '=') {
@@ -285,6 +286,12 @@  static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
 
 	if (ceph_parse_fsid(fsid_start, &fsid))
 		return invalfc(fc, "Invalid FSID");
+	if (!(copts->flags & CEPH_OPT_FSID)) {
+		copts->fsid = fsid;
+		copts->flags |= CEPH_OPT_FSID;
+	} else if (ceph_fsid_compare(&fsid, &copts->fsid)) {
+		return invalfc(fc, "Mismatching cluster FSID");
+	}
 
 	++fs_name_start; /* start of file system name */
 	len = dev_name_end - fs_name_start;
@@ -298,6 +305,16 @@  static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
 	}
 	dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
 
+	len = fsid_start - dev_name - 1;
+	if (!copts->name) {
+		copts->name = kstrndup(dev_name, len, GFP_KERNEL);
+		if (!copts->name)
+			return -ENOMEM;
+	} else if (!strstrn_equals(copts->name, dev_name, len)) {
+		return invalfc(fc, "Mismatching cephx name");
+	}
+	dout("cephx name '%s'\n", copts->name);
+
 	fsopt->new_dev_syntax = true;
 	return 0;
 }