From 458214e2e7a89992ef3443595e94ae38e7f61fd6 Mon Sep 17 00:00:00 2001 From: Changpeng Liu Date: Wed, 11 Mar 2020 21:37:42 +0800 Subject: [PATCH] nvme/opal: remove the revert asynchronous API The revert asynchronous API doesn't run as the *real* asynchronous way, because the drive can only support synchronous module and only 1 session is supported. The reason why we added this API is that RPC call has the default timeout value here, while the revert may take over several minutes, the API itself doesn't short the revert action, so just remove it and use the synchronous API instead. The revert action will erase all the users data and bring the drive back to the factory state, it should run in the synchronous mode, so just remove the asynchronous API and we can increase the timeout value when using RPC to call this API. Change-Id: I08a082edea6385e378399423bbb229d05f8bc262 Signed-off-by: Changpeng Liu Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/1232 Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Jim Harris --- include/spdk/opal.h | 20 ----- lib/nvme/nvme_opal.c | 90 ------------------- lib/nvme/nvme_opal_internal.h | 1 - module/bdev/nvme/Makefile | 2 +- module/bdev/nvme/common.c | 5 -- module/bdev/nvme/common.h | 1 - module/bdev/nvme/vbdev_opal.c | 32 ------- module/bdev/nvme/vbdev_opal.h | 2 - module/bdev/nvme/vbdev_opal_rpc.c | 16 +--- .../lib/bdev/bdev_ocssd.c/bdev_ocssd_ut.c | 1 - 10 files changed, 2 insertions(+), 168 deletions(-) diff --git a/include/spdk/opal.h b/include/spdk/opal.h index 86a2bca3e..670cf68de 100644 --- a/include/spdk/opal.h +++ b/include/spdk/opal.h @@ -97,8 +97,6 @@ struct spdk_opal_locking_range_info { struct spdk_opal_dev; -typedef void (*spdk_opal_revert_cb)(struct spdk_opal_dev *dev, void *ctx, int rc); - struct spdk_opal_dev *spdk_opal_dev_construct(struct spdk_nvme_ctrlr *ctrlr); void spdk_opal_dev_destruct(struct spdk_opal_dev *dev); @@ -108,24 +106,6 @@ bool spdk_opal_supported(struct spdk_opal_dev *dev); int spdk_opal_cmd_take_ownership(struct spdk_opal_dev *dev, char *new_passwd); -/** - * Users should periodically call spdk_opal_revert_poll to check if the response is received. - * Once a final result is received, no matter success or failure, dev->revert_cb_fn will be called. - * Error code is put to dev->revert_cb_fn. - * - * Return: -EAGAIN for no result yet. 0 for final result received. - */ -int spdk_opal_revert_poll(struct spdk_opal_dev *dev); - -/** - * asynchronous function: Just send cmd and return. - * - * Users should periodically call spdk_opal_revert_poll to check if the response is received. - * Because usually revert TPer operation will take a while. - */ -int spdk_opal_cmd_revert_tper_async(struct spdk_opal_dev *dev, const char *passwd, - spdk_opal_revert_cb cb_fn, void *cb_ctx); - /** * synchronous function: send and then receive. * diff --git a/lib/nvme/nvme_opal.c b/lib/nvme/nvme_opal.c index df64c82f8..31e709f17 100644 --- a/lib/nvme/nvme_opal.c +++ b/lib/nvme/nvme_opal.c @@ -1971,96 +1971,6 @@ end: return ret; } -int -spdk_opal_revert_poll(struct spdk_opal_dev *dev) -{ - void *response = dev->resp; - struct spdk_opal_compacket *header = response; - int ret; - - assert(dev->revert_cb_fn); - - ret = spdk_nvme_ctrlr_security_receive(dev->ctrlr, SPDK_SCSI_SECP_TCG, dev->comid, - 0, dev->resp, IO_BUFFER_LENGTH); - if (ret) { - SPDK_ERRLOG("Security Receive Error on dev = %p\n", dev); - dev->revert_cb_fn(dev, dev->ctx, ret); - return 0; - } - - if (header->outstanding_data == 0 && - header->min_transfer == 0) { - ret = opal_parse_and_check_status(dev, NULL); - dev->revert_cb_fn(dev, dev->ctx, ret); - return 0; - } else { - memset(response, 0, IO_BUFFER_LENGTH); - } - - return -EAGAIN; -} - -int -spdk_opal_cmd_revert_tper_async(struct spdk_opal_dev *dev, const char *passwd, - spdk_opal_revert_cb cb_fn, void *cb_ctx) -{ - int ret; - struct spdk_opal_key opal_key = {}; - - if (dev->supported == false) { - return -ENOTSUP; - } - - if (cb_fn == NULL) { - SPDK_ERRLOG("No revert callback function specified.\n"); - return -EFAULT; - } - - dev->revert_cb_fn = cb_fn; - dev->ctx = cb_ctx; - - ret = opal_init_key(&opal_key, passwd, OPAL_LOCKING_RANGE_GLOBAL); - if (ret) { - SPDK_ERRLOG("Init key failed\n"); - return ret; - } - - pthread_mutex_lock(&dev->mutex_lock); - opal_setup_dev(dev); - - ret = opal_start_generic_session(dev, UID_SID, UID_ADMINSP, - opal_key.key, opal_key.key_len); - if (ret) { - opal_end_session(dev); - SPDK_ERRLOG("Error on starting admin SP session with error %d\n", ret); - goto end; - } - - ret = opal_revert_tper(dev); - if (ret) { - opal_end_session(dev); - SPDK_ERRLOG("Error on reverting TPer with error %d\n", ret); - goto end; - } - - ret = opal_cmd_finalize(dev, dev->hsn, dev->tsn, true); /* true: end of data */ - if (ret) { - SPDK_ERRLOG("Error finalizing command buffer: %d\n", ret); - goto end; - } - - ret = opal_send_cmd(dev); - if (ret) { - SPDK_ERRLOG("Error sending opal command: %d\n", ret); - } - - /* Controller will terminate session. No "end session" here needed. */ - -end: - pthread_mutex_unlock(&dev->mutex_lock); - return ret; -} - int spdk_opal_cmd_activate_locking_sp(struct spdk_opal_dev *dev, const char *passwd) { diff --git a/lib/nvme/nvme_opal_internal.h b/lib/nvme/nvme_opal_internal.h index 3bd64bc15..b3fac07ef 100644 --- a/lib/nvme/nvme_opal_internal.h +++ b/lib/nvme/nvme_opal_internal.h @@ -291,7 +291,6 @@ struct spdk_opal_dev { struct spdk_opal_locking_range_info locking_ranges[SPDK_OPAL_MAX_LOCKING_RANGE]; pthread_mutex_t mutex_lock; /* some structs are accessed by current thread only */ - spdk_opal_revert_cb revert_cb_fn; void *ctx; /* user context data */ }; diff --git a/module/bdev/nvme/Makefile b/module/bdev/nvme/Makefile index 17fed6f4c..b3b24c97f 100644 --- a/module/bdev/nvme/Makefile +++ b/module/bdev/nvme/Makefile @@ -35,7 +35,7 @@ SPDK_ROOT_DIR := $(abspath $(CURDIR)/../../..) include $(SPDK_ROOT_DIR)/mk/spdk.common.mk SO_VER := 2 -SO_MINOR := 0 +SO_MINOR := 1 SO_SUFFIX := $(SO_VER).$(SO_MINOR) C_SRCS = bdev_nvme.c bdev_nvme_rpc.c nvme_rpc.c common.c bdev_ocssd.c bdev_ocssd_rpc.c diff --git a/module/bdev/nvme/common.c b/module/bdev/nvme/common.c index 8f8296ea2..d2a2c6463 100644 --- a/module/bdev/nvme/common.c +++ b/module/bdev/nvme/common.c @@ -155,11 +155,6 @@ nvme_bdev_ctrlr_destruct(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr) spdk_poller_unregister(&nvme_bdev_ctrlr->destruct_poller); if (nvme_bdev_ctrlr->opal_dev) { - if (nvme_bdev_ctrlr->opal_poller != NULL) { - spdk_poller_unregister(&nvme_bdev_ctrlr->opal_poller); - /* wait until we get the result */ - while (spdk_opal_revert_poll(nvme_bdev_ctrlr->opal_dev) == -EAGAIN); - } spdk_opal_dev_destruct(nvme_bdev_ctrlr->opal_dev); nvme_bdev_ctrlr->opal_dev = NULL; } diff --git a/module/bdev/nvme/common.h b/module/bdev/nvme/common.h index 01e576060..92b8dc052 100644 --- a/module/bdev/nvme/common.h +++ b/module/bdev/nvme/common.h @@ -92,7 +92,6 @@ struct nvme_bdev_ctrlr { struct nvme_bdev_ns **namespaces; struct spdk_opal_dev *opal_dev; - struct spdk_poller *opal_poller; struct spdk_poller *adminq_timer_poller; struct spdk_poller *destruct_poller; diff --git a/module/bdev/nvme/vbdev_opal.c b/module/bdev/nvme/vbdev_opal.c index 8ed141791..182652665 100644 --- a/module/bdev/nvme/vbdev_opal.c +++ b/module/bdev/nvme/vbdev_opal.c @@ -586,38 +586,6 @@ vbdev_opal_examine(struct spdk_bdev *bdev) spdk_bdev_module_examine_done(&opal_if); } -static int -vbdev_opal_recv_poll(void *arg) -{ - struct nvme_bdev_ctrlr *nvme_ctrlr = arg; - int rc; - - rc = spdk_opal_revert_poll(nvme_ctrlr->opal_dev); - if (rc == -EAGAIN) { - return -1; - } - - /* receive end */ - spdk_poller_unregister(&nvme_ctrlr->opal_poller); - nvme_ctrlr->opal_poller = NULL; - return 1; -} - -int -spdk_vbdev_opal_revert_tper(struct nvme_bdev_ctrlr *nvme_ctrlr, const char *password, - spdk_opal_revert_cb cb_fn, void *cb_ctx) -{ - int rc; - - rc = spdk_opal_cmd_revert_tper_async(nvme_ctrlr->opal_dev, password, cb_fn, cb_ctx); - if (rc) { - SPDK_ERRLOG("%s revert tper failure: %d\n", nvme_ctrlr->name, rc); - return rc; - } - nvme_ctrlr->opal_poller = spdk_poller_register(vbdev_opal_recv_poll, nvme_ctrlr, 50); - return 0; -} - int spdk_vbdev_opal_set_lock_state(const char *bdev_name, uint16_t user_id, const char *password, const char *lock_state) diff --git a/module/bdev/nvme/vbdev_opal.h b/module/bdev/nvme/vbdev_opal.h index 81123fc77..a509aa15a 100644 --- a/module/bdev/nvme/vbdev_opal.h +++ b/module/bdev/nvme/vbdev_opal.h @@ -45,8 +45,6 @@ struct spdk_opal_locking_range_info *spdk_vbdev_opal_get_info_from_bdev(const ch int spdk_vbdev_opal_destruct(const char *bdev_name, const char *password); -int spdk_vbdev_opal_revert_tper(struct nvme_bdev_ctrlr *nvme_ctrlr, const char *password, - spdk_opal_revert_cb cb_fn, void *cb_ctx); int spdk_vbdev_opal_enable_new_user(const char *bdev_name, const char *admin_password, uint16_t user_id, const char *user_password); diff --git a/module/bdev/nvme/vbdev_opal_rpc.c b/module/bdev/nvme/vbdev_opal_rpc.c index 48f7f70f5..c8893df76 100644 --- a/module/bdev/nvme/vbdev_opal_rpc.c +++ b/module/bdev/nvme/vbdev_opal_rpc.c @@ -134,19 +134,6 @@ static const struct spdk_json_object_decoder rpc_bdev_nvme_opal_revert_decoders[ {"password", offsetof(struct rpc_bdev_nvme_opal_revert, password), spdk_json_decode_string}, }; -static void -revert_tper_done(struct spdk_opal_dev *dev, void *data, int rc) -{ - struct nvme_bdev_ctrlr *ctrlr = data; - - if (rc != 0) { - SPDK_ERRLOG("%s revert TPer failed\n", ctrlr->name); - return; - } - - SPDK_NOTICELOG("%s revert TPer done\n", ctrlr->name); -} - static void spdk_rpc_bdev_nvme_opal_revert(struct spdk_jsonrpc_request *request, const struct spdk_json_val *params) @@ -175,8 +162,7 @@ spdk_rpc_bdev_nvme_opal_revert(struct spdk_jsonrpc_request *request, /* TODO: delete all opal vbdev before revert TPer */ - rc = spdk_vbdev_opal_revert_tper(nvme_ctrlr, req.password, revert_tper_done, - nvme_ctrlr); + rc = spdk_opal_cmd_revert_tper(nvme_ctrlr->opal_dev, req.password); if (rc) { SPDK_ERRLOG("Revert TPer failure: %d\n", rc); spdk_jsonrpc_send_error_response(request, SPDK_JSONRPC_ERROR_INTERNAL_ERROR, "Internal error"); diff --git a/test/unit/lib/bdev/bdev_ocssd.c/bdev_ocssd_ut.c b/test/unit/lib/bdev/bdev_ocssd.c/bdev_ocssd_ut.c index 2c8e72fa9..8005e1c1e 100644 --- a/test/unit/lib/bdev/bdev_ocssd.c/bdev_ocssd_ut.c +++ b/test/unit/lib/bdev/bdev_ocssd.c/bdev_ocssd_ut.c @@ -50,7 +50,6 @@ DEFINE_STUB(spdk_nvme_ctrlr_is_ocssd_ns, bool, (struct spdk_nvme_ctrlr *ctrlr, u DEFINE_STUB(spdk_nvme_ns_get_extended_sector_size, uint32_t, (struct spdk_nvme_ns *ns), 4096); DEFINE_STUB(spdk_nvme_ns_is_active, bool, (struct spdk_nvme_ns *ns), true); DEFINE_STUB_V(spdk_opal_dev_destruct, (struct spdk_opal_dev *dev)); -DEFINE_STUB(spdk_opal_revert_poll, int, (struct spdk_opal_dev *dev), 0); DEFINE_STUB_V(spdk_bdev_io_complete_nvme_status, (struct spdk_bdev_io *bdev_io, uint32_t cdw0, int sct, int sc)); DEFINE_STUB(spdk_bdev_io_get_io_channel, struct spdk_io_channel *, (struct spdk_bdev_io *bdev_io),