From 3056c8ac02a27b01f84fdc8e0cf796c3d8737909 Mon Sep 17 00:00:00 2001 From: Konrad Sztyber Date: Thu, 21 Apr 2022 13:19:13 +0200 Subject: [PATCH] nvmf/tcp: delay qpair destruction This patch adds an extra spdk_thread_send_msg() call to destroy a qpair to make sure that it isn't freed from the context of a socket write callback. Otherwise, spdk_sock_close() won't abort pending requests, causing their completions to be exected after the qpair is freed. Fixes #2471 Signed-off-by: Konrad Sztyber Change-Id: Ia510d5d754baccca1e444afdb10696ab9b58e28b Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12332 Tested-by: SPDK CI Jenkins Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Reviewed-by: Shuhei Matsumoto Reviewed-by: Jim Harris --- lib/nvmf/tcp.c | 31 ++++++++++++++++++++++++++----- test/unit/lib/nvmf/tcp.c/tcp_ut.c | 11 +++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/lib/nvmf/tcp.c b/lib/nvmf/tcp.c index 20f409f36..8c31ad5f6 100644 --- a/lib/nvmf/tcp.c +++ b/lib/nvmf/tcp.c @@ -311,6 +311,8 @@ struct spdk_nvmf_tcp_qpair { */ struct spdk_poller *timeout_poller; + spdk_nvmf_transport_qpair_fini_cb fini_cb_fn; + void *fini_cb_arg; TAILQ_ENTRY(spdk_nvmf_tcp_qpair) link; }; @@ -524,8 +526,11 @@ nvmf_tcp_dump_qpair_req_contents(struct spdk_nvmf_tcp_qpair *tqpair) } static void -nvmf_tcp_qpair_destroy(struct spdk_nvmf_tcp_qpair *tqpair) +_nvmf_tcp_qpair_destroy(void *_tqpair) { + struct spdk_nvmf_tcp_qpair *tqpair = _tqpair; + spdk_nvmf_transport_qpair_fini_cb cb_fn = tqpair->fini_cb_fn; + void *cb_arg = tqpair->fini_cb_arg; int err = 0; spdk_trace_record(TRACE_TCP_QP_DESTROY, 0, 0, (uintptr_t)tqpair); @@ -551,9 +556,24 @@ nvmf_tcp_qpair_destroy(struct spdk_nvmf_tcp_qpair *tqpair) free(tqpair->reqs); spdk_free(tqpair->bufs); free(tqpair); + + if (cb_fn != NULL) { + cb_fn(cb_arg); + } + SPDK_DEBUGLOG(nvmf_tcp, "Leave\n"); } +static void +nvmf_tcp_qpair_destroy(struct spdk_nvmf_tcp_qpair *tqpair) +{ + /* Delay the destruction to make sure it isn't performed from the context of a sock + * callback. Otherwise, spdk_sock_close() might not abort pending requests, causing their + * completions to be executed after the qpair is freed. (Note: this fixed issue #2471.) + */ + spdk_thread_send_msg(spdk_get_thread(), _nvmf_tcp_qpair_destroy, tqpair); +} + static void nvmf_tcp_dump_opts(struct spdk_nvmf_transport *transport, struct spdk_json_write_ctx *w) { @@ -3154,12 +3174,13 @@ nvmf_tcp_close_qpair(struct spdk_nvmf_qpair *qpair, SPDK_DEBUGLOG(nvmf_tcp, "Qpair: %p\n", qpair); tqpair = SPDK_CONTAINEROF(qpair, struct spdk_nvmf_tcp_qpair, qpair); + + assert(tqpair->fini_cb_fn == NULL); + tqpair->fini_cb_fn = cb_fn; + tqpair->fini_cb_arg = cb_arg; + nvmf_tcp_qpair_set_state(tqpair, NVME_TCP_QPAIR_STATE_EXITED); nvmf_tcp_qpair_destroy(tqpair); - - if (cb_fn) { - cb_fn(cb_arg); - } } static int diff --git a/test/unit/lib/nvmf/tcp.c/tcp_ut.c b/test/unit/lib/nvmf/tcp.c/tcp_ut.c index 34684aa55..af45b92c6 100644 --- a/test/unit/lib/nvmf/tcp.c/tcp_ut.c +++ b/test/unit/lib/nvmf/tcp.c/tcp_ut.c @@ -766,6 +766,11 @@ test_nvmf_tcp_qpair_init_mem_resource(void) int rc; struct spdk_nvmf_tcp_qpair *tqpair = NULL; struct spdk_nvmf_transport transport = {}; + struct spdk_thread *thread; + + thread = spdk_thread_create(NULL, NULL); + SPDK_CU_ASSERT_FATAL(thread != NULL); + spdk_set_thread(thread); tqpair = calloc(1, sizeof(*tqpair)); tqpair->qpair.transport = &transport; @@ -820,6 +825,12 @@ test_nvmf_tcp_qpair_init_mem_resource(void) /* Free all of tqpair resource */ nvmf_tcp_qpair_destroy(tqpair); + + spdk_thread_exit(thread); + while (!spdk_thread_is_exited(thread)) { + spdk_thread_poll(thread, 0, 0); + } + spdk_thread_destroy(thread); } static void