From 3e826ccfce3a42fa75ed0b63518eb8c5b1f599cf Mon Sep 17 00:00:00 2001 From: Chris Horn Date: Tue, 14 Nov 2023 09:35:15 -0700 Subject: [PATCH] LU-17838 kfilnd: Prevent simultaneous hellos There is a race condition with checking, setting and clearing the kp_hello_pending flag that can result in multiple hello requests being sent for the same peer. If no hello response is received after the LND timeout then multiple threads can race with each other in clearing the kp_hello_pending flag and posting a new hello request message. Thread 1: sets kp_hello_pending and posts hello request message Thread 2: Clears kp_hello_pending, then sets kp_hello_sending Thread 3: Clears kp_hello_pending, then sets kp_hello_sending Thread 2/3: Both post hello request message To resolve this issue we change kp_hello_pending from a simple binary to instead track three states of a hello request: KP_HELLO_NONE, KP_HELLO_INIT, and KP_HELLO_SENT. State is NONE when there is no hello in the process of being sent. State is INIT when a thread is allocating a HELLO request in preparation for sending. State is SENT when the HELLO request is being posted. Now, when some threads detect that we have not received hello response after LND timeout seconds then only one of them will be able to transition to the hello state from SENT -> NONE. Add CFS_KFI_REPLAY_IDLE_EVENT fail_loc that can be used to delay processing of TNs in the idle state depending on the TN event value specified in fail_val. HPE-bug-id: LUS-11974 Test-Parameters: trivial Fixes: 11a32d886b ("LU-16213 kfilnd: Allow one HELLO in-flight per peer") Signed-off-by: Chris Horn Change-Id: I4dddf57971848a80a550df7523d55ad03f4a083e Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/55069 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Ian Ziemba Reviewed-by: Ron Gredvig Reviewed-by: Caleb Carlson Reviewed-by: Oleg Drokin --- lnet/klnds/kfilnd/kfilnd.c | 8 ++++++-- lnet/klnds/kfilnd/kfilnd.h | 32 +++++++++++++++++++------------- lnet/klnds/kfilnd/kfilnd_peer.c | 6 +++--- lnet/klnds/kfilnd/kfilnd_tn.c | 7 +++++-- 4 files changed, 33 insertions(+), 20 deletions(-) diff --git a/lnet/klnds/kfilnd/kfilnd.c b/lnet/klnds/kfilnd/kfilnd.c index 53bf5ca..2066d41 100644 --- a/lnet/klnds/kfilnd/kfilnd.c +++ b/lnet/klnds/kfilnd/kfilnd.c @@ -63,7 +63,9 @@ int kfilnd_send_hello_request(struct kfilnd_dev *dev, int cpt, struct kfilnd_transaction *tn; int rc; - if (kfilnd_peer_set_check_hello_pending(kp)) { + /* Only one thread may progress state from NONE -> INIT */ + if (atomic_cmpxchg(&kp->kp_hello_state, KP_HELLO_NONE, KP_HELLO_INIT) != + KP_HELLO_NONE) { CDEBUG(D_NET, "Hello already pending to peer %s(%px)\n", libcfs_nid2str(kp->kp_nid), kp); return 0; @@ -73,7 +75,7 @@ int kfilnd_send_hello_request(struct kfilnd_dev *dev, int cpt, if (IS_ERR(tn)) { rc = PTR_ERR(tn); CERROR("Failed to allocate transaction struct: rc=%d\n", rc); - kfilnd_peer_clear_hello_pending(kp); + atomic_set(&kp->kp_hello_state, KP_HELLO_NONE); return rc; } @@ -86,6 +88,8 @@ int kfilnd_send_hello_request(struct kfilnd_dev *dev, int cpt, kp->kp_hello_ts = ktime_get_seconds(); + atomic_set(&kp->kp_hello_state, KP_HELLO_SENDING); + kfilnd_tn_event_handler(tn, TN_EVENT_TX_HELLO, 0); return 0; diff --git a/lnet/klnds/kfilnd/kfilnd.h b/lnet/klnds/kfilnd/kfilnd.h index 93c7ad2..92b1a75 100644 --- a/lnet/klnds/kfilnd/kfilnd.h +++ b/lnet/klnds/kfilnd/kfilnd.h @@ -104,6 +104,7 @@ #define CFS_KFI_FAIL_WAIT_SEND_COMP1 0xF115 #define CFS_KFI_FAIL_WAIT_SEND_COMP2 0xF116 #define CFS_KFI_FAIL_WAIT_SEND_COMP3 0xF117 +#define CFS_KFI_REPLAY_IDLE_EVENT 0xF118 /* Maximum number of transaction keys supported. */ #define KFILND_EP_KEY_BITS 16U @@ -254,7 +255,7 @@ struct kfilnd_peer { u16 kp_version; u32 kp_local_session_key; u32 kp_remote_session_key; - atomic_t kp_hello_pending; + atomic_t kp_hello_state; time64_t kp_hello_ts; atomic_t kp_state; }; @@ -264,18 +265,20 @@ static inline bool kfilnd_peer_deleted(struct kfilnd_peer *kp) return atomic_read(&kp->kp_remove_peer) > 0; } -/* Sets kp_hello_sending - * Returns true if it was already set - * Returns false otherwise +/* Values for kp_hello_state. Valid transitions: + * NONE -> INIT + * INIT -> NONE (only when fail to allocate kfilnd_tn for hello req) + * INIT -> SENDING + * SENDING -> NONE */ -static inline bool kfilnd_peer_set_check_hello_pending(struct kfilnd_peer *kp) -{ - return (atomic_cmpxchg(&kp->kp_hello_pending, 0, 1) == 1); -} +#define KP_HELLO_NONE 0 /* There is no hello request being sent */ +#define KP_HELLO_INIT 1 /* Hello request is initializing */ +#define KP_HELLO_SENDING 2 /* Hello request TN is in the state machine */ -static inline void kfilnd_peer_clear_hello_pending(struct kfilnd_peer *kp) +/* If kp_hello_state is SENDING then set to NONE */ +static inline void kfilnd_peer_clear_hello_state(struct kfilnd_peer *kp) { - atomic_set(&kp->kp_hello_pending, 0); + atomic_cmpxchg(&kp->kp_hello_state, KP_HELLO_SENDING, KP_HELLO_NONE); } static inline bool kfilnd_peer_is_new_peer(struct kfilnd_peer *kp) @@ -303,7 +306,9 @@ static inline bool kfilnd_peer_needs_throttle(struct kfilnd_peer *kp) static inline bool kfilnd_peer_needs_hello(struct kfilnd_peer *kp, bool proactive_handshake) { - if (atomic_read(&kp->kp_hello_pending) == 0) { + int hello_state = atomic_read(&kp->kp_hello_state); + + if (hello_state == KP_HELLO_NONE) { if (atomic_read(&kp->kp_state) != KP_STATE_UPTODATE) return true; else if (proactive_handshake && @@ -311,7 +316,8 @@ static inline bool kfilnd_peer_needs_hello(struct kfilnd_peer *kp, lnet_get_lnd_timeout() * 2, ktime_get_seconds())) return true; - } else if (ktime_before(kp->kp_hello_ts + lnet_get_lnd_timeout(), + } else if (hello_state == KP_HELLO_SENDING && + ktime_before(kp->kp_hello_ts + lnet_get_lnd_timeout(), ktime_get_seconds())) { /* Sent hello but never received reply */ CDEBUG(D_NET, @@ -319,7 +325,7 @@ static inline bool kfilnd_peer_needs_hello(struct kfilnd_peer *kp, libcfs_nid2str(kp->kp_nid), kp, kp->kp_addr, ktime_sub(ktime_get_seconds(), kp->kp_hello_ts)); - kfilnd_peer_clear_hello_pending(kp); + kfilnd_peer_clear_hello_state(kp); return true; } diff --git a/lnet/klnds/kfilnd/kfilnd_peer.c b/lnet/klnds/kfilnd/kfilnd_peer.c index 7288208..2c8ab95 100644 --- a/lnet/klnds/kfilnd/kfilnd_peer.c +++ b/lnet/klnds/kfilnd/kfilnd_peer.c @@ -251,7 +251,7 @@ again: kp->kp_nid = nid; atomic_set(&kp->kp_rx_base, 0); atomic_set(&kp->kp_remove_peer, 0); - atomic_set(&kp->kp_hello_pending, 0); + atomic_set(&kp->kp_hello_state, KP_HELLO_NONE); atomic_set(&kp->kp_state, KP_STATE_NEW); kp->kp_local_session_key = kfilnd_dev_get_session_key(dev); kp->kp_hello_ts = ktime_get_seconds(); @@ -375,10 +375,10 @@ void kfilnd_peer_process_hello(struct kfilnd_peer *kp, struct kfilnd_msg *msg) CDEBUG(D_NET, "kp %s(%p):0x%llx is up-to-date\n", libcfs_nid2str(kp->kp_nid), kp, kp->kp_addr); - /* Clear kp_hello_pending if we've received the hello response, + /* Clear kp_hello_state if we've received the hello response, * otherwise this is an incoming hello request and we may have our * own hello request to this peer still outstanding */ if (msg->type == KFILND_MSG_HELLO_RSP) - kfilnd_peer_clear_hello_pending(kp); + kfilnd_peer_clear_hello_state(kp); } diff --git a/lnet/klnds/kfilnd/kfilnd_tn.c b/lnet/klnds/kfilnd/kfilnd_tn.c index cb9fbbe..eba1077 100644 --- a/lnet/klnds/kfilnd/kfilnd_tn.c +++ b/lnet/klnds/kfilnd/kfilnd_tn.c @@ -700,6 +700,9 @@ static int kfilnd_tn_state_idle(struct kfilnd_transaction *tn, goto out; } + if (CFS_FAIL_CHECK_VALUE(CFS_KFI_REPLAY_IDLE_EVENT, event)) + return -EAGAIN; + switch (event) { case TN_EVENT_INIT_IMMEDIATE: case TN_EVENT_TX_HELLO: @@ -736,7 +739,7 @@ static int kfilnd_tn_state_idle(struct kfilnd_transaction *tn, libcfs_nid2str(tn->tn_kp->kp_nid), tn->tn_target_addr, rc); if (event == TN_EVENT_TX_HELLO) - kfilnd_peer_clear_hello_pending(tn->tn_kp); + kfilnd_peer_clear_hello_state(tn->tn_kp); kfilnd_tn_status_update(tn, rc, LNET_MSG_STATUS_LOCAL_ERROR); } @@ -924,7 +927,7 @@ static int kfilnd_tn_state_imm_send(struct kfilnd_transaction *tn, */ kfilnd_peer_tn_failed(tn->tn_kp, status, false); if (tn->msg_type == KFILND_MSG_HELLO_REQ) - kfilnd_peer_clear_hello_pending(tn->tn_kp); + kfilnd_peer_clear_hello_state(tn->tn_kp); break; case TN_EVENT_TX_OK: -- 1.8.3.1