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

nvme: simplify wait for completion interface 



All usages pass the controller’s mutex and use the same timeout value
from the controller’s options. The interface can therefore be simplified
to improve readability and reduce the chances of misuse.

Put adminq into the function name to make it clear it is not for general use.

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


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

lib/nvme/nvme.c

+6 −10
Original line number Diff line number Diff line
@@ -297,13 +297,11 @@ error: 
}



/**

 * Poll qpair for completions until a command completes.

 * Poll admin qpair for completions until a command completes.

 *

 * \param qpair queue to poll

 * \param ctrlr ctrlr with adminq 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

 * \param timeout_in_usecs optional timeout

 *

 * \return 0 if command completed without error,

 * -EIO if command completed with error,
@@ -313,12 +311,10 @@ error: 
 *  and status as the callback argument.

 */

int

nvme_wait_for_completion_robust_lock_timeout(

	struct spdk_nvme_qpair *qpair,

	struct nvme_completion_poll_status *status,

	pthread_mutex_t *robust_mutex,

	uint64_t timeout_in_usecs)

nvme_wait_for_adminq_completion(struct spdk_nvme_ctrlr *ctrlr,

				struct nvme_completion_poll_status *status)

{

	uint64_t timeout_in_usecs = ctrlr->opts.admin_timeout_ms * 1000;

	int rc;



	if (timeout_in_usecs) {
@@ -330,7 +326,7 @@ nvme_wait_for_completion_robust_lock_timeout( 


	status->cpl.status_raw = 0;

	do {

		rc = nvme_wait_for_completion_robust_lock_timeout_poll(qpair, status, robust_mutex);

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

	} while (rc == -EAGAIN);



	return rc;

lib/nvme/nvme_ctrlr.c

+13 −26
Original line number Diff line number Diff line
@@ -801,8 +801,7 @@ nvme_ctrlr_update_ana_log_page(struct spdk_nvme_ctrlr *ctrlr) 
		return rc;

	}



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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

		if (!status->timed_out) {

			free(status);

		}
@@ -968,8 +967,7 @@ nvme_ctrlr_set_arbitration_feature(struct spdk_nvme_ctrlr *ctrlr) 
		return;

	}



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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

		NVME_CTRLR_ERRLOG(ctrlr, "Timeout to set arbitration feature\n");

	}
@@ -2598,8 +2596,7 @@ nvme_ctrlr_identify_active_ns(struct spdk_nvme_ctrlr *ctrlr) 
	}



	nvme_ctrlr_identify_active_ns_async(ctx);

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

			ctrlr->opts.admin_timeout_ms * 1000);

	rc = nvme_wait_for_adminq_completion(ctrlr, &ctx->status);

	if (rc || ctx->state == NVME_ACTIVE_NS_STATE_ERROR) {

		if (!ctx->status.timed_out) {

			nvme_active_ns_ctx_destroy(ctx);
@@ -3290,8 +3287,7 @@ nvme_ctrlr_clear_changed_ns_log(struct spdk_nvme_ctrlr_aer_completion *async_eve 
		goto free_buffer;

	}



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

			ctrlr->opts.admin_timeout_ms * 1000);

	rc = nvme_wait_for_adminq_completion(ctrlr, status);

	if (!status->timed_out) {

		free(status);

	}
@@ -4933,8 +4929,7 @@ spdk_nvme_ctrlr_attach_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid, 
		free(status);

		return res;

	}

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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

		if (!status->timed_out) {

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

		return res;

	}

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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

		if (!status->timed_out) {

			free(status);
@@ -5011,8 +5005,7 @@ 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_timeout(ctrlr->adminq, status, &ctrlr->ctrlr_lock,

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

		if (!status->timed_out) {

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

		return res;

	}

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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

		if (!status->timed_out) {

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

		return res;

	}

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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

		if (!status->timed_out) {

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

		}



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

				ctrlr->opts.admin_timeout_ms * 1000)) {

		if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

			if (!status->timed_out) {

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

	}



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

			ctrlr->opts.admin_timeout_ms * 1000);

	res = nvme_wait_for_adminq_completion(ctrlr, status);



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

		return res;

	}

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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

		if (!status->timed_out) {

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

		return res;

	}

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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

		if (!status->timed_out) {

			free(status);

lib/nvme/nvme_fabric.c

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

	}



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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

		if (!status->timed_out) {

			free(status);

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

	}



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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

		if (!status->timed_out) {

			free(status);

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

	}



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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

		if (!status->timed_out) {

			free(status);

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

	}



	if (nvme_wait_for_completion_robust_lock_timeout(discovery_ctrlr->adminq, status,

			&discovery_ctrlr->ctrlr_lock, discovery_ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(discovery_ctrlr, status)) {

		SPDK_ERRLOG("nvme_identify_controller failed!\n");

		nvme_ctrlr_destruct(discovery_ctrlr);

		if (!status->timed_out) {

lib/nvme/nvme_internal.h

+2 −4
Original line number Diff line number Diff line
@@ -1299,10 +1299,8 @@ 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_robust_lock_timeout(struct spdk_nvme_qpair *qpair,

		struct nvme_completion_poll_status *status,

		pthread_mutex_t *robust_mutex,

		uint64_t timeout_in_usecs);

int	nvme_wait_for_adminq_completion(struct spdk_nvme_ctrlr *ctrlr,

					struct nvme_completion_poll_status *status);

int	nvme_wait_for_completion_robust_lock_timeout_poll(struct spdk_nvme_qpair *qpair,

		struct nvme_completion_poll_status *status,

		pthread_mutex_t *robust_mutex);

lib/nvme/nvme_ns.c

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

	}



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

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

	if (nvme_wait_for_adminq_completion(ns->ctrlr, status)) {

		if (!status->timed_out) {

			free(status);

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

	}



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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

		spdk_free(nsdata_zns);

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

	}



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

			ctrlr->opts.admin_timeout_ms * 1000)) {

	if (nvme_wait_for_adminq_completion(ctrlr, status)) {

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

		spdk_free(nsdata_nvm);

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

	}



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

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

	rc = nvme_wait_for_adminq_completion(ns->ctrlr, status);

	if (rc != 0) {

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

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