Commit 1160d8e6 authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Tomasz Zawadzki
Browse files

lib/iscsi: Submit only subtasks for Data-OUT PDU sequence 



This change follows the large read which submits only subtasks, and
simplifies large write cases.

Associate the PDU which sends a SCSI Write PDU with immediate data
with both the primary task and the first secondary task. Then stop
incrementing reference count of the primary task twice.

As same as the last patch, copy the failure status directly among
the primary task and the secondary tasks because the primary task
is not submitted now. Then remove related data from struct
spdk_iscsi_task and related helper functions from conn.c.

Finally simplify unit tests for process_non_read_task_completion().

Signed-off-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: I54aa38c9b9fb7d7352da040dcdd8bcc1b1756a83
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6344


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
Reviewed-by: default avatarZiye Yang <ziye.yang@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent b7115d46

lib/iscsi/conn.c

+22 −53
Original line number Diff line number Diff line
@@ -1076,25 +1076,6 @@ iscsi_task_mgmt_cpl(struct spdk_scsi_task *scsi_task) 
	iscsi_task_put(task);

}



static void

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

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_in_order(struct spdk_iscsi_conn *conn,

		struct spdk_iscsi_task *primary)
@@ -1189,31 +1170,23 @@ process_non_read_task_completion(struct spdk_iscsi_conn *conn, 
{

	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 == primary) {

		/* This was a small write with no R2T. */

		iscsi_task_response(conn, task);

		iscsi_task_put(task);

		return;

	}



	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);

	} else if (primary->scsi.status == SPDK_SCSI_STATUS_GOOD) {

		/* If the status of this subtask is the first failure, copy it to

		 * the primary task.

		 */

		spdk_scsi_task_copy_status(&primary->scsi, &task->scsi);

	}



	if (primary->bytes_completed == primary->scsi.transfer_len) {

		/*

		 * 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) {

		/* If LUN is removed in the middle of the iSCSI write sequence,

		 *  primary might complete the write to the initiator because it is not

		 *  ensured that the initiator will send all data requested by R2Ts.
@@ -1222,12 +1195,8 @@ process_non_read_task_completion(struct spdk_iscsi_conn *conn, 
		 *  iscsi_clear_all_transfer_task() in iscsi.c.)

		 */

		if (primary->is_r2t_active) {

				if (primary->rsp_scsi_status != SPDK_SCSI_STATUS_GOOD) {

					iscsi_task_copy_from_rsp_scsi_status(&primary->scsi, primary);

				}

			iscsi_task_response(conn, primary);

			iscsi_del_transfer_task(conn, primary->tag);

			}

		} else {

			iscsi_task_response(conn, task);

		}

lib/iscsi/iscsi.c

+12 −7
Original line number Diff line number Diff line
@@ -3257,6 +3257,7 @@ iscsi_pdu_payload_op_scsi_write(struct spdk_iscsi_conn *conn, struct spdk_iscsi_ 
	struct iscsi_bhs_scsi_req *reqh;

	uint32_t transfer_len;

	uint32_t scsi_data_len;

	struct spdk_iscsi_task *subtask;

	int rc;



	pdu = iscsi_task_get_pdu(task);
@@ -3281,14 +3282,18 @@ iscsi_pdu_payload_op_scsi_write(struct spdk_iscsi_conn *conn, struct spdk_iscsi_ 
		}



		/* Non-immediate writes */

		if (pdu->data_segment_len == 0) {

			return 0;

		} else {

		if (pdu->data_segment_len != 0) {

			/* we are doing the first partial write task */

			task->scsi.ref++;

			spdk_scsi_task_set_data(&task->scsi, pdu->data, scsi_data_len);

			task->scsi.length = pdu->data_segment_len;

			subtask = iscsi_task_get(conn, task, iscsi_task_cpl);

			assert(subtask != NULL);



			spdk_scsi_task_set_data(&subtask->scsi, pdu->data, scsi_data_len);

			subtask->scsi.length = pdu->data_segment_len;

			iscsi_task_associate_pdu(subtask, pdu);



			iscsi_queue_task(conn, subtask);

		}

		return 0;

	}



	if (pdu->data_segment_len == transfer_len) {
@@ -3354,7 +3359,7 @@ iscsi_pdu_hdr_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;

	task->scsi.status = SPDK_SCSI_STATUS_GOOD;



	if (task->scsi.lun == NULL) {

		spdk_scsi_task_process_null_lun(&task->scsi);

lib/iscsi/task.h

+0 −4
Original line number Diff line number Diff line
@@ -44,10 +44,6 @@ 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

+17 −33
Original line number Diff line number Diff line
@@ -387,7 +387,7 @@ propagate_scsi_error_status_for_split_read_tasks(void) 
	conn.sess->DataSequenceInOrder = true;



	primary.scsi.transfer_len = 512 * 6;

	primary.rsp_scsi_status = SPDK_SCSI_STATUS_GOOD;

	primary.scsi.status = SPDK_SCSI_STATUS_GOOD;

	TAILQ_INIT(&primary.subtask_list);

	primary.scsi.ref = 7;
@@ -467,7 +467,7 @@ process_non_read_task_completion_test(void) 


	primary.bytes_completed = 0;

	primary.scsi.transfer_len = 4096 * 3;

	primary.rsp_scsi_status = SPDK_SCSI_STATUS_GOOD;

	primary.scsi.status = SPDK_SCSI_STATUS_GOOD;

	primary.scsi.ref = 1;

	TAILQ_INSERT_TAIL(&conn.active_r2t_tasks, &primary, link);

	primary.is_r2t_active = true;
@@ -485,7 +485,7 @@ process_non_read_task_completion_test(void) 
	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);

	CU_ASSERT(primary.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);

	CU_ASSERT(task.scsi.ref == 0);

	CU_ASSERT(primary.scsi.ref == 1);
@@ -501,7 +501,7 @@ process_non_read_task_completion_test(void) 
	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);

	CU_ASSERT(primary.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);

	CU_ASSERT(task.scsi.ref == 0);

	CU_ASSERT(primary.scsi.ref == 1);
@@ -517,44 +517,28 @@ process_non_read_task_completion_test(void) 
	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);

	CU_ASSERT(primary.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);

	CU_ASSERT(task.scsi.ref == 0);

	CU_ASSERT(primary.scsi.ref == 0);



	/* Tricky case when the last task completed was the initial task. */

	primary.scsi.length = 4096;

	/* A tricky case that the R2T was already terminated when the last task completed. */

	primary.scsi.ref = 0;

	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;

	primary.scsi.ref = 2;

	TAILQ_INSERT_TAIL(&conn.active_r2t_tasks, &primary, link);

	primary.is_r2t_active = true;



	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);

	CU_ASSERT(primary.scsi.ref == 0);



	/* Further tricky case when the last task completed ws the initial task,

	 * and the R2T was already terminated.

	 */

	primary.scsi.ref = 1;

	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;

	primary.scsi.status = SPDK_SCSI_STATUS_CHECK_CONDITION;

	primary.is_r2t_active = false;

	task.scsi.length = 4096;

	task.scsi.data_transferred = 4096;

	task.scsi.status = SPDK_SCSI_STATUS_GOOD;

	task.scsi.ref = 1;

	task.parent = &primary;

	primary.scsi.ref++;



	process_non_read_task_completion(&conn, &primary, &primary);

	process_non_read_task_completion(&conn, &task, &primary);

	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);

	CU_ASSERT(primary.scsi.data_transferred == 4096 * 3);

	CU_ASSERT(primary.scsi.status == SPDK_SCSI_STATUS_CHECK_CONDITION);

	CU_ASSERT(primary.scsi.ref == 0);

}