From 128548191798c2215f735b85a8726dce4521d9e8 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Thu, 10 Feb 2022 10:52:59 +0900 Subject: [PATCH] nvme: Free I/O qpair now even if it is in poll group completion spdk_nvme_poll_group has followed spdk_nvme_qpair about how to process I/O qpair deletion inside of a completion context. spdk_nvme_qpair_process_completions() accesses qpair after returning from nvme_transport_qpair_process_completions(). So this is reasonable. On the other hand, if spdk_nvme_poll_group_process_completions() can execute spdk_nvme_ctrlr_free_io_qpair() inside of a completion context, the target qpair is ensured to be deleted after returning from spdk_nvme_ctrlr_free_io_qpair(). Then the target qpair is not accessed anymore in spdk_nvme_poll_group_process_completions(). Remove two variables, in_completion_context and num_qpairs_to_delete, of spdk_nvme_transport_poll_group and the related code. This change is really necessary to support the following case. In the NVMe bdev module, a nvme_qpair has a qpair and a poll_group channel. disconnected_qpair_cb calls spdk_nvme_ctrlr_free_io_qpair() for the qpair and spdk_put_io_channel() to the poll_group_channel. spdk_nvme_ctrlr_free_io_qpair() is executed after unwinding stack but spdk_put_io_channel() is executed now. The callback to spdk_put_io_channel() calls spdk_nvme_poll_group_destroy(). However, spdk_nvme_ctrlr_free_io_qpair() is not executed. Hence spdk_nvme_poll_group_destroy() fails. Update the corresponding stub in unit test together. Signed-off-by: Shuhei Matsumoto Change-Id: Icd1f1daf049c6c7ffb28790fe87989a1060f8952 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11496 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Reviewed-by: Ben Walker Reviewed-by: Jim Harris Tested-by: SPDK CI Jenkins --- lib/nvme/nvme_ctrlr.c | 7 ---- lib/nvme/nvme_internal.h | 2 -- lib/nvme/nvme_transport.c | 33 +------------------ .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 32 ------------------ 4 files changed, 1 insertion(+), 73 deletions(-) diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index 04958338b..44be95050 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -619,13 +619,6 @@ spdk_nvme_ctrlr_free_io_qpair(struct spdk_nvme_qpair *qpair) return 0; } - if (qpair->poll_group && qpair->poll_group->in_completion_context) { - /* Same as above, but in a poll group. */ - qpair->poll_group->num_qpairs_to_delete++; - qpair->delete_after_completion_context = 1; - return 0; - } - nvme_transport_ctrlr_disconnect_qpair(ctrlr, qpair); if (qpair->poll_group) { diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index 4400ae302..e94b6968b 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -502,8 +502,6 @@ struct spdk_nvme_transport_poll_group { STAILQ_HEAD(, spdk_nvme_qpair) connected_qpairs; STAILQ_HEAD(, spdk_nvme_qpair) disconnected_qpairs; STAILQ_ENTRY(spdk_nvme_transport_poll_group) link; - bool in_completion_context; - uint64_t num_qpairs_to_delete; }; struct spdk_nvme_ns { diff --git a/lib/nvme/nvme_transport.c b/lib/nvme/nvme_transport.c index b10a61863..925d9b628 100644 --- a/lib/nvme/nvme_transport.c +++ b/lib/nvme/nvme_transport.c @@ -707,39 +707,8 @@ int64_t nvme_transport_poll_group_process_completions(struct spdk_nvme_transport_poll_group *tgroup, uint32_t completions_per_qpair, spdk_nvme_disconnected_qpair_cb disconnected_qpair_cb) { - struct spdk_nvme_qpair *qpair; - int64_t rc; - - tgroup->in_completion_context = true; - rc = tgroup->transport->ops.poll_group_process_completions(tgroup, completions_per_qpair, + return tgroup->transport->ops.poll_group_process_completions(tgroup, completions_per_qpair, disconnected_qpair_cb); - tgroup->in_completion_context = false; - - if (spdk_unlikely(tgroup->num_qpairs_to_delete > 0)) { - /* deleted qpairs are more likely to be in the disconnected qpairs list. */ - STAILQ_FOREACH(qpair, &tgroup->disconnected_qpairs, poll_group_stailq) { - if (spdk_unlikely(qpair->delete_after_completion_context)) { - spdk_nvme_ctrlr_free_io_qpair(qpair); - if (--tgroup->num_qpairs_to_delete == 0) { - return rc; - } - } - } - - STAILQ_FOREACH(qpair, &tgroup->connected_qpairs, poll_group_stailq) { - if (spdk_unlikely(qpair->delete_after_completion_context)) { - spdk_nvme_ctrlr_free_io_qpair(qpair); - if (--tgroup->num_qpairs_to_delete == 0) { - return rc; - } - } - } - /* Just in case. */ - SPDK_DEBUGLOG(nvme, "Mismatch between qpairs to delete and poll group number.\n"); - tgroup->num_qpairs_to_delete = 0; - } - - return rc; } int diff --git a/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c b/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c index ec9bbf891..6faf059f5 100644 --- a/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c +++ b/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c @@ -261,8 +261,6 @@ struct spdk_nvme_poll_group { struct spdk_nvme_accel_fn_table accel_fn_table; TAILQ_HEAD(, spdk_nvme_qpair) connected_qpairs; TAILQ_HEAD(, spdk_nvme_qpair) disconnected_qpairs; - bool in_completion_context; - uint64_t num_qpairs_to_delete; }; struct spdk_nvme_probe_ctx { @@ -746,12 +744,6 @@ spdk_nvme_ctrlr_free_io_qpair(struct spdk_nvme_qpair *qpair) return 0; } - if (qpair->poll_group && qpair->poll_group->in_completion_context) { - qpair->poll_group->num_qpairs_to_delete++; - qpair->delete_after_completion_context = true; - return 0; - } - spdk_nvme_ctrlr_disconnect_io_qpair(qpair); if (qpair->poll_group != NULL) { @@ -1136,8 +1128,6 @@ spdk_nvme_poll_group_process_completions(struct spdk_nvme_poll_group *group, return -EINVAL; } - group->in_completion_context = true; - TAILQ_FOREACH_SAFE(qpair, &group->disconnected_qpairs, poll_group_tailq, tmp_qpair) { disconnected_qpair_cb(qpair, group->ctx); } @@ -1158,28 +1148,6 @@ spdk_nvme_poll_group_process_completions(struct spdk_nvme_poll_group *group, } } - group->in_completion_context = false; - - if (group->num_qpairs_to_delete > 0) { - TAILQ_FOREACH_SAFE(qpair, &group->disconnected_qpairs, poll_group_tailq, tmp_qpair) { - if (qpair->delete_after_completion_context) { - spdk_nvme_ctrlr_free_io_qpair(qpair); - CU_ASSERT(group->num_qpairs_to_delete > 0); - group->num_qpairs_to_delete--; - } - } - - TAILQ_FOREACH_SAFE(qpair, &group->connected_qpairs, poll_group_tailq, tmp_qpair) { - if (qpair->delete_after_completion_context) { - spdk_nvme_ctrlr_free_io_qpair(qpair); - CU_ASSERT(group->num_qpairs_to_delete > 0); - group->num_qpairs_to_delete--; - } - } - - CU_ASSERT(group->num_qpairs_to_delete == 0); - } - return error_reason ? error_reason : num_completions; }