From 73e62f65a1329d79685eb04d742e4099b2c146e6 Mon Sep 17 00:00:00 2001 From: schmidt-scaled Date: Wed, 1 Jul 2026 20:38:26 +0300 Subject: [PATCH] bdev/nvme: abort in-flight qpair I/O when destructing a controller Once a controller is being destructed, bdev_nvme_reset_ctrlr_unsafe() and bdev_nvme_failover_ctrlr_unsafe() both return -ENXIO, so the reset/failover path can no longer tear down the I/O qpairs. If the transport peer is unreachable (e.g. the remote target was stopped), I/O already submitted to a qpair is never completed by a wire response and never aborted by a reset. Consumers keep their channels open until that I/O finishes, which pins the controller indefinitely and blocks the destruct from ever completing. This was observed in the field on a remote_jm bdev whose journal-client held a perpetually-outstanding heartbeat read: a graceful delete set destruct=true, the peer then vanished, the admin-queue-failure and per-I/O-timeout both requested a reset that returned -ENXIO, and the controller sat as a ghost (Keep Alive failed every ~5s) for ~10 minutes until an unrelated leadership change made the client release the read. Fix: on destruct, proactively disconnect the I/O qpairs (unless a reset is already doing so), reusing the existing bdev_nvme_reset_destroy_qpair path. This aborts the outstanding I/O with DNR set so it is failed rather than retried, letting consumers complete their I/O and close their channels so the destruct can finish. Adds test_destruct_abort_inflight_io to verify that destruct disconnects the qpairs on its own, with no reset in progress and channels still held. Co-Authored-By: Claude Opus 4.8 --- module/bdev/nvme/bdev_nvme.c | 51 ++++++++++++ .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 83 +++++++++++++++++++ 2 files changed, 134 insertions(+) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index c3a8fe30de4..7a7099436f4 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -6078,12 +6078,63 @@ attach_cb(void *cb_ctx, const struct spdk_nvme_transport_id *trid, free(name); } +static void +bdev_nvme_destruct_abort_qpairs_done(struct nvme_ctrlr *nvme_ctrlr, void *ctx, int status) +{ + NVME_CTRLR_INFOLOG(nvme_ctrlr, "qpairs were disconnected for destruct.\n"); + + /* Drop the reference taken in bdev_nvme_destruct_abort_qpairs() that kept + * the controller alive across the asynchronous channel iteration. + */ + nvme_ctrlr_put_ref(nvme_ctrlr); +} + +/* Once a controller is being destructed, bdev_nvme_reset_ctrlr_unsafe() and + * bdev_nvme_failover_ctrlr_unsafe() both return -ENXIO, so the reset/failover + * path can no longer tear down the I/O qpairs. If the transport peer is + * unreachable (e.g. the remote target was stopped), any I/O already submitted + * to a qpair will therefore never be completed by a wire response, and never be + * aborted by a reset. Consumers keep their channels open until that I/O + * finishes, which pins the controller indefinitely and blocks the destruct from + * ever completing. Break that deadlock by disconnecting the qpairs here, which + * aborts the outstanding I/O (with DNR so it is failed rather than retried), + * allowing consumers to complete their I/O and close their channels. + */ +static void +bdev_nvme_destruct_abort_qpairs(struct nvme_ctrlr *nvme_ctrlr) +{ + pthread_mutex_lock(&nvme_ctrlr->mutex); + + /* A reset/failover already in progress will tear down the qpairs and + * abort their outstanding I/O, so there is nothing to do here. + */ + if (nvme_ctrlr->resetting) { + pthread_mutex_unlock(&nvme_ctrlr->mutex); + return; + } + + /* The controller is going away; fail aborted I/O instead of retrying it. */ + nvme_ctrlr->dont_retry = true; + pthread_mutex_unlock(&nvme_ctrlr->mutex); + + /* Keep the controller alive while the per-channel iteration runs; + * released in bdev_nvme_destruct_abort_qpairs_done(). + */ + nvme_ctrlr_get_ref(nvme_ctrlr); + + nvme_ctrlr_for_each_channel(nvme_ctrlr, + bdev_nvme_reset_destroy_qpair, + NULL, + bdev_nvme_destruct_abort_qpairs_done); +} + static void _nvme_ctrlr_destruct(void *ctx) { struct nvme_ctrlr *nvme_ctrlr = ctx; nvme_ctrlr_depopulate_namespaces(nvme_ctrlr); + bdev_nvme_destruct_abort_qpairs(nvme_ctrlr); nvme_ctrlr_put_ref(nvme_ctrlr); } 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 b88c1a01b31..16bec820b2f 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 @@ -1676,6 +1676,88 @@ test_race_between_reset_and_destruct_ctrlr(void) CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); } +static void +test_destruct_abort_inflight_io(void) +{ + struct spdk_nvme_transport_id trid = {}; + struct spdk_nvme_ctrlr ctrlr = {}; + struct nvme_ctrlr *nvme_ctrlr; + struct spdk_io_channel *ch1, *ch2; + struct nvme_ctrlr_channel *ctrlr_ch1, *ctrlr_ch2; + int rc; + + ut_init_trid(&trid); + TAILQ_INIT(&ctrlr.active_io_qpairs); + + set_thread(0); + + rc = nvme_ctrlr_create(&ctrlr, "nvme0", &trid, NULL); + CU_ASSERT(rc == 0); + + nvme_ctrlr = nvme_ctrlr_get_by_name("nvme0"); + SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL); + + /* Two consumers (e.g. journal clients) open I/O channels. Each creates a + * connected qpair and holds a reference on the controller. + */ + ch1 = spdk_get_io_channel(nvme_ctrlr); + SPDK_CU_ASSERT_FATAL(ch1 != NULL); + ctrlr_ch1 = spdk_io_channel_get_ctx(ch1); + SPDK_CU_ASSERT_FATAL(ctrlr_ch1->qpair != NULL); + SPDK_CU_ASSERT_FATAL(ctrlr_ch1->qpair->qpair != NULL); + + set_thread(1); + + ch2 = spdk_get_io_channel(nvme_ctrlr); + SPDK_CU_ASSERT_FATAL(ch2 != NULL); + ctrlr_ch2 = spdk_io_channel_get_ctx(ch2); + SPDK_CU_ASSERT_FATAL(ctrlr_ch2->qpair != NULL); + SPDK_CU_ASSERT_FATAL(ctrlr_ch2->qpair->qpair != NULL); + + /* Delete (destruct) the controller while both consumers still hold their + * channels. The transport peer is silently gone -- the qpairs have NOT + * been flagged as failed, so nothing outside destruct would disconnect + * them, and the reset/failover path is now disabled by destruct. + */ + set_thread(0); + + rc = bdev_nvme_delete("nvme0", &g_any_path, NULL, NULL); + CU_ASSERT(rc == 0); + CU_ASSERT(nvme_ctrlr->destruct == true); + + poll_threads(); + + /* Regression check: no reset ran (destruct suppresses it) ... */ + CU_ASSERT(nvme_ctrlr->resetting == false); + + /* ... yet destruct disconnected the I/O qpairs on both channels on its + * own, aborting any outstanding I/O. Before the fix these stayed + * connected (and their in-flight I/O un-abortable) until the consumers + * closed their channels -- which, with the peer gone, never happened -- + * pinning the controller indefinitely. + */ + CU_ASSERT(ctrlr_ch1->qpair->qpair == NULL); + CU_ASSERT(ctrlr_ch2->qpair->qpair == NULL); + + /* The controller is still referenced by the two open channels, so it is + * not freed yet -- but the deadlock is broken. Closing the channels now + * lets the destruct complete. + */ + CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == nvme_ctrlr); + + set_thread(0); + spdk_put_io_channel(ch1); + + set_thread(1); + spdk_put_io_channel(ch2); + + poll_threads(); + spdk_delay_us(1000); + poll_threads(); + + CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); +} + static void test_failover_ctrlr(void) { @@ -8173,6 +8255,7 @@ main(int argc, char **argv) CU_ADD_TEST(suite, test_create_ctrlr); CU_ADD_TEST(suite, test_reset_ctrlr); CU_ADD_TEST(suite, test_race_between_reset_and_destruct_ctrlr); + CU_ADD_TEST(suite, test_destruct_abort_inflight_io); CU_ADD_TEST(suite, test_failover_ctrlr); CU_ADD_TEST(suite, test_race_between_failover_and_add_secondary_trid); CU_ADD_TEST(suite, test_pending_reset);