Commit 292c9c42 authored by Jim Harris's avatar Jim Harris
Browse files

scsi: simplify lun task execution 



An old prototype SPDK AHCI driver would return
TASK_SET_FULL if all NCQ slots were full on a given
disk.  This would kick the SCSI task back to the LUN
to be retried later.  Since then, we have pushed
responsibility onto the bdev modules themselves
to handle this kind of queueing/retry logic.

Removing this logic allows us to make some additional
changes that enable tasks to get completed inline without
an extra event callback to handle completion.  We also
no longer need to worry about checking if pending tasks
need to be executed in the complete_task() routine, since
the execute() routine will now always exhaust the pending_task
list.

Signed-off-by: default avatarJim Harris <james.r.harris@intel.com>
Change-Id: If2dc3ab017e0dbc225c8f627e1f87c5a8e9b1e3e
parent f3c45ea0

lib/scsi/lun.c

+6 −13
Original line number Diff line number Diff line
@@ -40,13 +40,12 @@ void 
spdk_scsi_lun_complete_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task)

{

	if (lun) {

		if (task->type == SPDK_SCSI_TASK_TYPE_CMD) {

			TAILQ_REMOVE(&lun->tasks, task, scsi_link);

		}

		spdk_trace_record(TRACE_SCSI_TASK_DONE, lun->dev->id, 0, (uintptr_t)task, 0);

	}

	spdk_event_call(task->cb_event);



	if (lun && !TAILQ_EMPTY(&lun->pending_tasks)) {

		spdk_scsi_lun_execute_tasks(lun);

	}

}



void
@@ -65,7 +64,6 @@ spdk_scsi_lun_clear_all(struct spdk_scsi_lun *lun) 
	 */



	TAILQ_FOREACH_SAFE(task, &lun->tasks, scsi_link, task_tmp) {

		TAILQ_REMOVE(&lun->tasks, task, scsi_link);

		spdk_scsi_task_set_status(task, SPDK_SCSI_STATUS_CHECK_CONDITION,

					  SPDK_SCSI_SENSE_ABORTED_COMMAND,

					  SPDK_SCSI_ASC_NO_ADDITIONAL_SENSE,
@@ -75,6 +73,7 @@ spdk_scsi_lun_clear_all(struct spdk_scsi_lun *lun) 


	TAILQ_FOREACH_SAFE(task, &lun->pending_tasks, scsi_link, task_tmp) {

		TAILQ_REMOVE(&lun->pending_tasks, task, scsi_link);

		TAILQ_INSERT_TAIL(&lun->tasks, task, scsi_link);

		spdk_scsi_task_set_status(task, SPDK_SCSI_STATUS_CHECK_CONDITION,

					  SPDK_SCSI_SENSE_ABORTED_COMMAND,

					  SPDK_SCSI_ASC_NO_ADDITIONAL_SENSE,
@@ -239,18 +238,12 @@ spdk_scsi_lun_execute_tasks(struct spdk_scsi_lun *lun) 
		task->status = SPDK_SCSI_STATUS_GOOD;

		task->ch = lun->io_channel;

		spdk_trace_record(TRACE_SCSI_TASK_START, lun->dev->id, task->length, (uintptr_t)task, 0);

		rc = spdk_bdev_scsi_execute(lun->bdev, task);



		/* Task is removed from the pending list if it gets the slot. */

		if (task->status == SPDK_SCSI_STATUS_TASK_SET_FULL) {

			break;

		}



		TAILQ_REMOVE(&lun->pending_tasks, task, scsi_link);

		TAILQ_INSERT_TAIL(&lun->tasks, task, scsi_link);

		rc = spdk_bdev_scsi_execute(lun->bdev, task);



		switch (rc) {

		case SPDK_SCSI_TASK_PENDING:

			TAILQ_INSERT_TAIL(&lun->tasks, task, scsi_link);

			break;



		case SPDK_SCSI_TASK_COMPLETE:

lib/scsi/scsi_bdev.c

+0 −3
Original line number Diff line number Diff line
@@ -1253,9 +1253,6 @@ spdk_bdev_scsi_task_complete(void *arg1, void *arg2) 
			}

			spdk_scsi_task_set_status(task, sc, sk, asc, ascq);

		}



		/* command completed. remove from outstanding task list */

		TAILQ_REMOVE(&task->lun->tasks, task, scsi_link);

	} else if (task->type == SPDK_SCSI_TASK_TYPE_MANAGE) {

		if (status == SPDK_BDEV_IO_STATUS_SUCCESS)

			task->response = SPDK_SCSI_TASK_MGMT_RESP_SUCCESS;

test/lib/scsi/lun/lun_ut.c

+6 −49
Original line number Diff line number Diff line
@@ -47,7 +47,6 @@ SPDK_LOG_REGISTER_TRACE_FLAG("scsi", SPDK_TRACE_SCSI) 
struct spdk_scsi_globals g_spdk_scsi;



static bool g_lun_execute_fail = false;

static bool g_lun_task_set_full_flag = false;

static int g_lun_execute_status = SPDK_SCSI_TASK_PENDING;

static uint32_t g_task_count = 0;
@@ -132,9 +131,6 @@ spdk_bdev_scsi_execute(struct spdk_bdev *bdev, struct spdk_scsi_task *task) 
	if (g_lun_execute_fail)

		return -EINVAL;

	else {

		if (g_lun_task_set_full_flag)

			task->status = SPDK_SCSI_STATUS_TASK_SET_FULL;

		else

		task->status = SPDK_SCSI_STATUS_GOOD;



		if (g_lun_execute_status == SPDK_SCSI_TASK_PENDING)
@@ -235,6 +231,7 @@ lun_task_mgmt_execute_abort_task_not_supported(void) 
	lun->dev = &dev;



	mgmt_task = spdk_get_task(NULL);

	mgmt_task->type = SPDK_SCSI_TASK_TYPE_MANAGE;

	mgmt_task->function = SPDK_SCSI_TASK_FUNC_ABORT_TASK;

	mgmt_task->lun = lun;

	mgmt_task->initiator_port = &initiator_port;
@@ -308,6 +305,7 @@ lun_task_mgmt_execute_abort_task_all_not_supported(void) 
	lun->dev = &dev;



	mgmt_task = spdk_get_task(NULL);

	mgmt_task->type = SPDK_SCSI_TASK_TYPE_MANAGE;

	mgmt_task->function = SPDK_SCSI_TASK_FUNC_ABORT_TASK_SET;

	mgmt_task->lun = lun;

	mgmt_task->initiator_port = &initiator_port;
@@ -349,6 +347,7 @@ lun_task_mgmt_execute_lun_reset_failure(void) 
	int rc;



	mgmt_task = spdk_get_task(NULL);

	mgmt_task->type = SPDK_SCSI_TASK_TYPE_MANAGE;

	mgmt_task->lun = NULL;

	mgmt_task->function = SPDK_SCSI_TASK_FUNC_LUN_RESET;
@@ -374,6 +373,7 @@ lun_task_mgmt_execute_lun_reset(void) 
	lun->dev = &dev;



	mgmt_task = spdk_get_task(NULL);

	mgmt_task->type = SPDK_SCSI_TASK_TYPE_MANAGE;

	mgmt_task->lun = lun;

	mgmt_task->function = SPDK_SCSI_TASK_FUNC_LUN_RESET;
@@ -401,6 +401,7 @@ lun_task_mgmt_execute_invalid_case(void) 
	lun->dev = &dev;



	mgmt_task = spdk_get_task(NULL);

	mgmt_task->type = SPDK_SCSI_TASK_TYPE_MANAGE;

	/* Pass an invalid value to the switch statement */

	mgmt_task->function = 5;
@@ -483,46 +484,6 @@ lun_append_task_null_lun_not_supported(void) 
	CU_ASSERT_EQUAL(g_task_count, 0);

}



static void

lun_execute_task_set_full(void)

{

	struct spdk_scsi_lun *lun;

	struct spdk_scsi_task *task;

	struct spdk_scsi_dev dev = { 0 };



	lun = lun_construct();



	task = spdk_get_task(NULL);

	task->lun = lun;

	lun->dev = &dev;



	g_lun_execute_fail = false;

	g_lun_task_set_full_flag = true;



	spdk_scsi_lun_append_task(lun, task);



	/* task should now be on the pending_task list */

	CU_ASSERT(!TAILQ_EMPTY(&lun->pending_tasks));



	/* but the tasks list should still be empty since it has not been

	   executed yet

	 */

	CU_ASSERT(TAILQ_EMPTY(&lun->tasks));



	spdk_scsi_lun_execute_tasks(lun);



	/* Assert the lun's task set is full; hence the  function

	has failed to add another task to the tasks queue */

	CU_ASSERT(TAILQ_EMPTY(&lun->tasks));

	CU_ASSERT(task->status == SPDK_SCSI_STATUS_TASK_SET_FULL);



	spdk_scsi_task_put(task);



	lun_destruct(lun);



	CU_ASSERT_EQUAL(g_task_count, 0);

}



static void

lun_execute_scsi_task_pending(void)

{
@@ -537,7 +498,6 @@ lun_execute_scsi_task_pending(void) 
	lun->dev = &dev;



	g_lun_execute_fail = false;

	g_lun_task_set_full_flag = false;

	g_lun_execute_status = SPDK_SCSI_TASK_PENDING;



	spdk_scsi_lun_append_task(lun, task);
@@ -576,7 +536,6 @@ lun_execute_scsi_task_complete(void) 
	lun->dev = &dev;



	g_lun_execute_fail = false;

	g_lun_task_set_full_flag = false;

	g_lun_execute_status = SPDK_SCSI_TASK_COMPLETE;



	spdk_scsi_lun_append_task(lun, task);
@@ -692,8 +651,6 @@ main(int argc, char **argv) 
			       lun_append_task_null_lun_alloc_len_lt_4096) == NULL

		|| CU_add_test(suite, "append task - unsupported lun",

			       lun_append_task_null_lun_not_supported) == NULL

		|| CU_add_test(suite, "execute task - task set full",

			       lun_execute_task_set_full) == NULL

		|| CU_add_test(suite, "execute task - scsi task pending",

			       lun_execute_scsi_task_pending) == NULL

		|| CU_add_test(suite, "execute task - scsi task complete",