From a115177b98e5ff1b7861849afbd52a2095d6bd21 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Mon, 26 Nov 2018 10:48:10 +0900 Subject: [PATCH] iscsi: Clear only R2T tasks prior to task management request As described in RFC3720, task management request will act on all the commands from the same session having a CmdSN lower than the task management CmdSN. Current implementation clears all R2T tasks without checking CmdSN. This patch changes the implementation to clear only R2T tasks whose CmdSN is smaller than the task management request. Additionally this patch adds to UT code to verify the change works as expected. Change-Id: I0b368cb13741bc05259924d27793438e9250b951 Signed-off-by: Shuhei Matsumoto Reviewed-on: https://review.gerrithub.io/434761 Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Changpeng Liu Reviewed-by: Ben Walker --- lib/iscsi/conn.c | 4 +- lib/iscsi/iscsi.c | 25 +++++++--- lib/iscsi/iscsi.h | 3 +- test/unit/lib/iscsi/conn.c/conn_ut.c | 3 +- test/unit/lib/iscsi/iscsi.c/iscsi_ut.c | 68 ++++++++++++++++++++++++-- 5 files changed, 87 insertions(+), 16 deletions(-) diff --git a/lib/iscsi/conn.c b/lib/iscsi/conn.c index 2c89feab9b..138ee44b1c 100644 --- a/lib/iscsi/conn.c +++ b/lib/iscsi/conn.c @@ -529,7 +529,7 @@ _spdk_iscsi_conn_destruct(struct spdk_iscsi_conn *conn) { int rc; - spdk_clear_all_transfer_task(conn, NULL); + spdk_clear_all_transfer_task(conn, NULL, NULL); spdk_iscsi_poll_group_remove_conn_sock(conn); spdk_sock_close(&conn->sock); spdk_poller_unregister(&conn->logout_timer); @@ -668,7 +668,7 @@ _iscsi_conn_remove_lun(void *arg1, void *arg2) return; } - spdk_clear_all_transfer_task(conn, lun); + spdk_clear_all_transfer_task(conn, lun, NULL); TAILQ_FOREACH_SAFE(pdu, &conn->write_pdu_list, tailq, tmp_pdu) { if (pdu->task && (lun == pdu->task->scsi.lun)) { TAILQ_REMOVE(&conn->write_pdu_list, pdu, tailq); diff --git a/lib/iscsi/iscsi.c b/lib/iscsi/iscsi.c index e5cfd26deb..31af178f4b 100644 --- a/lib/iscsi/iscsi.c +++ b/lib/iscsi/iscsi.c @@ -2991,11 +2991,11 @@ spdk_abort_transfer_task_in_task_mgmt_resp(struct spdk_iscsi_conn *conn, /* abort all tasks issued via this session on the LUN */ case ISCSI_TASK_FUNC_ABORT_TASK_SET: - spdk_clear_all_transfer_task(conn, task->scsi.lun); + spdk_clear_all_transfer_task(conn, task->scsi.lun, pdu); break; case ISCSI_TASK_FUNC_LOGICAL_UNIT_RESET: - spdk_clear_all_transfer_task(conn, task->scsi.lun); + spdk_clear_all_transfer_task(conn, task->scsi.lun, pdu); break; } } @@ -3523,9 +3523,12 @@ void spdk_del_transfer_task(struct spdk_iscsi_conn *conn, uint32_t task_tag) static void spdk_del_connection_queued_task(struct spdk_iscsi_conn *conn, void *tailq, - struct spdk_scsi_lun *lun) + struct spdk_scsi_lun *lun, + struct spdk_iscsi_pdu *pdu) { struct spdk_iscsi_task *task, *task_tmp; + struct spdk_iscsi_pdu *pdu_tmp; + /* * Temporary used to index spdk_scsi_task related * queues of the connection. @@ -3534,7 +3537,9 @@ spdk_del_connection_queued_task(struct spdk_iscsi_conn *conn, void *tailq, head = (struct queued_tasks *)tailq; TAILQ_FOREACH_SAFE(task, head, link, task_tmp) { - if (lun == NULL || lun == task->scsi.lun) { + pdu_tmp = spdk_iscsi_task_get_pdu(task); + if ((lun == NULL || lun == task->scsi.lun) && + (pdu == NULL || SN32_LT(pdu_tmp->cmd_sn, pdu->cmd_sn))) { TAILQ_REMOVE(head, task, link); if (lun != NULL && spdk_scsi_lun_is_removing(lun)) { spdk_scsi_task_process_null_lun(&task->scsi); @@ -3546,15 +3551,19 @@ spdk_del_connection_queued_task(struct spdk_iscsi_conn *conn, void *tailq, } void spdk_clear_all_transfer_task(struct spdk_iscsi_conn *conn, - struct spdk_scsi_lun *lun) + struct spdk_scsi_lun *lun, + struct spdk_iscsi_pdu *pdu) { int i, j, pending_r2t; struct spdk_iscsi_task *task; + struct spdk_iscsi_pdu *pdu_tmp; pending_r2t = conn->pending_r2t; for (i = 0; i < pending_r2t; i++) { task = conn->outstanding_r2t_tasks[i]; - if (lun == NULL || lun == task->scsi.lun) { + pdu_tmp = spdk_iscsi_task_get_pdu(task); + if ((lun == NULL || lun == task->scsi.lun) && + (pdu == NULL || SN32_LT(pdu_tmp->cmd_sn, pdu->cmd_sn))) { conn->outstanding_r2t_tasks[i] = NULL; task->outstanding_r2t = 0; task->next_r2t_offset = 0; @@ -3577,8 +3586,8 @@ void spdk_clear_all_transfer_task(struct spdk_iscsi_conn *conn, } } - spdk_del_connection_queued_task(conn, &conn->active_r2t_tasks, lun); - spdk_del_connection_queued_task(conn, &conn->queued_r2t_tasks, lun); + spdk_del_connection_queued_task(conn, &conn->active_r2t_tasks, lun, pdu); + spdk_del_connection_queued_task(conn, &conn->queued_r2t_tasks, lun, pdu); spdk_start_queued_transfer_tasks(conn); } diff --git a/lib/iscsi/iscsi.h b/lib/iscsi/iscsi.h index 343207b03a..169a4799ac 100644 --- a/lib/iscsi/iscsi.h +++ b/lib/iscsi/iscsi.h @@ -425,7 +425,8 @@ int spdk_iscsi_sess_params_init(struct iscsi_param **params); void spdk_free_sess(struct spdk_iscsi_sess *sess); void spdk_clear_all_transfer_task(struct spdk_iscsi_conn *conn, - struct spdk_scsi_lun *lun); + struct spdk_scsi_lun *lun, + struct spdk_iscsi_pdu *pdu); void spdk_del_transfer_task(struct spdk_iscsi_conn *conn, uint32_t CmdSN); bool spdk_iscsi_is_deferred_free_pdu(struct spdk_iscsi_pdu *pdu); diff --git a/test/unit/lib/iscsi/conn.c/conn_ut.c b/test/unit/lib/iscsi/conn.c/conn_ut.c index 45296d893e..604a19bb58 100644 --- a/test/unit/lib/iscsi/conn.c/conn_ut.c +++ b/test/unit/lib/iscsi/conn.c/conn_ut.c @@ -216,7 +216,8 @@ spdk_iscsi_conn_params_init(struct iscsi_param **params) } void spdk_clear_all_transfer_task(struct spdk_iscsi_conn *conn, - struct spdk_scsi_lun *lun) + struct spdk_scsi_lun *lun, + struct spdk_iscsi_pdu *pdu) { } diff --git a/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c b/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c index 63874dab89..0e07098204 100644 --- a/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c +++ b/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c @@ -810,9 +810,11 @@ clear_all_transfer_tasks_test(void) { struct spdk_iscsi_sess sess; struct spdk_iscsi_conn conn; - struct spdk_iscsi_task *task1, *task2, *task3, *task4, *task5; - struct spdk_iscsi_pdu *pdu1, *pdu2, *pdu3, *pdu4, *pdu5, *pdu; + struct spdk_iscsi_task *task1, *task2, *task3, *task4, *task5, *task6; + struct spdk_iscsi_pdu *pdu1, *pdu2, *pdu3, *pdu4, *pdu5, *pdu6, *pdu; + struct spdk_iscsi_pdu *mgmt_pdu1, *mgmt_pdu2; struct spdk_scsi_lun lun1, lun2; + uint32_t alloc_cmd_sn; int rc; memset(&sess, 0, sizeof(sess)); @@ -827,12 +829,16 @@ clear_all_transfer_tasks_test(void) TAILQ_INIT(&conn.active_r2t_tasks); TAILQ_INIT(&conn.queued_r2t_tasks); + alloc_cmd_sn = 10; + task1 = spdk_iscsi_task_get(&conn, NULL, NULL); SPDK_CU_ASSERT_FATAL(task1 != NULL); pdu1 = spdk_get_pdu(); SPDK_CU_ASSERT_FATAL(pdu1 != NULL); pdu1->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + pdu1->cmd_sn = alloc_cmd_sn; + alloc_cmd_sn++; task1->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; task1->scsi.lun = &lun1; spdk_iscsi_task_set_pdu(task1, pdu1); @@ -840,12 +846,20 @@ clear_all_transfer_tasks_test(void) rc = spdk_add_transfer_task(&conn, task1); CU_ASSERT(rc == SPDK_SUCCESS); + mgmt_pdu1 = spdk_get_pdu(); + SPDK_CU_ASSERT_FATAL(mgmt_pdu1 != NULL); + + mgmt_pdu1->cmd_sn = alloc_cmd_sn; + alloc_cmd_sn++; + task2 = spdk_iscsi_task_get(&conn, NULL, NULL); SPDK_CU_ASSERT_FATAL(task2 != NULL); pdu2 = spdk_get_pdu(); SPDK_CU_ASSERT_FATAL(pdu2 != NULL); pdu2->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + pdu2->cmd_sn = alloc_cmd_sn; + alloc_cmd_sn++; task2->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; task2->scsi.lun = &lun1; spdk_iscsi_task_set_pdu(task2, pdu2); @@ -859,6 +873,8 @@ clear_all_transfer_tasks_test(void) SPDK_CU_ASSERT_FATAL(pdu3 != NULL); pdu3->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + pdu3->cmd_sn = alloc_cmd_sn; + alloc_cmd_sn++; task3->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; task3->scsi.lun = &lun1; spdk_iscsi_task_set_pdu(task3, pdu3); @@ -872,6 +888,8 @@ clear_all_transfer_tasks_test(void) SPDK_CU_ASSERT_FATAL(pdu4 != NULL); pdu4->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + pdu4->cmd_sn = alloc_cmd_sn; + alloc_cmd_sn++; task4->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; task4->scsi.lun = &lun2; spdk_iscsi_task_set_pdu(task4, pdu4); @@ -885,6 +903,8 @@ clear_all_transfer_tasks_test(void) SPDK_CU_ASSERT_FATAL(pdu5 != NULL); pdu5->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + pdu5->cmd_sn = alloc_cmd_sn; + alloc_cmd_sn++; task5->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; task5->scsi.lun = &lun2; spdk_iscsi_task_set_pdu(task5, pdu5); @@ -892,6 +912,27 @@ clear_all_transfer_tasks_test(void) rc = spdk_add_transfer_task(&conn, task5); CU_ASSERT(rc == SPDK_SUCCESS); + mgmt_pdu2 = spdk_get_pdu(); + SPDK_CU_ASSERT_FATAL(mgmt_pdu2 != NULL); + + mgmt_pdu2->cmd_sn = alloc_cmd_sn; + alloc_cmd_sn++; + + task6 = spdk_iscsi_task_get(&conn, NULL, NULL); + SPDK_CU_ASSERT_FATAL(task6 != NULL); + pdu6 = spdk_get_pdu(); + SPDK_CU_ASSERT_FATAL(pdu6 != NULL); + + pdu6->data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + pdu6->cmd_sn = alloc_cmd_sn; + alloc_cmd_sn++; + task5->scsi.transfer_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + task6->scsi.lun = &lun2; + spdk_iscsi_task_set_pdu(task6, pdu6); + + rc = spdk_add_transfer_task(&conn, task6); + CU_ASSERT(rc == SPDK_SUCCESS); + CU_ASSERT(conn.ttt == 4); CU_ASSERT(spdk_get_transfer_task(&conn, 1) == task1); @@ -900,7 +941,17 @@ clear_all_transfer_tasks_test(void) CU_ASSERT(spdk_get_transfer_task(&conn, 4) == task4); CU_ASSERT(spdk_get_transfer_task(&conn, 5) == NULL); - spdk_clear_all_transfer_task(&conn, &lun1); + spdk_clear_all_transfer_task(&conn, &lun1, mgmt_pdu1); + + CU_ASSERT(!TAILQ_EMPTY(&conn.queued_r2t_tasks)); + CU_ASSERT(spdk_get_transfer_task(&conn, 1) == NULL); + CU_ASSERT(spdk_get_transfer_task(&conn, 2) == task2); + CU_ASSERT(spdk_get_transfer_task(&conn, 3) == task3); + CU_ASSERT(spdk_get_transfer_task(&conn, 4) == task4); + CU_ASSERT(spdk_get_transfer_task(&conn, 5) == task5); + CU_ASSERT(spdk_get_transfer_task(&conn, 6) == NULL); + + spdk_clear_all_transfer_task(&conn, &lun1, NULL); CU_ASSERT(TAILQ_EMPTY(&conn.queued_r2t_tasks)); CU_ASSERT(spdk_get_transfer_task(&conn, 1) == NULL); @@ -908,11 +959,17 @@ clear_all_transfer_tasks_test(void) CU_ASSERT(spdk_get_transfer_task(&conn, 3) == NULL); CU_ASSERT(spdk_get_transfer_task(&conn, 4) == task4); CU_ASSERT(spdk_get_transfer_task(&conn, 5) == task5); + CU_ASSERT(spdk_get_transfer_task(&conn, 6) == task6); - spdk_clear_all_transfer_task(&conn, NULL); + spdk_clear_all_transfer_task(&conn, &lun2, mgmt_pdu2); CU_ASSERT(spdk_get_transfer_task(&conn, 4) == NULL); CU_ASSERT(spdk_get_transfer_task(&conn, 5) == NULL); + CU_ASSERT(spdk_get_transfer_task(&conn, 6) == task6); + + spdk_clear_all_transfer_task(&conn, NULL, NULL); + + CU_ASSERT(spdk_get_transfer_task(&conn, 6) == NULL); CU_ASSERT(TAILQ_EMPTY(&conn.active_r2t_tasks)); while (!TAILQ_EMPTY(&g_write_pdu_list)) { @@ -921,6 +978,9 @@ clear_all_transfer_tasks_test(void) spdk_put_pdu(pdu); } + spdk_put_pdu(mgmt_pdu2); + spdk_put_pdu(mgmt_pdu1); + spdk_put_pdu(pdu6); spdk_put_pdu(pdu5); spdk_put_pdu(pdu4); spdk_put_pdu(pdu3);