Commit 3056c8ac authored by Konrad Sztyber's avatar Konrad Sztyber Committed by Tomasz Zawadzki
Browse files

nvmf/tcp: delay qpair destruction 



This patch adds an extra spdk_thread_send_msg() call to destroy a qpair
to make sure that it isn't freed from the context of a socket write
callback.  Otherwise, spdk_sock_close() won't abort pending requests,
causing their completions to be exected after the qpair is freed.

Fixes #2471

Signed-off-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Change-Id: Ia510d5d754baccca1e444afdb10696ab9b58e28b
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12332


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 avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent 494eb6e5

lib/nvmf/tcp.c

+26 −5
Original line number Diff line number Diff line
@@ -311,6 +311,8 @@ struct spdk_nvmf_tcp_qpair { 
	 */

	struct spdk_poller			*timeout_poller;



	spdk_nvmf_transport_qpair_fini_cb	fini_cb_fn;

	void					*fini_cb_arg;



	TAILQ_ENTRY(spdk_nvmf_tcp_qpair)	link;

};
@@ -524,8 +526,11 @@ nvmf_tcp_dump_qpair_req_contents(struct spdk_nvmf_tcp_qpair *tqpair) 
}



static void

nvmf_tcp_qpair_destroy(struct spdk_nvmf_tcp_qpair *tqpair)

_nvmf_tcp_qpair_destroy(void *_tqpair)

{

	struct spdk_nvmf_tcp_qpair *tqpair = _tqpair;

	spdk_nvmf_transport_qpair_fini_cb cb_fn = tqpair->fini_cb_fn;

	void *cb_arg = tqpair->fini_cb_arg;

	int err = 0;



	spdk_trace_record(TRACE_TCP_QP_DESTROY, 0, 0, (uintptr_t)tqpair);
@@ -551,9 +556,24 @@ nvmf_tcp_qpair_destroy(struct spdk_nvmf_tcp_qpair *tqpair) 
	free(tqpair->reqs);

	spdk_free(tqpair->bufs);

	free(tqpair);



	if (cb_fn != NULL) {

		cb_fn(cb_arg);

	}



	SPDK_DEBUGLOG(nvmf_tcp, "Leave\n");

}



static void

nvmf_tcp_qpair_destroy(struct spdk_nvmf_tcp_qpair *tqpair)

{

	/* Delay the destruction to make sure it isn't performed from the context of a sock

	 * callback.  Otherwise, spdk_sock_close() might not abort pending requests, causing their

	 * completions to be executed after the qpair is freed.  (Note: this fixed issue #2471.)

	 */

	spdk_thread_send_msg(spdk_get_thread(), _nvmf_tcp_qpair_destroy, tqpair);

}



static void

nvmf_tcp_dump_opts(struct spdk_nvmf_transport *transport, struct spdk_json_write_ctx *w)

{
@@ -3154,12 +3174,13 @@ nvmf_tcp_close_qpair(struct spdk_nvmf_qpair *qpair, 
	SPDK_DEBUGLOG(nvmf_tcp, "Qpair: %p\n", qpair);



	tqpair = SPDK_CONTAINEROF(qpair, struct spdk_nvmf_tcp_qpair, qpair);



	assert(tqpair->fini_cb_fn == NULL);

	tqpair->fini_cb_fn = cb_fn;

	tqpair->fini_cb_arg = cb_arg;



	nvmf_tcp_qpair_set_state(tqpair, NVME_TCP_QPAIR_STATE_EXITED);

	nvmf_tcp_qpair_destroy(tqpair);



	if (cb_fn) {

		cb_fn(cb_arg);

	}

}



static int

test/unit/lib/nvmf/tcp.c/tcp_ut.c

+11 −0
Original line number Diff line number Diff line
@@ -766,6 +766,11 @@ test_nvmf_tcp_qpair_init_mem_resource(void) 
	int rc;

	struct spdk_nvmf_tcp_qpair *tqpair = NULL;

	struct spdk_nvmf_transport transport = {};

	struct spdk_thread *thread;



	thread = spdk_thread_create(NULL, NULL);

	SPDK_CU_ASSERT_FATAL(thread != NULL);

	spdk_set_thread(thread);



	tqpair = calloc(1, sizeof(*tqpair));

	tqpair->qpair.transport = &transport;
@@ -820,6 +825,12 @@ test_nvmf_tcp_qpair_init_mem_resource(void) 


	/* Free all of tqpair resource */

	nvmf_tcp_qpair_destroy(tqpair);



	spdk_thread_exit(thread);

	while (!spdk_thread_is_exited(thread)) {

		spdk_thread_poll(thread, 0, 0);

	}

	spdk_thread_destroy(thread);

}



static void