diff --git a/lib/iscsi/conn.c b/lib/iscsi/conn.c index ae8f46473..9accb971d 100644 --- a/lib/iscsi/conn.c +++ b/lib/iscsi/conn.c @@ -1086,6 +1086,50 @@ process_read_task_completion(struct spdk_iscsi_conn *conn, process_completed_read_subtask_list(conn, primary); } +static void +process_non_read_task_completion(struct spdk_iscsi_conn *conn, + struct spdk_iscsi_task *task, + struct spdk_iscsi_task *primary) +{ + primary->bytes_completed += task->scsi.length; + + /* If the status of the subtask is the first failure, remember it as + * the status of the command and set it to the status of the primary + * task later. + * + * If the first failed task is the primary, two copies can be avoided + * but code simplicity is prioritized. + */ + if (task->scsi.status == SPDK_SCSI_STATUS_GOOD) { + if (task != primary) { + primary->scsi.data_transferred += task->scsi.data_transferred; + } + } else if (primary->rsp_scsi_status == SPDK_SCSI_STATUS_GOOD) { + iscsi_task_copy_to_rsp_scsi_status(primary, &task->scsi); + } + + if (primary->bytes_completed == primary->scsi.transfer_len) { + spdk_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); + } + spdk_iscsi_task_response(conn, primary); + /* + * Check if this is the last task completed for an iSCSI write + * that required child subtasks. If task != primary, we know + * for sure that it was part of an iSCSI write with child subtasks. + * The trickier case is when the last task completed was the initial + * task - in this case the task will have a smaller length than + * the overall transfer length. + */ + if (task != primary || task->scsi.length != task->scsi.transfer_len) { + TAILQ_REMOVE(&conn->active_r2t_tasks, primary, link); + spdk_iscsi_task_put(primary); + } + } + spdk_iscsi_task_put(task); +} + void spdk_iscsi_task_cpl(struct spdk_scsi_task *scsi_task) { @@ -1102,43 +1146,7 @@ spdk_iscsi_task_cpl(struct spdk_scsi_task *scsi_task) if (spdk_iscsi_task_is_read(primary)) { process_read_task_completion(conn, task, primary); } else { - primary->bytes_completed += task->scsi.length; - - /* If the status of the subtask is the first failure, remember it as - * the status of the command and set it to the status of the primary - * task later. - * - * If the first failed task is the primary, two copies can be avoided - * but code simplicity is prioritized. - */ - if (task->scsi.status == SPDK_SCSI_STATUS_GOOD) { - if (task != primary) { - primary->scsi.data_transferred += task->scsi.data_transferred; - } - } else if (primary->rsp_scsi_status == SPDK_SCSI_STATUS_GOOD) { - iscsi_task_copy_to_rsp_scsi_status(primary, &task->scsi); - } - - if (primary->bytes_completed == primary->scsi.transfer_len) { - spdk_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); - } - spdk_iscsi_task_response(conn, primary); - /* - * Check if this is the last task completed for an iSCSI write - * that required child subtasks. If task != primary, we know - * for sure that it was part of an iSCSI write with child subtasks. - * The trickier case is when the last task completed was the initial - * task - in this case the task will have a smaller length than - * the overall transfer length. - */ - if (task != primary || task->scsi.length != task->scsi.transfer_len) { - TAILQ_REMOVE(&conn->active_r2t_tasks, primary, link); - spdk_iscsi_task_put(primary); - } - } - spdk_iscsi_task_put(task); + process_non_read_task_completion(conn, task, primary); } if (!task->parent) { spdk_trace_record(TRACE_ISCSI_PDU_COMPLETED, 0, 0, (uintptr_t)pdu, 0); diff --git a/test/unit/lib/iscsi/conn.c/conn_ut.c b/test/unit/lib/iscsi/conn.c/conn_ut.c index 3b7261a82..e64d66542 100644 --- a/test/unit/lib/iscsi/conn.c/conn_ut.c +++ b/test/unit/lib/iscsi/conn.c/conn_ut.c @@ -358,6 +358,69 @@ propagate_scsi_error_status_for_split_read_tasks(void) CU_ASSERT(TAILQ_EMPTY(&primary.subtask_list)); } +static void +process_non_read_task_completion_test(void) +{ + struct spdk_iscsi_conn conn = {}; + struct spdk_iscsi_task primary = {}; + struct spdk_iscsi_task task = {}; + + TAILQ_INIT(&conn.active_r2t_tasks); + + primary.bytes_completed = 0; + primary.scsi.transfer_len = 4096 * 3; + primary.rsp_scsi_status = SPDK_SCSI_STATUS_GOOD; + TAILQ_INSERT_TAIL(&conn.active_r2t_tasks, &primary, link); + + /* First subtask which failed. */ + task.scsi.length = 4096; + task.scsi.data_transferred = 4096; + task.scsi.status = SPDK_SCSI_STATUS_CHECK_CONDITION; + + process_non_read_task_completion(&conn, &task, &primary); + CU_ASSERT(!TAILQ_EMPTY(&conn.active_r2t_tasks)); + CU_ASSERT(primary.bytes_completed == 4096); + CU_ASSERT(primary.scsi.data_transferred == 0); + CU_ASSERT(primary.rsp_scsi_status == SPDK_SCSI_STATUS_CHECK_CONDITION); + + /* Second subtask which succeeded. */ + task.scsi.length = 4096; + task.scsi.data_transferred = 4096; + task.scsi.status = SPDK_SCSI_STATUS_GOOD; + + process_non_read_task_completion(&conn, &task, &primary); + CU_ASSERT(!TAILQ_EMPTY(&conn.active_r2t_tasks)); + CU_ASSERT(primary.bytes_completed == 4096 * 2); + CU_ASSERT(primary.scsi.data_transferred == 4096); + CU_ASSERT(primary.rsp_scsi_status == SPDK_SCSI_STATUS_CHECK_CONDITION); + + /* Third and final subtask which succeeded. */ + task.scsi.length = 4096; + task.scsi.data_transferred = 4096; + task.scsi.status = SPDK_SCSI_STATUS_GOOD; + + process_non_read_task_completion(&conn, &task, &primary); + CU_ASSERT(TAILQ_EMPTY(&conn.active_r2t_tasks)); + CU_ASSERT(primary.bytes_completed == 4096 * 3); + CU_ASSERT(primary.scsi.data_transferred == 4096 * 2); + CU_ASSERT(primary.rsp_scsi_status == SPDK_SCSI_STATUS_CHECK_CONDITION); + + /* Tricky case when the last task completed was the initial task. */ + primary.scsi.length = 4096; + primary.bytes_completed = 4096 * 2; + primary.scsi.data_transferred = 4096 * 2; + primary.scsi.transfer_len = 4096 * 3; + primary.scsi.status = SPDK_SCSI_STATUS_GOOD; + primary.rsp_scsi_status = SPDK_SCSI_STATUS_GOOD; + TAILQ_INSERT_TAIL(&conn.active_r2t_tasks, &primary, link); + + process_non_read_task_completion(&conn, &primary, &primary); + CU_ASSERT(TAILQ_EMPTY(&conn.active_r2t_tasks)); + CU_ASSERT(primary.bytes_completed == 4096 * 3); + CU_ASSERT(primary.scsi.data_transferred == 4096 * 2); + CU_ASSERT(primary.rsp_scsi_status == SPDK_SCSI_STATUS_GOOD); +} + static void recursive_flush_pdus_calls(void) { @@ -584,6 +647,8 @@ main(int argc, char **argv) read_task_split_reverse_order_case) == NULL || CU_add_test(suite, "propagate_scsi_error_status_for_split_read_tasks", propagate_scsi_error_status_for_split_read_tasks) == NULL || + 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 ) {