Commit b331db74 authored by Jacek Kalwas's avatar Jacek Kalwas Committed by Tomasz Zawadzki
Browse files

nvme: respect admin timeout when waiting for command completion 



Most usages of command completion waiting do not track timeouts.
As a result, the admin timeout setting is not respected.

Since the controller mutex is held during these flow, it not only blocks
the execution thread but also other threads waiting for the lock.

Remove functions w/o timeout tracking as these are no longer in use.

Change-Id: Iac4a4d1e2c5940859b784d7e332916a903a8a308
Signed-off-by: default avatarJacek Kalwas <jacek.kalwas@nutanix.com>
Reviewed-on: https://review.spdk.io/c/spdk/spdk/+/26494


Reviewed-by: default avatarTomasz Zawadzki <tomasz@tzawadzki.com>
Tested-by: default avatarSPDK Automated Test System <spdkbot@gmail.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarJim Harris <jim.harris@nvidia.com>
Reviewed-by: default avatarBen Walker <ben@nvidia.com>
parent e953ea6d

lib/nvme/nvme.c

+0 −31
Original line number Diff line number Diff line
@@ -336,37 +336,6 @@ nvme_wait_for_completion_robust_lock_timeout( 
	return rc;

}



/**

 * Poll qpair for completions until a command completes.

 *

 * \param qpair queue to poll

 * \param status completion status. The user must fill this structure with zeroes before calling

 * this function

 * \param robust_mutex optional robust mutex to lock while polling qpair

 *

 * \return 0 if command completed without error,

 * -EIO if command completed with error,

 * -ECANCELED if command is not completed due to transport/device error

 *

 * The command to wait upon must be submitted with nvme_completion_poll_cb as the callback

 * and status as the callback argument.

 */

int

nvme_wait_for_completion_robust_lock(

	struct spdk_nvme_qpair *qpair,

	struct nvme_completion_poll_status *status,

	pthread_mutex_t *robust_mutex)

{

	return nvme_wait_for_completion_robust_lock_timeout(qpair, status, robust_mutex, 0);

}



int

nvme_wait_for_completion(struct spdk_nvme_qpair *qpair,

			 struct nvme_completion_poll_status *status)

{

	return nvme_wait_for_completion_robust_lock_timeout(qpair, status, NULL, 0);

}



/**

 * Poll qpair for completions until a command completes.

 *

lib/nvme/nvme_ctrlr.c

+18 −9
Original line number Diff line number Diff line
@@ -4933,7 +4933,8 @@ spdk_nvme_ctrlr_attach_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid, 
		free(status);

		return res;

	}

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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

		NVME_CTRLR_ERRLOG(ctrlr, "spdk_nvme_ctrlr_attach_ns failed!\n");

		if (!status->timed_out) {

			free(status);
@@ -4979,7 +4980,8 @@ spdk_nvme_ctrlr_detach_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid, 
		free(status);

		return res;

	}

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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

		NVME_CTRLR_ERRLOG(ctrlr, "spdk_nvme_ctrlr_detach_ns failed!\n");

		if (!status->timed_out) {

			free(status);
@@ -5009,7 +5011,8 @@ spdk_nvme_ctrlr_create_ns(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_ns_dat 
		free(status);

		return 0;

	}

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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

		NVME_CTRLR_ERRLOG(ctrlr, "spdk_nvme_ctrlr_create_ns failed!\n");

		if (!status->timed_out) {

			free(status);
@@ -5047,7 +5050,8 @@ spdk_nvme_ctrlr_delete_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid) 
		free(status);

		return res;

	}

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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

		NVME_CTRLR_ERRLOG(ctrlr, "spdk_nvme_ctrlr_delete_ns failed!\n");

		if (!status->timed_out) {

			free(status);
@@ -5078,7 +5082,8 @@ spdk_nvme_ctrlr_format(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid, 
		free(status);

		return res;

	}

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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

		NVME_CTRLR_ERRLOG(ctrlr, "spdk_nvme_ctrlr_format failed!\n");

		if (!status->timed_out) {

			free(status);
@@ -5143,7 +5148,8 @@ spdk_nvme_ctrlr_update_firmware(struct spdk_nvme_ctrlr *ctrlr, void *payload, ui 
			return res;

		}



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

		if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

				ctrlr->opts.admin_timeout_ms * 1000)) {

			NVME_CTRLR_ERRLOG(ctrlr, "spdk_nvme_ctrlr_fw_image_download failed!\n");

			if (!status->timed_out) {

				free(status);
@@ -5168,7 +5174,8 @@ spdk_nvme_ctrlr_update_firmware(struct spdk_nvme_ctrlr *ctrlr, void *payload, ui 
		return res;

	}



	res = nvme_wait_for_completion_robust_lock(ctrlr->adminq, status, &ctrlr->ctrlr_lock);

	res = nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000);



	memcpy(completion_status, &status->cpl.status, sizeof(struct spdk_nvme_status));
@@ -5513,7 +5520,8 @@ spdk_nvme_ctrlr_security_receive(struct spdk_nvme_ctrlr *ctrlr, uint8_t secp, 
		free(status);

		return res;

	}

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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

		NVME_CTRLR_ERRLOG(ctrlr, "spdk_nvme_ctrlr_cmd_security_receive failed!\n");

		if (!status->timed_out) {

			free(status);
@@ -5545,7 +5553,8 @@ spdk_nvme_ctrlr_security_send(struct spdk_nvme_ctrlr *ctrlr, uint8_t secp, 
		free(status);

		return res;

	}

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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

		NVME_CTRLR_ERRLOG(ctrlr, "spdk_nvme_ctrlr_cmd_security_send failed!\n");

		if (!status->timed_out) {

			free(status);

lib/nvme/nvme_fabric.c

+7 −4
Original line number Diff line number Diff line
@@ -59,7 +59,8 @@ nvme_fabric_prop_set_cmd_sync(struct spdk_nvme_ctrlr *ctrlr, 
		return rc;

	}



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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

		if (!status->timed_out) {

			free(status);

		}
@@ -145,7 +146,8 @@ nvme_fabric_prop_get_cmd_sync(struct spdk_nvme_ctrlr *ctrlr, 
		return rc;

	}



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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

		if (!status->timed_out) {

			free(status);

		}
@@ -364,7 +366,7 @@ nvme_fabric_get_discovery_log_page(struct spdk_nvme_ctrlr *ctrlr, 
		return -1;

	}



	if (nvme_wait_for_completion(ctrlr->adminq, status)) {

	if (nvme_wait_for_completion_timeout(ctrlr->adminq, status, ctrlr->opts.admin_timeout_ms * 1000)) {

		if (!status->timed_out) {

			free(status);

		}
@@ -425,7 +427,8 @@ nvme_fabric_ctrlr_scan(struct spdk_nvme_probe_ctx *probe_ctx, 
		return rc;

	}



	if (nvme_wait_for_completion(discovery_ctrlr->adminq, status)) {

	if (nvme_wait_for_completion_timeout(discovery_ctrlr->adminq, status,

					     discovery_ctrlr->opts.admin_timeout_ms * 1000)) {

		SPDK_ERRLOG("nvme_identify_controller failed!\n");

		nvme_ctrlr_destruct(discovery_ctrlr);

		if (!status->timed_out) {

lib/nvme/nvme_internal.h

+0 −5
Original line number Diff line number Diff line
@@ -1299,11 +1299,6 @@ int nvme_ctrlr_cmd_sanitize(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid, 
				struct spdk_nvme_sanitize *sanitize, uint32_t cdw11,

				spdk_nvme_cmd_cb cb_fn, void *cb_arg);

void	nvme_completion_poll_cb(void *arg, const struct spdk_nvme_cpl *cpl);

int	nvme_wait_for_completion(struct spdk_nvme_qpair *qpair,

				 struct nvme_completion_poll_status *status);

int	nvme_wait_for_completion_robust_lock(struct spdk_nvme_qpair *qpair,

		struct nvme_completion_poll_status *status,

		pthread_mutex_t *robust_mutex);

int	nvme_wait_for_completion_timeout(struct spdk_nvme_qpair *qpair,

		struct nvme_completion_poll_status *status,

		uint64_t timeout_in_usecs);

lib/nvme/nvme_ns.c

+8 −5
Original line number Diff line number Diff line
@@ -122,8 +122,8 @@ nvme_ctrlr_identify_ns(struct spdk_nvme_ns *ns) 
		return rc;

	}



	if (nvme_wait_for_completion_robust_lock(ns->ctrlr->adminq, status,

			&ns->ctrlr->ctrlr_lock)) {

	if (nvme_wait_for_completion_robust_lock_timeout(ns->ctrlr->adminq, status, &ns->ctrlr->ctrlr_lock,

			ns->ctrlr->opts.admin_timeout_ms * 1000)) {

		if (!status->timed_out) {

			free(status);

		}
@@ -171,7 +171,8 @@ nvme_ctrlr_identify_ns_zns_specific(struct spdk_nvme_ns *ns) 
		return rc;

	}



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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

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

		spdk_free(nsdata_zns);

		if (!status->timed_out) {
@@ -217,7 +218,8 @@ nvme_ctrlr_identify_ns_nvm_specific(struct spdk_nvme_ns *ns) 
		return rc;

	}



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

	if (nvme_wait_for_completion_robust_lock_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

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

		spdk_free(nsdata_nvm);

		if (!status->timed_out) {
@@ -284,7 +286,8 @@ nvme_ctrlr_identify_id_desc(struct spdk_nvme_ns *ns) 
		return rc;

	}



	rc = nvme_wait_for_completion_robust_lock(ns->ctrlr->adminq, status, &ns->ctrlr->ctrlr_lock);

	rc = nvme_wait_for_completion_robust_lock_timeout(ns->ctrlr->adminq, status, &ns->ctrlr->ctrlr_lock,

			ns->ctrlr->opts.admin_timeout_ms * 1000);

	if (rc != 0) {

		SPDK_WARNLOG("Failed to retrieve NS ID Descriptor List\n");

		memset(ns->id_desc_list, 0, sizeof(ns->id_desc_list));