From d5f3f48b60684d15d0d968661dc769913f81b875 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Mon, 29 Jun 2020 13:18:43 +0900 Subject: [PATCH] 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: Shuhei Matsumoto Change-Id: I93f02b2fb670dcee4c32d61c264e3ad5b4f9f43e Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/3108 Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Changpeng Liu Reviewed-by: Jim Harris --- lib/iscsi/conn.c | 5 +- lib/iscsi/iscsi.c | 6 ++ test/unit/lib/iscsi/conn.c/conn_ut.c | 19 ++++++- test/unit/lib/iscsi/iscsi.c/iscsi_ut.c | 79 ++++++++++++++++---------- 4 files changed, 72 insertions(+), 37 deletions(-) diff --git a/lib/iscsi/conn.c b/lib/iscsi/conn.c index 570c34651..741b54be8 100644 --- a/lib/iscsi/conn.c +++ b/lib/iscsi/conn.c @@ -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); diff --git a/lib/iscsi/iscsi.c b/lib/iscsi/iscsi.c index 5f9b200cb..5d9fc4c84 100644 --- a/lib/iscsi/iscsi.c +++ b/lib/iscsi/iscsi.c @@ -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; } diff --git a/test/unit/lib/iscsi/conn.c/conn_ut.c b/test/unit/lib/iscsi/conn.c/conn_ut.c index dc016847d..967e16ec1 100644 --- a/test/unit/lib/iscsi/conn.c/conn_ut.c +++ b/test/unit/lib/iscsi/conn.c/conn_ut.c @@ -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; diff --git a/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c b/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c index 0d70485d7..1a8298fdd 100644 --- a/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c +++ b/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c @@ -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);