From 7f6738e12d927478b31d5e04675736abbe19db06 Mon Sep 17 00:00:00 2001 From: paul luse Date: Tue, 9 Oct 2018 13:38:29 -0700 Subject: [PATCH] crypto: add proper reset handling Previously there was no consideration for IO that were outstanding to the crypto device when handling a reset. This patch makes sure that those IO are completed with FAIL status prior to completing the reset that we pass down the stack. It does so by sending down the reset first and in the completion using spdk_for_each_channel and the poller to quiesce each channel allowing the crypto side to complete all IOs before we finally complete the reset IO after the last channel is quiesced. Resets are tracked on a per bdev basis. Addresses github issue #449. Change-Id: Iadb07bada1fcaad33d9f224a60d983a7eb835236 Signed-off-by: paul luse Signed-off-by: Ben Walker Reviewed-on: https://review.gerrithub.io/428552 Chandler-Test-Pool: SPDK Automated Test System Reviewed-by: Seth Howell Reviewed-by: Jim Harris Tested-by: SPDK CI Jenkins --- lib/bdev/crypto/vbdev_crypto.c | 109 +++++++++++++++++++----- test/unit/lib/bdev/crypto.c/crypto_ut.c | 28 +++--- 2 files changed, 106 insertions(+), 31 deletions(-) diff --git a/lib/bdev/crypto/vbdev_crypto.c b/lib/bdev/crypto/vbdev_crypto.c index 510e84964..807940645 100644 --- a/lib/bdev/crypto/vbdev_crypto.c +++ b/lib/bdev/crypto/vbdev_crypto.c @@ -168,6 +168,8 @@ struct crypto_io_channel { struct spdk_io_channel *base_ch; /* IO channel of base device */ struct spdk_poller *poller; /* completion poller */ struct device_qp *device_qp; /* unique device/qp combination for this channel */ + TAILQ_HEAD(, spdk_bdev_io) pending_cry_ios; /* outstanding operations to the crypto device */ + struct spdk_io_channel_iter *iter; /* used with for_each_channel in reset */ }; /* This is the crypto per IO context that the bdev layer allocates for us opaquely and attaches to @@ -388,41 +390,43 @@ _crypto_operation_complete(struct spdk_bdev_io *bdev_io) struct spdk_bdev_io *free_me = io_ctx->read_io; int rc = 0; - if (bdev_io->internal.status != SPDK_BDEV_IO_STATUS_FAILED) { - if (bdev_io->type == SPDK_BDEV_IO_TYPE_READ) { + TAILQ_REMOVE(&crypto_ch->pending_cry_ios, bdev_io, module_link); - /* Complete the original IO and then free the one that we created - * as a result of issuing an IO via submit_reqeust. - */ + if (bdev_io->type == SPDK_BDEV_IO_TYPE_READ) { + + /* Complete the original IO and then free the one that we created + * as a result of issuing an IO via submit_reqeust. + */ + if (bdev_io->internal.status != SPDK_BDEV_IO_STATUS_FAILED) { spdk_bdev_io_complete(bdev_io, SPDK_BDEV_IO_STATUS_SUCCESS); - spdk_bdev_free_io(free_me); + } else { + SPDK_ERRLOG("Issue with decryption on bdev_io %p\n", bdev_io); + rc = -EINVAL; + } + spdk_bdev_free_io(free_me); - } else if (bdev_io->type == SPDK_BDEV_IO_TYPE_WRITE) { + } else if (bdev_io->type == SPDK_BDEV_IO_TYPE_WRITE) { + if (bdev_io->internal.status != SPDK_BDEV_IO_STATUS_FAILED) { /* Write the encrypted data. */ rc = spdk_bdev_writev_blocks(crypto_bdev->base_desc, crypto_ch->base_ch, &io_ctx->cry_iov, 1, io_ctx->cry_offset_blocks, io_ctx->cry_num_blocks, _complete_internal_write, bdev_io); } else { - - /* Something really went haywire if this function got called with a type - * other than read or write. - */ - rc = -1; + SPDK_ERRLOG("Issue with encryption on bdev_io %p\n", bdev_io); + rc = -EINVAL; } + } else { - /* If the poller found that one of the crypto ops had failed as part of this - * bdev_io it would have updated the internal status indicate failure. - */ - rc = -1; + SPDK_ERRLOG("Unknown bdev type %u on crypto operation completion\n", + bdev_io->type); + rc = -EINVAL; } - if (rc != 0) { - SPDK_ERRLOG("ERROR on crypto operation completion!\n"); + if (rc) { spdk_bdev_io_complete(bdev_io, SPDK_BDEV_IO_STATUS_FAILED); } - } /* This is the poller for the crypto device. It uses a single API to dequeue whatever is ready at @@ -484,6 +488,13 @@ crypto_dev_poller(void *args) /* done encrypting, complete the bdev_io */ if (--io_ctx->cryop_cnt_remaining == 0) { + /* If we're completing this with an outstanding reset we need + * to fail it. + */ + if (crypto_ch->iter) { + bdev_io->internal.status = SPDK_BDEV_IO_STATUS_FAILED; + } + /* Complete the IO */ _crypto_operation_complete(bdev_io); @@ -504,6 +515,16 @@ crypto_dev_poller(void *args) num_mbufs); } + /* If the channel iter is not NULL, we need to continue to poll + * until the pending list is empty, then we can move on to the + * next channel. + */ + if (crypto_ch->iter && TAILQ_EMPTY(&crypto_ch->pending_cry_ios)) { + SPDK_NOTICELOG("Channel %p has been quiesced.\n", crypto_ch); + spdk_for_each_channel_continue(crypto_ch->iter, 0); + crypto_ch->iter = NULL; + } + return num_dequeued_ops; } @@ -726,6 +747,9 @@ _crypto_operation(struct spdk_bdev_io *bdev_io, enum rte_crypto_cipher_operation } } while (enqueued < cryop_cnt); + /* Add this bdev_io to our outstanding list. */ + TAILQ_INSERT_TAIL(&crypto_ch->pending_cry_ios, bdev_io, module_link); + return rc; /* Error cleanup paths. */ @@ -752,6 +776,35 @@ error_get_dst: return rc; } +/* This function is called after all channels have been quiesced following + * a bdev reset. + */ +static void +_ch_quiesce_done(struct spdk_io_channel_iter *i, int status) +{ + struct crypto_bdev_io *io_ctx = spdk_io_channel_iter_get_ctx(i); + + assert(TAILQ_EMPTY(&io_ctx->crypto_ch->pending_cry_ios)); + assert(io_ctx->orig_io != NULL); + + spdk_bdev_io_complete(io_ctx->orig_io, SPDK_BDEV_IO_STATUS_SUCCESS); +} + +/* This function is called per channel to quiesce IOs before completing a + * bdev reset that we received. + */ +static void +_ch_quiesce(struct spdk_io_channel_iter *i) +{ + struct spdk_io_channel *ch = spdk_io_channel_iter_get_channel(i); + struct crypto_io_channel *crypto_ch = spdk_io_channel_get_ctx(ch); + + crypto_ch->iter = i; + /* When the poller runs, it will see the non-NULL iter and handle + * the quiesce. + */ +} + /* Completion callback for IO that were issued from this bdev other than read/write. * They have their own for readability. */ @@ -761,6 +814,20 @@ _complete_internal_io(struct spdk_bdev_io *bdev_io, bool success, void *cb_arg) struct spdk_bdev_io *orig_io = cb_arg; int status = success ? SPDK_BDEV_IO_STATUS_SUCCESS : SPDK_BDEV_IO_STATUS_FAILED; + if (bdev_io->type == SPDK_BDEV_IO_TYPE_RESET) { + struct crypto_bdev_io *orig_ctx = (struct crypto_bdev_io *)orig_io->driver_ctx; + + assert(orig_io == orig_ctx->orig_io); + + spdk_bdev_free_io(bdev_io); + + spdk_for_each_channel(orig_ctx->crypto_bdev, + _ch_quiesce, + orig_ctx, + _ch_quiesce_done); + return; + } + spdk_bdev_io_complete(orig_io, status); spdk_bdev_free_io(bdev_io); } @@ -1015,6 +1082,10 @@ crypto_bdev_ch_create_cb(void *io_device, void *ctx_buf) } pthread_mutex_unlock(&g_device_qp_lock); assert(crypto_ch->device_qp); + + /* We use this queue to track outstanding IO in our lyaer. */ + TAILQ_INIT(&crypto_ch->pending_cry_ios); + return 0; } diff --git a/test/unit/lib/bdev/crypto.c/crypto_ut.c b/test/unit/lib/bdev/crypto.c/crypto_ut.c index f01aba19f..29726fb3f 100644 --- a/test/unit/lib/bdev/crypto.c/crypto_ut.c +++ b/test/unit/lib/bdev/crypto.c/crypto_ut.c @@ -52,7 +52,6 @@ DEFINE_STUB(spdk_conf_section_get_nval, char *, (struct spdk_conf_section *sp, const char *key, int idx), NULL); DEFINE_STUB(spdk_conf_section_get_nmval, char *, (struct spdk_conf_section *sp, const char *key, int idx1, int idx2), NULL); - DEFINE_STUB_V(spdk_bdev_module_list_add, (struct spdk_bdev_module *bdev_module)); DEFINE_STUB_V(spdk_bdev_free_io, (struct spdk_bdev_io *g_bdev_io)); DEFINE_STUB(spdk_bdev_io_type_supported, bool, (struct spdk_bdev *bdev, @@ -130,9 +129,9 @@ uint16_t g_dequeue_mock; uint16_t g_enqueue_mock; unsigned ut_rte_crypto_op_bulk_alloc; int ut_rte_crypto_op_attach_sym_session = 0; - int ut_rte_cryptodev_info_get = 0; bool ut_rte_cryptodev_info_get_mocked = false; + void rte_cryptodev_info_get(uint8_t dev_id, struct rte_cryptodev_info *dev_info) { @@ -320,6 +319,7 @@ test_setup(void) g_crypto_ch->device_qp = &g_dev_qp; g_test_config = calloc(1, sizeof(struct rte_config)); g_test_config->lcore_count = 1; + TAILQ_INIT(&g_crypto_ch->pending_cry_ios); /* Allocate a real mbuf pool so we can test error paths */ g_mbuf_mp = spdk_mempool_create("mbuf_mp", NUM_MBUFS, sizeof(struct rte_mbuf), @@ -379,6 +379,7 @@ test_error_paths(void) /* same thing but switch to reads to test error path in _crypto_complete_io() */ g_bdev_io->type = SPDK_BDEV_IO_TYPE_READ; g_bdev_io->internal.status = SPDK_BDEV_IO_STATUS_SUCCESS; + TAILQ_INSERT_TAIL(&g_crypto_ch->pending_cry_ios, g_bdev_io, module_link); vbdev_crypto_submit_request(g_io_ch, g_bdev_io); CU_ASSERT(g_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_FAILED); /* Now with the read_blocks failing */ @@ -695,21 +696,22 @@ test_passthru(void) CU_ASSERT(g_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_FAILED); MOCK_CLEAR(spdk_bdev_flush_blocks); - g_bdev_io->type = SPDK_BDEV_IO_TYPE_RESET; - MOCK_SET(spdk_bdev_reset, 0); - vbdev_crypto_submit_request(g_io_ch, g_bdev_io); - CU_ASSERT(g_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_SUCCESS); - MOCK_SET(spdk_bdev_reset, -1); - vbdev_crypto_submit_request(g_io_ch, g_bdev_io); - CU_ASSERT(g_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_FAILED); - MOCK_CLEAR(spdk_bdev_reset); - /* We should never get a WZ command, we report that we don't support it. */ g_bdev_io->type = SPDK_BDEV_IO_TYPE_WRITE_ZEROES; vbdev_crypto_submit_request(g_io_ch, g_bdev_io); CU_ASSERT(g_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_FAILED); } +static void +test_reset(void) +{ + /* TODO: There are a few different ways to do this given that + * the code uses spdk_for_each_channel() to implement reset + * handling. SUbmitting w/o UT for this function for now and + * will follow up with something shortly. + */ +} + static void test_initdrivers(void) { @@ -894,7 +896,9 @@ main(int argc, char **argv) CU_add_test(suite, "test_crypto_op_complete", test_crypto_op_complete) == NULL || CU_add_test(suite, "test_supported_io", - test_supported_io) == NULL + test_supported_io) == NULL || + CU_add_test(suite, "test_reset", + test_reset) == NULL ) { CU_cleanup_registry(); return CU_get_error();