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

[Luis Henriques <lhenriques@suse.de>]

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