Commit 12854819 authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Tomasz Zawadzki
Browse files

nvme: Free I/O qpair now even if it is in poll group completion 



spdk_nvme_poll_group has followed spdk_nvme_qpair about how to
process I/O qpair deletion inside of a completion context.

spdk_nvme_qpair_process_completions() accesses qpair after
returning from nvme_transport_qpair_process_completions().

So this is reasonable.

On the other hand, if spdk_nvme_poll_group_process_completions()
can execute spdk_nvme_ctrlr_free_io_qpair() inside of a completion
context, the target qpair is ensured to be deleted after returning
from spdk_nvme_ctrlr_free_io_qpair(). Then the target qpair is
not accessed anymore in spdk_nvme_poll_group_process_completions().

Remove two variables, in_completion_context and num_qpairs_to_delete,
of spdk_nvme_transport_poll_group and the related code.

This change is really necessary to support the following case.

In the NVMe bdev module, a nvme_qpair has a qpair and a poll_group
channel. disconnected_qpair_cb calls spdk_nvme_ctrlr_free_io_qpair()
for the qpair and spdk_put_io_channel() to the poll_group_channel.
spdk_nvme_ctrlr_free_io_qpair() is executed after unwinding stack
but spdk_put_io_channel() is executed now. The callback to
spdk_put_io_channel() calls spdk_nvme_poll_group_destroy(). However,
spdk_nvme_ctrlr_free_io_qpair() is not executed. Hence
spdk_nvme_poll_group_destroy() fails.

Update the corresponding stub in unit test together.

Signed-off-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Change-Id: Icd1f1daf049c6c7ffb28790fe87989a1060f8952
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11496


Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent c113e4cd

lib/nvme/nvme_ctrlr.c

+0 −7
Original line number Diff line number Diff line
@@ -619,13 +619,6 @@ spdk_nvme_ctrlr_free_io_qpair(struct spdk_nvme_qpair *qpair) 
		return 0;

	}



	if (qpair->poll_group && qpair->poll_group->in_completion_context) {

		/* Same as above, but in a poll group. */

		qpair->poll_group->num_qpairs_to_delete++;

		qpair->delete_after_completion_context = 1;

		return 0;

	}



	nvme_transport_ctrlr_disconnect_qpair(ctrlr, qpair);



	if (qpair->poll_group) {

lib/nvme/nvme_internal.h

+0 −2
Original line number Diff line number Diff line
@@ -502,8 +502,6 @@ struct spdk_nvme_transport_poll_group { 
	STAILQ_HEAD(, spdk_nvme_qpair)			connected_qpairs;

	STAILQ_HEAD(, spdk_nvme_qpair)			disconnected_qpairs;

	STAILQ_ENTRY(spdk_nvme_transport_poll_group)	link;

	bool						in_completion_context;

	uint64_t					num_qpairs_to_delete;

};



struct spdk_nvme_ns {

lib/nvme/nvme_transport.c

+1 −32
Original line number Diff line number Diff line
@@ -707,39 +707,8 @@ int64_t 
nvme_transport_poll_group_process_completions(struct spdk_nvme_transport_poll_group *tgroup,

		uint32_t completions_per_qpair, spdk_nvme_disconnected_qpair_cb disconnected_qpair_cb)

{

	struct spdk_nvme_qpair *qpair;

	int64_t rc;



	tgroup->in_completion_context = true;

	rc = tgroup->transport->ops.poll_group_process_completions(tgroup, completions_per_qpair,

	return tgroup->transport->ops.poll_group_process_completions(tgroup, completions_per_qpair,

			disconnected_qpair_cb);

	tgroup->in_completion_context = false;



	if (spdk_unlikely(tgroup->num_qpairs_to_delete > 0)) {

		/* deleted qpairs are more likely to be in the disconnected qpairs list. */

		STAILQ_FOREACH(qpair, &tgroup->disconnected_qpairs, poll_group_stailq) {

			if (spdk_unlikely(qpair->delete_after_completion_context)) {

				spdk_nvme_ctrlr_free_io_qpair(qpair);

				if (--tgroup->num_qpairs_to_delete == 0) {

					return rc;

				}

			}

		}



		STAILQ_FOREACH(qpair, &tgroup->connected_qpairs, poll_group_stailq) {

			if (spdk_unlikely(qpair->delete_after_completion_context)) {

				spdk_nvme_ctrlr_free_io_qpair(qpair);

				if (--tgroup->num_qpairs_to_delete == 0) {

					return rc;

				}

			}

		}

		/* Just in case. */

		SPDK_DEBUGLOG(nvme, "Mismatch between qpairs to delete and poll group number.\n");

		tgroup->num_qpairs_to_delete = 0;

	}



	return rc;

}



int

test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c

+0 −32
Original line number Diff line number Diff line
@@ -261,8 +261,6 @@ struct spdk_nvme_poll_group { 
	struct spdk_nvme_accel_fn_table	accel_fn_table;

	TAILQ_HEAD(, spdk_nvme_qpair)	connected_qpairs;

	TAILQ_HEAD(, spdk_nvme_qpair)	disconnected_qpairs;

	bool				in_completion_context;

	uint64_t			num_qpairs_to_delete;

};



struct spdk_nvme_probe_ctx {
@@ -746,12 +744,6 @@ spdk_nvme_ctrlr_free_io_qpair(struct spdk_nvme_qpair *qpair) 
		return 0;

	}



	if (qpair->poll_group && qpair->poll_group->in_completion_context) {

		qpair->poll_group->num_qpairs_to_delete++;

		qpair->delete_after_completion_context = true;

		return 0;

	}



	spdk_nvme_ctrlr_disconnect_io_qpair(qpair);



	if (qpair->poll_group != NULL) {
@@ -1136,8 +1128,6 @@ spdk_nvme_poll_group_process_completions(struct spdk_nvme_poll_group *group, 
		return -EINVAL;

	}



	group->in_completion_context = true;



	TAILQ_FOREACH_SAFE(qpair, &group->disconnected_qpairs, poll_group_tailq, tmp_qpair) {

		disconnected_qpair_cb(qpair, group->ctx);

	}
@@ -1158,28 +1148,6 @@ spdk_nvme_poll_group_process_completions(struct spdk_nvme_poll_group *group, 
		}

	}



	group->in_completion_context = false;



	if (group->num_qpairs_to_delete > 0) {

		TAILQ_FOREACH_SAFE(qpair, &group->disconnected_qpairs, poll_group_tailq, tmp_qpair) {

			if (qpair->delete_after_completion_context) {

				spdk_nvme_ctrlr_free_io_qpair(qpair);

				CU_ASSERT(group->num_qpairs_to_delete > 0);

				group->num_qpairs_to_delete--;

			}

		}



		TAILQ_FOREACH_SAFE(qpair, &group->connected_qpairs, poll_group_tailq, tmp_qpair) {

			if (qpair->delete_after_completion_context) {

				spdk_nvme_ctrlr_free_io_qpair(qpair);

				CU_ASSERT(group->num_qpairs_to_delete > 0);

				group->num_qpairs_to_delete--;

			}

		}



		CU_ASSERT(group->num_qpairs_to_delete == 0);

	}



	return error_reason ? error_reason : num_completions;

}