From 3b16c6ddc2a2c36b28083ebaa3a459d187068bf6 Mon Sep 17 00:00:00 2001 From: Ziye Yang Date: Fri, 27 Nov 2020 00:48:10 +0800 Subject: [PATCH] lib/nvmf: support ABI compatibility for spdk_nvmf_transport_opts This patch is used to support ABI compatibility related with spdk_nvmf_transport_opts structure. We add a field opts_size in spdk_nvmf_transport_opts and change the related two functions. Fixes issue: 1485 Change-Id: Ifed3dc482bbc8fb54eb7089f7a1931718682f214 Signed-off-by: Ziye Yang Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/5293 Community-CI: Broadcom CI Tested-by: SPDK CI Jenkins Reviewed-by: Changpeng Liu Reviewed-by: Shuhei Matsumoto Reviewed-by: Jacek Kalwas --- CHANGELOG.md | 4 ++ include/spdk/nvmf.h | 14 +++++- lib/nvmf/nvmf_rpc.c | 2 +- lib/nvmf/transport.c | 82 +++++++++++++++++++++++++++++---- test/unit/lib/nvmf/fc.c/fc_ut.c | 1 + 5 files changed, 90 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 449ec1cc6..053e3c697 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,10 @@ The SPDK nvmf target now supports async event notification for discovery log cha This allows the initiator to create persistent connection to discovery controller and be notified of any discovery log changes. +An `opts_size`element was added in the `spdk_nvmf_transport_opts` structure +to solve the ABI compatiblity issue between different SPDK version. And also add +`opts_size` parameter in spdk_nvmf_transport_opts_init function. + ### json A new API `spdk_jsonrpc_send_bool_response` was added to allow sending response for diff --git a/include/spdk/nvmf.h b/include/spdk/nvmf.h index 8ef78feb4..e955b1d89 100644 --- a/include/spdk/nvmf.h +++ b/include/spdk/nvmf.h @@ -88,6 +88,14 @@ struct spdk_nvmf_transport_opts { uint32_t association_timeout; const struct spdk_json_val *transport_specific; + + /** + * The size of spdk_nvmf_transport_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. + * After that, new added fields should be put after opts_size. + */ + size_t opts_size; }; struct spdk_nvmf_poll_group_stat { @@ -907,19 +915,21 @@ uint32_t spdk_nvmf_subsystem_get_max_nsid(struct spdk_nvmf_subsystem *subsystem) * * \param transport_name The transport type to create * \param opts The transport options (e.g. max_io_size) + * \param opts_size Must be set to sizeof(struct spdk_nvmf_transport_opts). * * \return bool. true if successful, false if transport type * not found. */ bool spdk_nvmf_transport_opts_init(const char *transport_name, - struct spdk_nvmf_transport_opts *opts); + struct spdk_nvmf_transport_opts *opts, size_t opts_size); /** * Create a protocol transport * * \param transport_name The transport type to create - * \param opts The transport options (e.g. max_io_size) + * \param opts The transport options (e.g. max_io_size). It should not be NULL, and opts_size + * pointed in this structure should not be zero value. * * \return new transport or NULL if create fails */ diff --git a/lib/nvmf/nvmf_rpc.c b/lib/nvmf/nvmf_rpc.c index fc8c45d05..fa3494829 100644 --- a/lib/nvmf/nvmf_rpc.c +++ b/lib/nvmf/nvmf_rpc.c @@ -1961,7 +1961,7 @@ rpc_nvmf_create_transport(struct spdk_jsonrpc_request *request, /* Initialize all the transport options (based on transport type) and decode the * parameters again to update any options passed in rpc create transport call. */ - if (!spdk_nvmf_transport_opts_init(ctx->trtype, &ctx->opts)) { + if (!spdk_nvmf_transport_opts_init(ctx->trtype, &ctx->opts, sizeof(ctx->opts))) { /* This can happen if user specifies PCIE transport type which isn't valid for * NVMe-oF. */ diff --git a/lib/nvmf/transport.c b/lib/nvmf/transport.c index b8eb25ed6..4d1fe82da 100644 --- a/lib/nvmf/transport.c +++ b/lib/nvmf/transport.c @@ -107,6 +107,41 @@ spdk_nvmf_get_transport_name(struct spdk_nvmf_transport *transport) return transport->ops->name; } +static void nvmf_transport_opts_copy(struct spdk_nvmf_transport_opts *opts, + struct spdk_nvmf_transport_opts *opts_src, + size_t opts_size) +{ + assert(opts); + assert(opts_src); + + opts->opts_size = opts_size; + +#define SET_FIELD(field) \ + if (offsetof(struct spdk_nvmf_transport_opts, field) + sizeof(opts->field) <= opts_size) { \ + opts->field = opts_src->field; \ + } \ + + SET_FIELD(max_queue_depth); + SET_FIELD(max_qpairs_per_ctrlr); + SET_FIELD(in_capsule_data_size); + SET_FIELD(max_io_size); + SET_FIELD(io_unit_size); + SET_FIELD(max_aq_depth); + SET_FIELD(buf_cache_size); + SET_FIELD(num_shared_buffers); + SET_FIELD(dif_insert_or_strip); + SET_FIELD(abort_timeout_sec); + SET_FIELD(association_timeout); + SET_FIELD(transport_specific); + + /* Do not remove this statement, you should always update this statement when you adding a new field, + * and do not forget to add the SET_FIELD statement for your added field. */ + SPDK_STATIC_ASSERT(sizeof(struct spdk_nvmf_transport_opts) == 56, "Incorrect size"); + +#undef SET_FIELD +#undef FILED_CHECK +} + struct spdk_nvmf_transport * spdk_nvmf_transport_create(const char *transport_name, struct spdk_nvmf_transport_opts *opts) { @@ -114,20 +149,32 @@ spdk_nvmf_transport_create(const char *transport_name, struct spdk_nvmf_transpor struct spdk_nvmf_transport *transport; char spdk_mempool_name[MAX_MEMPOOL_NAME_LENGTH]; int chars_written; + struct spdk_nvmf_transport_opts opts_local = {}; + + if (!opts) { + SPDK_ERRLOG("opts should not be NULL\n"); + return NULL; + } + + if (!opts->opts_size) { + SPDK_ERRLOG("The opts_size in opts structure should not be zero\n"); + return NULL; + } ops = nvmf_get_transport_ops(transport_name); if (!ops) { SPDK_ERRLOG("Transport type '%s' unavailable.\n", transport_name); return NULL; } + nvmf_transport_opts_copy(&opts_local, opts, opts->opts_size); - if (opts->max_aq_depth < SPDK_NVMF_MIN_ADMIN_MAX_SQ_SIZE) { + if (opts_local.max_aq_depth < SPDK_NVMF_MIN_ADMIN_MAX_SQ_SIZE) { SPDK_ERRLOG("max_aq_depth %u is less than minimum defined by NVMf spec, use min value\n", - opts->max_aq_depth); - opts->max_aq_depth = SPDK_NVMF_MIN_ADMIN_MAX_SQ_SIZE; + opts_local.max_aq_depth); + opts_local.max_aq_depth = SPDK_NVMF_MIN_ADMIN_MAX_SQ_SIZE; } - transport = ops->create(opts); + transport = ops->create(&opts_local); if (!transport) { SPDK_ERRLOG("Unable to create new transport of type %s\n", transport_name); return NULL; @@ -136,7 +183,8 @@ spdk_nvmf_transport_create(const char *transport_name, struct spdk_nvmf_transpor TAILQ_INIT(&transport->listeners); transport->ops = ops; - transport->opts = *opts; + transport->opts = opts_local; + chars_written = snprintf(spdk_mempool_name, MAX_MEMPOOL_NAME_LENGTH, "%s_%s_%s", "spdk_nvmf", transport_name, "data"); if (chars_written < 0) { @@ -146,8 +194,8 @@ spdk_nvmf_transport_create(const char *transport_name, struct spdk_nvmf_transpor } transport->data_buf_pool = spdk_mempool_create(spdk_mempool_name, - opts->num_shared_buffers, - opts->io_unit_size + NVMF_DATA_BUFFER_ALIGNMENT, + opts_local.num_shared_buffers, + opts_local.io_unit_size + NVMF_DATA_BUFFER_ALIGNMENT, SPDK_MEMPOOL_DEFAULT_CACHE_SIZE, SPDK_ENV_SOCKET_ID_ANY); @@ -511,9 +559,10 @@ nvmf_transport_qpair_abort_request(struct spdk_nvmf_qpair *qpair, bool spdk_nvmf_transport_opts_init(const char *transport_name, - struct spdk_nvmf_transport_opts *opts) + struct spdk_nvmf_transport_opts *opts, size_t opts_size) { const struct spdk_nvmf_transport_ops *ops; + struct spdk_nvmf_transport_opts opts_local = {}; ops = nvmf_get_transport_ops(transport_name); if (!ops) { @@ -521,8 +570,21 @@ spdk_nvmf_transport_opts_init(const char *transport_name, return false; } - opts->association_timeout = NVMF_TRANSPORT_DEFAULT_ASSOCIATION_TIMEOUT_IN_MS; - ops->opts_init(opts); + if (!opts) { + SPDK_ERRLOG("opts should not be NULL\n"); + return false; + } + + if (!opts_size) { + SPDK_ERRLOG("opts_size inside opts should not be zero value\n"); + return false; + } + + opts_local.association_timeout = NVMF_TRANSPORT_DEFAULT_ASSOCIATION_TIMEOUT_IN_MS; + ops->opts_init(&opts_local); + + nvmf_transport_opts_copy(opts, &opts_local, opts_size); + return true; } diff --git a/test/unit/lib/nvmf/fc.c/fc_ut.c b/test/unit/lib/nvmf/fc.c/fc_ut.c index c90d1e724..0f588641f 100644 --- a/test/unit/lib/nvmf/fc.c/fc_ut.c +++ b/test/unit/lib/nvmf/fc.c/fc_ut.c @@ -293,6 +293,7 @@ create_transport_test(void) ops->opts_init(&opts); g_lld_init_called = false; + opts.opts_size = sizeof(opts); g_nvmf_tprt = spdk_nvmf_transport_create("FC", &opts); SPDK_CU_ASSERT_FATAL(g_nvmf_tprt != NULL);