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@xxxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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;
> > > > }
>
