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

lib/iscsi: Fix iscsi_del_transfer_task deletes from both array and tailq 



Previously iscsi_del_transfer_task() dequeued the task only from
the array conn->outstanding_r2t_tasks[].

process_non_read_task_completion() had dequeued the task from
the tailq conn->active_r2t_tasks then.

However abort_transfer_task_in_task_mgmt_resp had not dequeued the
task from the tailq conn->active_r2t_tasks then.

This was an apparent bug, and is fixed here. Update unit tests
accordingly.

Signed-off-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: I93f02b2fb670dcee4c32d61c264e3ad5b4f9f43e
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/3108


Community-CI: Mellanox Build Bot
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent 41f59559

lib/iscsi/conn.c

+1 −4
Original line number Diff line number Diff line
@@ -1226,14 +1226,11 @@ process_non_read_task_completion(struct spdk_iscsi_conn *conn, 
			 *  iscsi_clear_all_transfer_task() in iscsi.c.)

			 */

			if (primary->is_r2t_active) {

				iscsi_del_transfer_task(conn, primary->tag);

				if (primary->rsp_scsi_status != SPDK_SCSI_STATUS_GOOD) {

					iscsi_task_copy_from_rsp_scsi_status(&primary->scsi, primary);

				}

				iscsi_task_response(conn, primary);

				TAILQ_REMOVE(&conn->active_r2t_tasks, primary, link);

				primary->is_r2t_active = false;

				iscsi_task_put(primary);

				iscsi_del_transfer_task(conn, primary->tag);

			}

		} else {

			iscsi_task_response(conn, task);

lib/iscsi/iscsi.c

+6 −0
Original line number Diff line number Diff line
@@ -2827,6 +2827,12 @@ iscsi_del_transfer_task(struct spdk_iscsi_conn *conn, uint32_t task_tag) 
				conn->outstanding_r2t_tasks[i] = conn->outstanding_r2t_tasks[i + 1];

			}

			conn->outstanding_r2t_tasks[conn->pending_r2t] = NULL;



			assert(task->is_r2t_active == true);

			TAILQ_REMOVE(&conn->active_r2t_tasks, task, link);

			task->is_r2t_active = false;

			iscsi_task_put(task);



			start_queued_transfer_tasks(conn);

			return true;

		}

test/unit/lib/iscsi/conn.c/conn_ut.c

+17 −2
Original line number Diff line number Diff line
@@ -209,8 +209,22 @@ DEFINE_STUB_V(iscsi_task_mgmt_response, 


DEFINE_STUB_V(iscsi_send_nopin, (struct spdk_iscsi_conn *conn));



DEFINE_STUB(iscsi_del_transfer_task, bool,

	    (struct spdk_iscsi_conn *conn, uint32_t task_tag), true);

bool

iscsi_del_transfer_task(struct spdk_iscsi_conn *conn, uint32_t task_tag)

{

	struct spdk_iscsi_task *task;



	task = TAILQ_FIRST(&conn->active_r2t_tasks);

	if (task == NULL || task->tag != task_tag) {

		return false;

	}



	TAILQ_REMOVE(&conn->active_r2t_tasks, task, link);

	task->is_r2t_active = false;

	iscsi_task_put(task);



	return true;

}



DEFINE_STUB(iscsi_handle_incoming_pdus, int, (struct spdk_iscsi_conn *conn), 0);
@@ -424,6 +438,7 @@ process_non_read_task_completion_test(void) 
	primary.scsi.ref = 1;

	TAILQ_INSERT_TAIL(&conn.active_r2t_tasks, &primary, link);

	primary.is_r2t_active = true;

	primary.tag = 1;



	/* First subtask which failed. */

	task.scsi.length = 4096;

test/unit/lib/iscsi/iscsi.c/iscsi_ut.c

+48 −31
Original line number Diff line number Diff line
@@ -779,7 +779,7 @@ del_transfer_task_test(void) 
{

	struct spdk_iscsi_sess sess = {};

	struct spdk_iscsi_conn conn = {};

	struct spdk_iscsi_task task1 = {}, task2 = {}, task3 = {}, task4 = {}, task5 = {}, *task;

	struct spdk_iscsi_task *task1, *task2, *task3, *task4, *task5;

	struct spdk_iscsi_pdu *pdu1, *pdu2, *pdu3, *pdu4, *pdu5, *pdu;

	int rc;
@@ -794,83 +794,100 @@ del_transfer_task_test(void) 
	SPDK_CU_ASSERT_FATAL(pdu1 != NULL);



	pdu1->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	task1.scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(&task1, pdu1);

	task1.tag = 11;



	rc = add_transfer_task(&conn, &task1);

	task1 = iscsi_task_get(&conn, NULL, NULL);

	SPDK_CU_ASSERT_FATAL(task1 != NULL);



	task1->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(task1, pdu1);

	task1->tag = 11;



	rc = add_transfer_task(&conn, task1);

	CU_ASSERT(rc == 0);



	pdu2 = iscsi_get_pdu(&conn);

	SPDK_CU_ASSERT_FATAL(pdu2 != NULL);



	pdu2->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	task2.scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(&task2, pdu2);

	task2.tag = 12;



	rc = add_transfer_task(&conn, &task2);

	task2 = iscsi_task_get(&conn, NULL, NULL);

	SPDK_CU_ASSERT_FATAL(task2 != NULL);



	task2->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(task2, pdu2);

	task2->tag = 12;



	rc = add_transfer_task(&conn, task2);

	CU_ASSERT(rc == 0);



	pdu3 = iscsi_get_pdu(&conn);

	SPDK_CU_ASSERT_FATAL(pdu3 != NULL);



	pdu3->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	task3.scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(&task3, pdu3);

	task3.tag = 13;



	rc = add_transfer_task(&conn, &task3);

	task3 = iscsi_task_get(&conn, NULL, NULL);

	SPDK_CU_ASSERT_FATAL(task3 != NULL);



	task3->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(task3, pdu3);

	task3->tag = 13;



	rc = add_transfer_task(&conn, task3);

	CU_ASSERT(rc == 0);



	pdu4 = iscsi_get_pdu(&conn);

	SPDK_CU_ASSERT_FATAL(pdu4 != NULL);



	pdu4->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	task4.scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(&task4, pdu4);

	task4.tag = 14;



	rc = add_transfer_task(&conn, &task4);

	task4 = iscsi_task_get(&conn, NULL, NULL);

	SPDK_CU_ASSERT_FATAL(task4 != NULL);



	task4->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(task4, pdu4);

	task4->tag = 14;



	rc = add_transfer_task(&conn, task4);

	CU_ASSERT(rc == 0);



	pdu5 = iscsi_get_pdu(&conn);

	SPDK_CU_ASSERT_FATAL(pdu5 != NULL);



	pdu5->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	task5.scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(&task5, pdu5);

	task5.tag = 15;



	rc = add_transfer_task(&conn, &task5);

	task5 = iscsi_task_get(&conn, NULL, NULL);

	SPDK_CU_ASSERT_FATAL(task5 != NULL);



	task5->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH;

	iscsi_task_set_pdu(task5, pdu5);

	task5->tag = 15;



	rc = add_transfer_task(&conn, task5);

	CU_ASSERT(rc == 0);



	CU_ASSERT(get_transfer_task(&conn, 1) == &task1);

	CU_ASSERT(get_transfer_task(&conn, 1) == task1);

	CU_ASSERT(get_transfer_task(&conn, 5) == NULL);

	iscsi_del_transfer_task(&conn, 11);

	CU_ASSERT(get_transfer_task(&conn, 1) == NULL);

	CU_ASSERT(get_transfer_task(&conn, 5) == &task5);

	CU_ASSERT(get_transfer_task(&conn, 5) == task5);



	CU_ASSERT(get_transfer_task(&conn, 2) == &task2);

	CU_ASSERT(get_transfer_task(&conn, 2) == task2);

	iscsi_del_transfer_task(&conn, 12);

	CU_ASSERT(get_transfer_task(&conn, 2) == NULL);



	CU_ASSERT(get_transfer_task(&conn, 3) == &task3);

	CU_ASSERT(get_transfer_task(&conn, 3) == task3);

	iscsi_del_transfer_task(&conn, 13);

	CU_ASSERT(get_transfer_task(&conn, 3) == NULL);



	CU_ASSERT(get_transfer_task(&conn, 4) == &task4);

	CU_ASSERT(get_transfer_task(&conn, 4) == task4);

	iscsi_del_transfer_task(&conn, 14);

	CU_ASSERT(get_transfer_task(&conn, 4) == NULL);



	CU_ASSERT(get_transfer_task(&conn, 5) == &task5);

	CU_ASSERT(get_transfer_task(&conn, 5) == task5);

	iscsi_del_transfer_task(&conn, 15);

	CU_ASSERT(get_transfer_task(&conn, 5) == NULL);



	while (!TAILQ_EMPTY(&conn.active_r2t_tasks)) {

		task = TAILQ_FIRST(&conn.active_r2t_tasks);

		TAILQ_REMOVE(&conn.active_r2t_tasks, task, link);

	}

	CU_ASSERT(TAILQ_EMPTY(&conn.active_r2t_tasks));



	while (!TAILQ_EMPTY(&g_write_pdu_list)) {

		pdu = TAILQ_FIRST(&g_write_pdu_list);