From acab54c1f40b782b5a99fc8d8845b83f1cb6fc86 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Thu, 28 Nov 2019 04:08:03 -0500 Subject: [PATCH] iscsi: correctly free the deferred pdu for ERL > 0 case The type of pdu deferred to be free only have two types, R2T or DATA_IN. And the two types of pdus are all assoicated a task, so updateing both the code and unit test case. Also for all pdu free, we should use spdk_iscsi_conn_free function since for normal pdu free, we all use this function. PS: I also tested the calsoft local, it does not trigger the assert. Fixes #1074. Signed-off-by: Ziye Yang Signed-off-by: Shuhei Matsumoto Change-Id: I0524965baf5349a100210ef717aedaa5f8ff105e Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/475657 Reviewed-by: Changpeng Liu Reviewed-by: Jim Harris Tested-by: SPDK CI Jenkins --- lib/iscsi/conn.c | 7 +++---- lib/iscsi/iscsi.c | 10 ++-------- test/unit/lib/iscsi/conn.c/conn_ut.c | 8 ++------ 3 files changed, 7 insertions(+), 18 deletions(-) diff --git a/lib/iscsi/conn.c b/lib/iscsi/conn.c index 97314a59c..a63e4355c 100644 --- a/lib/iscsi/conn.c +++ b/lib/iscsi/conn.c @@ -331,11 +331,10 @@ _iscsi_conn_free_tasks(struct spdk_iscsi_conn *conn, struct spdk_scsi_lun *lun) } TAILQ_FOREACH_SAFE(pdu, &conn->snack_pdu_list, tailq, tmp_pdu) { - TAILQ_REMOVE(&conn->snack_pdu_list, pdu, tailq); - if (pdu->task && (lun == NULL || lun == pdu->task->scsi.lun)) { - spdk_iscsi_task_put(pdu->task); + if (lun == NULL || lun == pdu->task->scsi.lun) { + TAILQ_REMOVE(&conn->snack_pdu_list, pdu, tailq); + spdk_iscsi_conn_free_pdu(conn, pdu); } - spdk_put_pdu(pdu); } TAILQ_FOREACH_SAFE(iscsi_task, &conn->queued_datain_tasks, link, tmp_iscsi_task) { diff --git a/lib/iscsi/iscsi.c b/lib/iscsi/iscsi.c index 08e0a4206..dcf65dc5c 100644 --- a/lib/iscsi/iscsi.c +++ b/lib/iscsi/iscsi.c @@ -2698,10 +2698,7 @@ iscsi_send_r2t_recovery(struct spdk_iscsi_conn *conn, task->next_expected_r2t_offset)); /* remove the old_r2t_pdu */ - if (pdu->task) { - spdk_iscsi_task_put(pdu->task); - } - spdk_put_pdu(pdu); + spdk_iscsi_conn_free_pdu(conn, pdu); /* re-send a new r2t pdu */ rc = iscsi_send_r2t(conn, task, task->next_expected_r2t_offset, @@ -4260,10 +4257,7 @@ iscsi_handle_data_ack(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) if ((from_be32(&datain_header->ttt) == transfer_tag) && (old_datasn == beg_run - 1)) { TAILQ_REMOVE(&conn->snack_pdu_list, old_pdu, tailq); - if (old_pdu->task) { - spdk_iscsi_task_put(old_pdu->task); - } - spdk_put_pdu(old_pdu); + spdk_iscsi_conn_free_pdu(conn, old_pdu); break; } } diff --git a/test/unit/lib/iscsi/conn.c/conn_ut.c b/test/unit/lib/iscsi/conn.c/conn_ut.c index c4cf65578..1887b1aa4 100644 --- a/test/unit/lib/iscsi/conn.c/conn_ut.c +++ b/test/unit/lib/iscsi/conn.c/conn_ut.c @@ -536,7 +536,6 @@ free_tasks_on_connection(void) TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu1, tailq); TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu2, tailq); TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu3, tailq); - TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu4, tailq); /* Free all PDUs and associated tasks when exiting connection. */ _iscsi_conn_free_tasks(&conn, NULL); @@ -544,7 +543,6 @@ free_tasks_on_connection(void) CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu1)); CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu2)); CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu3)); - CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu4)); CU_ASSERT(task1.scsi.ref == 0); CU_ASSERT(task2.scsi.ref == 0); CU_ASSERT(task3.scsi.ref == 0); @@ -555,15 +553,13 @@ free_tasks_on_connection(void) TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu1, tailq); TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu2, tailq); TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu3, tailq); - TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu4, tailq); /* Free all PDUs and free associated tasks whose lun matches the passed LUN. */ _iscsi_conn_free_tasks(&conn, &lun1); CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu1)); - CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu2)); - CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu3)); - CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu4)); + CU_ASSERT(dequeue_pdu(&conn.snack_pdu_list, &pdu2)); + CU_ASSERT(dequeue_pdu(&conn.snack_pdu_list, &pdu3)); CU_ASSERT(task1.scsi.ref == 0); CU_ASSERT(task2.scsi.ref == 1); CU_ASSERT(task3.scsi.ref == 1);