From 29904135df671c624b1e542fdda94b221d76e667 Mon Sep 17 00:00:00 2001 From: Yang Sheng Date: Mon, 15 Oct 2018 17:37:21 +0800 Subject: [PATCH] LU-11542 import: fix race between imp_state & imp_invalid 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. Signed-off-by: Yang Sheng Change-Id: Ib4cec0bcaf6f4b221ba260edb94749a4e523f5e6 Reviewed-on: https://review.whamcloud.com/33395 Tested-by: jenkins Reviewed-by: Wang Shilong Reviewed-by: Andreas Dilger Tested-by: Maloo Reviewed-by: Oleg Drokin --- lustre/include/lustre_ha.h | 2 +- lustre/lov/lov_obd.c | 12 +-- lustre/ptlrpc/client.c | 6 +- lustre/ptlrpc/import.c | 172 +++++++++++++++++++++++----------------- lustre/ptlrpc/pinger.c | 13 ++- lustre/ptlrpc/ptlrpc_internal.h | 3 +- lustre/ptlrpc/recover.c | 62 +++++++-------- 7 files changed, 141 insertions(+), 129 deletions(-) diff --git a/lustre/include/lustre_ha.h b/lustre/include/lustre_ha.h index 7c22d98..2cb4969 100644 --- a/lustre/include/lustre_ha.h +++ b/lustre/include/lustre_ha.h @@ -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); diff --git a/lustre/lov/lov_obd.c b/lustre/lov/lov_obd.c index 467cfd0..ac63489 100644 --- a/lustre/lov/lov_obd.c +++ b/lustre/lov/lov_obd.c @@ -146,12 +146,12 @@ int lov_connect_osc(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) { diff --git a/lustre/ptlrpc/client.c b/lustre/ptlrpc/client.c index 0d01b0f..529a6fd 100644 --- a/lustre/ptlrpc/client.c +++ b/lustre/ptlrpc/client.c @@ -3249,14 +3249,14 @@ int ptlrpc_replay_req(struct ptlrpc_request *req) 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. * 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 @@ -3301,8 +3301,6 @@ void ptlrpc_abort_inflight(struct obd_import *imp) if (imp->imp_replayable) ptlrpc_free_committed(imp); - spin_unlock(&imp->imp_lock); - EXIT; } diff --git a/lustre/ptlrpc/import.c b/lustre/ptlrpc/import.c index e765481..0760bce 100644 --- a/lustre/ptlrpc/import.c +++ b/lustre/ptlrpc/import.c @@ -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); @@ -1301,31 +1313,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); @@ -1347,17 +1361,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); @@ -1556,20 +1579,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); @@ -1790,11 +1812,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; } diff --git a/lustre/ptlrpc/pinger.c b/lustre/ptlrpc/pinger.c index 4965cdf..f24e792 100644 --- a/lustre/ptlrpc/pinger.c +++ b/lustre/ptlrpc/pinger.c @@ -242,8 +242,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), @@ -253,22 +251,21 @@ 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 || imp->imp_connect_error == -EAGAIN) ptlrpc_initiate_recovery(imp); - } else if (level != LUSTRE_IMP_FULL || - imp->imp_obd->obd_no_recov || + } 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); } } diff --git a/lustre/ptlrpc/ptlrpc_internal.h b/lustre/ptlrpc/ptlrpc_internal.h index 623e3a0..19d0487 100644 --- a/lustre/ptlrpc/ptlrpc_internal.h +++ b/lustre/ptlrpc/ptlrpc_internal.h @@ -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); diff --git a/lustre/ptlrpc/recover.c b/lustre/ptlrpc/recover.c index 4d5f28b..c923ab9 100644 --- a/lustre/ptlrpc/recover.c +++ b/lustre/ptlrpc/recover.c @@ -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); -- 1.8.3.1