diff --git a/lib/iscsi/conn.c b/lib/iscsi/conn.c index a0833eb0a..7d50516f3 100644 --- a/lib/iscsi/conn.c +++ b/lib/iscsi/conn.c @@ -317,19 +317,6 @@ _iscsi_conn_free_tasks(struct spdk_iscsi_conn *conn, struct spdk_scsi_lun *lun) struct spdk_iscsi_pdu *pdu, *tmp_pdu; struct spdk_iscsi_task *iscsi_task, *tmp_iscsi_task; - TAILQ_FOREACH_SAFE(pdu, &conn->write_pdu_list, tailq, tmp_pdu) { - /* If connection is exited (no LUN is specified) or the PDU's LUN matches - * the LUN that was removed, free this PDU immediately. If the pdu's LUN - * is NULL, then we know the datain handling code already detected the hot - * removal, so we can free that PDU as well. - */ - if ((lun == NULL) || - (pdu->task && (lun == pdu->task->scsi.lun || NULL == pdu->task->scsi.lun))) { - TAILQ_REMOVE(&conn->write_pdu_list, pdu, tailq); - spdk_iscsi_conn_free_pdu(conn, pdu); - } - } - TAILQ_FOREACH_SAFE(pdu, &conn->snack_pdu_list, tailq, tmp_pdu) { if (lun == NULL || lun == pdu->task->scsi.lun) { TAILQ_REMOVE(&conn->snack_pdu_list, pdu, tailq); @@ -343,6 +330,25 @@ _iscsi_conn_free_tasks(struct spdk_iscsi_conn *conn, struct spdk_scsi_lun *lun) spdk_iscsi_task_put(iscsi_task); } } + + /* We have to parse conn->write_pdu_list in the end. In spdk_iscsi_conn_free_pdu(), + * spdk_iscsi_conn_handle_queued_datain_tasks() may be called, and + * spdk_iscsi_conn_handle_queued_datain_tasks() will parse conn->queued_datain_tasks + * and may stack some PDUs to conn->write_pdu_list. Hence when we come here, we + * have to ensure there is no associated task in conn->queued_datain_tasks. + */ + TAILQ_FOREACH_SAFE(pdu, &conn->write_pdu_list, tailq, tmp_pdu) { + /* If connection is exited (no LUN is specified) or the PDU's LUN matches + * the LUN that was removed, free this PDU immediately. If the pdu's LUN + * is NULL, then we know the datain handling code already detected the hot + * removal, so we can free that PDU as well. + */ + if ((lun == NULL) || + (pdu->task && (lun == pdu->task->scsi.lun || NULL == pdu->task->scsi.lun))) { + TAILQ_REMOVE(&conn->write_pdu_list, pdu, tailq); + spdk_iscsi_conn_free_pdu(conn, pdu); + } + } } static int diff --git a/test/unit/lib/iscsi/conn.c/conn_ut.c b/test/unit/lib/iscsi/conn.c/conn_ut.c index 2dc3e68bd..bf7c5c4ad 100644 --- a/test/unit/lib/iscsi/conn.c/conn_ut.c +++ b/test/unit/lib/iscsi/conn.c/conn_ut.c @@ -170,6 +170,15 @@ DEFINE_STUB(spdk_del_transfer_task, bool, int spdk_iscsi_conn_handle_queued_datain_tasks(struct spdk_iscsi_conn *conn) { + struct spdk_iscsi_task *task, *tmp; + + TAILQ_FOREACH_SAFE(task, &conn->queued_datain_tasks, link, tmp) { + TAILQ_REMOVE(&conn->queued_datain_tasks, task, link); + if (task->pdu) { + TAILQ_INSERT_TAIL(&conn->write_pdu_list, task->pdu, tailq); + } + } + return 0; } @@ -679,6 +688,51 @@ free_tasks_on_connection(void) CU_ASSERT(task3.scsi.ref == 1); } +static void +free_tasks_with_queued_datain(void) +{ + struct spdk_iscsi_conn conn = {}; + struct spdk_iscsi_pdu pdu1 = {}, pdu2 = {}, pdu3 = {}, pdu4 = {}, pdu5 = {}, pdu6 = {}; + struct spdk_iscsi_task task1 = {}, task2 = {}, task3 = {}, task4 = {}, task5 = {}, task6 = {}; + + TAILQ_INIT(&conn.write_pdu_list); + TAILQ_INIT(&conn.snack_pdu_list); + TAILQ_INIT(&conn.queued_datain_tasks); + + pdu1.task = &task1; + pdu2.task = &task2; + pdu3.task = &task3; + + task1.scsi.ref = 1; + task2.scsi.ref = 1; + task3.scsi.ref = 1; + + pdu3.bhs.opcode = ISCSI_OP_SCSI_DATAIN; + task3.scsi.offset = 1; + conn.data_in_cnt = 1; + + TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu1, tailq); + TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu2, tailq); + TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu3, tailq); + + task4.scsi.ref = 1; + task5.scsi.ref = 1; + task6.scsi.ref = 1; + + task4.pdu = &pdu4; + task5.pdu = &pdu5; + task6.pdu = &pdu6; + + TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task4, link); + TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task5, link); + TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task6, link); + + _iscsi_conn_free_tasks(&conn, NULL); + + CU_ASSERT(TAILQ_EMPTY(&conn.write_pdu_list)); + CU_ASSERT(TAILQ_EMPTY(&conn.queued_datain_tasks)); +} + int main(int argc, char **argv) { @@ -704,7 +758,8 @@ main(int argc, char **argv) CU_add_test(suite, "process_non_read_task_completion_test", process_non_read_task_completion_test) == NULL || CU_add_test(suite, "recursive_flush_pdus_calls", recursive_flush_pdus_calls) == NULL || - CU_add_test(suite, "free_tasks_on_connection", free_tasks_on_connection) == NULL + CU_add_test(suite, "free_tasks_on_connection", free_tasks_on_connection) == NULL || + CU_add_test(suite, "free_tasks_with_queued_datain", free_tasks_with_queued_datain) == NULL ) { CU_cleanup_registry(); return CU_get_error();