Re: [PATCH] ceph: try to reconnect to the export targets

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2021-08-12 at 12:10 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> In case the export MDS is crashed just after the EImportStart journal
> is flushed, so when a standby MDS takes over it and when replaying
> the EImportStart journal the MDS will wait the client to reconnect,
> but the client may never register/open the sessions yet.
> 
> We will try to reconnect that MDSes if they're in the export targets
> and in RECONNECT state.
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/mds_client.c | 58 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 14e44de05812..7dfe7a804320 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -4182,13 +4182,24 @@ static void check_new_map(struct ceph_mds_client *mdsc,
>  			  struct ceph_mdsmap *newmap,
>  			  struct ceph_mdsmap *oldmap)
>  {
> -	int i;
> +	int i, err;
> +	int *export_targets;
>  	int oldstate, newstate;
>  	struct ceph_mds_session *s;
> +	struct ceph_mds_info *m_info;
>  
>  	dout("check_new_map new %u old %u\n",
>  	     newmap->m_epoch, oldmap->m_epoch);
>  
> +	m_info = newmap->m_info;
> +	export_targets = kcalloc(newmap->possible_max_rank, sizeof(int), GFP_NOFS);
> +	if (export_targets && m_info) {
> +		for (i = 0; i < m_info->num_export_targets; i++) {
> +			BUG_ON(m_info->export_targets[i] >= newmap->possible_max_rank);

In general, we shouldn't BUG() in response to bad info sent by the MDS.
It would probably be better to check these values in
ceph_mdsmap_decode() and return an error there if it doesn't look right.
That way we can just toss out the new map instead of crashing.

> +			export_targets[m_info->export_targets[i]] = 1;
> +		}
> +	}
> +
>  	for (i = 0; i < oldmap->possible_max_rank && i < mdsc->max_sessions; i++) {
>  		if (!mdsc->sessions[i])
>  			continue;
> @@ -4242,6 +4253,8 @@ static void check_new_map(struct ceph_mds_client *mdsc,
>  		if (s->s_state == CEPH_MDS_SESSION_RESTARTING &&
>  		    newstate >= CEPH_MDS_STATE_RECONNECT) {
>  			mutex_unlock(&mdsc->mutex);
> +			if (export_targets)
> +				export_targets[i] = 0;
>  			send_mds_reconnect(mdsc, s);
>  			mutex_lock(&mdsc->mutex);
>  		}
> @@ -4264,6 +4277,47 @@ static void check_new_map(struct ceph_mds_client *mdsc,
>  		}
>  	}
>  
> +	for (i = 0; i < newmap->possible_max_rank; i++) {

The condition on this loop is slightly different from the one below it,
and I'm not sure why. Should this also be checking this?

    i < newmap->possible_max_rank && i < mdsc->max_sessions

...do we need to look at export targets where i >= mdsc->max_sessions ?

> +		if (!export_targets)
> +			break;
> +
> +		/*
> +		 * Only open and reconnect sessions that don't
> +		 * exist yet.
> +		 */
> +		if (!export_targets[i] || __have_session(mdsc, i))
> +			continue;
> +
> +		/*
> +		 * In case the export MDS is crashed just after
> +		 * the EImportStart journal is flushed, so when
> +		 * a standby MDS takes over it and is replaying
> +		 * the EImportStart journal the new MDS daemon
> +		 * will wait the client to reconnect it, but the
> +		 * client may never register/open the sessions
> +		 * yet.
> +		 *
> +		 * It will try to reconnect that MDS daemons if
> +		 * the MDSes are in the export targets and is the
> +		 * RECONNECT state.
> +		 */
> +		newstate = ceph_mdsmap_get_state(newmap, i);
> +		if (newstate != CEPH_MDS_STATE_RECONNECT)
> +			continue;
> +		s = __open_export_target_session(mdsc, i);
> +		if (IS_ERR(s)) {
> +			err = PTR_ERR(s);
> +			pr_err("failed to open export target session, err %d\n",
> +			       err);
> +			continue;
> +		}
> +		dout("send reconnect to target mds.%d\n", i);
> +		mutex_unlock(&mdsc->mutex);
> +		send_mds_reconnect(mdsc, s);
> +		mutex_lock(&mdsc->mutex);
> +		ceph_put_mds_session(s);

Suppose we end up in this part of the code, and we have to drop the
mdsc->mutex like this. What ensures that an earlier session in the array
won't end up going back into CEPH_MDS_STATE_RECONNECT before we can get
into the loop below? This looks racy.

> +	}
> +
>  	for (i = 0; i < newmap->possible_max_rank && i < mdsc->max_sessions; i++) {
>  		s = mdsc->sessions[i];
>  		if (!s)
> @@ -4278,6 +4332,8 @@ static void check_new_map(struct ceph_mds_client *mdsc,
>  			__open_export_target_sessions(mdsc, s);
>  		}
>  	}
> +
> +	kfree(export_targets);
>  }
>  
>  

-- 
Jeff Layton <jlayton@kernel.org>