From 48d24ebd6d51873a6c560000ea3b638fdae22a27 Mon Sep 17 00:00:00 2001 From: Fan Yong Date: Fri, 24 Jun 2016 12:04:01 +0800 Subject: [PATCH] LU-8408 mgc: handle config_llog_data::cld_refcount properly Originally, the logic of handling config_llog_data::cld_refcount is some confusing, it may cause the cld_refcount to be leaked or trigger "LASSERT(atomic_read(&cld->cld_refcount) > 0);" when put the reference. This patch clean related logic as following: 1) When the 'cld' is created, its reference is set as 1. 2) No need additional reference when add the 'cld' into the list 'config_llog_list'. 3) Inrease 'cld_refcount' when set lock data after mgc_enqueue() done successfully by mgc_process_log(). 4) When mgc_requeue_thread() traversals the 'config_llog_list', it needs to take additional reference on each 'cld' to avoid being freed during subsequent processing. The reference also prevents the 'cld' to be dropped from the 'config_llog_list', then the mgc_requeue_thread() can safely locate next 'cld', and then decrease the 'cld_refcount' for previous one. 5) mgc_blocking_ast() will drop the reference of 'cld_refcount' that is taken in mgc_process_log(). 6) The others need to call config_log_find() to find the 'cld' if want to access related config log data. That will increase the 'cld_refcount' to avoid being freed during accessing. The sponsor needs to call config_log_put() after using the 'cld'. 7) Other confused or redundant logic are dropped. On the other hand, the patch also enhances the protection for 'config_llog_data' flags, such as 'cld_stopping'/'cld_lostlock' as following. a) Use 'config_list_lock' (spinlock) to handle the possible parallel accessing of these flags among mgc_requeue_thread() and others config llog data visitors, such as mount/umount, blocking_ast, and so on. b) Use 'config_llog_data::cld_lock' (mutex) to pretect other parallel accessing of these flags among kinds of blockable operations, such as mount, umount, and blocking ast. The 'config_llog_data::cld_lock' is also used for protecting the sub-cld members, such as 'cld_sptlrpc'/'cld_params', and so on. Signed-off-by: Fan Yong Change-Id: I9fb6c3b7ae23dcea147aca7ffec240e0f33ef746 Reviewed-on: http://review.whamcloud.com/21616 Tested-by: Jenkins Tested-by: Maloo Reviewed-by: Alex Zhuravlev Reviewed-by: Hongchao Zhang Reviewed-by: Oleg Drokin --- lustre/mgc/mgc_request.c | 230 +++++++++++++++++++++++------------------------ 1 file changed, 115 insertions(+), 115 deletions(-) diff --git a/lustre/mgc/mgc_request.c b/lustre/mgc/mgc_request.c index 6b51d52..1c98fa7 100644 --- a/lustre/mgc/mgc_request.c +++ b/lustre/mgc/mgc_request.c @@ -151,12 +151,12 @@ static void config_log_put(struct config_llog_data *cld) if (cld->cld_recover) config_log_put(cld->cld_recover); - if (cld->cld_sptlrpc) - config_log_put(cld->cld_sptlrpc); if (cld->cld_params) config_log_put(cld->cld_params); if (cld->cld_nodemap) config_log_put(cld->cld_nodemap); + if (cld->cld_sptlrpc) + config_log_put(cld->cld_sptlrpc); if (cld_is_sptlrpc(cld)) sptlrpc_conf_log_stop(cld->cld_logname); @@ -182,20 +182,17 @@ struct config_llog_data *config_log_find(char *logname, instance = cfg ? cfg->cfg_instance : NULL; spin_lock(&config_list_lock); list_for_each_entry(cld, &config_llog_list, cld_list_chain) { - /* check if instance equals */ - if (instance != cld->cld_cfg.cfg_instance) - continue; + /* check if instance equals */ + if (instance != cld->cld_cfg.cfg_instance) + continue; /* instance may be NULL, should check name */ if (strcmp(logname, cld->cld_logname) == 0) { found = cld; + config_log_get(found); break; } } - if (found) { - atomic_inc(&found->cld_refcount); - LASSERT(found->cld_stopping == 0 || cld_is_sptlrpc(found) == 0); - } spin_unlock(&config_list_lock); RETURN(found); } @@ -219,6 +216,12 @@ struct config_llog_data *do_config_log_add(struct obd_device *obd, if (!cld) RETURN(ERR_PTR(-ENOMEM)); + rc = mgc_logname2resid(logname, &cld->cld_resid, type); + if (rc) { + OBD_FREE(cld, sizeof(*cld) + strlen(cld->cld_logname) + 1); + RETURN(ERR_PTR(rc)); + } + strcpy(cld->cld_logname, logname); if (cfg) cld->cld_cfg = *cfg; @@ -239,17 +242,10 @@ struct config_llog_data *do_config_log_add(struct obd_device *obd, cld->cld_cfg.cfg_obdname = obd->obd_name; } - rc = mgc_logname2resid(logname, &cld->cld_resid, type); - spin_lock(&config_list_lock); list_add(&cld->cld_list_chain, &config_llog_list); spin_unlock(&config_list_lock); - if (rc) { - config_log_put(cld); - RETURN(ERR_PTR(rc)); - } - if (cld_is_sptlrpc(cld) || cld_is_nodemap(cld)) { rc = mgc_process_log(obd, cld); if (rc && rc != -ENOENT) @@ -313,18 +309,19 @@ static struct config_llog_data *config_params_log_add(struct obd_device *obd, * We have one active log per "mount" - client instance or servername. * Each instance may be at a different point in the log. */ -static int config_log_add(struct obd_device *obd, char *logname, - struct config_llog_instance *cfg, - struct super_block *sb) +static struct config_llog_data * +config_log_add(struct obd_device *obd, char *logname, + struct config_llog_instance *cfg, struct super_block *sb) { - struct lustre_sb_info *lsi = s2lsi(sb); + struct lustre_sb_info *lsi = s2lsi(sb); struct config_llog_data *cld; struct config_llog_data *sptlrpc_cld; struct config_llog_data *params_cld; struct config_llog_data *nodemap_cld; - char seclogname[32]; - char *ptr; - int rc; + char seclogname[32]; + char *ptr; + int rc; + bool locked = false; ENTRY; CDEBUG(D_MGC, "adding config log %s:%p\n", logname, cfg->cfg_instance); @@ -336,7 +333,7 @@ static int config_log_add(struct obd_device *obd, char *logname, ptr = strrchr(logname, '-'); if (ptr == NULL || ptr - logname > 8) { CERROR("logname %s is too long\n", logname); - RETURN(-EINVAL); + RETURN(ERR_PTR(-EINVAL)); } memcpy(seclogname, logname, ptr - logname); @@ -381,27 +378,33 @@ static int config_log_add(struct obd_device *obd, char *logname, LASSERT(lsi->lsi_lmd); if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR)) { struct config_llog_data *recover_cld; + ptr = strrchr(seclogname, '-'); if (ptr != NULL) { *ptr = 0; - } - else { + } else { CERROR("%s: sptlrpc log name not correct, %s: " "rc = %d\n", obd->obd_name, seclogname, -EINVAL); - config_log_put(cld); - RETURN(-EINVAL); + GOTO(out_cld, rc = -EINVAL); } + recover_cld = config_recover_log_add(obd, seclogname, cfg, sb); if (IS_ERR(recover_cld)) GOTO(out_cld, rc = PTR_ERR(recover_cld)); + + mutex_lock(&cld->cld_lock); + locked = true; cld->cld_recover = recover_cld; } - cld->cld_sptlrpc = sptlrpc_cld; + if (!locked) + mutex_lock(&cld->cld_lock); cld->cld_params = params_cld; cld->cld_nodemap = nodemap_cld; + cld->cld_sptlrpc = sptlrpc_cld; + mutex_unlock(&cld->cld_lock); - RETURN(0); + RETURN(cld); out_cld: config_log_put(cld); @@ -416,11 +419,20 @@ out_sptlrpc: config_log_put(sptlrpc_cld); out: - return rc; + return ERR_PTR(rc); } DEFINE_MUTEX(llog_process_lock); +static inline void config_mark_cld_stop(struct config_llog_data *cld) +{ + mutex_lock(&cld->cld_lock); + spin_lock(&config_list_lock); + cld->cld_stopping = 1; + spin_unlock(&config_list_lock); + mutex_unlock(&cld->cld_lock); +} + /** Stop watching for updates on this log. */ static int config_log_end(char *logname, struct config_llog_instance *cfg) @@ -453,35 +465,27 @@ static int config_log_end(char *logname, struct config_llog_instance *cfg) RETURN(rc); } + spin_lock(&config_list_lock); cld->cld_stopping = 1; + spin_unlock(&config_list_lock); cld_recover = cld->cld_recover; cld->cld_recover = NULL; - mutex_unlock(&cld->cld_lock); - - if (cld_recover) { - mutex_lock(&cld_recover->cld_lock); - cld_recover->cld_stopping = 1; - mutex_unlock(&cld_recover->cld_lock); - config_log_put(cld_recover); - } - - spin_lock(&config_list_lock); - cld_sptlrpc = cld->cld_sptlrpc; - cld->cld_sptlrpc = NULL; cld_params = cld->cld_params; cld->cld_params = NULL; cld_nodemap = cld->cld_nodemap; cld->cld_nodemap = NULL; - spin_unlock(&config_list_lock); + cld_sptlrpc = cld->cld_sptlrpc; + cld->cld_sptlrpc = NULL; + mutex_unlock(&cld->cld_lock); - if (cld_sptlrpc) - config_log_put(cld_sptlrpc); + if (cld_recover) { + config_mark_cld_stop(cld_recover); + config_log_put(cld_recover); + } if (cld_params) { - mutex_lock(&cld_params->cld_lock); - cld_params->cld_stopping = 1; - mutex_unlock(&cld_params->cld_lock); + config_mark_cld_stop(cld_params); config_log_put(cld_params); } @@ -489,6 +493,9 @@ static int config_log_end(char *logname, struct config_llog_instance *cfg) if (cld_nodemap) config_log_put(cld_nodemap); + if (cld_sptlrpc) + config_log_put(cld_sptlrpc); + /* drop the ref from the find */ config_log_put(cld); /* drop the start ref */ @@ -589,11 +596,10 @@ static int mgc_requeue_thread(void *data) /* Keep trying failed locks periodically */ spin_lock(&config_list_lock); rq_state |= RQ_RUNNING; - while (1) { + while (!(rq_state & RQ_STOP)) { struct l_wait_info lwi; struct config_llog_data *cld, *cld_prev; int rand = cfs_rand() & MGC_TIMEOUT_RAND_CENTISEC; - int stopped = !!(rq_state & RQ_STOP); int to; /* Any new or requeued lostlocks will change the state */ @@ -628,43 +634,39 @@ static int mgc_requeue_thread(void *data) spin_lock(&config_list_lock); rq_state &= ~RQ_PRECLEANUP; list_for_each_entry(cld, &config_llog_list, - cld_list_chain) { - if (!cld->cld_lostlock) + cld_list_chain) { + if (!cld->cld_lostlock || cld->cld_stopping) continue; + /* hold reference to avoid being freed during + * subsequent processing. */ + config_log_get(cld); + cld->cld_lostlock = 0; spin_unlock(&config_list_lock); - LASSERT(atomic_read(&cld->cld_refcount) > 0); - - /* Whether we enqueued again or not in mgc_process_log, - * we're done with the ref from the old enqueue */ if (cld_prev) config_log_put(cld_prev); cld_prev = cld; - cld->cld_lostlock = 0; - if (likely(!stopped)) + if (likely(!(rq_state & RQ_STOP))) { do_requeue(cld); - - spin_lock(&config_list_lock); + spin_lock(&config_list_lock); + } else { + spin_lock(&config_list_lock); + break; + } } spin_unlock(&config_list_lock); if (cld_prev) config_log_put(cld_prev); - /* break after scanning the list so that we can drop - * refcount to losing lock clds */ - if (unlikely(stopped)) { - spin_lock(&config_list_lock); - break; - } - /* Wait a bit to see if anyone else needs a requeue */ lwi = (struct l_wait_info) { 0 }; l_wait_event(rq_waitq, rq_state & (RQ_NOW | RQ_STOP), &lwi); spin_lock(&config_list_lock); } + /* spinlock and while guarantee RQ_NOW and RQ_LATER are not set */ rq_state &= ~RQ_RUNNING; spin_unlock(&config_list_lock); @@ -679,6 +681,7 @@ static int mgc_requeue_thread(void *data) We are responsible for dropping the config log reference from here on out. */ static void mgc_requeue_add(struct config_llog_data *cld) { + bool wakeup = false; ENTRY; CDEBUG(D_INFO, "log %s: requeue (r=%d sp=%d st=%x)\n", @@ -687,26 +690,17 @@ static void mgc_requeue_add(struct config_llog_data *cld) LASSERT(atomic_read(&cld->cld_refcount) > 0); mutex_lock(&cld->cld_lock); - if (cld->cld_stopping || cld->cld_lostlock) { - mutex_unlock(&cld->cld_lock); - RETURN_EXIT; - } - /* this refcount will be released in mgc_requeue_thread. */ - config_log_get(cld); - cld->cld_lostlock = 1; - mutex_unlock(&cld->cld_lock); - - /* Hold lock for rq_state */ spin_lock(&config_list_lock); - if (rq_state & RQ_STOP) { - spin_unlock(&config_list_lock); - cld->cld_lostlock = 0; - config_log_put(cld); - } else { + if (!(rq_state & RQ_STOP) && !cld->cld_stopping && !cld->cld_lostlock) { + cld->cld_lostlock = 1; rq_state |= RQ_NOW; - spin_unlock(&config_list_lock); - wake_up(&rq_waitq); + wakeup = true; } + spin_unlock(&config_list_lock); + mutex_unlock(&cld->cld_lock); + if (wakeup) + wake_up(&rq_waitq); + EXIT; } @@ -1007,6 +1001,8 @@ static int mgc_blocking_ast(struct ldlm_lock *lock, struct ldlm_lock_desc *desc, /* held at mgc_process_log(). */ LASSERT(atomic_read(&cld->cld_refcount) > 0); + + lock->l_ast_data = NULL; /* Are we done with this log? */ if (cld->cld_stopping) { CDEBUG(D_MGC, "log %s: stopping, won't requeue\n", @@ -2052,18 +2048,19 @@ restart: goto restart; } else { mutex_lock(&cld->cld_lock); + spin_lock(&config_list_lock); cld->cld_lostlock = 1; + spin_unlock(&config_list_lock); } } else { /* mark cld_lostlock so that it will requeue * after MGC becomes available. */ + spin_lock(&config_list_lock); cld->cld_lostlock = 1; + spin_unlock(&config_list_lock); } - /* Get extra reference, it will be put in requeue thread */ - config_log_get(cld); } - if (cld_is_recover(cld) || cld_is_nodemap(cld)) { if (!rcl) rc = mgc_process_recover_nodemap_log(mgc, cld); @@ -2074,7 +2071,9 @@ restart: if (!rcl) { CERROR("%s: recover log %s failed, not fatal: rc = %d\n", mgc->obd_name, cld->cld_logname, rc); + spin_lock(&config_list_lock); cld->cld_lostlock = 1; + spin_unlock(&config_list_lock); } rc = 0; /* this is not a fatal error for recover log */ } @@ -2146,33 +2145,35 @@ static int mgc_process_config(struct obd_device *obd, size_t len, void *buf) cfg->cfg_last_idx); /* We're only called through here on the initial mount */ - rc = config_log_add(obd, logname, cfg, sb); - if (rc) - break; - cld = config_log_find(logname, cfg); - if (cld == NULL) { - rc = -ENOENT; - break; - } + cld = config_log_add(obd, logname, cfg, sb); + if (IS_ERR(cld)) { + rc = PTR_ERR(cld); + break; + } - /* COMPAT_146 */ - /* FIXME only set this for old logs! Right now this forces - us to always skip the "inside markers" check */ - cld->cld_cfg.cfg_flags |= CFG_F_COMPAT146; - - rc = mgc_process_log(obd, cld); - if (rc == 0 && cld->cld_recover != NULL) { - if (OCD_HAS_FLAG(&obd->u.cli.cl_import-> - imp_connect_data, IMP_RECOV)) { - rc = mgc_process_log(obd, cld->cld_recover); - } else { - struct config_llog_data *cir = cld->cld_recover; - cld->cld_recover = NULL; - config_log_put(cir); - } - if (rc) - CERROR("Cannot process recover llog %d\n", rc); - } + /* COMPAT_146 */ + /* FIXME only set this for old logs! Right now this forces + us to always skip the "inside markers" check */ + cld->cld_cfg.cfg_flags |= CFG_F_COMPAT146; + + rc = mgc_process_log(obd, cld); + if (rc == 0 && cld->cld_recover != NULL) { + if (OCD_HAS_FLAG(&obd->u.cli.cl_import-> + imp_connect_data, IMP_RECOV)) { + rc = mgc_process_log(obd, cld->cld_recover); + } else { + struct config_llog_data *cir; + + mutex_lock(&cld->cld_lock); + cir = cld->cld_recover; + cld->cld_recover = NULL; + mutex_unlock(&cld->cld_lock); + config_log_put(cir); + } + + if (rc) + CERROR("Cannot process recover llog %d\n", rc); + } if (rc == 0 && cld->cld_params != NULL) { rc = mgc_process_log(obd, cld->cld_params); @@ -2186,7 +2187,6 @@ static int mgc_process_config(struct obd_device *obd, size_t len, void *buf) CERROR("%s: can't process params llog: rc = %d\n", obd->obd_name, rc); } - config_log_put(cld); break; } -- 1.8.3.1