| Message ID | TYCP286MB206604655F7CAB7C50C6218FC0709@TYCP286MB2066.JPNP286.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: account for name and fsid in new device spec | expand |
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; > }
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; > > } >
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; >>> }
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; > > > > } >
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 --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; }