lib/iscsi: Simplify DIF insert/strip by doing at each PDU type handler

We have separated PDU header handler and payload handler, and have
PDU header handlers for each PDU type now.

By using this refinement, we can remove an aggregated helper function
spdk_iscsi_get_dif_ctx() and embed spdk_scsi_lun_get_dif_ctx() into
each PDU header handler.

Signed-off-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: Ib4d9939b625858466224647c545cb67a04babf86
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/471699
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Paul Luse <paul.e.luse@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>

lib/iscsi/conn.c

@@ -1398,14 +1398,13 @@ spdk_iscsi_conn_write_pdu(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *p
 	uint32_t crc32c;
 	int rc;
 
-	if (spdk_unlikely(spdk_iscsi_get_dif_ctx(conn, pdu, &pdu->dif_ctx))) {
+	if (spdk_unlikely(pdu->dif_insert_or_strip)) {
 		rc = iscsi_dif_verify(pdu, &pdu->dif_ctx);
 		if (rc != 0) {
 			spdk_iscsi_conn_free_pdu(conn, pdu);
 			conn->state = ISCSI_CONN_STATE_EXITING;
 			return;
 		}
-		pdu->dif_insert_or_strip = true;
 	}
 
 	if (pdu->bhs.opcode != ISCSI_OP_LOGIN_RSP) {

lib/iscsi/iscsi.c

@@ -2915,6 +2915,7 @@ iscsi_send_datain(struct spdk_iscsi_conn *conn,
 	uint32_t transfer_tag;
 	int F_bit, U_bit, O_bit, S_bit;
 	struct spdk_iscsi_task *primary;
+	struct spdk_scsi_lun *lun_dev;
 
 	primary = spdk_iscsi_task_get_primary(task);
 
@@ -2996,6 +2997,14 @@ iscsi_send_datain(struct spdk_iscsi_conn *conn,
 		to_be32(&rsph->res_cnt, residual_len);
 	}
 
+	lun_dev = spdk_scsi_dev_get_lun(conn->dev, task->lun_id);
+	if (spdk_likely(lun_dev != NULL)) {
+		if (spdk_unlikely(spdk_scsi_lun_get_dif_ctx(lun_dev, task->scsi.cdb, offset,
+				  &rsp_pdu->dif_ctx))) {
+			rsp_pdu->dif_insert_or_strip = true;
+		}
+	}
+
 	spdk_iscsi_conn_write_pdu(conn, rsp_pdu);
 
 	return DataSN;
@@ -3457,6 +3466,10 @@ iscsi_pdu_hdr_op_scsi(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu)
 			spdk_iscsi_task_put(task);
 			return iscsi_reject(conn, pdu, ISCSI_REASON_PROTOCOL_ERROR);
 		}
+
+		if (spdk_unlikely(spdk_scsi_lun_get_dif_ctx(task->scsi.lun, cdb, 0, &pdu->dif_ctx))) {
+			pdu->dif_insert_or_strip = true;
+		}
 	} else {
 		/* neither R nor W bit set */
 		task->scsi.dxfer_dir = SPDK_SCSI_DIR_NONE;
@@ -4486,6 +4499,11 @@ iscsi_pdu_hdr_op_data(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu)
 		return 0;
 	}
 
+	if (spdk_unlikely(spdk_scsi_lun_get_dif_ctx(lun_dev, subtask->scsi.cdb, buffer_offset,
+			  &pdu->dif_ctx))) {
+		pdu->dif_insert_or_strip = true;
+	}
+
 	pdu->task = subtask;
 	return 0;
 
@@ -4902,10 +4920,6 @@ iscsi_read_pdu(struct spdk_iscsi_conn *conn)
 				pdu->data_buf = pdu->mobj->buf;
 				pdu->data = pdu->mobj->buf;
 				pdu->data_from_mempool = true;
-
-				if (spdk_unlikely(spdk_iscsi_get_dif_ctx(conn, pdu, &pdu->dif_ctx))) {
-					pdu->dif_insert_or_strip = true;
-				}
 			}
 
 			/* copy the actual data into local buffer */
@@ -5011,84 +5025,6 @@ spdk_iscsi_handle_incoming_pdus(struct spdk_iscsi_conn *conn)
 	return i;
 }
 
-bool
-spdk_iscsi_get_dif_ctx(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu,
-		       struct spdk_dif_ctx *dif_ctx)
-{
-	struct iscsi_bhs *bhs;
-	uint32_t data_offset = 0;
-	uint8_t *cdb = NULL;
-	uint64_t lun;
-	int lun_id = 0;
-	struct spdk_scsi_lun *lun_dev;
-
-	/* connection is not in full feature phase but non-login opcode
-	 * was received.
-	 */
-	if ((!conn->full_feature && conn->state == ISCSI_CONN_STATE_RUNNING) ||
-	    conn->state == ISCSI_CONN_STATE_INVALID) {
-		return false;
-	}
-
-	/* SCSI Command is allowed only in normal session */
-	if (conn->sess == NULL ||
-	    conn->sess->session_type != SESSION_TYPE_NORMAL) {
-		return false;
-	}
-
-	bhs = &pdu->bhs;
-
-	switch (bhs->opcode) {
-	case ISCSI_OP_SCSI: {
-		struct iscsi_bhs_scsi_req *sbhs;
-
-		sbhs = (struct iscsi_bhs_scsi_req *)bhs;
-		data_offset = 0;
-		cdb = sbhs->cdb;
-		lun = from_be64(&sbhs->lun);
-		lun_id = spdk_scsi_lun_id_fmt_to_int(lun);
-		break;
-	}
-	case ISCSI_OP_SCSI_DATAOUT: {
-		struct iscsi_bhs_data_out *dbhs;
-		struct spdk_iscsi_task *task;
-		int transfer_tag;
-
-		dbhs = (struct iscsi_bhs_data_out *)bhs;
-		data_offset = from_be32(&dbhs->buffer_offset);
-		transfer_tag = from_be32(&dbhs->ttt);
-		task = get_transfer_task(conn, transfer_tag);
-		if (task == NULL) {
-			return false;
-		}
-		cdb = task->scsi.cdb;
-		lun_id = task->lun_id;
-		break;
-	}
-	case ISCSI_OP_SCSI_DATAIN: {
-		struct iscsi_bhs_data_in *dbhs;
-		struct spdk_iscsi_task *task;
-
-		dbhs = (struct iscsi_bhs_data_in *)bhs;
-		data_offset = from_be32(&dbhs->buffer_offset);
-		task = pdu->task;
-		assert(task != NULL);
-		cdb = task->scsi.cdb;
-		lun_id = task->lun_id;
-		break;
-	}
-	default:
-		return false;
-	}
-
-	lun_dev = spdk_scsi_dev_get_lun(conn->dev, lun_id);
-	if (lun_dev == NULL) {
-		return false;
-	}
-
-	return spdk_scsi_lun_get_dif_ctx(lun_dev, cdb, data_offset, dif_ctx);
-}
-
 bool spdk_iscsi_is_deferred_free_pdu(struct spdk_iscsi_pdu *pdu)
 {
 	if (pdu == NULL) {

lib/iscsi/iscsi.h

@@ -419,8 +419,6 @@ void spdk_iscsi_task_response(struct spdk_iscsi_conn *conn,
 int spdk_iscsi_build_iovs(struct spdk_iscsi_conn *conn, struct iovec *iovs, int iovcnt,
 			  struct spdk_iscsi_pdu *pdu, uint32_t *mapped_length);
 int spdk_iscsi_handle_incoming_pdus(struct spdk_iscsi_conn *conn);
-bool spdk_iscsi_get_dif_ctx(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu,
-			    struct spdk_dif_ctx *dif_ctx);
 void spdk_iscsi_task_mgmt_response(struct spdk_iscsi_conn *conn,
 				   struct spdk_iscsi_task *task);
 

test/unit/lib/iscsi/iscsi.c/iscsi_ut.c

@@ -121,6 +121,10 @@ DEFINE_STUB(spdk_scsi_lun_id_int_to_fmt, uint64_t, (int lun_id), 0);
 
 DEFINE_STUB(spdk_scsi_lun_id_fmt_to_int, int, (uint64_t lun_fmt), 0);
 
+DEFINE_STUB(spdk_scsi_lun_get_dif_ctx, bool,
+	    (struct spdk_scsi_lun *lun, uint8_t *cdb, uint32_t data_offset,
+	     struct spdk_dif_ctx *dif_ctx), false);
+
 static void
 op_login_check_target_test(void)
 {
@@ -347,6 +351,8 @@ underflow_for_read_transfer_test(void)
 	struct spdk_iscsi_sess sess;
 	struct spdk_iscsi_conn conn;
 	struct spdk_iscsi_task task;
+	struct spdk_scsi_dev dev;
+	struct spdk_scsi_lun lun;
 	struct spdk_iscsi_pdu *pdu;
 	struct iscsi_bhs_scsi_req *scsi_req;
 	struct iscsi_bhs_data_in *datah;
@@ -355,12 +361,17 @@ underflow_for_read_transfer_test(void)
 	memset(&sess, 0, sizeof(sess));
 	memset(&conn, 0, sizeof(conn));
 	memset(&task, 0, sizeof(task));
+	memset(&dev, 0, sizeof(dev));
+	memset(&lun, 0, sizeof(lun));
 
 	sess.MaxBurstLength = SPDK_ISCSI_MAX_BURST_LENGTH;
 
 	conn.sess = &sess;
 	conn.MaxRecvDataSegmentLength = 8192;
 
+	dev.lun[0] = &lun;
+	conn.dev = &dev;
+
 	pdu = spdk_get_pdu();
 	SPDK_CU_ASSERT_FATAL(pdu != NULL);
 
@@ -409,6 +420,8 @@ underflow_for_zero_read_transfer_test(void)
 	struct spdk_iscsi_sess sess;
 	struct spdk_iscsi_conn conn;
 	struct spdk_iscsi_task task;
+	struct spdk_scsi_dev dev;
+	struct spdk_scsi_lun lun;
 	struct spdk_iscsi_pdu *pdu;
 	struct iscsi_bhs_scsi_req *scsi_req;
 	struct iscsi_bhs_scsi_resp *resph;
@@ -417,12 +430,17 @@ underflow_for_zero_read_transfer_test(void)
 	memset(&sess, 0, sizeof(sess));
 	memset(&conn, 0, sizeof(conn));
 	memset(&task, 0, sizeof(task));
+	memset(&dev, 0, sizeof(dev));
+	memset(&lun, 0, sizeof(lun));
 
 	sess.MaxBurstLength = SPDK_ISCSI_MAX_BURST_LENGTH;
 
 	conn.sess = &sess;
 	conn.MaxRecvDataSegmentLength = 8192;
 
+	dev.lun[0] = &lun;
+	conn.dev = &dev;
+
 	pdu = spdk_get_pdu();
 	SPDK_CU_ASSERT_FATAL(pdu != NULL);
 
@@ -472,6 +490,8 @@ underflow_for_request_sense_test(void)
 	struct spdk_iscsi_sess sess;
 	struct spdk_iscsi_conn conn;
 	struct spdk_iscsi_task task;
+	struct spdk_scsi_dev dev;
+	struct spdk_scsi_lun lun;
 	struct spdk_iscsi_pdu *pdu1, *pdu2;
 	struct iscsi_bhs_scsi_req *scsi_req;
 	struct iscsi_bhs_data_in *datah;
@@ -481,12 +501,17 @@ underflow_for_request_sense_test(void)
 	memset(&sess, 0, sizeof(sess));
 	memset(&conn, 0, sizeof(conn));
 	memset(&task, 0, sizeof(task));
+	memset(&dev, 0, sizeof(dev));
+	memset(&lun, 0, sizeof(lun));
 
 	sess.MaxBurstLength = SPDK_ISCSI_MAX_BURST_LENGTH;
 
 	conn.sess = &sess;
 	conn.MaxRecvDataSegmentLength = 8192;
 
+	dev.lun[0] = &lun;
+	conn.dev = &dev;
+
 	pdu1 = spdk_get_pdu();
 	SPDK_CU_ASSERT_FATAL(pdu1 != NULL);
 
@@ -562,6 +587,8 @@ underflow_for_check_condition_test(void)
 	struct spdk_iscsi_sess sess;
 	struct spdk_iscsi_conn conn;
 	struct spdk_iscsi_task task;
+	struct spdk_scsi_dev dev;
+	struct spdk_scsi_lun lun;
 	struct spdk_iscsi_pdu *pdu;
 	struct iscsi_bhs_scsi_req *scsi_req;
 	struct iscsi_bhs_scsi_resp *resph;
@@ -570,12 +597,17 @@ underflow_for_check_condition_test(void)
 	memset(&sess, 0, sizeof(sess));
 	memset(&conn, 0, sizeof(conn));
 	memset(&task, 0, sizeof(task));
+	memset(&dev, 0, sizeof(dev));
+	memset(&lun, 0, sizeof(lun));
 
 	sess.MaxBurstLength = SPDK_ISCSI_MAX_BURST_LENGTH;
 
 	conn.sess = &sess;
 	conn.MaxRecvDataSegmentLength = 8192;
 
+	dev.lun[0] = &lun;
+	conn.dev = &dev;
+
 	pdu = spdk_get_pdu();
 	SPDK_CU_ASSERT_FATAL(pdu != NULL);