Whamcloud - gitweb
LU-11542 import: fix race between imp_state & imp_invalid 96/35796/3
authorYang Sheng <ys@whamcloud.com>
Mon, 15 Oct 2018 09:37:21 +0000 (17:37 +0800)
committerOleg Drokin <green@whamcloud.com>
Sat, 28 Sep 2019 06:49:36 +0000 (06:49 +0000)
We set import to LUSTRE_IMP_DISCON and then deactive when
it is unreplayable. Someone may set this import up between
those two operations. So we will get a invalid import with
FULL state.

Lustre-change: https://review.whamcloud.com/33395
Lustre-commit: 29904135df671c624b1e542fdda94b221d76e667

Signed-off-by: Yang Sheng <ys@whamcloud.com>
Change-Id: Ib4cec0bcaf6f4b221ba260edb94749a4e523f5e6
Reviewed-by: Wang Shilong <wshilong@ddn.com>
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
Signed-off-by: Minh Diep <mdiep@whamcloud.com>
Reviewed-on: https://review.whamcloud.com/35796
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: James Simmons <jsimmons@infradead.org>
lustre/include/lustre_ha.h
lustre/lov/lov_obd.c
lustre/ptlrpc/client.c
lustre/ptlrpc/import.c
lustre/ptlrpc/pinger.c
lustre/ptlrpc/ptlrpc_internal.h
lustre/ptlrpc/recover.c

index 7c22d98..2cb4969 100644 (file)
@@ -50,7 +50,7 @@ void ptlrpc_free_committed(struct obd_import *imp);
 void ptlrpc_wake_delayed(struct obd_import *imp);
 int ptlrpc_recover_import(struct obd_import *imp, char *new_uuid, int async);
 int ptlrpc_set_import_active(struct obd_import *imp, int active);
-void ptlrpc_activate_import(struct obd_import *imp);
+void ptlrpc_activate_import(struct obd_import *imp, bool set_state_full);
 void ptlrpc_deactivate_import(struct obd_import *imp);
 void ptlrpc_invalidate_import(struct obd_import *imp);
 void ptlrpc_fail_import(struct obd_import *imp, __u32 conn_cnt);
index 2965c1d..e3e2e64 100644 (file)
@@ -146,12 +146,12 @@ int lov_connect_obd(struct obd_device *obd, u32 index, int activate,
          */
         imp = tgt_obd->u.cli.cl_import;
 
-        if (activate) {
-                tgt_obd->obd_no_recov = 0;
-                /* FIXME this is probably supposed to be
-                   ptlrpc_set_import_active.  Horrible naming. */
-                ptlrpc_activate_import(imp);
-        }
+       if (activate) {
+               tgt_obd->obd_no_recov = 0;
+               /* FIXME this is probably supposed to be
+                  ptlrpc_set_import_active.  Horrible naming. */
+               ptlrpc_activate_import(imp, false);
+       }
 
         rc = obd_register_observer(tgt_obd, obd);
         if (rc) {
index 99090e3..aaf5786 100644 (file)
@@ -3127,11 +3127,12 @@ void ptlrpc_abort_inflight(struct obd_import *imp)
        struct list_head *tmp, *n;
        ENTRY;
 
-       /* Make sure that no new requests get processed for this import.
+       /*
+        * Make sure that no new requests get processed for this import.
         * ptlrpc_{queue,set}_wait must (and does) hold imp_lock while testing
         * this flag and then putting requests on sending_list or delayed_list.
         */
-       spin_lock(&imp->imp_lock);
+       assert_spin_locked(&imp->imp_lock);
 
        /* XXX locking?  Maybe we should remove each request with the list
         * locked?  Also, how do we know if the requests on the list are
@@ -3173,8 +3174,6 @@ void ptlrpc_abort_inflight(struct obd_import *imp)
        if (imp->imp_replayable)
                ptlrpc_free_committed(imp);
 
-       spin_unlock(&imp->imp_lock);
-
        EXIT;
 }
 
index 787c4c6..72781f9 100644 (file)
@@ -148,6 +148,21 @@ void deuuidify(char *uuid, const char *prefix, char **uuid_start, int *uuid_len)
                 *uuid_len -= strlen(UUID_STR);
 }
 
+/* Must be called with imp_lock held! */
+static void ptlrpc_deactivate_import_nolock(struct obd_import *imp)
+{
+       ENTRY;
+
+       assert_spin_locked(&imp->imp_lock);
+       CDEBUG(D_HA, "setting import %s INVALID\n", obd2cli_tgt(imp->imp_obd));
+       imp->imp_invalid = 1;
+       imp->imp_generation++;
+
+       ptlrpc_abort_inflight(imp);
+
+       EXIT;
+}
+
 /**
  * Returns true if import was FULL, false if import was already not
  * connected.
@@ -158,8 +173,10 @@ void deuuidify(char *uuid, const char *prefix, char **uuid_start, int *uuid_len)
  *             bulk requests) and if one has already caused a reconnection
  *             (increasing the import->conn_cnt) the older failure should
  *             not also cause a reconnection.  If zero it forces a reconnect.
+ * @invalid - set import invalid flag
  */
-int ptlrpc_set_import_discon(struct obd_import *imp, __u32 conn_cnt)
+int ptlrpc_set_import_discon(struct obd_import *imp,
+                            __u32 conn_cnt, bool invalid)
 {
        int rc = 0;
 
@@ -169,10 +186,12 @@ int ptlrpc_set_import_discon(struct obd_import *imp, __u32 conn_cnt)
             (conn_cnt == 0 || conn_cnt == imp->imp_conn_cnt)) {
                 char *target_start;
                 int   target_len;
+               bool  inact = false;
 
                 deuuidify(obd2cli_tgt(imp->imp_obd), NULL,
                           &target_start, &target_len);
 
+               import_set_state_nolock(imp, LUSTRE_IMP_DISCON);
                 if (imp->imp_replayable) {
                         LCONSOLE_WARN("%s: Connection to %.*s (at %s) was "
                                "lost; in progress operations using this "
@@ -185,14 +204,25 @@ int ptlrpc_set_import_discon(struct obd_import *imp, __u32 conn_cnt)
                               "operations using this service will fail\n",
                               imp->imp_obd->obd_name, target_len, target_start,
                               obd_import_nid2str(imp));
+                       if (invalid) {
+                               CDEBUG(D_HA, "import %s@%s for %s not "
+                                      "replayable, auto-deactivating\n",
+                                      obd2cli_tgt(imp->imp_obd),
+                                      imp->imp_connection->c_remote_uuid.uuid,
+                                      imp->imp_obd->obd_name);
+                               ptlrpc_deactivate_import_nolock(imp);
+                               inact = true;
+                       }
                }
-               import_set_state_nolock(imp, LUSTRE_IMP_DISCON);
                spin_unlock(&imp->imp_lock);
 
                if (obd_dump_on_timeout)
                        libcfs_debug_dumplog();
 
                obd_import_event(imp->imp_obd, imp, IMP_EVENT_DISCON);
+
+               if (inact)
+                       obd_import_event(imp->imp_obd, imp, IMP_EVENT_INACTIVE);
                rc = 1;
        } else {
                spin_unlock(&imp->imp_lock);
@@ -207,23 +237,6 @@ int ptlrpc_set_import_discon(struct obd_import *imp, __u32 conn_cnt)
         return rc;
 }
 
-/* Must be called with imp_lock held! */
-static void ptlrpc_deactivate_and_unlock_import(struct obd_import *imp)
-{
-       ENTRY;
-       assert_spin_locked(&imp->imp_lock);
-
-       CDEBUG(D_HA, "setting import %s INVALID\n", obd2cli_tgt(imp->imp_obd));
-       imp->imp_invalid = 1;
-       imp->imp_generation++;
-       spin_unlock(&imp->imp_lock);
-
-       ptlrpc_abort_inflight(imp);
-       obd_import_event(imp->imp_obd, imp, IMP_EVENT_INACTIVE);
-
-       EXIT;
-}
-
 /*
  * This acts as a barrier; all existing requests are rejected, and
  * no new requests will be accepted until the import is valid again.
@@ -231,7 +244,10 @@ static void ptlrpc_deactivate_and_unlock_import(struct obd_import *imp)
 void ptlrpc_deactivate_import(struct obd_import *imp)
 {
        spin_lock(&imp->imp_lock);
-       ptlrpc_deactivate_and_unlock_import(imp);
+       ptlrpc_deactivate_import_nolock(imp);
+       spin_unlock(&imp->imp_lock);
+
+       obd_import_event(imp->imp_obd, imp, IMP_EVENT_INACTIVE);
 }
 EXPORT_SYMBOL(ptlrpc_deactivate_import);
 
@@ -402,17 +418,23 @@ void ptlrpc_invalidate_import(struct obd_import *imp)
 EXPORT_SYMBOL(ptlrpc_invalidate_import);
 
 /* unset imp_invalid */
-void ptlrpc_activate_import(struct obd_import *imp)
+void ptlrpc_activate_import(struct obd_import *imp, bool set_state_full)
 {
        struct obd_device *obd = imp->imp_obd;
 
        spin_lock(&imp->imp_lock);
        if (imp->imp_deactive != 0) {
+               LASSERT(imp->imp_state != LUSTRE_IMP_FULL);
+               if (imp->imp_state != LUSTRE_IMP_DISCON)
+                       import_set_state_nolock(imp, LUSTRE_IMP_DISCON);
                spin_unlock(&imp->imp_lock);
                return;
        }
+       if (set_state_full)
+               import_set_state_nolock(imp, LUSTRE_IMP_FULL);
 
        imp->imp_invalid = 0;
+
        spin_unlock(&imp->imp_lock);
        obd_import_event(obd, imp, IMP_EVENT_ACTIVE);
 }
@@ -434,22 +456,13 @@ EXPORT_SYMBOL(ptlrpc_pinger_force);
 
 void ptlrpc_fail_import(struct obd_import *imp, __u32 conn_cnt)
 {
-        ENTRY;
+       ENTRY;
 
-        LASSERT(!imp->imp_dlm_fake);
-
-        if (ptlrpc_set_import_discon(imp, conn_cnt)) {
-                if (!imp->imp_replayable) {
-                        CDEBUG(D_HA, "import %s@%s for %s not replayable, "
-                               "auto-deactivating\n",
-                               obd2cli_tgt(imp->imp_obd),
-                               imp->imp_connection->c_remote_uuid.uuid,
-                               imp->imp_obd->obd_name);
-                        ptlrpc_deactivate_import(imp);
-                }
+       LASSERT(!imp->imp_dlm_fake);
 
+       if (ptlrpc_set_import_discon(imp, conn_cnt, true))
                ptlrpc_pinger_force(imp);
-       }
+
        EXIT;
 }
 
@@ -472,7 +485,7 @@ int ptlrpc_reconnect_import(struct obd_import *imp)
               ptlrpc_import_state_name(imp->imp_state));
        return rc;
 #else
-       ptlrpc_set_import_discon(imp, 0);
+       ptlrpc_set_import_discon(imp, 0, false);
        /* Force a new connect attempt */
        ptlrpc_invalidate_import(imp);
        /* Do a fresh connect next time by zeroing the handle */
@@ -493,7 +506,7 @@ int ptlrpc_reconnect_import(struct obd_import *imp)
        /* Allow reconnect attempts */
        imp->imp_obd->obd_no_recov = 0;
        /* Remove 'invalid' flag */
-       ptlrpc_activate_import(imp);
+       ptlrpc_activate_import(imp, false);
        /* Attempt a new connect */
        ptlrpc_recover_import(imp, NULL, 0);
        return 0;
@@ -1135,12 +1148,10 @@ static int ptlrpc_connect_interpret(const struct lu_env *env,
                spin_lock(&imp->imp_lock);
                if (msg_flags & MSG_CONNECT_REPLAYABLE) {
                        imp->imp_replayable = 1;
-                       spin_unlock(&imp->imp_lock);
                        CDEBUG(D_HA, "connected to replayable target: %s\n",
                               obd2cli_tgt(imp->imp_obd));
                } else {
                        imp->imp_replayable = 0;
-                       spin_unlock(&imp->imp_lock);
                }
 
                 /* if applies, adjust the imp->imp_msg_magic here
@@ -1155,10 +1166,11 @@ static int ptlrpc_connect_interpret(const struct lu_env *env,
                 if (msg_flags & MSG_CONNECT_RECOVERING) {
                         CDEBUG(D_HA, "connect to %s during recovery\n",
                                obd2cli_tgt(imp->imp_obd));
-                       import_set_state(imp, LUSTRE_IMP_REPLAY_LOCKS);
+                       import_set_state_nolock(imp, LUSTRE_IMP_REPLAY_LOCKS);
+                       spin_unlock(&imp->imp_lock);
                 } else {
-                       import_set_state(imp, LUSTRE_IMP_FULL);
-                       ptlrpc_activate_import(imp);
+                       spin_unlock(&imp->imp_lock);
+                       ptlrpc_activate_import(imp, true);
                 }
 
                 GOTO(finish, rc = 0);
@@ -1292,31 +1304,33 @@ finish:
        }
 
 out:
-       spin_lock(&imp->imp_lock);
-       imp->imp_connected = 0;
-       imp->imp_connect_tried = 1;
-       spin_unlock(&imp->imp_lock);
-
        if (exp != NULL)
                class_export_put(exp);
 
-        if (rc != 0) {
-               import_set_state(imp, LUSTRE_IMP_DISCON);
-                if (rc == -EACCES) {
-                        /*
-                         * Give up trying to reconnect
-                         * EACCES means client has no permission for connection
-                         */
-                        imp->imp_obd->obd_no_recov = 1;
-                        ptlrpc_deactivate_import(imp);
-                }
+       spin_lock(&imp->imp_lock);
+       imp->imp_connected = 0;
+       imp->imp_connect_tried = 1;
 
-                if (rc == -EPROTO) {
-                        struct obd_connect_data *ocd;
+       if (rc != 0) {
+               bool inact = false;
 
-                        /* reply message might not be ready */
-                        if (request->rq_repmsg == NULL)
-                                RETURN(-EPROTO);
+               import_set_state_nolock(imp, LUSTRE_IMP_DISCON);
+               if (rc == -EACCES) {
+                       /*
+                        * Give up trying to reconnect
+                        * EACCES means client has no permission for connection
+                        */
+                       imp->imp_obd->obd_no_recov = 1;
+                       ptlrpc_deactivate_import_nolock(imp);
+                       inact = true;
+               } else if (rc == -EPROTO) {
+                       struct obd_connect_data *ocd;
+
+                       /* reply message might not be ready */
+                       if (request->rq_repmsg == NULL) {
+                               spin_unlock(&imp->imp_lock);
+                               RETURN(-EPROTO);
+                       }
 
                        ocd = req_capsule_server_get(&request->rq_pill,
                                                     &RMF_CONNECT_DATA);
@@ -1338,17 +1352,26 @@ out:
                                         OBD_OCD_VERSION_PATCH(ocd->ocd_version),
                                         OBD_OCD_VERSION_FIX(ocd->ocd_version),
                                         LUSTRE_VERSION_STRING);
-                                ptlrpc_deactivate_import(imp);
-                               import_set_state(imp, LUSTRE_IMP_CLOSED);
-                        }
-                        RETURN(-EPROTO);
-                }
+                               ptlrpc_deactivate_import_nolock(imp);
+                               import_set_state_nolock(imp, LUSTRE_IMP_CLOSED);
+                               inact = true;
+                       }
+               }
+               spin_unlock(&imp->imp_lock);
+
+               if (inact)
+                       obd_import_event(imp->imp_obd, imp, IMP_EVENT_INACTIVE);
+
+               if (rc == -EPROTO)
+                       RETURN(rc);
 
                ptlrpc_maybe_ping_import_soon(imp);
 
                CDEBUG(D_HA, "recovery of %s on %s failed (%d)\n",
                       obd2cli_tgt(imp->imp_obd),
                       (char *)imp->imp_connection->c_remote_uuid.uuid, rc);
+       } else {
+               spin_unlock(&imp->imp_lock);
        }
 
        wake_up_all(&imp->imp_recovery_waitq);
@@ -1548,20 +1571,19 @@ int ptlrpc_import_recovery_state_machine(struct obd_import *imp)
                }
        }
 
-        if (imp->imp_state == LUSTRE_IMP_RECOVER) {
+       if (imp->imp_state == LUSTRE_IMP_RECOVER) {
                struct ptlrpc_connection *conn = imp->imp_connection;
 
-                rc = ptlrpc_resend(imp);
-                if (rc)
-                        GOTO(out, rc);
-               import_set_state(imp, LUSTRE_IMP_FULL);
-                ptlrpc_activate_import(imp);
+               rc = ptlrpc_resend(imp);
+               if (rc)
+                       GOTO(out, rc);
+               ptlrpc_activate_import(imp, true);
 
                LCONSOLE_INFO("%s: Connection restored to %s (at %s)\n",
                              imp->imp_obd->obd_name,
                              obd_uuid2str(&conn->c_remote_uuid),
                              obd_import_nid2str(imp));
-        }
+       }
 
        if (imp->imp_state == LUSTRE_IMP_FULL) {
                wake_up_all(&imp->imp_recovery_waitq);
@@ -1782,11 +1804,13 @@ void ptlrpc_cleanup_imp(struct obd_import *imp)
        ENTRY;
 
        spin_lock(&imp->imp_lock);
+
        import_set_state_nolock(imp, LUSTRE_IMP_CLOSED);
        imp->imp_generation++;
-       spin_unlock(&imp->imp_lock);
        ptlrpc_abort_inflight(imp);
 
+       spin_unlock(&imp->imp_lock);
+
        EXIT;
 }
 
index ef808d8..fa5f1d3 100644 (file)
@@ -239,8 +239,6 @@ static void ptlrpc_pinger_process_import(struct obd_import *imp,
 
        imp->imp_force_next_verify = 0;
 
-       spin_unlock(&imp->imp_lock);
-
        CDEBUG(level == LUSTRE_IMP_FULL ? D_INFO : D_HA, "%s->%s: level %s/%u "
               "force %u force_next %u deactive %u pingable %u suppress %u\n",
               imp->imp_obd->obd_uuid.uuid, obd2cli_tgt(imp->imp_obd),
@@ -250,21 +248,20 @@ static void ptlrpc_pinger_process_import(struct obd_import *imp,
         if (level == LUSTRE_IMP_DISCON && !imp_is_deactive(imp)) {
                 /* wait for a while before trying recovery again */
                 imp->imp_next_ping = ptlrpc_next_reconnect(imp);
+               spin_unlock(&imp->imp_lock);
                 if (!imp->imp_no_pinger_recover)
                         ptlrpc_initiate_recovery(imp);
-        } else if (level != LUSTRE_IMP_FULL ||
-                   imp->imp_obd->obd_no_recov ||
-                   imp_is_deactive(imp)) {
+       } else if (level != LUSTRE_IMP_FULL || imp->imp_obd->obd_no_recov ||
+                  imp_is_deactive(imp)) {
                CDEBUG(D_HA, "%s->%s: not pinging (in recovery "
                       "or recovery disabled: %s)\n",
                       imp->imp_obd->obd_uuid.uuid, obd2cli_tgt(imp->imp_obd),
                       ptlrpc_import_state_name(level));
-               if (force) {
-                       spin_lock(&imp->imp_lock);
+               if (force)
                        imp->imp_force_verify = 1;
-                       spin_unlock(&imp->imp_lock);
-               }
+               spin_unlock(&imp->imp_lock);
        } else if ((imp->imp_pingable && !suppress) || force_next || force) {
+               spin_unlock(&imp->imp_lock);
                ptlrpc_ping(imp);
        }
 }
index 623e3a0..19d0487 100644 (file)
@@ -97,7 +97,8 @@ void ptlrpc_exit_portals(void);
 void ptlrpc_request_handle_notconn(struct ptlrpc_request *);
 void lustre_assert_wire_constants(void);
 int ptlrpc_import_in_recovery(struct obd_import *imp);
-int ptlrpc_set_import_discon(struct obd_import *imp, __u32 conn_cnt);
+int ptlrpc_set_import_discon(struct obd_import *imp, __u32 conn_cnt,
+                            bool invalid);
 void ptlrpc_handle_failed_import(struct obd_import *imp);
 int ptlrpc_replay_next(struct obd_import *imp, int *inflight);
 void ptlrpc_initiate_recovery(struct obd_import *imp);
index 4d5f28b..c923ab9 100644 (file)
@@ -228,30 +228,22 @@ void ptlrpc_wake_delayed(struct obd_import *imp)
 
 void ptlrpc_request_handle_notconn(struct ptlrpc_request *failed_req)
 {
-        struct obd_import *imp = failed_req->rq_import;
-        ENTRY;
+       struct obd_import *imp = failed_req->rq_import;
+       int conn = lustre_msg_get_conn_cnt(failed_req->rq_reqmsg);
+       ENTRY;
 
-        CDEBUG(D_HA, "import %s of %s@%s abruptly disconnected: reconnecting\n",
-               imp->imp_obd->obd_name, obd2cli_tgt(imp->imp_obd),
-               imp->imp_connection->c_remote_uuid.uuid);
-
-        if (ptlrpc_set_import_discon(imp,
-                              lustre_msg_get_conn_cnt(failed_req->rq_reqmsg))) {
-                if (!imp->imp_replayable) {
-                        CDEBUG(D_HA, "import %s@%s for %s not replayable, "
-                               "auto-deactivating\n",
-                               obd2cli_tgt(imp->imp_obd),
-                               imp->imp_connection->c_remote_uuid.uuid,
-                               imp->imp_obd->obd_name);
-                        ptlrpc_deactivate_import(imp);
-                }
-                /* to control recovery via lctl {disable|enable}_recovery */
-                if (imp->imp_deactive == 0)
-                        ptlrpc_connect_import(imp);
-        }
+       CDEBUG(D_HA, "import %s of %s@%s abruptly disconnected: reconnecting\n",
+               imp->imp_obd->obd_name, obd2cli_tgt(imp->imp_obd),
+               imp->imp_connection->c_remote_uuid.uuid);
+
+       if (ptlrpc_set_import_discon(imp, conn, true)) {
+               /* to control recovery via lctl {disable|enable}_recovery */
+               if (imp->imp_deactive == 0)
+                       ptlrpc_connect_import(imp);
+       }
 
-        /* Wait for recovery to complete and resend. If evicted, then
-           this request will be errored out later.*/
+       /* Wait for recovery to complete and resend. If evicted, then
+          this request will be errored out later.*/
        spin_lock(&failed_req->rq_lock);
        if (!failed_req->rq_no_resend)
                failed_req->rq_resend = 1;
@@ -261,7 +253,7 @@ void ptlrpc_request_handle_notconn(struct ptlrpc_request *failed_req)
 }
 
 /**
- * Administratively active/deactive a client. 
+ * Administratively active/deactive a client.
  * This should only be called by the ioctl interface, currently
  *  - the lctl deactivate and activate commands
  *  - echo 0/1 >> /proc/osc/XXX/active
@@ -320,21 +312,21 @@ int ptlrpc_recover_import(struct obd_import *imp, char *new_uuid, int async)
            atomic_read(&imp->imp_inval_count))
                rc = -EINVAL;
        spin_unlock(&imp->imp_lock);
-        if (rc)
-                GOTO(out, rc);
+       if (rc)
+               GOTO(out, rc);
 
-        /* force import to be disconnected. */
-        ptlrpc_set_import_discon(imp, 0);
+       /* force import to be disconnected. */
+       ptlrpc_set_import_discon(imp, 0, false);
 
-        if (new_uuid) {
-                struct obd_uuid uuid;
+       if (new_uuid) {
+               struct obd_uuid uuid;
 
-                /* intruct import to use new uuid */
-                obd_str2uuid(&uuid, new_uuid);
-                rc = import_set_conn_priority(imp, &uuid);
-                if (rc)
-                        GOTO(out, rc);
-        }
+               /* intruct import to use new uuid */
+               obd_str2uuid(&uuid, new_uuid);
+               rc = import_set_conn_priority(imp, &uuid);
+               if (rc)
+                       GOTO(out, rc);
+       }
 
         /* Check if reconnect is already in progress */
        spin_lock(&imp->imp_lock);