Commit b012ac91 authored by Tomasz Zawadzki's avatar Tomasz Zawadzki
Browse files

nvme: replace ZNS identify data in place during namespace update 



This is a workaround for #3126. The issue is intermittent
due to perf app error handling, see next patch addressing it.

During nvmf_lvol test logical volume on NVMe-oF target is resized,
which sends AER to the initiator perf application.
While handling Namespace Attribute Changed, nvme_ctrlr_update_namespaces()
destroys and constructs all active namespaces.
This behavior changed in patch:
(6fb6fda9) nvme: Fix ZNS assert for NS_ATTR_CHANGED AEN

With patch above a race condition was made more prevalent.
During the NS update, all perf threads still issue I/O based
on fields in spdk_nvme_ns - see _nvme_ns_cmd_rw().
Since NS update destroys it briefly (sets fields to 0),
some of the I/O fails.

This patch works around this issue by proposing alternative approach
to filling out ZNS fields during NS update.
Most of the nvme_ns_construct(), already replaced the fields that are
zeroed out in nvme_ns_destruct(). ZNS fields were the exception,
so remove the troublesome assumption that nsdata_zns is not allocated.
Free ZNS specific data first, then only assign it to ns if it has
been filled out.

NOTE: More holistic approach might be needed to address the core of the
race condition between ns update and I/O threads using ns fields.

Signed-off-by: default avatarTomasz Zawadzki <tomasz.zawadzki@intel.com>
Change-Id: I227fbba6b79bb1c705702a8d473a9705b83e7783
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/21144


Community-CI: Mellanox Build Bot
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Reviewed-by: default avatarJim Harris <jim.harris@samsung.com>
Reviewed-by: default avatarMateusz Kozlowski <mateusz.kozlowski@solidigm.com>
parent 1204ddff

lib/nvme/nvme_ctrlr.c

+0 −1
Original line number Diff line number Diff line
@@ -2992,7 +2992,6 @@ nvme_ctrlr_update_namespaces(struct spdk_nvme_ctrlr *ctrlr) 
	for (nsid = spdk_nvme_ctrlr_get_first_active_ns(ctrlr);

	     nsid != 0; nsid = spdk_nvme_ctrlr_get_next_active_ns(ctrlr, nsid)) {

		ns = spdk_nvme_ctrlr_get_ns(ctrlr, nsid);

		nvme_ns_destruct(ns);

		nvme_ns_construct(ns, nsid, ctrlr);

	}

}

lib/nvme/nvme_ns.c

+11 −8
Original line number Diff line number Diff line
@@ -132,6 +132,7 @@ nvme_ctrlr_identify_ns_iocs_specific(struct spdk_nvme_ns *ns) 
{

	struct nvme_completion_poll_status *status;

	struct spdk_nvme_ctrlr *ctrlr = ns->ctrlr;

	struct spdk_nvme_zns_ns_data *nsdata_zns;

	int rc;



	switch (ns->csi) {
@@ -146,38 +147,40 @@ nvme_ctrlr_identify_ns_iocs_specific(struct spdk_nvme_ns *ns) 
		assert(0);

	}



	assert(!ns->nsdata_zns);

	ns->nsdata_zns = spdk_zmalloc(sizeof(*ns->nsdata_zns), 64, NULL, SPDK_ENV_SOCKET_ID_ANY,

	nvme_ns_free_zns_specific_data(ns);



	nsdata_zns = spdk_zmalloc(sizeof(*nsdata_zns), 64, NULL, SPDK_ENV_SOCKET_ID_ANY,

				  SPDK_MALLOC_SHARE);

	if (!ns->nsdata_zns) {

	if (!nsdata_zns) {

		return -ENOMEM;

	}



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

	if (!status) {

		SPDK_ERRLOG("Failed to allocate status tracker\n");

		nvme_ns_free_zns_specific_data(ns);

		spdk_free(nsdata_zns);

		return -ENOMEM;

	}



	rc = nvme_ctrlr_cmd_identify(ctrlr, SPDK_NVME_IDENTIFY_NS_IOCS, 0, ns->id, ns->csi,

				     ns->nsdata_zns, sizeof(*ns->nsdata_zns),

				     nsdata_zns, sizeof(*nsdata_zns),

				     nvme_completion_poll_cb, status);

	if (rc != 0) {

		nvme_ns_free_zns_specific_data(ns);

		spdk_free(nsdata_zns);

		free(status);

		return rc;

	}



	if (nvme_wait_for_completion_robust_lock(ctrlr->adminq, status, &ctrlr->ctrlr_lock)) {

		SPDK_ERRLOG("Failed to retrieve Identify IOCS Specific Namespace Data Structure\n");

		nvme_ns_free_zns_specific_data(ns);

		spdk_free(nsdata_zns);

		if (!status->timed_out) {

			free(status);

		}

		return -ENXIO;

	}

	free(status);

	ns->nsdata_zns = nsdata_zns;



	return 0;

}