Re: [PATCH v4 4/5] ceph: record updated mon_addr on remount

[Jeff Layton <jlayton@redhat.com>]

On Wed, 2021-07-14 at 17:35 +0100, Luis Henriques wrote:
> On Wed, Jul 14, 2021 at 12:17:33PM -0400, Jeff Layton wrote:
> > On Wed, 2021-07-14 at 15:35 +0530, Venky Shankar wrote:
> > > Note that the new monitors are just shown in /proc/mounts.
> > > Ceph does not (re)connect to new monitors yet.
> > > 
> > > Signed-off-by: Venky Shankar <vshankar@redhat.com>
> > > ---
> > >  fs/ceph/super.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > index d8c6168b7fcd..d3a5a3729c5b 100644
> > > --- a/fs/ceph/super.c
> > > +++ b/fs/ceph/super.c
> > > @@ -1268,6 +1268,13 @@ static int ceph_reconfigure_fc(struct fs_context *fc)
> > >  	else
> > >  		ceph_clear_mount_opt(fsc, ASYNC_DIROPS);
> > >  
> > > +	if (strcmp(fsc->mount_options->mon_addr, fsopt->mon_addr)) {
> > > +		kfree(fsc->mount_options->mon_addr);
> > > +		fsc->mount_options->mon_addr = fsopt->mon_addr;
> > > +		fsopt->mon_addr = NULL;
> > > +		printk(KERN_NOTICE "ceph: monitor addresses recorded, but not used for reconnection");
> > 
> > It's currently more in-vogue to use pr_notice() for this. I'll plan to
> > make that (minor) change before I merge. No need to resend.
> 
> Yeah, this was the only comment I had too.  I saw some issues in the
> previous revision but the changes to ceph_parse_source() seem to fix it in
> this revision.
> 
> The other annoying thing I found isn't related with this patchset but with
> a change that's been done some time ago by Xiubo (added to CC): it looks
> like that if we have an invalid parameter (for example, wrong secret)
> we'll always get -EHOSTUNREACH.
> 
> See below a possible fix (although I'm not entirely sure that's the correct
> one).
> 
> Cheers,
> --
> Luís
> 
> From a988d24d8e72fc4933459f3dd5d303cbc9a566ed Mon Sep 17 00:00:00 2001
> From: Luis Henriques <lhenriques@suse.de>
> Date: Wed, 14 Jul 2021 16:56:36 +0100
> Subject: [PATCH] ceph: don't hide error code if we don't have mdsmap
> 
> Since commit 97820058fb28 ("ceph: check availability of mds cluster on mount
> after wait timeout") we're returning -EHOSTUNREACH, even if the error isn't
> related with the MDSs availability.  For example, we'll get it even if we're
> trying to mounting a filesystem with an invalid username or secret.
> 
> Only return this error if we get -EIO.
> 
> Fixes: 97820058fb28 ("ceph: check availability of mds cluster on mount after wait timeout")
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
>  fs/ceph/super.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 086a1ceec9d8..67d70059ce9f 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -1230,7 +1230,8 @@ static int ceph_get_tree(struct fs_context *fc)
>  	return 0;
>  
>  out_splat:
> -	if (!ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
> +	if ((err == -EIO) &&
> +	    !ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
>  		pr_info("No mds server is up or the cluster is laggy\n");
>  		err = -EHOSTUNREACH;
>  	}

Yeah, I've noticed that message pop up under all sorts of circumstances
and it is an annoyance. I'm happy to consider such a patch if you send
it separately.

That said, I'm honestly not sure this message is really helpful, and
overriding errors like this at a high level seems sort of sketchy. Maybe
we should just drop that message, or figure out a way to limit it to
_just_ that situation.

--
Jeff Layton <jlayton@redhat.com>