lib/iscsi: Make the max number of read subtasks for large read I/O configurable

For some use case that there is heavy large read I/O, the performance
bottleneck due to MAX_LARGE_DATAIN_PER_CONNECTION was reported.

The following assumes that all I/Os are large read.

Large read primary task whose I/O size is more than
SPDK_BDEV_LARGE_BUF_MAX_SIZE (=64KB) is split into multiple
read subtasks.

spdk_iscsi_globals::MaxQueueDepth limits maximum number of outstanding
read primary tasks, and MAX_LARGE_DATAIN_PER_CONNECTION (=64)
limits maximum number of outstanding read subtasks.

MAX_LARGE_DATAIN_PER_CONNECTION is also used to calculate PDU pool.

To remove the performance bottleneck, change the macro constant
MAX_LARGE_DATAIN_PER_CONNECTION to a global variable
spdk_iscsi_globals::MaxLargeDataInPerConnection.

We don't see any negative side effect if we set
spdk_iscsi_globals::MaxLargeDataInPerConnection to 64.

The use case that reported the performance issue will change the
value of spdk_iscsi_globals::MaxLargeDataInPerConnection by its own
responsibility.

The next patch will add the value of
spdk_iscsi_globals::MaxLargeDataInPerConnection to iSCSI options,
and make it configurable by JSON RPC.

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

lib/iscsi/conn.c

@@ -976,7 +976,7 @@ _iscsi_conn_abort_queued_datain_task(struct spdk_iscsi_conn *conn,
 	struct spdk_iscsi_task *subtask;
 	uint32_t remaining_size;
 
-	if (conn->data_in_cnt >= MAX_LARGE_DATAIN_PER_CONNECTION) {
+	if (conn->data_in_cnt >= g_iscsi.MaxLargeDataInPerConnection) {
 		return -1;
 	}
 
@@ -1048,7 +1048,7 @@ iscsi_conn_handle_queued_datain_tasks(struct spdk_iscsi_conn *conn)
 	struct spdk_iscsi_task *task;
 
 	while (!TAILQ_EMPTY(&conn->queued_datain_tasks) &&
-	       conn->data_in_cnt < MAX_LARGE_DATAIN_PER_CONNECTION) {
+	       conn->data_in_cnt < g_iscsi.MaxLargeDataInPerConnection) {
 		task = TAILQ_FIRST(&conn->queued_datain_tasks);
 		assert(task->current_datain_offset <= task->scsi.transfer_len);
 		if (task->current_datain_offset < task->scsi.transfer_len) {

lib/iscsi/iscsi.h

@@ -91,11 +91,11 @@
 #define MAX_DATA_OUT_PER_CONNECTION 16
 
 /*
- * Defines maximum number of data in buffers each connection can have in
+ * Defines default maximum number of data in buffers each connection can have in
  *  use at any given time. So this limit does not affect I/O smaller than
  *  SPDK_BDEV_SMALL_BUF_MAX_SIZE.
  */
-#define MAX_LARGE_DATAIN_PER_CONNECTION 64
+#define DEFAULT_MAX_LARGE_DATAIN_PER_CONNECTION 64
 
 #define SPDK_ISCSI_MAX_BURST_LENGTH	\
 		(SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH * MAX_DATA_OUT_PER_CONNECTION)
@@ -351,6 +351,7 @@ struct spdk_iscsi_globals {
 	bool ImmediateData;
 	uint32_t ErrorRecoveryLevel;
 	bool AllowDuplicateIsid;
+	uint32_t MaxLargeDataInPerConnection;
 
 	struct spdk_mempool *pdu_pool;
 	struct spdk_mempool *pdu_immediate_data_pool;

lib/iscsi/iscsi_subsystem.c

@@ -139,7 +139,7 @@ mobj_ctor(struct spdk_mempool *mp, __attribute__((unused)) void *arg,
 			  ~ISCSI_DATA_BUFFER_MASK);
 }
 
-#define NUM_PDU_PER_CONNECTION(iscsi)	(2 * (iscsi->MaxQueueDepth + MAX_LARGE_DATAIN_PER_CONNECTION + 8))
+#define NUM_PDU_PER_CONNECTION(iscsi)	(2 * (iscsi->MaxQueueDepth + iscsi->MaxLargeDataInPerConnection + 8))
 #define PDU_POOL_SIZE(iscsi)		(iscsi->MaxConnections * NUM_PDU_PER_CONNECTION(iscsi))
 #define IMMEDIATE_DATA_POOL_SIZE(iscsi)	(iscsi->MaxConnections * 128)
 #define DATA_OUT_POOL_SIZE(iscsi)	(iscsi->MaxConnections * MAX_DATA_OUT_PER_CONNECTION)
@@ -376,6 +376,9 @@ iscsi_log_globals(void)
 			      "DiscoveryAuthGroup AuthGroup%d\n",
 			      g_iscsi.chap_group);
 	}
+
+	SPDK_DEBUGLOG(SPDK_LOG_ISCSI, "MaxLargeDataInPerConnection %d\n",
+		      g_iscsi.MaxLargeDataInPerConnection);
 }
 
 static void
@@ -767,6 +770,7 @@ iscsi_set_global_params(struct spdk_iscsi_opts *opts)
 	g_iscsi.require_chap = opts->require_chap;
 	g_iscsi.mutual_chap = opts->mutual_chap;
 	g_iscsi.chap_group = opts->chap_group;
+	g_iscsi.MaxLargeDataInPerConnection = DEFAULT_MAX_LARGE_DATAIN_PER_CONNECTION;
 
 	iscsi_log_globals();
 

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

@@ -56,7 +56,10 @@ struct spdk_scsi_lun {
 	uint8_t reserved;
 };
 
-struct spdk_iscsi_globals g_iscsi;
+struct spdk_iscsi_globals g_iscsi = {
+	.MaxLargeDataInPerConnection = DEFAULT_MAX_LARGE_DATAIN_PER_CONNECTION,
+};
+
 static TAILQ_HEAD(read_tasks_head, spdk_iscsi_task) g_ut_read_tasks =
 	TAILQ_HEAD_INITIALIZER(g_ut_read_tasks);
 static struct spdk_iscsi_task *g_new_task = NULL;
@@ -570,7 +573,7 @@ free_tasks_on_connection(void)
 	TAILQ_INIT(&conn.write_pdu_list);
 	TAILQ_INIT(&conn.snack_pdu_list);
 	TAILQ_INIT(&conn.queued_datain_tasks);
-	conn.data_in_cnt = MAX_LARGE_DATAIN_PER_CONNECTION;
+	conn.data_in_cnt = g_iscsi.MaxLargeDataInPerConnection;
 
 	pdu1.task = &task1;
 	pdu2.task = &task2;
@@ -724,7 +727,7 @@ abort_queued_datain_task_test(void)
 	TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task, link);
 
 	/* No slots for sub read tasks */
-	conn.data_in_cnt = MAX_LARGE_DATAIN_PER_CONNECTION;
+	conn.data_in_cnt = g_iscsi.MaxLargeDataInPerConnection;
 	rc = _iscsi_conn_abort_queued_datain_task(&conn, &task);
 	CU_ASSERT(rc != 0);
 	CU_ASSERT(!TAILQ_EMPTY(&conn.queued_datain_tasks));
@@ -747,7 +750,7 @@ abort_queued_datain_task_test(void)
 	TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task, link);
 
 	/* No slots for sub read tasks */
-	conn.data_in_cnt = MAX_LARGE_DATAIN_PER_CONNECTION;
+	conn.data_in_cnt = g_iscsi.MaxLargeDataInPerConnection;
 	rc = _iscsi_conn_abort_queued_datain_task(&conn, &task);
 	CU_ASSERT(rc != 0);
 	CU_ASSERT(!TAILQ_EMPTY(&conn.queued_datain_tasks));