From 18450e8b82bcbf131df22318e32eb84ce4344fe1 Mon Sep 17 00:00:00 2001 From: Ziye Yang Date: Fri, 24 Apr 2020 22:43:20 +0800 Subject: [PATCH] nvme: solve the spdk_nvme_connect compatibilty issue. This is used to make spdk_nvme_connect can support the old library for compatibility. Signed-off-by: Ziye Yang Change-Id: I49d92fb473c3cbabd8e1240785b920480202eee9 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/1998 Tested-by: SPDK CI Jenkins Community-CI: Broadcom CI Reviewed-by: Changpeng Liu Reviewed-by: Shuhei Matsumoto Reviewed-by: Aleksey Marchuk Reviewed-by: Jim Harris --- CHANGELOG.md | 5 ++ include/spdk/nvme.h | 7 ++ lib/nvme/nvme.c | 121 +++++++++++++++++++++++++++- lib/nvme/nvme_ctrlr.c | 5 ++ test/unit/lib/nvme/nvme.c/nvme_ut.c | 18 ++++- 5 files changed, 148 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6192d16d2..4d2120cd1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## v20.07: (Upcoming Release) +### nvme + +Add `opts_size` in `spdk_nvme_ctrlr_opts` structure in order to solve the compatiblity issue +for different ABI version. + ### accel A new API was added `spdk_accel_get_capabilities` that allows applications to diff --git a/include/spdk/nvme.h b/include/spdk/nvme.h index 30e3f28a1..7068d3381 100644 --- a/include/spdk/nvme.h +++ b/include/spdk/nvme.h @@ -241,6 +241,13 @@ struct spdk_nvme_ctrlr_opts { * The queue depth of NVMe Admin queue. */ uint16_t admin_queue_size; + + /** + * The size of spdk_nvme_ctrlr_opts according to the caller of this library is used for ABI + * compatibility. The library uses this field to know how many fields in this + * structure are valid. And the library will populate any remaining fields with default values. + */ + size_t opts_size; }; /** diff --git a/lib/nvme/nvme.c b/lib/nvme/nvme.c index 885513b1d..9393810a6 100644 --- a/lib/nvme/nvme.c +++ b/lib/nvme/nvme.c @@ -727,6 +727,117 @@ nvme_connect_probe_cb(void *cb_ctx, const struct spdk_nvme_transport_id *trid, return true; } +static void +nvme_ctrlr_opts_init(struct spdk_nvme_ctrlr_opts *opts, + const struct spdk_nvme_ctrlr_opts *opts_user, + size_t opts_size_user) +{ + assert(opts); + assert(opts_user); + + spdk_nvme_ctrlr_get_default_ctrlr_opts(opts, opts_size_user); + +#define FIELD_OK(field) \ + offsetof(struct spdk_nvme_ctrlr_opts, field) + sizeof(opts->field) <= (opts->opts_size) + + if (FIELD_OK(num_io_queues)) { + opts->num_io_queues = opts_user->num_io_queues; + } + + if (FIELD_OK(use_cmb_sqs)) { + opts->use_cmb_sqs = opts_user->use_cmb_sqs; + } + + if (FIELD_OK(no_shn_notification)) { + opts->no_shn_notification = opts_user->no_shn_notification; + } + + if (FIELD_OK(arb_mechanism)) { + opts->arb_mechanism = opts_user->arb_mechanism; + } + + if (FIELD_OK(arbitration_burst)) { + opts->arbitration_burst = opts_user->arbitration_burst; + } + + if (FIELD_OK(low_priority_weight)) { + opts->low_priority_weight = opts_user->low_priority_weight; + } + + if (FIELD_OK(medium_priority_weight)) { + opts->medium_priority_weight = opts_user->medium_priority_weight; + } + + if (FIELD_OK(high_priority_weight)) { + opts->high_priority_weight = opts_user->high_priority_weight; + } + + if (FIELD_OK(keep_alive_timeout_ms)) { + opts->keep_alive_timeout_ms = opts_user->keep_alive_timeout_ms; + } + + if (FIELD_OK(transport_retry_count)) { + opts->transport_retry_count = opts_user->transport_retry_count; + } + + if (FIELD_OK(io_queue_size)) { + opts->io_queue_size = opts_user->io_queue_size; + } + + if (FIELD_OK(hostnqn)) { + memcpy(opts->hostnqn, opts_user->hostnqn, sizeof(opts_user->hostnqn)); + } + + if (FIELD_OK(io_queue_requests)) { + opts->io_queue_requests = opts_user->io_queue_requests; + } + + if (FIELD_OK(src_addr)) { + memcpy(opts->src_addr, opts_user->src_addr, sizeof(opts_user->src_addr)); + } + + if (FIELD_OK(src_svcid)) { + memcpy(opts->src_svcid, opts_user->src_svcid, sizeof(opts_user->src_svcid)); + } + + if (FIELD_OK(host_id)) { + memcpy(opts->host_id, opts_user->host_id, sizeof(opts_user->host_id)); + } + if (FIELD_OK(extended_host_id)) { + memcpy(opts->extended_host_id, opts_user->extended_host_id, + sizeof(opts_user->extended_host_id)); + } + + if (FIELD_OK(command_set)) { + opts->command_set = opts_user->command_set; + } + + if (FIELD_OK(admin_timeout_ms)) { + opts->admin_timeout_ms = opts_user->admin_timeout_ms; + } + + if (FIELD_OK(header_digest)) { + opts->header_digest = opts_user->header_digest; + } + + if (FIELD_OK(data_digest)) { + opts->data_digest = opts_user->data_digest; + } + + if (FIELD_OK(disable_error_logging)) { + opts->disable_error_logging = opts_user->disable_error_logging; + } + + if (FIELD_OK(transport_ack_timeout)) { + opts->transport_ack_timeout = opts_user->transport_ack_timeout; + } + + if (FIELD_OK(admin_queue_size)) { + opts->admin_queue_size = opts_user->admin_queue_size; + } +#undef FIELD_OK +} + struct spdk_nvme_ctrlr * spdk_nvme_connect(const struct spdk_nvme_transport_id *trid, const struct spdk_nvme_ctrlr_opts *opts, size_t opts_size) @@ -734,18 +845,20 @@ spdk_nvme_connect(const struct spdk_nvme_transport_id *trid, int rc; struct spdk_nvme_ctrlr *ctrlr = NULL; struct spdk_nvme_probe_ctx *probe_ctx; + struct spdk_nvme_ctrlr_opts *opts_local_p = NULL; + struct spdk_nvme_ctrlr_opts opts_local; if (trid == NULL) { SPDK_ERRLOG("No transport ID specified\n"); return NULL; } - if (opts && (opts_size != sizeof(*opts))) { - SPDK_ERRLOG("Invalid opts size\n"); - return NULL; + if (opts) { + opts_local_p = &opts_local; + nvme_ctrlr_opts_init(opts_local_p, opts, opts_size); } - probe_ctx = spdk_nvme_connect_async(trid, opts, NULL); + probe_ctx = spdk_nvme_connect_async(trid, opts_local_p, NULL); if (!probe_ctx) { SPDK_ERRLOG("Create probe context failed\n"); return NULL; diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index 601427cf8..79bd1caf6 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -90,6 +90,9 @@ nvme_ctrlr_get_cmbsz(struct spdk_nvme_ctrlr *ctrlr, union spdk_nvme_cmbsz_regist &cmbsz->raw); } +/* When the field in spdk_nvme_ctrlr_opts are changed and you change this function, please + * also update the nvme_ctrl_opts_init function in nvme_ctrlr.c + */ void spdk_nvme_ctrlr_get_default_ctrlr_opts(struct spdk_nvme_ctrlr_opts *opts, size_t opts_size) { @@ -97,6 +100,8 @@ spdk_nvme_ctrlr_get_default_ctrlr_opts(struct spdk_nvme_ctrlr_opts *opts, size_t assert(opts); + opts->opts_size = opts_size; + #define FIELD_OK(field) \ offsetof(struct spdk_nvme_ctrlr_opts, field) + sizeof(opts->field) <= opts_size diff --git a/test/unit/lib/nvme/nvme.c/nvme_ut.c b/test/unit/lib/nvme/nvme.c/nvme_ut.c index e96e9a614..cf51a14bd 100644 --- a/test/unit/lib/nvme/nvme.c/nvme_ut.c +++ b/test/unit/lib/nvme/nvme.c/nvme_ut.c @@ -76,7 +76,8 @@ nvme_ctrlr_destruct(struct spdk_nvme_ctrlr *ctrlr) void spdk_nvme_ctrlr_get_default_ctrlr_opts(struct spdk_nvme_ctrlr_opts *opts, size_t opts_size) { - memset(opts, 0, sizeof(*opts)); + memset(opts, 0, opts_size); + opts->opts_size = opts_size; } static void @@ -269,9 +270,18 @@ test_spdk_nvme_connect(void) ret_ctrlr = spdk_nvme_connect(&trid, &opts, sizeof(opts)); CU_ASSERT(ret_ctrlr == &ctrlr); CU_ASSERT_EQUAL(ret_ctrlr->opts.num_io_queues, 1); - /* opts_size must be sizeof(*opts) if opts != NULL */ - ret_ctrlr = spdk_nvme_connect(&trid, &opts, sizeof(opts) + 1); - CU_ASSERT(ret_ctrlr == NULL); + CU_ASSERT_EQUAL(ret_ctrlr->opts.opts_size, sizeof(opts)); + + /* opts_size is 0 */ + ret_ctrlr = spdk_nvme_connect(&trid, &opts, 0); + CU_ASSERT(ret_ctrlr == &ctrlr); + CU_ASSERT_EQUAL(ret_ctrlr->opts.opts_size, 0); + + /* opts_size is less than sizeof(*opts) if opts != NULL */ + ret_ctrlr = spdk_nvme_connect(&trid, &opts, 4); + CU_ASSERT(ret_ctrlr == &ctrlr); + CU_ASSERT_EQUAL(ret_ctrlr->opts.num_io_queues, 1); + CU_ASSERT_EQUAL(ret_ctrlr->opts.opts_size, 4); /* remove the attached ctrlr on the attached_list */ CU_ASSERT(spdk_nvme_detach(&ctrlr) == 0); CU_ASSERT(TAILQ_EMPTY(&g_spdk_nvme_driver->shared_attached_ctrlrs));