Whamcloud - gitweb
LU-8408 mgc: handle config_llog_data::cld_refcount properly 16/21616/7
authorFan Yong <fan.yong@intel.com>
Fri, 24 Jun 2016 04:04:01 +0000 (12:04 +0800)
committerOleg Drokin <oleg.drokin@intel.com>
Fri, 2 Sep 2016 02:21:27 +0000 (02:21 +0000)
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 <fan.yong@intel.com>
Change-Id: I9fb6c3b7ae23dcea147aca7ffec240e0f33ef746
Reviewed-on: http://review.whamcloud.com/21616
Tested-by: Jenkins
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: Alex Zhuravlev <alexey.zhuravlev@intel.com>
Reviewed-by: Hongchao Zhang <hongchao.zhang@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
lustre/mgc/mgc_request.c

index 6b51d52..1c98fa7 100644 (file)
@@ -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;
         }