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();