Commit fb4398ef authored by Alexey Marchuk's avatar Alexey Marchuk Committed by Tomasz Zawadzki
Browse files

nvmf: Correct the error path of transport creation. 



An error might occur after succesful transport creation
when the new transport is added to nvmf poll groups, e.g.
in nvmf_transport_poll_group_create. In that case
transport is not detroyed and poll groups are not fully
functional. To correct this behaviour, destroy transport if
spdk_nvmf_tgt_add_transport fails. Also update nvmf_tgt
initialization step to check that all poll groups were
created.

Change-Id: I116e6944729d846c1755c2844c77825f65db8c12
Signed-off-by: default avatarAlexey Marchuk <alexeymar@mellanox.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9255


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarJacek Kalwas <jacek.kalwas@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarTomasz Zawadzki <tomasz.zawadzki@intel.com>
parent d409971b

lib/nvmf/nvmf.c

+8 −1
Original line number Diff line number Diff line
@@ -116,6 +116,7 @@ nvmf_tgt_create_poll_group(void *io_device, void *ctx_buf) 
	struct spdk_nvmf_transport *transport;

	struct spdk_thread *thread = spdk_get_thread();

	uint32_t sid;

	int rc;



	SPDK_DTRACE_PROBE1(nvmf_create_poll_group, spdk_thread_get_id(thread));
@@ -123,7 +124,10 @@ nvmf_tgt_create_poll_group(void *io_device, void *ctx_buf) 
	TAILQ_INIT(&group->qpairs);



	TAILQ_FOREACH(transport, &tgt->transports, link) {

		nvmf_poll_group_add_transport(group, transport);

		rc = nvmf_poll_group_add_transport(group, transport);

		if (rc != 0) {

			return rc;

		}

	}



	group->num_sgroups = tgt->max_subsystems;
@@ -714,6 +718,9 @@ _nvmf_tgt_add_transport_done(struct spdk_io_channel_iter *i, int status) 
{

	struct spdk_nvmf_tgt_add_transport_ctx *ctx = spdk_io_channel_iter_get_ctx(i);



	if (status) {

		TAILQ_REMOVE(&ctx->tgt->transports, ctx->transport, link);

	}

	ctx->cb_fn(ctx->cb_arg, status);



	free(ctx);

lib/nvmf/nvmf_rpc.c

+18 −11
Original line number Diff line number Diff line
@@ -1787,6 +1787,8 @@ struct nvmf_rpc_create_transport_ctx { 
	char				*tgt_name;

	struct spdk_nvmf_transport_opts	opts;

	struct spdk_jsonrpc_request	*request;

	struct spdk_nvmf_transport	*transport;

	int				status;

};



/**
@@ -1890,23 +1892,29 @@ nvmf_rpc_create_transport_ctx_free(struct nvmf_rpc_create_transport_ctx *ctx) 
}



static void

nvmf_rpc_tgt_add_transport_done(void *cb_arg, int status)

nvmf_rpc_transport_destroy_done_cb(void *cb_arg)

{

	struct nvmf_rpc_create_transport_ctx *ctx = cb_arg;

	struct spdk_jsonrpc_request *request;



	request = ctx->request;

	spdk_jsonrpc_send_error_response_fmt(ctx->request, SPDK_JSONRPC_ERROR_INTERNAL_ERROR,

					     "Failed to add transport to tgt.(%d)", ctx->status);

	nvmf_rpc_create_transport_ctx_free(ctx);

}



static void

nvmf_rpc_tgt_add_transport_done(void *cb_arg, int status)

{

	struct nvmf_rpc_create_transport_ctx *ctx = cb_arg;



	if (status) {

		SPDK_ERRLOG("Failed to add transport to tgt.(%d)\n", status);

		spdk_jsonrpc_send_error_response_fmt(request, SPDK_JSONRPC_ERROR_INTERNAL_ERROR,

						     "Failed to add transport to tgt.(%d)",

						     status);

		ctx->status = status;

		spdk_nvmf_transport_destroy(ctx->transport, nvmf_rpc_transport_destroy_done_cb, ctx);

		return;

	}



	spdk_jsonrpc_send_bool_response(request, true);

	spdk_jsonrpc_send_bool_response(ctx->request, true);

	nvmf_rpc_create_transport_ctx_free(ctx);

}



static void
@@ -1915,7 +1923,6 @@ rpc_nvmf_create_transport(struct spdk_jsonrpc_request *request, 
{

	struct nvmf_rpc_create_transport_ctx *ctx;

	enum spdk_nvme_transport_type trtype;

	struct spdk_nvmf_transport *transport;

	struct spdk_nvmf_tgt *tgt;



	ctx = calloc(1, sizeof(*ctx));
@@ -1985,9 +1992,9 @@ rpc_nvmf_create_transport(struct spdk_jsonrpc_request *request, 
	/* Transport can parse additional params themselves */

	ctx->opts.transport_specific = params;



	transport = spdk_nvmf_transport_create(ctx->trtype, &ctx->opts);

	ctx->transport = spdk_nvmf_transport_create(ctx->trtype, &ctx->opts);



	if (!transport) {

	if (!ctx->transport) {

		SPDK_ERRLOG("Transport type '%s' create failed\n", ctx->trtype);

		spdk_jsonrpc_send_error_response_fmt(request, SPDK_JSONRPC_ERROR_INTERNAL_ERROR,

						     "Transport type '%s' create failed", ctx->trtype);
@@ -1997,7 +2004,7 @@ rpc_nvmf_create_transport(struct spdk_jsonrpc_request *request, 


	/* add transport to target */

	ctx->request = request;

	spdk_nvmf_tgt_add_transport(tgt, transport, nvmf_rpc_tgt_add_transport_done, ctx);

	spdk_nvmf_tgt_add_transport(tgt, ctx->transport, nvmf_rpc_tgt_add_transport_done, ctx);

}

SPDK_RPC_REGISTER("nvmf_create_transport", rpc_nvmf_create_transport, SPDK_RPC_RUNTIME)

module/event/subsystems/nvmf/nvmf_tgt.c

+12 −1
Original line number Diff line number Diff line
@@ -3,6 +3,7 @@ 
 *

 *   Copyright (c) Intel Corporation.

 *   All rights reserved.

 *   Copyright (c) 2021 NVIDIA CORPORATION & AFFILIATES. All rights reserved.

 *

 *   Redistribution and use in source and binary forms, with or without

 *   modification, are permitted provided that the following conditions
@@ -166,12 +167,22 @@ nvmf_tgt_create_poll_group_done(void *ctx) 
{

	struct nvmf_tgt_poll_group *pg = ctx;



	assert(pg);



	if (!pg->group) {

		SPDK_ERRLOG("Failed to create nvmf poll group\n");

		/* Change the state to error but wait for completions from all other threads */

		g_tgt_state = NVMF_TGT_ERROR;

	}



	TAILQ_INSERT_TAIL(&g_poll_groups, pg, link);



	assert(g_num_poll_groups < nvmf_get_cpuset_count());



	if (++g_num_poll_groups == nvmf_get_cpuset_count()) {

		if (g_tgt_state != NVMF_TGT_ERROR) {

			g_tgt_state = NVMF_TGT_INIT_START_SUBSYSTEMS;

		}

		nvmf_tgt_advance_state();

	}

}

test/unit/lib/nvmf/nvmf.c/nvmf_ut.c

+3 −0
Original line number Diff line number Diff line
@@ -169,6 +169,7 @@ test_nvmf_tgt_create_poll_group(void) 
	struct spdk_nvmf_ns		ns = {};

	struct spdk_bdev		bdev = {};

	struct spdk_io_channel		ch = {};

	struct spdk_nvmf_transport_poll_group transport_pg = {};



	thread = spdk_thread_create(NULL, NULL);

	SPDK_CU_ASSERT_FATAL(thread != NULL);
@@ -202,7 +203,9 @@ test_nvmf_tgt_create_poll_group(void) 
	transport.tgt = &tgt;

	TAILQ_INSERT_TAIL(&tgt.transports, &transport, link);



	MOCK_SET(nvmf_transport_poll_group_create, &transport_pg);

	rc = nvmf_tgt_create_poll_group((void *)&tgt, (void *)&group);

	MOCK_SET(nvmf_transport_poll_group_create, NULL);

	CU_ASSERT(rc == 0);

	CU_ASSERT(group.num_sgroups == 1);

	CU_ASSERT(group.sgroups != NULL);