iscsi: Propagate SCSI error status to split SCSI read/write commands

For split SCSI read command, if there is any failure in the sequence of
it, the first error must be propagated to all subtasks of it.

For split SCSI write command, if there is any failure in the sequence
of it, the first error must be propagated to the primary subtask.

Before this patch,
for read task:
- any failure is propagated to already completed subtasks, but
  is not propagated to any subtasks not completed yet, and
- if any failure occurs in non-primary subtasks, it is not propagated
  to the primary subtask.

for write task:
- if the primary subtask completes after any failure of non-primary
  subtasks, the failure will be overwritten by the success of the
  primary task.

This patch fixes these issues.

Change-Id: I2d878798cbb40a8c5bd6a6fe5efb32b8de4a8ecd
Signed-off-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-on: https://review.gerrithub.io/436673
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>

lib/iscsi/conn.c

@@ -914,6 +914,25 @@ spdk_iscsi_task_mgmt_cpl(struct spdk_scsi_task *scsi_task)
 	spdk_iscsi_task_put(task);
 }
 
+static void
+spdk_iscsi_task_copy_to_rsp_scsi_status(struct spdk_iscsi_task *primary,
+					struct spdk_scsi_task *task)
+{
+	memcpy(primary->rsp_sense_data, task->sense_data, task->sense_data_len);
+	primary->rsp_sense_data_len = task->sense_data_len;
+	primary->rsp_scsi_status = task->status;
+}
+
+static void
+spdk_iscsi_task_copy_from_rsp_scsi_status(struct spdk_scsi_task *task,
+		struct spdk_iscsi_task *primary)
+{
+	memcpy(task->sense_data, primary->rsp_sense_data,
+	       primary->rsp_sense_data_len);
+	task->sense_data_len = primary->rsp_sense_data_len;
+	task->status = primary->rsp_scsi_status;
+}
+
 static void
 process_completed_read_subtask_list(struct spdk_iscsi_conn *conn,
 				    struct spdk_iscsi_task *primary)
@@ -939,10 +958,23 @@ process_read_task_completion(struct spdk_iscsi_conn *conn,
 {
 	struct spdk_iscsi_task *tmp;
 
+	/* If the status of the completed subtask is the first failure,
+	 * copy it to out-of-order subtasks and remember it as the status
+	 * of the command,
+	 *
+	 * Even if the status of the completed task is success,
+	 * there are any failed subtask ever, copy the first failed status
+	 * to it.
+	 */
 	if (task->scsi.status != SPDK_SCSI_STATUS_GOOD) {
-		TAILQ_FOREACH(tmp, &primary->subtask_list, subtask_link) {
+		if (primary->rsp_scsi_status == SPDK_SCSI_STATUS_GOOD) {
-			spdk_scsi_task_copy_status(&tmp->scsi, &task->scsi);
+			TAILQ_FOREACH(tmp, &primary->subtask_list, subtask_link) {
+				spdk_scsi_task_copy_status(&tmp->scsi, &task->scsi);
+			}
+			spdk_iscsi_task_copy_to_rsp_scsi_status(primary, &task->scsi);
 		}
+	} else if (primary->rsp_scsi_status != SPDK_SCSI_STATUS_GOOD) {
+		spdk_iscsi_task_copy_from_rsp_scsi_status(&task->scsi, primary);
 	}
 
 	if ((task != primary) &&
@@ -987,16 +1019,27 @@ spdk_iscsi_task_cpl(struct spdk_scsi_task *scsi_task)
 		process_read_task_completion(conn, task, primary);
 	} else {
 		primary->bytes_completed += task->scsi.length;
-		if (task != primary) {
+
-			if (task->scsi.status == SPDK_SCSI_STATUS_GOOD) {
+		/* 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 {
-				spdk_scsi_task_copy_status(&primary->scsi, &task->scsi);
 			}
+		} else if (primary->rsp_scsi_status == SPDK_SCSI_STATUS_GOOD) {
+			spdk_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) {
+				spdk_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

lib/iscsi/iscsi.c

@@ -2881,6 +2881,7 @@ spdk_iscsi_op_scsi(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu)
 	task->scsi.target_port = conn->target_port;
 	task->scsi.initiator_port = conn->initiator_port;
 	task->parent = NULL;
+	task->rsp_scsi_status = SPDK_SCSI_STATUS_GOOD;
 
 	if (task->scsi.lun == NULL) {
 		spdk_scsi_task_process_null_lun(&task->scsi);

lib/iscsi/task.h

@@ -44,6 +44,10 @@ struct spdk_iscsi_task {
 
 	struct spdk_iscsi_task *parent;
 
+	uint8_t rsp_scsi_status;
+	uint8_t rsp_sense_data[32];
+	size_t rsp_sense_data_len;
+
 	struct spdk_iscsi_conn *conn;
 	struct spdk_iscsi_pdu *pdu;
 	uint32_t outstanding_r2t;

test/unit/lib/iscsi/conn.c/conn_ut.c

@@ -120,8 +120,12 @@ DEFINE_STUB(spdk_scsi_lun_get_id, int, (const struct spdk_scsi_lun *lun), 0);
 DEFINE_STUB(spdk_scsi_port_get_name, const char *,
 	    (const struct spdk_scsi_port *port), NULL);
 
-DEFINE_STUB_V(spdk_scsi_task_copy_status,
+void
-	      (struct spdk_scsi_task *dst, struct spdk_scsi_task *src));
+spdk_scsi_task_copy_status(struct spdk_scsi_task *dst,
+			   struct spdk_scsi_task *src)
+{
+	dst->status = src->status;
+}
 
 DEFINE_STUB_V(spdk_put_pdu, (struct spdk_iscsi_pdu *pdu));
 
@@ -264,6 +268,69 @@ read_task_split_in_order_case(void)
 	CU_ASSERT(TAILQ_EMPTY(&g_ut_read_tasks));
 }
 
+static void
+propagate_scsi_error_status_for_split_read_tasks(void)
+{
+	struct spdk_iscsi_task primary, task1, task2, task3, task4, task5, task6;
+
+	memset(&primary, 0, sizeof(struct spdk_iscsi_task));
+	primary.scsi.length = 512;
+	primary.scsi.status = SPDK_SCSI_STATUS_GOOD;
+	primary.rsp_scsi_status = SPDK_SCSI_STATUS_GOOD;
+	TAILQ_INIT(&primary.subtask_list);
+
+	memset(&task1, 0, sizeof(struct spdk_iscsi_task));
+	task1.scsi.offset = 512;
+	task1.scsi.length = 512;
+	task1.scsi.status = SPDK_SCSI_STATUS_GOOD;
+
+	memset(&task2, 0, sizeof(struct spdk_iscsi_task));
+	task2.scsi.offset = 512 * 2;
+	task2.scsi.length = 512;
+	task2.scsi.status = SPDK_SCSI_STATUS_CHECK_CONDITION;
+
+	memset(&task3, 0, sizeof(struct spdk_iscsi_task));
+	task3.scsi.offset = 512 * 3;
+	task3.scsi.length = 512;
+	task3.scsi.status = SPDK_SCSI_STATUS_GOOD;
+
+	memset(&task4, 0, sizeof(struct spdk_iscsi_task));
+	task4.scsi.offset = 512 * 4;
+	task4.scsi.length = 512;
+	task4.scsi.status = SPDK_SCSI_STATUS_GOOD;
+
+	memset(&task5, 0, sizeof(struct spdk_iscsi_task));
+	task5.scsi.offset = 512 * 5;
+	task5.scsi.length = 512;
+	task5.scsi.status = SPDK_SCSI_STATUS_GOOD;
+
+	memset(&task6, 0, sizeof(struct spdk_iscsi_task));
+	task6.scsi.offset = 512 * 6;
+	task6.scsi.length = 512;
+	task6.scsi.status = SPDK_SCSI_STATUS_GOOD;
+
+	/* task2 has check condition status, and verify if the check condition
+	 * status is propagated to remaining tasks correctly when these tasks complete
+	 * by the following order, task4, task3, task2, task1, primary, task5, and task6.
+	 */
+	process_read_task_completion(NULL, &task4, &primary);
+	process_read_task_completion(NULL, &task3, &primary);
+	process_read_task_completion(NULL, &task2, &primary);
+	process_read_task_completion(NULL, &task1, &primary);
+	process_read_task_completion(NULL, &primary, &primary);
+	process_read_task_completion(NULL, &task5, &primary);
+	process_read_task_completion(NULL, &task6, &primary);
+
+	CU_ASSERT(primary.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);
+	CU_ASSERT(task1.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);
+	CU_ASSERT(task2.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);
+	CU_ASSERT(task3.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);
+	CU_ASSERT(task4.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);
+	CU_ASSERT(task5.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);
+	CU_ASSERT(task6.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);
+	CU_ASSERT(TAILQ_EMPTY(&primary.subtask_list));
+}
+
 int
 main(int argc, char **argv)
 {
@@ -281,7 +348,9 @@ main(int argc, char **argv)
 	}
 
 	if (
-		CU_add_test(suite, "read task split in order", read_task_split_in_order_case) == NULL
+		CU_add_test(suite, "read task split in order", read_task_split_in_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_cleanup_registry();
 		return CU_get_error();