[PATCH v6 0/2] tcmu: Dynamic growing data area support

[PATCH v6 0/2] tcmu: Dynamic growing data area support

[lixiubo]

From: Xiubo Li <lixiubo@cmss.chinamobile.com>

Changed for V6:
- Remove the tcmu_vma_close(). Since the unmap thread will do the same for it 
- The unmap thread will skip the busy devices.
- Using and testing the V5 version 3 weeks and the V6 for about 1 week, all in a higher IOPS environment:
  * using fio and dd commands
  * using about 4 targets based user:rbd/user:file backend
  * set the global pool size to 512 * 1024 blocks * block_size, for 4K page size, the size is 2G.
  * each target here needs more than 1100 blocks.
  * fio: -iodepth 16 -thread -rw=[read write] -bs=[1M] -size=20G -numjobs=10 -runtime=1000  ...
  * restart the tcmu-runner at any time.

Changed for V5:
- Rebase to the newest target-pending repository.
- Add as many comments as possbile to make the patch more readable.
- Move tcmu_handle_completions() in timeout handler to unmap thread
  and then replace the spin lock with mutex lock(because the unmap_*
  or zap_* may goto sleep) to simplify the patch and the code.
- Thanks very much for Mike's tips and suggestions.
- Tested this for more than 3 days by:
  * using fio and dd commands
  * using about 1~5 targets
  * set the global pool size to [512 1024 2048 512 * 1024] blocks * block_size
  * each target here needs more than 450 blocks when running in my environments.
  * fio: -iodepth [1 2 4 8 16] -thread -rw=[read write] -bs=[1K 2K 3K 5K 7K 16K 64K 1M] -size=20G -numjobs=10 -runtime=1000  ...
  * in the tcmu-runner, try to touch blocks out of tcmu_cmds' iov[] manually
  * restart the tcmu-runner at any time.
  * in my environment for the low IOPS case: the read throughput goes from about 5200KB/s to 6700KB/s; the write throughput goes from about 3000KB/s to 3700KB/s.

Xiubo Li (2):
  tcmu: Add dynamic growing data area feature support
  tcmu: Add global data block pool support

 drivers/target/target_core_user.c | 598 ++++++++++++++++++++++++++++++--------
 1 file changed, 469 insertions(+), 129 deletions(-)

-- 
1.8.3.1

[PATCH v6 1/2] tcmu: Add dynamic growing data area feature support

[lixiubo]

From: Xiubo Li <lixiubo@cmss.chinamobile.com>

Currently for the TCMU, the ring buffer size is fixed to 64K cmd
area + 1M data area, and this will be bottlenecks for high iops.

The struct tcmu_cmd_entry {} size is fixed about 112 bytes with
iovec[N] & N <= 4, and the size of struct iovec is about 16 bytes.

If N == 0, the ratio will be sizeof(cmd entry) : sizeof(datas) ==
112Bytes : (N * 4096)Bytes = 28 : 0, no data area is need.

If 0 < N <=4, the ratio will be sizeof(cmd entry) : sizeof(datas)
== 112Bytes : (N * 4096)Bytes = 28 : (N * 1024), so the max will
be 28 : 1024.

If N > 4, the sizeof(cmd entry) will be [(N - 4) *16 + 112] bytes,
and its corresponding data size will be [N * 4096], so the ratio
of sizeof(cmd entry) : sizeof(datas) == [(N - 4) * 16 + 112)Bytes
: (N * 4096)Bytes == 4/1024 - 12/(N * 1024), so the max is about
4 : 1024.

When N is bigger, the ratio will be smaller.

As the initial patch, we will set the cmd area size to 2M, and
the cmd area size to 32M. The TCMU will dynamically grows the data
area from 0 to max 32M size as needed.

The cmd area memory will be allocated through vmalloc(), and the
data area's blocks will be allocated individually later when needed.

The allocated data area block memory will be managed via radix tree.
For now the bitmap still be the most efficient way to search and
manage the block index, this could be update later.

Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
Signed-off-by: Jianfei Hu <hujianfei@cmss.chinamobile.com>
---
 drivers/target/target_core_user.c | 334 ++++++++++++++++++++++++++------------
 1 file changed, 233 insertions(+), 101 deletions(-)

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index f615c3b..8491752 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -2,6 +2,7 @@
  * Copyright (C) 2013 Shaohua Li <shli@kernel.org>
  * Copyright (C) 2014 Red Hat, Inc.
  * Copyright (C) 2015 Arrikto, Inc.
+ * Copyright (C) 2017 Chinamobile, Inc.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -25,6 +26,7 @@
 #include <linux/parser.h>
 #include <linux/vmalloc.h>
 #include <linux/uio_driver.h>
+#include <linux/radix-tree.h>
 #include <linux/stringify.h>
 #include <linux/bitops.h>
 #include <linux/highmem.h>
@@ -63,15 +65,17 @@
  * this may have a 'UAM' comment.
  */
 
-
 #define TCMU_TIME_OUT (30 * MSEC_PER_SEC)
 
-#define DATA_BLOCK_BITS 256
-#define DATA_BLOCK_SIZE 4096
+/* For cmd area, the size is fixed 2M */
+#define CMDR_SIZE (2 * 1024 * 1024)
 
-#define CMDR_SIZE (16 * 4096)
+/* For data area, the size is fixed 32M */
+#define DATA_BLOCK_BITS (8 * 1024)
+#define DATA_BLOCK_SIZE 4096
 #define DATA_SIZE (DATA_BLOCK_BITS * DATA_BLOCK_SIZE)
 
+/* The ring buffer size is 34M */
 #define TCMU_RING_SIZE (CMDR_SIZE + DATA_SIZE)
 
 static struct device *tcmu_root_device;
@@ -103,12 +107,14 @@ struct tcmu_dev {
 	size_t data_off;
 	size_t data_size;
 
-	DECLARE_BITMAP(data_bitmap, DATA_BLOCK_BITS);
-
 	wait_queue_head_t wait_cmdr;
 	/* TODO should this be a mutex? */
 	spinlock_t cmdr_lock;
 
+	uint32_t dbi_max;
+	DECLARE_BITMAP(data_bitmap, DATA_BLOCK_BITS);
+	struct radix_tree_root data_blocks;
+
 	struct idr commands;
 	spinlock_t commands_lock;
 
@@ -130,7 +136,9 @@ struct tcmu_cmd {
 
 	/* Can't use se_cmd when cleaning up expired cmds, because if
 	   cmd has been completed then accessing se_cmd is off limits */
-	DECLARE_BITMAP(data_bitmap, DATA_BLOCK_BITS);
+	uint32_t dbi_cnt;
+	uint32_t dbi_cur;
+	uint32_t *dbi;
 
 	unsigned long deadline;
 
@@ -161,6 +169,84 @@ enum tcmu_multicast_groups {
 	.netnsok = true,
 };
 
+#define tcmu_cmd_set_dbi_cur(cmd, index) ((cmd)->dbi_cur = (index))
+#define tcmu_cmd_reset_dbi_cur(cmd) tcmu_cmd_set_dbi_cur(cmd, 0)
+#define tcmu_cmd_set_dbi(cmd, index) ((cmd)->dbi[(cmd)->dbi_cur++] = (index))
+#define tcmu_cmd_get_dbi(cmd) ((cmd)->dbi[(cmd)->dbi_cur++])
+
+static void tcmu_cmd_free_data(struct tcmu_cmd *tcmu_cmd)
+{
+	struct tcmu_dev *udev = tcmu_cmd->tcmu_dev;
+	uint32_t i;
+
+	for (i = 0; i < tcmu_cmd->dbi_cnt; i++)
+		clear_bit(tcmu_cmd->dbi[i], udev->data_bitmap);
+}
+
+static int tcmu_get_empty_block(struct tcmu_dev *udev, void **addr)
+{
+	void *p;
+	uint32_t dbi;
+	int ret;
+
+	dbi = find_first_zero_bit(udev->data_bitmap, DATA_BLOCK_BITS);
+	if (dbi > udev->dbi_max)
+		udev->dbi_max = dbi;
+
+	set_bit(dbi, udev->data_bitmap);
+
+	p = radix_tree_lookup(&udev->data_blocks, dbi);
+	if (!p) {
+		p = kzalloc(DATA_BLOCK_SIZE, GFP_ATOMIC);
+		if (!p) {
+			clear_bit(dbi, udev->data_bitmap);
+			return -ENOMEM;
+		}
+
+		ret = radix_tree_insert(&udev->data_blocks, dbi, p);
+		if (ret) {
+			kfree(p);
+			clear_bit(dbi, udev->data_bitmap);
+			return ret;
+		}
+	}
+
+	*addr = p;
+	return dbi;
+}
+
+static void *tcmu_get_block_addr(struct tcmu_dev *udev, uint32_t dbi)
+{
+	return radix_tree_lookup(&udev->data_blocks, dbi);
+}
+
+static inline void tcmu_free_cmd(struct tcmu_cmd *tcmu_cmd)
+{
+	kfree(tcmu_cmd->dbi);
+	kmem_cache_free(tcmu_cmd_cache, tcmu_cmd);
+}
+
+static inline size_t tcmu_cmd_get_data_length(struct tcmu_cmd *tcmu_cmd)
+{
+	struct se_cmd *se_cmd = tcmu_cmd->se_cmd;
+	size_t data_length = round_up(se_cmd->data_length, DATA_BLOCK_SIZE);
+
+	if (se_cmd->se_cmd_flags & SCF_BIDI) {
+		BUG_ON(!(se_cmd->t_bidi_data_sg && se_cmd->t_bidi_data_nents));
+		data_length += round_up(se_cmd->t_bidi_data_sg->length,
+				DATA_BLOCK_SIZE);
+	}
+
+	return data_length;
+}
+
+static inline uint32_t tcmu_cmd_get_block_cnt(struct tcmu_cmd *tcmu_cmd)
+{
+	size_t data_length = tcmu_cmd_get_data_length(tcmu_cmd);
+
+	return data_length / DATA_BLOCK_SIZE;
+}
+
 static struct tcmu_cmd *tcmu_alloc_cmd(struct se_cmd *se_cmd)
 {
 	struct se_device *se_dev = se_cmd->se_dev;
@@ -178,6 +264,15 @@ static struct tcmu_cmd *tcmu_alloc_cmd(struct se_cmd *se_cmd)
 		tcmu_cmd->deadline = jiffies +
 					msecs_to_jiffies(udev->cmd_time_out);
 
+	tcmu_cmd_reset_dbi_cur(tcmu_cmd);
+	tcmu_cmd->dbi_cnt = tcmu_cmd_get_block_cnt(tcmu_cmd);
+	tcmu_cmd->dbi = kcalloc(tcmu_cmd->dbi_cnt, sizeof(uint32_t),
+				GFP_KERNEL);
+	if (!tcmu_cmd->dbi) {
+		kmem_cache_free(tcmu_cmd_cache, tcmu_cmd);
+		return NULL;
+	}
+
 	idr_preload(GFP_KERNEL);
 	spin_lock_irq(&udev->commands_lock);
 	cmd_id = idr_alloc(&udev->commands, tcmu_cmd, 0,
@@ -186,7 +281,7 @@ static struct tcmu_cmd *tcmu_alloc_cmd(struct se_cmd *se_cmd)
 	idr_preload_end();
 
 	if (cmd_id < 0) {
-		kmem_cache_free(tcmu_cmd_cache, tcmu_cmd);
+		tcmu_free_cmd(tcmu_cmd);
 		return NULL;
 	}
 	tcmu_cmd->cmd_id = cmd_id;
@@ -248,10 +343,10 @@ static inline void new_iov(struct iovec **iov, int *iov_cnt,
 #define UPDATE_HEAD(head, used, size) smp_store_release(&head, ((head % size) + used) % size)
 
 /* offset is relative to mb_addr */
-static inline size_t get_block_offset(struct tcmu_dev *dev,
-		int block, int remaining)
+static inline size_t get_block_offset_user(struct tcmu_dev *dev,
+		int dbi, int remaining)
 {
-	return dev->data_off + block * DATA_BLOCK_SIZE +
+	return dev->data_off + dbi * DATA_BLOCK_SIZE +
 		DATA_BLOCK_SIZE - remaining;
 }
 
@@ -260,14 +355,15 @@ static inline size_t iov_tail(struct tcmu_dev *udev, struct iovec *iov)
 	return (size_t)iov->iov_base + iov->iov_len;
 }
 
-static void alloc_and_scatter_data_area(struct tcmu_dev *udev,
-	struct scatterlist *data_sg, unsigned int data_nents,
-	struct iovec **iov, int *iov_cnt, bool copy_data)
+static int alloc_and_scatter_data_area(struct tcmu_dev *udev,
+	struct tcmu_cmd *tcmu_cmd, struct scatterlist *data_sg,
+	unsigned int data_nents, struct iovec **iov,
+	int *iov_cnt, bool copy_data)
 {
-	int i, block;
+	int i, dbi;
 	int block_remaining = 0;
-	void *from, *to;
-	size_t copy_bytes, to_offset;
+	void *from, *to = NULL;
+	size_t copy_bytes, to_offset, offset;
 	struct scatterlist *sg;
 
 	for_each_sg(data_sg, sg, data_nents, i) {
@@ -275,22 +371,26 @@ static void alloc_and_scatter_data_area(struct tcmu_dev *udev,
 		from = kmap_atomic(sg_page(sg)) + sg->offset;
 		while (sg_remaining > 0) {
 			if (block_remaining == 0) {
-				block = find_first_zero_bit(udev->data_bitmap,
-						DATA_BLOCK_BITS);
 				block_remaining = DATA_BLOCK_SIZE;
-				set_bit(block, udev->data_bitmap);
+				dbi = tcmu_get_empty_block(udev, &to);
+				if (dbi < 0)
+					return dbi;
+				tcmu_cmd_set_dbi(tcmu_cmd, dbi);
 			}
+
 			copy_bytes = min_t(size_t, sg_remaining,
 					block_remaining);
-			to_offset = get_block_offset(udev, block,
+			to_offset = get_block_offset_user(udev, dbi,
 					block_remaining);
-			to = (void *)udev->mb_addr + to_offset;
+			offset = DATA_BLOCK_SIZE - block_remaining;
+			to = (void *)(unsigned long)to + offset;
+
 			if (*iov_cnt != 0 &&
 			    to_offset == iov_tail(udev, *iov)) {
 				(*iov)->iov_len += copy_bytes;
 			} else {
 				new_iov(iov, iov_cnt, udev);
-				(*iov)->iov_base = (void __user *) to_offset;
+				(*iov)->iov_base = (void __user *)to_offset;
 				(*iov)->iov_len = copy_bytes;
 			}
 			if (copy_data) {
@@ -303,33 +403,26 @@ static void alloc_and_scatter_data_area(struct tcmu_dev *udev,
 		}
 		kunmap_atomic(from - sg->offset);
 	}
-}
 
-static void free_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd)
-{
-	bitmap_xor(udev->data_bitmap, udev->data_bitmap, cmd->data_bitmap,
-		   DATA_BLOCK_BITS);
+	return 0;
 }
 
 static void gather_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
 			     bool bidi)
 {
 	struct se_cmd *se_cmd = cmd->se_cmd;
-	int i, block;
+	int i, dbi;
 	int block_remaining = 0;
 	void *from, *to;
-	size_t copy_bytes, from_offset;
+	size_t copy_bytes, offset;
 	struct scatterlist *sg, *data_sg;
 	unsigned int data_nents;
-	DECLARE_BITMAP(bitmap, DATA_BLOCK_BITS);
-
-	bitmap_copy(bitmap, cmd->data_bitmap, DATA_BLOCK_BITS);
+	uint32_t count = 0;
 
 	if (!bidi) {
 		data_sg = se_cmd->t_data_sg;
 		data_nents = se_cmd->t_data_nents;
 	} else {
-		uint32_t count;
 
 		/*
 		 * For bidi case, the first count blocks are for Data-Out
@@ -337,30 +430,26 @@ static void gather_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
 		 * the Data-Out buffer blocks should be discarded.
 		 */
 		count = DIV_ROUND_UP(se_cmd->data_length, DATA_BLOCK_SIZE);
-		while (count--) {
-			block = find_first_bit(bitmap, DATA_BLOCK_BITS);
-			clear_bit(block, bitmap);
-		}
 
 		data_sg = se_cmd->t_bidi_data_sg;
 		data_nents = se_cmd->t_bidi_data_nents;
 	}
 
+	tcmu_cmd_set_dbi_cur(cmd, count);
+
 	for_each_sg(data_sg, sg, data_nents, i) {
 		int sg_remaining = sg->length;
 		to = kmap_atomic(sg_page(sg)) + sg->offset;
 		while (sg_remaining > 0) {
 			if (block_remaining == 0) {
-				block = find_first_bit(bitmap,
-						DATA_BLOCK_BITS);
 				block_remaining = DATA_BLOCK_SIZE;
-				clear_bit(block, bitmap);
+				dbi = tcmu_cmd_get_dbi(cmd);
+				from = tcmu_get_block_addr(udev, dbi);
 			}
 			copy_bytes = min_t(size_t, sg_remaining,
 					block_remaining);
-			from_offset = get_block_offset(udev, block,
-					block_remaining);
-			from = (void *) udev->mb_addr + from_offset;
+			offset = DATA_BLOCK_SIZE - block_remaining;
+			from = (void *)(unsigned long)from + offset;
 			tcmu_flush_dcache_range(from, copy_bytes);
 			memcpy(to + sg->length - sg_remaining, from,
 					copy_bytes);
@@ -420,27 +509,6 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 	return true;
 }
 
-static inline size_t tcmu_cmd_get_data_length(struct tcmu_cmd *tcmu_cmd)
-{
-	struct se_cmd *se_cmd = tcmu_cmd->se_cmd;
-	size_t data_length = round_up(se_cmd->data_length, DATA_BLOCK_SIZE);
-
-	if (se_cmd->se_cmd_flags & SCF_BIDI) {
-		BUG_ON(!(se_cmd->t_bidi_data_sg && se_cmd->t_bidi_data_nents));
-		data_length += round_up(se_cmd->t_bidi_data_sg->length,
-				DATA_BLOCK_SIZE);
-	}
-
-	return data_length;
-}
-
-static inline uint32_t tcmu_cmd_get_block_cnt(struct tcmu_cmd *tcmu_cmd)
-{
-	size_t data_length = tcmu_cmd_get_data_length(tcmu_cmd);
-
-	return data_length / DATA_BLOCK_SIZE;
-}
-
 static sense_reason_t
 tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
 {
@@ -450,12 +518,11 @@ static inline uint32_t tcmu_cmd_get_block_cnt(struct tcmu_cmd *tcmu_cmd)
 	struct tcmu_mailbox *mb;
 	struct tcmu_cmd_entry *entry;
 	struct iovec *iov;
-	int iov_cnt;
+	int iov_cnt, ret;
 	uint32_t cmd_head;
 	uint64_t cdb_off;
 	bool copy_to_data_area;
 	size_t data_length = tcmu_cmd_get_data_length(tcmu_cmd);
-	DECLARE_BITMAP(old_bitmap, DATA_BLOCK_BITS);
 
 	if (test_bit(TCMU_DEV_BIT_BROKEN, &udev->flags))
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
@@ -539,15 +606,18 @@ static inline uint32_t tcmu_cmd_get_block_cnt(struct tcmu_cmd *tcmu_cmd)
 	entry->hdr.kflags = 0;
 	entry->hdr.uflags = 0;
 
-	bitmap_copy(old_bitmap, udev->data_bitmap, DATA_BLOCK_BITS);
-
 	/* Handle allocating space from the data area */
 	iov = &entry->req.iov[0];
 	iov_cnt = 0;
 	copy_to_data_area = (se_cmd->data_direction == DMA_TO_DEVICE
 		|| se_cmd->se_cmd_flags & SCF_BIDI);
-	alloc_and_scatter_data_area(udev, se_cmd->t_data_sg,
-		se_cmd->t_data_nents, &iov, &iov_cnt, copy_to_data_area);
+	ret = alloc_and_scatter_data_area(udev, tcmu_cmd,
+			se_cmd->t_data_sg, se_cmd->t_data_nents,
+			&iov, &iov_cnt, copy_to_data_area);
+	if (ret) {
+		pr_err("tcmu: alloc and scatter data failed\n");
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+	}
 	entry->req.iov_cnt = iov_cnt;
 	entry->req.iov_dif_cnt = 0;
 
@@ -555,14 +625,16 @@ static inline uint32_t tcmu_cmd_get_block_cnt(struct tcmu_cmd *tcmu_cmd)
 	if (se_cmd->se_cmd_flags & SCF_BIDI) {
 		iov_cnt = 0;
 		iov++;
-		alloc_and_scatter_data_area(udev, se_cmd->t_bidi_data_sg,
-				se_cmd->t_bidi_data_nents, &iov, &iov_cnt,
-				false);
+		ret = alloc_and_scatter_data_area(udev, tcmu_cmd,
+					se_cmd->t_bidi_data_sg,
+					se_cmd->t_bidi_data_nents,
+					&iov, &iov_cnt, false);
+		if (ret) {
+			pr_err("tcmu: alloc and scatter bidi data failed\n");
+			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+		}
 		entry->req.iov_bidi_cnt = iov_cnt;
 	}
-	/* cmd's data_bitmap is what changed in process */
-	bitmap_xor(tcmu_cmd->data_bitmap, old_bitmap, udev->data_bitmap,
-			DATA_BLOCK_BITS);
 
 	/* All offsets relative to mb_addr, not start of entry! */
 	cdb_off = CMDR_OFF + cmd_head + base_command_size;
@@ -604,7 +676,7 @@ static inline uint32_t tcmu_cmd_get_block_cnt(struct tcmu_cmd *tcmu_cmd)
 		idr_remove(&udev->commands, tcmu_cmd->cmd_id);
 		spin_unlock_irq(&udev->commands_lock);
 
-		kmem_cache_free(tcmu_cmd_cache, tcmu_cmd);
+		tcmu_free_cmd(tcmu_cmd);
 	}
 
 	return ret;
@@ -615,44 +687,40 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
 	struct se_cmd *se_cmd = cmd->se_cmd;
 	struct tcmu_dev *udev = cmd->tcmu_dev;
 
-	if (test_bit(TCMU_CMD_BIT_EXPIRED, &cmd->flags)) {
-		/*
-		 * cmd has been completed already from timeout, just reclaim
-		 * data area space and free cmd
-		 */
-		free_data_area(udev, cmd);
+	/*
+	 * cmd has been completed already from timeout, just reclaim
+	 * data area space and free cmd
+	 */
+	if (test_bit(TCMU_CMD_BIT_EXPIRED, &cmd->flags))
+		goto out;
 
-		kmem_cache_free(tcmu_cmd_cache, cmd);
-		return;
-	}
+	tcmu_cmd_reset_dbi_cur(cmd);
 
 	if (entry->hdr.uflags & TCMU_UFLAG_UNKNOWN_OP) {
-		free_data_area(udev, cmd);
 		pr_warn("TCMU: Userspace set UNKNOWN_OP flag on se_cmd %p\n",
 			cmd->se_cmd);
 		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
 	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
 		memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
 			       se_cmd->scsi_sense_length);
-		free_data_area(udev, cmd);
 	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
 		/* Get Data-In buffer before clean up */
 		gather_data_area(udev, cmd, true);
-		free_data_area(udev, cmd);
 	} else if (se_cmd->data_direction == DMA_FROM_DEVICE) {
 		gather_data_area(udev, cmd, false);
-		free_data_area(udev, cmd);
 	} else if (se_cmd->data_direction == DMA_TO_DEVICE) {
-		free_data_area(udev, cmd);
+		/* TODO: */
 	} else if (se_cmd->data_direction != DMA_NONE) {
 		pr_warn("TCMU: data direction was %d!\n",
 			se_cmd->data_direction);
 	}
 
 	target_complete_cmd(cmd->se_cmd, entry->rsp.scsi_status);
-	cmd->se_cmd = NULL;
 
-	kmem_cache_free(tcmu_cmd_cache, cmd);
+out:
+	cmd->se_cmd = NULL;
+	tcmu_cmd_free_data(cmd);
+	tcmu_free_cmd(cmd);
 }
 
 static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
@@ -810,6 +878,54 @@ static int tcmu_irqcontrol(struct uio_info *info, s32 irq_on)
 	return 0;
 }
 
+static void tcmu_blocks_release(struct tcmu_dev *udev, bool release_pending)
+{
+	uint32_t dbi, end;
+	void *addr;
+
+	spin_lock_irq(&udev->cmdr_lock);
+
+	end = udev->dbi_max + 1;
+
+	/* try to release all unused blocks */
+	dbi = find_first_zero_bit(udev->data_bitmap, end);
+	if (dbi >= end) {
+		spin_unlock_irq(&udev->cmdr_lock);
+		return;
+	}
+	do {
+		addr = radix_tree_delete(&udev->data_blocks, dbi);
+		kfree(addr);
+
+		dbi = find_next_zero_bit(udev->data_bitmap, end, dbi + 1);
+	} while (dbi < end);
+
+	if (!release_pending)
+		return;
+
+	/* try to release all pending blocks */
+	dbi = find_first_bit(udev->data_bitmap, end);
+	if (dbi >= end) {
+		spin_unlock_irq(&udev->cmdr_lock);
+		return;
+	}
+	do {
+		addr = radix_tree_delete(&udev->data_blocks, dbi);
+		kfree(addr);
+
+		dbi = find_next_bit(udev->data_bitmap, end, dbi + 1);
+	} while (dbi < end);
+
+	spin_unlock_irq(&udev->cmdr_lock);
+}
+
+static void tcmu_vma_close(struct vm_area_struct *vma)
+{
+	struct tcmu_dev *udev = vma->vm_private_data;
+
+	tcmu_blocks_release(udev, false);
+}
+
 /*
  * mmap code from uio.c. Copied here because we want to hook mmap()
  * and this stuff must come along.
@@ -845,17 +961,28 @@ static int tcmu_vma_fault(struct vm_fault *vmf)
 	 */
 	offset = (vmf->pgoff - mi) << PAGE_SHIFT;
 
-	addr = (void *)(unsigned long)info->mem[mi].addr + offset;
-	if (info->mem[mi].memtype == UIO_MEM_LOGICAL)
-		page = virt_to_page(addr);
-	else
+	if (offset < udev->data_off) {
+		/* For the vmalloc()ed cmd area pages */
+		addr = (void *)(unsigned long)info->mem[mi].addr + offset;
 		page = vmalloc_to_page(addr);
+	} else {
+		/* For the dynamically growing data area pages */
+		uint32_t dbi;
+
+		dbi = (offset - udev->data_off) / DATA_BLOCK_SIZE;
+		addr = tcmu_get_block_addr(udev, dbi);
+		if (!addr)
+			return VM_FAULT_NOPAGE;
+		page = virt_to_page(addr);
+	}
+
 	get_page(page);
 	vmf->page = page;
 	return 0;
 }
 
 static const struct vm_operations_struct tcmu_vm_ops = {
+	.close = tcmu_vma_close,
 	.fault = tcmu_vma_fault,
 };
 
@@ -963,7 +1090,7 @@ static int tcmu_configure_device(struct se_device *dev)
 
 	info->name = str;
 
-	udev->mb_addr = vzalloc(TCMU_RING_SIZE);
+	udev->mb_addr = vzalloc(CMDR_SIZE);
 	if (!udev->mb_addr) {
 		ret = -ENOMEM;
 		goto err_vzalloc;
@@ -972,8 +1099,9 @@ static int tcmu_configure_device(struct se_device *dev)
 	/* mailbox fits in first part of CMDR space */
 	udev->cmdr_size = CMDR_SIZE - CMDR_OFF;
 	udev->data_off = CMDR_SIZE;
-	udev->data_size = TCMU_RING_SIZE - CMDR_SIZE;
+	udev->data_size = DATA_SIZE;
 
+	/* Initialise the mailbox of the ring buffer */
 	mb = udev->mb_addr;
 	mb->version = TCMU_MAILBOX_VERSION;
 	mb->flags = TCMU_MAILBOX_FLAG_CAP_OOOC;
@@ -984,12 +1112,14 @@ static int tcmu_configure_device(struct se_device *dev)
 	WARN_ON(udev->data_size % PAGE_SIZE);
 	WARN_ON(udev->data_size % DATA_BLOCK_SIZE);
 
+	INIT_RADIX_TREE(&udev->data_blocks, GFP_ATOMIC);
+
 	info->version = __stringify(TCMU_MAILBOX_VERSION);
 
 	info->mem[0].name = "tcm-user command & data buffer";
 	info->mem[0].addr = (phys_addr_t)(uintptr_t)udev->mb_addr;
 	info->mem[0].size = TCMU_RING_SIZE;
-	info->mem[0].memtype = UIO_MEM_VIRTUAL;
+	info->mem[0].memtype = UIO_MEM_NONE;
 
 	info->irqcontrol = tcmu_irqcontrol;
 	info->irq = UIO_IRQ_CUSTOM;
@@ -1070,6 +1200,8 @@ static void tcmu_free_device(struct se_device *dev)
 	spin_unlock_irq(&udev->commands_lock);
 	WARN_ON(!all_expired);
 
+	tcmu_blocks_release(udev, true);
+
 	if (tcmu_dev_configured(udev)) {
 		tcmu_netlink_event(TCMU_CMD_REMOVED_DEVICE, udev->uio_info.name,
 				   udev->uio_info.uio_dev->minor);
-- 
1.8.3.1

Re: [PATCH v6 1/2] tcmu: Add dynamic growing data area feature support

[Mike Christie]

On 04/26/2017 01:25 AM, lixiubo@cmss.chinamobile.com wrote:
>  	for_each_sg(data_sg, sg, data_nents, i) {
> @@ -275,22 +371,26 @@ static void alloc_and_scatter_data_area(struct tcmu_dev *udev,
>  		from = kmap_atomic(sg_page(sg)) + sg->offset;
>  		while (sg_remaining > 0) {
>  			if (block_remaining == 0) {
> -				block = find_first_zero_bit(udev->data_bitmap,
> -						DATA_BLOCK_BITS);
>  				block_remaining = DATA_BLOCK_SIZE;
> -				set_bit(block, udev->data_bitmap);
> +				dbi = tcmu_get_empty_block(udev, &to);
> +				if (dbi < 0)


I know it you fixed the missing kunmap_atomic here and missing unlock in
tcmu_queue_cmd_ring in the next patch, but I think normally people
prefer that one patch does not add a bug, then the next patch fixes it.

Re: [PATCH v6 1/2] tcmu: Add dynamic growing data area featuresupport

[Xiubo Li]

On 2017年04月30日 13:48, Mike Christie wrote:
> On 04/26/2017 01:25 AM, lixiubo@cmss.chinamobile.com wrote:
>>   	for_each_sg(data_sg, sg, data_nents, i) {
>> @@ -275,22 +371,26 @@ static void alloc_and_scatter_data_area(struct tcmu_dev *udev,
>>   		from = kmap_atomic(sg_page(sg)) + sg->offset;
>>   		while (sg_remaining > 0) {
>>   			if (block_remaining == 0) {
>> -				block = find_first_zero_bit(udev->data_bitmap,
>> -						DATA_BLOCK_BITS);
>>   				block_remaining = DATA_BLOCK_SIZE;
>> -				set_bit(block, udev->data_bitmap);
>> +				dbi = tcmu_get_empty_block(udev, &to);
>> +				if (dbi < 0)
>
> I know it you fixed the missing kunmap_atomic here and missing unlock in
> tcmu_queue_cmd_ring in the next patch, but I think normally people
> prefer that one patch does not add a bug, then the next patch fixes it.
Do you mean the following kmap_atomic() ?

from = kmap_atomic(sg_page(sg)) + sg->offset;

In this patch there has no new kmap/kunmap introduced. This is the old 
code and
the kunmap is at the end of aasda().

This as the initial patch, the memory is from slab cache now. But since 
the second
patch followed will covert to use memory page from the buddy directly.


Thanks,

BRs
Xiubo Li

Re: [PATCH v6 1/2] tcmu: Add dynamic growing data area featuresupport

[Mike Christie]

On 04/30/2017 05:22 AM, Xiubo Li wrote:
> On 2017年04月30日 13:48, Mike Christie wrote:
>> On 04/26/2017 01:25 AM, lixiubo@cmss.chinamobile.com wrote:
>>>       for_each_sg(data_sg, sg, data_nents, i) {
>>> @@ -275,22 +371,26 @@ static void alloc_and_scatter_data_area(struct
>>> tcmu_dev *udev,
>>>           from = kmap_atomic(sg_page(sg)) + sg->offset;
>>>           while (sg_remaining > 0) {
>>>               if (block_remaining == 0) {
>>> -                block = find_first_zero_bit(udev->data_bitmap,
>>> -                        DATA_BLOCK_BITS);
>>>                   block_remaining = DATA_BLOCK_SIZE;
>>> -                set_bit(block, udev->data_bitmap);
>>> +                dbi = tcmu_get_empty_block(udev, &to);
>>> +                if (dbi < 0)
>>
>> I know it you fixed the missing kunmap_atomic here and missing unlock in
>> tcmu_queue_cmd_ring in the next patch, but I think normally people
>> prefer that one patch does not add a bug, then the next patch fixes it.
> Do you mean the following kmap_atomic() ?
> 
> from = kmap_atomic(sg_page(sg)) + sg->offset;
> 
> In this patch there has no new kmap/kunmap introduced. This is the old
> code and
> the kunmap is at the end of aasda().

You added a new return in the error path in this patch in the if case
above, but did not add a kunmap_atomic.

[PATCH v6 2/2] tcmu: Add global data block pool support

[lixiubo]

From: Xiubo Li <lixiubo@cmss.chinamobile.com>

For each target there will be one ring, when the target number
grows larger and larger, it could eventually runs out of the
system memories.

In this patch for each target ring, currently for the cmd area
the size will be fixed to 8MB and for the data area the size
will grow from 0 to max 256K * PAGE_SIZE(1G for 4K page size).

For all the targets' data areas, they will get empty blocks
from the "global data block pool", which has limited to 512K *
PAGE_SIZE(2G for 4K page size) for now.

When the "global data block pool" has been used up, then any
target could wake up the unmap thread routine to shrink other
targets' data area memories. And the unmap thread routine will
always try to truncate the ring vma from the last using block
offset.

When user space has touched the data blocks out of tcmu_cmd
iov[], the tcmu_page_fault() will try to return one zeroed blocks.

Here we move the timeout's tcmu_handle_completions() into unmap
thread routine, that's to say when the timeout fired, it will
only do the tcmu_check_expired_cmd() and then wake up the unmap
thread to do the completions() and then try to shrink its idle
memories. Then the cmdr_lock could be a mutex and could simplify
this patch because the unmap_mapping_range() or zap_* may go to
sleep.

Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
Signed-off-by: Jianfei Hu <hujianfei@cmss.chinamobile.com>
---
 drivers/target/target_core_user.c | 446 ++++++++++++++++++++++++++++----------
 1 file changed, 327 insertions(+), 119 deletions(-)

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 8491752..46f5a1c 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -31,6 +31,8 @@
 #include <linux/bitops.h>
 #include <linux/highmem.h>
 #include <linux/configfs.h>
+#include <linux/mutex.h>
+#include <linux/kthread.h>
 #include <net/genetlink.h>
 #include <scsi/scsi_common.h>
 #include <scsi/scsi_proto.h>
@@ -67,17 +69,24 @@
 
 #define TCMU_TIME_OUT (30 * MSEC_PER_SEC)
 
-/* For cmd area, the size is fixed 2M */
-#define CMDR_SIZE (2 * 1024 * 1024)
+/* For cmd area, the size is fixed 8MB */
+#define CMDR_SIZE (8 * 1024 * 1024)
 
-/* For data area, the size is fixed 32M */
-#define DATA_BLOCK_BITS (8 * 1024)
-#define DATA_BLOCK_SIZE 4096
+/*
+ * For data area, the block size is PAGE_SIZE and
+ * the total size is 256K * PAGE_SIZE.
+ */
+#define DATA_BLOCK_SIZE PAGE_SIZE
+#define DATA_BLOCK_BITS (256 * 1024)
 #define DATA_SIZE (DATA_BLOCK_BITS * DATA_BLOCK_SIZE)
+#define DATA_BLOCK_INIT_BITS 128
 
-/* The ring buffer size is 34M */
+/* The total size of the ring is 8M + 256K * PAGE_SIZE */
 #define TCMU_RING_SIZE (CMDR_SIZE + DATA_SIZE)
 
+/* Default maximum of the global data blocks(512K * PAGE_SIZE) */
+#define TCMU_GLOBAL_MAX_BLOCKS (512 * 1024)
+
 static struct device *tcmu_root_device;
 
 struct tcmu_hba {
@@ -87,6 +96,8 @@ struct tcmu_hba {
 #define TCMU_CONFIG_LEN 256
 
 struct tcmu_dev {
+	struct list_head node;
+
 	struct se_device se_dev;
 
 	char *name;
@@ -98,6 +109,8 @@ struct tcmu_dev {
 
 	struct uio_info uio_info;
 
+	struct inode *inode;
+
 	struct tcmu_mailbox *mb_addr;
 	size_t dev_size;
 	u32 cmdr_size;
@@ -108,10 +121,11 @@ struct tcmu_dev {
 	size_t data_size;
 
 	wait_queue_head_t wait_cmdr;
-	/* TODO should this be a mutex? */
-	spinlock_t cmdr_lock;
+	struct mutex cmdr_lock;
 
+	bool waiting_global;
 	uint32_t dbi_max;
+	uint32_t dbi_thresh;
 	DECLARE_BITMAP(data_bitmap, DATA_BLOCK_BITS);
 	struct radix_tree_root data_blocks;
 
@@ -146,6 +160,13 @@ struct tcmu_cmd {
 	unsigned long flags;
 };
 
+static struct task_struct *unmap_thread;
+static wait_queue_head_t unmap_wait;
+static DEFINE_MUTEX(root_udev_mutex);
+static LIST_HEAD(root_udev);
+
+static atomic_t global_db_count = ATOMIC_INIT(0);
+
 static struct kmem_cache *tcmu_cmd_cache;
 
 /* multicast group */
@@ -174,48 +195,79 @@ enum tcmu_multicast_groups {
 #define tcmu_cmd_set_dbi(cmd, index) ((cmd)->dbi[(cmd)->dbi_cur++] = (index))
 #define tcmu_cmd_get_dbi(cmd) ((cmd)->dbi[(cmd)->dbi_cur++])
 
-static void tcmu_cmd_free_data(struct tcmu_cmd *tcmu_cmd)
+static void tcmu_cmd_free_data(struct tcmu_cmd *tcmu_cmd, uint32_t len)
 {
 	struct tcmu_dev *udev = tcmu_cmd->tcmu_dev;
 	uint32_t i;
 
-	for (i = 0; i < tcmu_cmd->dbi_cnt; i++)
+	for (i = 0; i < len; i++)
 		clear_bit(tcmu_cmd->dbi[i], udev->data_bitmap);
 }
 
-static int tcmu_get_empty_block(struct tcmu_dev *udev, void **addr)
+static inline bool tcmu_get_empty_block(struct tcmu_dev *udev,
+					struct tcmu_cmd *tcmu_cmd)
 {
-	void *p;
-	uint32_t dbi;
-	int ret;
+	struct page *page;
+	int ret, dbi;
 
-	dbi = find_first_zero_bit(udev->data_bitmap, DATA_BLOCK_BITS);
-	if (dbi > udev->dbi_max)
-		udev->dbi_max = dbi;
+	dbi = find_first_zero_bit(udev->data_bitmap, udev->dbi_thresh);
+	if (dbi == udev->dbi_thresh)
+		return false;
 
-	set_bit(dbi, udev->data_bitmap);
+	page = radix_tree_lookup(&udev->data_blocks, dbi);
+	if (!page) {
 
-	p = radix_tree_lookup(&udev->data_blocks, dbi);
-	if (!p) {
-		p = kzalloc(DATA_BLOCK_SIZE, GFP_ATOMIC);
-		if (!p) {
-			clear_bit(dbi, udev->data_bitmap);
-			return -ENOMEM;
+		if (atomic_add_return(1, &global_db_count) >
+					TCMU_GLOBAL_MAX_BLOCKS) {
+			atomic_dec(&global_db_count);
+			return false;
 		}
 
-		ret = radix_tree_insert(&udev->data_blocks, dbi, p);
+		/* try to get new page from the mm */
+		page = alloc_page(GFP_KERNEL);
+		if (!page)
+			return false;
+
+		ret = radix_tree_insert(&udev->data_blocks, dbi, page);
 		if (ret) {
-			kfree(p);
-			clear_bit(dbi, udev->data_bitmap);
-			return ret;
+			__free_page(page);
+			return false;
 		}
+
 	}
 
-	*addr = p;
-	return dbi;
+	if (dbi > udev->dbi_max)
+		udev->dbi_max = dbi;
+
+	set_bit(dbi, udev->data_bitmap);
+	tcmu_cmd_set_dbi(tcmu_cmd, dbi);
+
+	return true;
+}
+
+static bool tcmu_get_empty_blocks(struct tcmu_dev *udev,
+				  struct tcmu_cmd *tcmu_cmd,
+				  uint32_t blocks_needed)
+{
+	int i;
+
+	udev->waiting_global = false;
+
+	for (i = tcmu_cmd->dbi_cur; i < tcmu_cmd->dbi_cnt; i++) {
+		if (!tcmu_get_empty_block(udev, tcmu_cmd))
+			goto err;
+	}
+	return true;
+
+err:
+	udev->waiting_global = true;
+	/* Try to wake up the unmap thread */
+	wake_up(&unmap_wait);
+	return false;
 }
 
-static void *tcmu_get_block_addr(struct tcmu_dev *udev, uint32_t dbi)
+static inline struct page *
+tcmu_get_block_page(struct tcmu_dev *udev, uint32_t dbi)
 {
 	return radix_tree_lookup(&udev->data_blocks, dbi);
 }
@@ -365,17 +417,20 @@ static int alloc_and_scatter_data_area(struct tcmu_dev *udev,
 	void *from, *to = NULL;
 	size_t copy_bytes, to_offset, offset;
 	struct scatterlist *sg;
+	struct page *page;
 
 	for_each_sg(data_sg, sg, data_nents, i) {
 		int sg_remaining = sg->length;
 		from = kmap_atomic(sg_page(sg)) + sg->offset;
 		while (sg_remaining > 0) {
 			if (block_remaining == 0) {
+				if (to)
+					kunmap_atomic(to);
+
 				block_remaining = DATA_BLOCK_SIZE;
-				dbi = tcmu_get_empty_block(udev, &to);
-				if (dbi < 0)
-					return dbi;
-				tcmu_cmd_set_dbi(tcmu_cmd, dbi);
+				dbi = tcmu_cmd_get_dbi(tcmu_cmd);
+				page = tcmu_get_block_page(udev, dbi);
+				to = kmap_atomic(page);
 			}
 
 			copy_bytes = min_t(size_t, sg_remaining,
@@ -403,6 +458,8 @@ static int alloc_and_scatter_data_area(struct tcmu_dev *udev,
 		}
 		kunmap_atomic(from - sg->offset);
 	}
+	if (to)
+		kunmap_atomic(to);
 
 	return 0;
 }
@@ -413,9 +470,10 @@ static void gather_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
 	struct se_cmd *se_cmd = cmd->se_cmd;
 	int i, dbi;
 	int block_remaining = 0;
-	void *from, *to;
+	void *from = NULL, *to;
 	size_t copy_bytes, offset;
 	struct scatterlist *sg, *data_sg;
+	struct page *page;
 	unsigned int data_nents;
 	uint32_t count = 0;
 
@@ -442,9 +500,13 @@ static void gather_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
 		to = kmap_atomic(sg_page(sg)) + sg->offset;
 		while (sg_remaining > 0) {
 			if (block_remaining == 0) {
+				if (from)
+					kunmap_atomic(from);
+
 				block_remaining = DATA_BLOCK_SIZE;
 				dbi = tcmu_cmd_get_dbi(cmd);
-				from = tcmu_get_block_addr(udev, dbi);
+				page = tcmu_get_block_page(udev, dbi);
+				from = kmap_atomic(page);
 			}
 			copy_bytes = min_t(size_t, sg_remaining,
 					block_remaining);
@@ -459,12 +521,13 @@ static void gather_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
 		}
 		kunmap_atomic(to - sg->offset);
 	}
+	if (from)
+		kunmap_atomic(from);
 }
 
-static inline size_t spc_bitmap_free(unsigned long *bitmap)
+static inline size_t spc_bitmap_free(unsigned long *bitmap, uint32_t thresh)
 {
-	return DATA_BLOCK_SIZE * (DATA_BLOCK_BITS -
-			bitmap_weight(bitmap, DATA_BLOCK_BITS));
+	return DATA_BLOCK_SIZE * (thresh - bitmap_weight(bitmap, thresh));
 }
 
 /*
@@ -473,9 +536,12 @@ static inline size_t spc_bitmap_free(unsigned long *bitmap)
  *
  * Called with ring lock held.
  */
-static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t data_needed)
+static bool is_ring_space_avail(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
+		size_t cmd_size, size_t data_needed)
 {
 	struct tcmu_mailbox *mb = udev->mb_addr;
+	uint32_t blocks_needed = (data_needed + DATA_BLOCK_SIZE - 1)
+				/ DATA_BLOCK_SIZE;
 	size_t space, cmd_needed;
 	u32 cmd_head;
 
@@ -499,13 +565,41 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 		return false;
 	}
 
-	space = spc_bitmap_free(udev->data_bitmap);
+	/* try to check and get the data blocks as needed */
+	space = spc_bitmap_free(udev->data_bitmap, udev->dbi_thresh);
 	if (space < data_needed) {
-		pr_debug("no data space: only %zu available, but ask for %zu\n",
-				space, data_needed);
-		return false;
+		uint32_t blocks_left = DATA_BLOCK_BITS - udev->dbi_thresh;
+		uint32_t grow;
+
+		if (blocks_left < blocks_needed) {
+			pr_debug("no data space: only %zu available, but ask for %zu\n",
+					blocks_left * DATA_BLOCK_SIZE,
+					data_needed);
+			return false;
+		}
+
+		/* Try to expand the thresh */
+		if (!udev->dbi_thresh) {
+			/* From idle state */
+			uint32_t init_thresh = DATA_BLOCK_INIT_BITS;
+
+			udev->dbi_thresh = max(blocks_needed, init_thresh);
+		} else {
+			/*
+			 * Grow the data area by max(blocks needed,
+			 * dbi_thresh / 2), but limited to the max
+			 * DATA_BLOCK_BITS size.
+			 */
+			grow = max(blocks_needed, udev->dbi_thresh / 2);
+			udev->dbi_thresh += grow;
+			if (udev->dbi_thresh > DATA_BLOCK_BITS)
+				udev->dbi_thresh = DATA_BLOCK_BITS;
+		}
 	}
 
+	if (!tcmu_get_empty_blocks(udev, cmd, blocks_needed))
+		return false;
+
 	return true;
 }
 
@@ -542,7 +636,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 
 	WARN_ON(command_size & (TCMU_OP_ALIGN_SIZE-1));
 
-	spin_lock_irq(&udev->cmdr_lock);
+	mutex_lock(&udev->cmdr_lock);
 
 	mb = udev->mb_addr;
 	cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
@@ -551,18 +645,18 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 		pr_warn("TCMU: Request of size %zu/%zu is too big for %u/%zu "
 			"cmd ring/data area\n", command_size, data_length,
 			udev->cmdr_size, udev->data_size);
-		spin_unlock_irq(&udev->cmdr_lock);
+		mutex_unlock(&udev->cmdr_lock);
 		return TCM_INVALID_CDB_FIELD;
 	}
 
-	while (!is_ring_space_avail(udev, command_size, data_length)) {
+	while (!is_ring_space_avail(udev, tcmu_cmd, command_size, data_length)) {
 		int ret;
 		DEFINE_WAIT(__wait);
 
 		prepare_to_wait(&udev->wait_cmdr, &__wait, TASK_INTERRUPTIBLE);
 
 		pr_debug("sleeping for ring space\n");
-		spin_unlock_irq(&udev->cmdr_lock);
+		mutex_unlock(&udev->cmdr_lock);
 		if (udev->cmd_time_out)
 			ret = schedule_timeout(
 					msecs_to_jiffies(udev->cmd_time_out));
@@ -574,7 +668,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		}
 
-		spin_lock_irq(&udev->cmdr_lock);
+		mutex_lock(&udev->cmdr_lock);
 
 		/* We dropped cmdr_lock, cmd_head is stale */
 		cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
@@ -607,6 +701,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 	entry->hdr.uflags = 0;
 
 	/* Handle allocating space from the data area */
+	tcmu_cmd_reset_dbi_cur(tcmu_cmd);
 	iov = &entry->req.iov[0];
 	iov_cnt = 0;
 	copy_to_data_area = (se_cmd->data_direction == DMA_TO_DEVICE
@@ -616,6 +711,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 			&iov, &iov_cnt, copy_to_data_area);
 	if (ret) {
 		pr_err("tcmu: alloc and scatter data failed\n");
+		mutex_unlock(&udev->cmdr_lock);
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	}
 	entry->req.iov_cnt = iov_cnt;
@@ -631,6 +727,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 					&iov, &iov_cnt, false);
 		if (ret) {
 			pr_err("tcmu: alloc and scatter bidi data failed\n");
+			mutex_unlock(&udev->cmdr_lock);
 			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		}
 		entry->req.iov_bidi_cnt = iov_cnt;
@@ -644,8 +741,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 
 	UPDATE_HEAD(mb->cmd_head, command_size, udev->cmdr_size);
 	tcmu_flush_dcache_range(mb, sizeof(*mb));
-
-	spin_unlock_irq(&udev->cmdr_lock);
+	mutex_unlock(&udev->cmdr_lock);
 
 	/* TODO: only if FLUSH and FUA? */
 	uio_event_notify(&udev->uio_info);
@@ -719,14 +815,13 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
 
 out:
 	cmd->se_cmd = NULL;
-	tcmu_cmd_free_data(cmd);
+	tcmu_cmd_free_data(cmd, cmd->dbi_cnt);
 	tcmu_free_cmd(cmd);
 }
 
 static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
 {
 	struct tcmu_mailbox *mb;
-	unsigned long flags;
 	int handled = 0;
 
 	if (test_bit(TCMU_DEV_BIT_BROKEN, &udev->flags)) {
@@ -734,8 +829,6 @@ static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
 		return 0;
 	}
 
-	spin_lock_irqsave(&udev->cmdr_lock, flags);
-
 	mb = udev->mb_addr;
 	tcmu_flush_dcache_range(mb, sizeof(*mb));
 
@@ -776,8 +869,6 @@ static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
 	if (mb->cmd_tail == mb->cmd_head)
 		del_timer(&udev->timeout); /* no more pending cmds */
 
-	spin_unlock_irqrestore(&udev->cmdr_lock, flags);
-
 	wake_up(&udev->wait_cmdr);
 
 	return handled;
@@ -804,16 +895,14 @@ static void tcmu_device_timedout(unsigned long data)
 {
 	struct tcmu_dev *udev = (struct tcmu_dev *)data;
 	unsigned long flags;
-	int handled;
-
-	handled = tcmu_handle_completions(udev);
-
-	pr_warn("%d completions handled from timeout\n", handled);
 
 	spin_lock_irqsave(&udev->commands_lock, flags);
 	idr_for_each(&udev->commands, tcmu_check_expired_cmd, NULL);
 	spin_unlock_irqrestore(&udev->commands_lock, flags);
 
+	/* Try to wake up the ummap thread */
+	wake_up(&unmap_wait);
+
 	/*
 	 * We don't need to wakeup threads on wait_cmdr since they have their
 	 * own timeout.
@@ -858,7 +947,7 @@ static struct se_device *tcmu_alloc_device(struct se_hba *hba, const char *name)
 	udev->cmd_time_out = TCMU_TIME_OUT;
 
 	init_waitqueue_head(&udev->wait_cmdr);
-	spin_lock_init(&udev->cmdr_lock);
+	mutex_init(&udev->cmdr_lock);
 
 	idr_init(&udev->commands);
 	spin_lock_init(&udev->commands_lock);
@@ -873,59 +962,13 @@ static int tcmu_irqcontrol(struct uio_info *info, s32 irq_on)
 {
 	struct tcmu_dev *tcmu_dev = container_of(info, struct tcmu_dev, uio_info);
 
+	mutex_lock(&tcmu_dev->cmdr_lock);
 	tcmu_handle_completions(tcmu_dev);
+	mutex_unlock(&tcmu_dev->cmdr_lock);
 
 	return 0;
 }
 
-static void tcmu_blocks_release(struct tcmu_dev *udev, bool release_pending)
-{
-	uint32_t dbi, end;
-	void *addr;
-
-	spin_lock_irq(&udev->cmdr_lock);
-
-	end = udev->dbi_max + 1;
-
-	/* try to release all unused blocks */
-	dbi = find_first_zero_bit(udev->data_bitmap, end);
-	if (dbi >= end) {
-		spin_unlock_irq(&udev->cmdr_lock);
-		return;
-	}
-	do {
-		addr = radix_tree_delete(&udev->data_blocks, dbi);
-		kfree(addr);
-
-		dbi = find_next_zero_bit(udev->data_bitmap, end, dbi + 1);
-	} while (dbi < end);
-
-	if (!release_pending)
-		return;
-
-	/* try to release all pending blocks */
-	dbi = find_first_bit(udev->data_bitmap, end);
-	if (dbi >= end) {
-		spin_unlock_irq(&udev->cmdr_lock);
-		return;
-	}
-	do {
-		addr = radix_tree_delete(&udev->data_blocks, dbi);
-		kfree(addr);
-
-		dbi = find_next_bit(udev->data_bitmap, end, dbi + 1);
-	} while (dbi < end);
-
-	spin_unlock_irq(&udev->cmdr_lock);
-}
-
-static void tcmu_vma_close(struct vm_area_struct *vma)
-{
-	struct tcmu_dev *udev = vma->vm_private_data;
-
-	tcmu_blocks_release(udev, false);
-}
-
 /*
  * mmap code from uio.c. Copied here because we want to hook mmap()
  * and this stuff must come along.
@@ -943,6 +986,57 @@ static int tcmu_find_mem_index(struct vm_area_struct *vma)
 	return -1;
 }
 
+static struct page *tcmu_try_get_block_page(struct tcmu_dev *udev, uint32_t dbi)
+{
+	struct page *page;
+	int ret;
+
+	mutex_lock(&udev->cmdr_lock);
+	page = tcmu_get_block_page(udev, dbi);
+	if (likely(page)) {
+		mutex_unlock(&udev->cmdr_lock);
+		return page;
+	}
+
+	/*
+	 * Normally it shouldn't be here:
+	 * Only when the userspace has touched the blocks which
+	 * are out of the tcmu_cmd's data iov[], and will return
+	 * one zeroed page.
+	 */
+	if (dbi >= udev->dbi_thresh) {
+		/* Extern the udev->dbi_thresh to dbi + 1 */
+		udev->dbi_thresh = dbi + 1;
+		udev->dbi_max = dbi;
+	}
+
+	page = radix_tree_lookup(&udev->data_blocks, dbi);
+	if (!page) {
+		page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+		if (!page) {
+			mutex_unlock(&udev->cmdr_lock);
+			return NULL;
+		}
+
+		ret = radix_tree_insert(&udev->data_blocks, dbi, page);
+		if (ret) {
+			mutex_unlock(&udev->cmdr_lock);
+			__free_page(page);
+			return NULL;
+		}
+
+		/*
+		 * Since this case is rare in page fault routine, here we
+		 * will allow the global_db_count >= TCMU_GLOBAL_MAX_BLOCKS
+		 * to reduce possible page fault call trace.
+		 */
+		atomic_inc(&global_db_count);
+	}
+	mutex_unlock(&udev->cmdr_lock);
+
+	return page;
+}
+
 static int tcmu_vma_fault(struct vm_fault *vmf)
 {
 	struct tcmu_dev *udev = vmf->vma->vm_private_data;
@@ -966,14 +1060,13 @@ static int tcmu_vma_fault(struct vm_fault *vmf)
 		addr = (void *)(unsigned long)info->mem[mi].addr + offset;
 		page = vmalloc_to_page(addr);
 	} else {
-		/* For the dynamically growing data area pages */
 		uint32_t dbi;
 
+		/* For the dynamically growing data area pages */
 		dbi = (offset - udev->data_off) / DATA_BLOCK_SIZE;
-		addr = tcmu_get_block_addr(udev, dbi);
-		if (!addr)
+		page = tcmu_try_get_block_page(udev, dbi);
+		if (!page)
 			return VM_FAULT_NOPAGE;
-		page = virt_to_page(addr);
 	}
 
 	get_page(page);
@@ -982,7 +1075,6 @@ static int tcmu_vma_fault(struct vm_fault *vmf)
 }
 
 static const struct vm_operations_struct tcmu_vm_ops = {
-	.close = tcmu_vma_close,
 	.fault = tcmu_vma_fault,
 };
 
@@ -1010,6 +1102,8 @@ static int tcmu_open(struct uio_info *info, struct inode *inode)
 	if (test_and_set_bit(TCMU_DEV_BIT_OPEN, &udev->flags))
 		return -EBUSY;
 
+	udev->inode = inode;
+
 	pr_debug("open\n");
 
 	return 0;
@@ -1100,6 +1194,8 @@ static int tcmu_configure_device(struct se_device *dev)
 	udev->cmdr_size = CMDR_SIZE - CMDR_OFF;
 	udev->data_off = CMDR_SIZE;
 	udev->data_size = DATA_SIZE;
+	udev->dbi_thresh = 0; /* Default in Idle state */
+	udev->waiting_global = false;
 
 	/* Initialise the mailbox of the ring buffer */
 	mb = udev->mb_addr;
@@ -1112,7 +1208,7 @@ static int tcmu_configure_device(struct se_device *dev)
 	WARN_ON(udev->data_size % PAGE_SIZE);
 	WARN_ON(udev->data_size % DATA_BLOCK_SIZE);
 
-	INIT_RADIX_TREE(&udev->data_blocks, GFP_ATOMIC);
+	INIT_RADIX_TREE(&udev->data_blocks, GFP_KERNEL);
 
 	info->version = __stringify(TCMU_MAILBOX_VERSION);
 
@@ -1145,6 +1241,10 @@ static int tcmu_configure_device(struct se_device *dev)
 	if (ret)
 		goto err_netlink;
 
+	mutex_lock(&root_udev_mutex);
+	list_add(&udev->node, &root_udev);
+	mutex_unlock(&root_udev_mutex);
+
 	return 0;
 
 err_netlink:
@@ -1179,6 +1279,23 @@ static bool tcmu_dev_configured(struct tcmu_dev *udev)
 	return udev->uio_info.uio_dev ? true : false;
 }
 
+static void tcmu_blocks_release(struct tcmu_dev *udev)
+{
+	int i;
+	struct page *page;
+
+	/* Try to release all block pages */
+	mutex_lock(&udev->cmdr_lock);
+	for (i = 0; i <= udev->dbi_max; i++) {
+		page = radix_tree_delete(&udev->data_blocks, i);
+		if (page) {
+			__free_page(page);
+			atomic_dec(&global_db_count);
+		}
+	}
+	mutex_unlock(&udev->cmdr_lock);
+}
+
 static void tcmu_free_device(struct se_device *dev)
 {
 	struct tcmu_dev *udev = TCMU_DEV(dev);
@@ -1188,6 +1305,10 @@ static void tcmu_free_device(struct se_device *dev)
 
 	del_timer_sync(&udev->timeout);
 
+	mutex_lock(&root_udev_mutex);
+	list_del(&udev->node);
+	mutex_unlock(&root_udev_mutex);
+
 	vfree(udev->mb_addr);
 
 	/* Upper layer should drain all requests before calling this */
@@ -1200,7 +1321,7 @@ static void tcmu_free_device(struct se_device *dev)
 	spin_unlock_irq(&udev->commands_lock);
 	WARN_ON(!all_expired);
 
-	tcmu_blocks_release(udev, true);
+	tcmu_blocks_release(udev);
 
 	if (tcmu_dev_configured(udev)) {
 		tcmu_netlink_event(TCMU_CMD_REMOVED_DEVICE, udev->uio_info.name,
@@ -1388,6 +1509,81 @@ static ssize_t tcmu_cmd_time_out_store(struct config_item *item, const char *pag
 	.tb_dev_attrib_attrs	= NULL,
 };
 
+static int unmap_thread_fn(void *data)
+{
+	struct tcmu_dev *udev;
+	loff_t off;
+	uint32_t start, end, block;
+	struct page *page;
+	int i;
+
+	while (1) {
+		DEFINE_WAIT(__wait);
+
+		prepare_to_wait(&unmap_wait, &__wait, TASK_INTERRUPTIBLE);
+		schedule();
+		finish_wait(&unmap_wait, &__wait);
+
+		mutex_lock(&root_udev_mutex);
+		list_for_each_entry(udev, &root_udev, node) {
+			mutex_lock(&udev->cmdr_lock);
+
+			/* Try to complete the finished commands first */
+			tcmu_handle_completions(udev);
+
+			/* Skip the udevs waiting the global pool or in idle */
+			if (udev->waiting_global || !udev->dbi_thresh) {
+				mutex_unlock(&udev->cmdr_lock);
+				continue;
+			}
+
+			end = udev->dbi_max + 1;
+			block = find_last_bit(udev->data_bitmap, end);
+			if (block == udev->dbi_max) {
+				/*
+				 * The last bit is dbi_max, so there is
+				 * no need to shrink any blocks.
+				 */
+				mutex_unlock(&udev->cmdr_lock);
+				continue;
+			} else if (block == end) {
+				/* The current udev will goto idle state */
+				udev->dbi_thresh = start = 0;
+				udev->dbi_max = 0;
+			} else {
+				udev->dbi_thresh = start = block + 1;
+				udev->dbi_max = block;
+			}
+
+			/* Here will truncate the data area from off */
+			off = udev->data_off + start * DATA_BLOCK_SIZE;
+			unmap_mapping_range(udev->inode->i_mapping, off, 0, 1);
+
+			/* Release the block pages */
+			for (i = start; i < end; i++) {
+				page = radix_tree_delete(&udev->data_blocks, i);
+				if (page) {
+					__free_page(page);
+					atomic_dec(&global_db_count);
+				}
+			}
+			mutex_unlock(&udev->cmdr_lock);
+		}
+
+		/*
+		 * Try to wake up the udevs who are waiting
+		 * for the global data pool.
+		 */
+		list_for_each_entry(udev, &root_udev, node) {
+			if (udev->waiting_global)
+				wake_up(&udev->wait_cmdr);
+		}
+		mutex_unlock(&root_udev_mutex);
+	}
+
+	return 0;
+}
+
 static int __init tcmu_module_init(void)
 {
 	int ret, i, len = 0;
@@ -1433,8 +1629,17 @@ static int __init tcmu_module_init(void)
 	if (ret)
 		goto out_attrs;
 
+	init_waitqueue_head(&unmap_wait);
+	unmap_thread = kthread_run(unmap_thread_fn, NULL, "tcmu_unmap");
+	if (IS_ERR(unmap_thread)) {
+		unmap_thread = NULL;
+		goto out_unreg_transport;
+	}
+
 	return 0;
 
+out_unreg_transport:
+	target_backend_unregister(&tcmu_ops);
 out_attrs:
 	kfree(tcmu_attrs);
 out_unreg_genl:
@@ -1449,6 +1654,9 @@ static int __init tcmu_module_init(void)
 
 static void __exit tcmu_module_exit(void)
 {
+	if (unmap_thread)
+		kthread_stop(unmap_thread);
+
 	target_backend_unregister(&tcmu_ops);
 	kfree(tcmu_attrs);
 	genl_unregister_family(&tcmu_genl_family);
-- 
1.8.3.1

Re: [PATCH v6 2/2] tcmu: Add global data block pool support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5308 bytes --]

Hi Xiubo,

[auto build test WARNING on next-20170424]
[also build test WARNING on v4.11-rc8]
[cannot apply to v4.9-rc8 v4.9-rc7 v4.9-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/lixiubo-cmss-chinamobile-com/tcmu-Dynamic-growing-data-area-support/20170426-162910
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:329:0,
                    from include/linux/kernel.h:13,
                    from include/linux/list.h:8,
                    from include/linux/preempt.h:10,
                    from include/linux/spinlock.h:50,
                    from drivers//target/target_core_user.c:21:
   drivers//target/target_core_user.c: In function 'is_ring_space_avail':
>> include/linux/dynamic_debug.h:74:16: warning: format '%zu' expects argument of type 'size_t', but argument 3 has type 'long unsigned int' [-Wformat=]
     static struct _ddebug  __aligned(8)   \
                   ^
   include/linux/dynamic_debug.h:110:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA_KEY'
     DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, 0, 0)
     ^
   include/linux/dynamic_debug.h:124:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA'
     DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
     ^
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
     dynamic_pr_debug(fmt, ##__VA_ARGS__)
     ^
   drivers//target/target_core_user.c:575:4: note: in expansion of macro 'pr_debug'
       pr_debug("no data space: only %zu available, but ask for %zu\n",
       ^
--
   In file included from include/linux/printk.h:329:0,
                    from include/linux/kernel.h:13,
                    from include/linux/list.h:8,
                    from include/linux/preempt.h:10,
                    from include/linux/spinlock.h:50,
                    from drivers/target/target_core_user.c:21:
   drivers/target/target_core_user.c: In function 'is_ring_space_avail':
>> include/linux/dynamic_debug.h:74:16: warning: format '%zu' expects argument of type 'size_t', but argument 3 has type 'long unsigned int' [-Wformat=]
     static struct _ddebug  __aligned(8)   \
                   ^
   include/linux/dynamic_debug.h:110:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA_KEY'
     DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, 0, 0)
     ^
   include/linux/dynamic_debug.h:124:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA'
     DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
     ^
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
     dynamic_pr_debug(fmt, ##__VA_ARGS__)
     ^
   drivers/target/target_core_user.c:575:4: note: in expansion of macro 'pr_debug'
       pr_debug("no data space: only %zu available, but ask for %zu\n",
       ^

vim +74 include/linux/dynamic_debug.h

b48420c1 Jim Cromie  2012-04-27  58  					const char *modname);
b48420c1 Jim Cromie  2012-04-27  59  
cbc46635 Joe Perches 2011-08-11  60  struct device;
cbc46635 Joe Perches 2011-08-11  61  
b9075fa9 Joe Perches 2011-10-31  62  extern __printf(3, 4)
906d2015 Joe Perches 2014-09-24  63  void __dynamic_dev_dbg(struct _ddebug *descriptor, const struct device *dev,
b9075fa9 Joe Perches 2011-10-31  64  		       const char *fmt, ...);
cbc46635 Joe Perches 2011-08-11  65  
ffa10cb4 Jason Baron 2011-08-11  66  struct net_device;
ffa10cb4 Jason Baron 2011-08-11  67  
b9075fa9 Joe Perches 2011-10-31  68  extern __printf(3, 4)
906d2015 Joe Perches 2014-09-24  69  void __dynamic_netdev_dbg(struct _ddebug *descriptor,
ffa10cb4 Jason Baron 2011-08-11  70  			  const struct net_device *dev,
b9075fa9 Joe Perches 2011-10-31  71  			  const char *fmt, ...);
ffa10cb4 Jason Baron 2011-08-11  72  
9049fc74 Jason Baron 2016-08-03  73  #define DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, key, init)	\
c0d2af63 Joe Perches 2012-10-18 @74  	static struct _ddebug  __aligned(8)			\
07613b0b Jason Baron 2011-10-04  75  	__attribute__((section("__verbose"))) name = {		\
07613b0b Jason Baron 2011-10-04  76  		.modname = KBUILD_MODNAME,			\
07613b0b Jason Baron 2011-10-04  77  		.function = __func__,				\
07613b0b Jason Baron 2011-10-04  78  		.filename = __FILE__,				\
07613b0b Jason Baron 2011-10-04  79  		.format = (fmt),				\
07613b0b Jason Baron 2011-10-04  80  		.lineno = __LINE__,				\
07613b0b Jason Baron 2011-10-04  81  		.flags = _DPRINTK_FLAGS_DEFAULT,		\
9049fc74 Jason Baron 2016-08-03  82  		dd_key_init(key, init)				\

:::::: The code at line 74 was first introduced by commit
:::::: c0d2af637863940b1a4fb208224ca7acb905c39f dynamic_debug: Remove unnecessary __used

:::::: TO: Joe Perches <joe@perches.com>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50029 bytes --]

Re: [PATCH v6 2/2] tcmu: Add global data block pool support

[Mike Christie]

On 04/26/2017 01:25 AM, lixiubo@cmss.chinamobile.com wrote:
> From: Xiubo Li <lixiubo@cmss.chinamobile.com>
> 
> For each target there will be one ring, when the target number
> grows larger and larger, it could eventually runs out of the
> system memories.
> 
> In this patch for each target ring, currently for the cmd area
> the size will be fixed to 8MB and for the data area the size
> will grow from 0 to max 256K * PAGE_SIZE(1G for 4K page size).
> 
> For all the targets' data areas, they will get empty blocks
> from the "global data block pool", which has limited to 512K *
> PAGE_SIZE(2G for 4K page size) for now.
> 
> When the "global data block pool" has been used up, then any
> target could wake up the unmap thread routine to shrink other
> targets' data area memories. And the unmap thread routine will
> always try to truncate the ring vma from the last using block
> offset.
> 
> When user space has touched the data blocks out of tcmu_cmd
> iov[], the tcmu_page_fault() will try to return one zeroed blocks.
> 
> Here we move the timeout's tcmu_handle_completions() into unmap
> thread routine, that's to say when the timeout fired, it will
> only do the tcmu_check_expired_cmd() and then wake up the unmap
> thread to do the completions() and then try to shrink its idle
> memories. Then the cmdr_lock could be a mutex and could simplify
> this patch because the unmap_mapping_range() or zap_* may go to
> sleep.
> 
> Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
> Signed-off-by: Jianfei Hu <hujianfei@cmss.chinamobile.com>
> ---
>  drivers/target/target_core_user.c | 446 ++++++++++++++++++++++++++++----------
>  1 file changed, 327 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 8491752..46f5a1c 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -31,6 +31,8 @@
>  #include <linux/bitops.h>
>  #include <linux/highmem.h>
>  #include <linux/configfs.h>
> +#include <linux/mutex.h>
> +#include <linux/kthread.h>
>  #include <net/genetlink.h>
>  #include <scsi/scsi_common.h>
>  #include <scsi/scsi_proto.h>
> @@ -67,17 +69,24 @@
>  
>  #define TCMU_TIME_OUT (30 * MSEC_PER_SEC)
>  
> -/* For cmd area, the size is fixed 2M */
> -#define CMDR_SIZE (2 * 1024 * 1024)
> +/* For cmd area, the size is fixed 8MB */
> +#define CMDR_SIZE (8 * 1024 * 1024)
>  
> -/* For data area, the size is fixed 32M */
> -#define DATA_BLOCK_BITS (8 * 1024)
> -#define DATA_BLOCK_SIZE 4096
> +/*
> + * For data area, the block size is PAGE_SIZE and
> + * the total size is 256K * PAGE_SIZE.
> + */
> +#define DATA_BLOCK_SIZE PAGE_SIZE
> +#define DATA_BLOCK_BITS (256 * 1024)
>  #define DATA_SIZE (DATA_BLOCK_BITS * DATA_BLOCK_SIZE)
> +#define DATA_BLOCK_INIT_BITS 128
>  
> -/* The ring buffer size is 34M */
> +/* The total size of the ring is 8M + 256K * PAGE_SIZE */
>  #define TCMU_RING_SIZE (CMDR_SIZE + DATA_SIZE)
>  
> +/* Default maximum of the global data blocks(512K * PAGE_SIZE) */
> +#define TCMU_GLOBAL_MAX_BLOCKS (512 * 1024)
> +
>  static struct device *tcmu_root_device;
>  
>  struct tcmu_hba {
> @@ -87,6 +96,8 @@ struct tcmu_hba {
>  #define TCMU_CONFIG_LEN 256
>  
>  struct tcmu_dev {
> +	struct list_head node;
> +
>  	struct se_device se_dev;
>  
>  	char *name;
> @@ -98,6 +109,8 @@ struct tcmu_dev {
>  
>  	struct uio_info uio_info;
>  
> +	struct inode *inode;
> +
>  	struct tcmu_mailbox *mb_addr;
>  	size_t dev_size;
>  	u32 cmdr_size;
> @@ -108,10 +121,11 @@ struct tcmu_dev {
>  	size_t data_size;
>  
>  	wait_queue_head_t wait_cmdr;
> -	/* TODO should this be a mutex? */
> -	spinlock_t cmdr_lock;
> +	struct mutex cmdr_lock;
>  
> +	bool waiting_global;
>  	uint32_t dbi_max;
> +	uint32_t dbi_thresh;
>  	DECLARE_BITMAP(data_bitmap, DATA_BLOCK_BITS);
>  	struct radix_tree_root data_blocks;
>  
> @@ -146,6 +160,13 @@ struct tcmu_cmd {
>  	unsigned long flags;
>  };
>  
> +static struct task_struct *unmap_thread;
> +static wait_queue_head_t unmap_wait;
> +static DEFINE_MUTEX(root_udev_mutex);
> +static LIST_HEAD(root_udev);
> +
> +static atomic_t global_db_count = ATOMIC_INIT(0);
> +
>  static struct kmem_cache *tcmu_cmd_cache;
>  
>  /* multicast group */
> @@ -174,48 +195,79 @@ enum tcmu_multicast_groups {
>  #define tcmu_cmd_set_dbi(cmd, index) ((cmd)->dbi[(cmd)->dbi_cur++] = (index))
>  #define tcmu_cmd_get_dbi(cmd) ((cmd)->dbi[(cmd)->dbi_cur++])
>  
> -static void tcmu_cmd_free_data(struct tcmu_cmd *tcmu_cmd)
> +static void tcmu_cmd_free_data(struct tcmu_cmd *tcmu_cmd, uint32_t len)
>  {
>  	struct tcmu_dev *udev = tcmu_cmd->tcmu_dev;
>  	uint32_t i;
>  
> -	for (i = 0; i < tcmu_cmd->dbi_cnt; i++)
> +	for (i = 0; i < len; i++)
>  		clear_bit(tcmu_cmd->dbi[i], udev->data_bitmap);
>  }
>  
> -static int tcmu_get_empty_block(struct tcmu_dev *udev, void **addr)
> +static inline bool tcmu_get_empty_block(struct tcmu_dev *udev,
> +					struct tcmu_cmd *tcmu_cmd)
>  {
> -	void *p;
> -	uint32_t dbi;
> -	int ret;
> +	struct page *page;
> +	int ret, dbi;
>  
> -	dbi = find_first_zero_bit(udev->data_bitmap, DATA_BLOCK_BITS);
> -	if (dbi > udev->dbi_max)
> -		udev->dbi_max = dbi;
> +	dbi = find_first_zero_bit(udev->data_bitmap, udev->dbi_thresh);
> +	if (dbi == udev->dbi_thresh)
> +		return false;
>  
> -	set_bit(dbi, udev->data_bitmap);
> +	page = radix_tree_lookup(&udev->data_blocks, dbi);
> +	if (!page) {
>  
> -	p = radix_tree_lookup(&udev->data_blocks, dbi);
> -	if (!p) {
> -		p = kzalloc(DATA_BLOCK_SIZE, GFP_ATOMIC);
> -		if (!p) {
> -			clear_bit(dbi, udev->data_bitmap);
> -			return -ENOMEM;
> +		if (atomic_add_return(1, &global_db_count) >
> +					TCMU_GLOBAL_MAX_BLOCKS) {
> +			atomic_dec(&global_db_count);
> +			return false;
>  		}
>  
> -		ret = radix_tree_insert(&udev->data_blocks, dbi, p);
> +		/* try to get new page from the mm */
> +		page = alloc_page(GFP_KERNEL);
> +		if (!page)
> +			return false;
> +
> +		ret = radix_tree_insert(&udev->data_blocks, dbi, page);
>  		if (ret) {
> -			kfree(p);
> -			clear_bit(dbi, udev->data_bitmap);
> -			return ret;
> +			__free_page(page);
> +			return false;
>  		}
> +
>  	}
>  
> -	*addr = p;
> -	return dbi;
> +	if (dbi > udev->dbi_max)
> +		udev->dbi_max = dbi;
> +
> +	set_bit(dbi, udev->data_bitmap);
> +	tcmu_cmd_set_dbi(tcmu_cmd, dbi);
> +
> +	return true;
> +}
> +
> +static bool tcmu_get_empty_blocks(struct tcmu_dev *udev,
> +				  struct tcmu_cmd *tcmu_cmd,
> +				  uint32_t blocks_needed)


Can drop blocks_needed.


> +{
> +	int i;
> +
> +	udev->waiting_global = false;
> +
> +	for (i = tcmu_cmd->dbi_cur; i < tcmu_cmd->dbi_cnt; i++) {
> +		if (!tcmu_get_empty_block(udev, tcmu_cmd))
> +			goto err;
> +	}
> +	return true;
> +
> +err:
> +	udev->waiting_global = true;
> +	/* Try to wake up the unmap thread */
> +	wake_up(&unmap_wait);
> +	return false;
>  }
>  
> -static void *tcmu_get_block_addr(struct tcmu_dev *udev, uint32_t dbi)
> +static inline struct page *
> +tcmu_get_block_page(struct tcmu_dev *udev, uint32_t dbi)
>  {
>  	return radix_tree_lookup(&udev->data_blocks, dbi);
>  }
> @@ -365,17 +417,20 @@ static int alloc_and_scatter_data_area(struct tcmu_dev *udev,


We don't actually alloc the area here any more. Could probably rename
it to be similar to gather_data_area.


>  	void *from, *to = NULL;
>  	size_t copy_bytes, to_offset, offset;
>  	struct scatterlist *sg;
> +	struct page *page;
>  
>  	for_each_sg(data_sg, sg, data_nents, i) {
>  		int sg_remaining = sg->length;
>  		from = kmap_atomic(sg_page(sg)) + sg->offset;
>  		while (sg_remaining > 0) {
>  			if (block_remaining == 0) {
> +				if (to)
> +					kunmap_atomic(to);
> +
>  				block_remaining = DATA_BLOCK_SIZE;
> -				dbi = tcmu_get_empty_block(udev, &to);
> -				if (dbi < 0)
> -					return dbi;
> -				tcmu_cmd_set_dbi(tcmu_cmd, dbi);
> +				dbi = tcmu_cmd_get_dbi(tcmu_cmd);
> +				page = tcmu_get_block_page(udev, dbi);
> +				to = kmap_atomic(page);
>  			}
>  
>  			copy_bytes = min_t(size_t, sg_remaining,
> @@ -403,6 +458,8 @@ static int alloc_and_scatter_data_area(struct tcmu_dev *udev,
>  		}
>  		kunmap_atomic(from - sg->offset);
>  	}
> +	if (to)
> +		kunmap_atomic(to);
>  
>  	return 0;
>  }
> @@ -413,9 +470,10 @@ static void gather_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
>  	struct se_cmd *se_cmd = cmd->se_cmd;
>  	int i, dbi;
>  	int block_remaining = 0;
> -	void *from, *to;
> +	void *from = NULL, *to;
>  	size_t copy_bytes, offset;
>  	struct scatterlist *sg, *data_sg;
> +	struct page *page;
>  	unsigned int data_nents;
>  	uint32_t count = 0;
>  
> @@ -442,9 +500,13 @@ static void gather_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
>  		to = kmap_atomic(sg_page(sg)) + sg->offset;
>  		while (sg_remaining > 0) {
>  			if (block_remaining == 0) {
> +				if (from)
> +					kunmap_atomic(from);
> +
>  				block_remaining = DATA_BLOCK_SIZE;
>  				dbi = tcmu_cmd_get_dbi(cmd);
> -				from = tcmu_get_block_addr(udev, dbi);
> +				page = tcmu_get_block_page(udev, dbi);
> +				from = kmap_atomic(page);
>  			}
>  			copy_bytes = min_t(size_t, sg_remaining,
>  					block_remaining);
> @@ -459,12 +521,13 @@ static void gather_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
>  		}
>  		kunmap_atomic(to - sg->offset);
>  	}
> +	if (from)
> +		kunmap_atomic(from);
>  }
>  
> -static inline size_t spc_bitmap_free(unsigned long *bitmap)
> +static inline size_t spc_bitmap_free(unsigned long *bitmap, uint32_t thresh)
>  {
> -	return DATA_BLOCK_SIZE * (DATA_BLOCK_BITS -
> -			bitmap_weight(bitmap, DATA_BLOCK_BITS));
> +	return DATA_BLOCK_SIZE * (thresh - bitmap_weight(bitmap, thresh));
>  }
>  
>  /*
> @@ -473,9 +536,12 @@ static inline size_t spc_bitmap_free(unsigned long *bitmap)
>   *
>   * Called with ring lock held.
>   */
> -static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t data_needed)
> +static bool is_ring_space_avail(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
> +		size_t cmd_size, size_t data_needed)
>  {
>  	struct tcmu_mailbox *mb = udev->mb_addr;
> +	uint32_t blocks_needed = (data_needed + DATA_BLOCK_SIZE - 1)
> +				/ DATA_BLOCK_SIZE;
>  	size_t space, cmd_needed;
>  	u32 cmd_head;
>  
> @@ -499,13 +565,41 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
>  		return false;
>  	}
>  
> -	space = spc_bitmap_free(udev->data_bitmap);
> +	/* try to check and get the data blocks as needed */
> +	space = spc_bitmap_free(udev->data_bitmap, udev->dbi_thresh);
>  	if (space < data_needed) {
> -		pr_debug("no data space: only %zu available, but ask for %zu\n",
> -				space, data_needed);
> -		return false;
> +		uint32_t blocks_left = DATA_BLOCK_BITS - udev->dbi_thresh;
> +		uint32_t grow;
> +
> +		if (blocks_left < blocks_needed) {
> +			pr_debug("no data space: only %zu available, but ask for %zu\n",
> +					blocks_left * DATA_BLOCK_SIZE,
> +					data_needed);
> +			return false;
> +		}
> +
> +		/* Try to expand the thresh */
> +		if (!udev->dbi_thresh) {
> +			/* From idle state */
> +			uint32_t init_thresh = DATA_BLOCK_INIT_BITS;
> +
> +			udev->dbi_thresh = max(blocks_needed, init_thresh);
> +		} else {
> +			/*
> +			 * Grow the data area by max(blocks needed,
> +			 * dbi_thresh / 2), but limited to the max
> +			 * DATA_BLOCK_BITS size.
> +			 */
> +			grow = max(blocks_needed, udev->dbi_thresh / 2);
> +			udev->dbi_thresh += grow;
> +			if (udev->dbi_thresh > DATA_BLOCK_BITS)
> +				udev->dbi_thresh = DATA_BLOCK_BITS;
> +		}
>  	}
>  
> +	if (!tcmu_get_empty_blocks(udev, cmd, blocks_needed))
> +		return false;
> +
>  	return true;
>  }
>  
> @@ -542,7 +636,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
>  
>  	WARN_ON(command_size & (TCMU_OP_ALIGN_SIZE-1));
>  
> -	spin_lock_irq(&udev->cmdr_lock);
> +	mutex_lock(&udev->cmdr_lock);
>  
>  	mb = udev->mb_addr;
>  	cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
> @@ -551,18 +645,18 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
>  		pr_warn("TCMU: Request of size %zu/%zu is too big for %u/%zu "
>  			"cmd ring/data area\n", command_size, data_length,
>  			udev->cmdr_size, udev->data_size);
> -		spin_unlock_irq(&udev->cmdr_lock);
> +		mutex_unlock(&udev->cmdr_lock);
>  		return TCM_INVALID_CDB_FIELD;
>  	}
>  
> -	while (!is_ring_space_avail(udev, command_size, data_length)) {
> +	while (!is_ring_space_avail(udev, tcmu_cmd, command_size, data_length)) {
>  		int ret;
>  		DEFINE_WAIT(__wait);
>  
>  		prepare_to_wait(&udev->wait_cmdr, &__wait, TASK_INTERRUPTIBLE);
>  
>  		pr_debug("sleeping for ring space\n");
> -		spin_unlock_irq(&udev->cmdr_lock);
> +		mutex_unlock(&udev->cmdr_lock);
>  		if (udev->cmd_time_out)
>  			ret = schedule_timeout(
>  					msecs_to_jiffies(udev->cmd_time_out));
> @@ -574,7 +668,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
>  			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  		}
>  
> -		spin_lock_irq(&udev->cmdr_lock);
> +		mutex_lock(&udev->cmdr_lock);
>  
>  		/* We dropped cmdr_lock, cmd_head is stale */
>  		cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
> @@ -607,6 +701,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
>  	entry->hdr.uflags = 0;
>  
>  	/* Handle allocating space from the data area */
> +	tcmu_cmd_reset_dbi_cur(tcmu_cmd);
>  	iov = &entry->req.iov[0];
>  	iov_cnt = 0;
>  	copy_to_data_area = (se_cmd->data_direction == DMA_TO_DEVICE
> @@ -616,6 +711,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
>  			&iov, &iov_cnt, copy_to_data_area);
>  	if (ret) {
>  		pr_err("tcmu: alloc and scatter data failed\n");
> +		mutex_unlock(&udev->cmdr_lock);


I think here and in the error case below, you need to do a
tcmu_cmd_free_data.


>  		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  	}
>  	entry->req.iov_cnt = iov_cnt;
> @@ -631,6 +727,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
>  					&iov, &iov_cnt, false);
>  		if (ret) {
>  			pr_err("tcmu: alloc and scatter bidi data failed\n");
> +			mutex_unlock(&udev->cmdr_lock);
>  			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  		}
>  		entry->req.iov_bidi_cnt = iov_cnt;
> @@ -644,8 +741,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
>  
>  	UPDATE_HEAD(mb->cmd_head, command_size, udev->cmdr_size);
>  	tcmu_flush_dcache_range(mb, sizeof(*mb));
> -
> -	spin_unlock_irq(&udev->cmdr_lock);
> +	mutex_unlock(&udev->cmdr_lock);
>  
>  	/* TODO: only if FLUSH and FUA? */
>  	uio_event_notify(&udev->uio_info);
> @@ -719,14 +815,13 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
>  
>  out:
>  	cmd->se_cmd = NULL;
> -	tcmu_cmd_free_data(cmd);
> +	tcmu_cmd_free_data(cmd, cmd->dbi_cnt);
>  	tcmu_free_cmd(cmd);
>  }
>  
>  static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
>  {
>  	struct tcmu_mailbox *mb;
> -	unsigned long flags;
>  	int handled = 0;
>  
>  	if (test_bit(TCMU_DEV_BIT_BROKEN, &udev->flags)) {
> @@ -734,8 +829,6 @@ static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
>  		return 0;
>  	}
>  
> -	spin_lock_irqsave(&udev->cmdr_lock, flags);
> -
>  	mb = udev->mb_addr;
>  	tcmu_flush_dcache_range(mb, sizeof(*mb));
>  
> @@ -776,8 +869,6 @@ static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
>  	if (mb->cmd_tail == mb->cmd_head)
>  		del_timer(&udev->timeout); /* no more pending cmds */
>  
> -	spin_unlock_irqrestore(&udev->cmdr_lock, flags);
> -
>  	wake_up(&udev->wait_cmdr);
>  
>  	return handled;
> @@ -804,16 +895,14 @@ static void tcmu_device_timedout(unsigned long data)
>  {
>  	struct tcmu_dev *udev = (struct tcmu_dev *)data;
>  	unsigned long flags;
> -	int handled;
> -
> -	handled = tcmu_handle_completions(udev);
> -
> -	pr_warn("%d completions handled from timeout\n", handled);
>  
>  	spin_lock_irqsave(&udev->commands_lock, flags);
>  	idr_for_each(&udev->commands, tcmu_check_expired_cmd, NULL);
>  	spin_unlock_irqrestore(&udev->commands_lock, flags);
>  
> +	/* Try to wake up the ummap thread */
> +	wake_up(&unmap_wait);
> +
>  	/*
>  	 * We don't need to wakeup threads on wait_cmdr since they have their
>  	 * own timeout.
> @@ -858,7 +947,7 @@ static struct se_device *tcmu_alloc_device(struct se_hba *hba, const char *name)
>  	udev->cmd_time_out = TCMU_TIME_OUT;
>  
>  	init_waitqueue_head(&udev->wait_cmdr);
> -	spin_lock_init(&udev->cmdr_lock);
> +	mutex_init(&udev->cmdr_lock);
>  
>  	idr_init(&udev->commands);
>  	spin_lock_init(&udev->commands_lock);
> @@ -873,59 +962,13 @@ static int tcmu_irqcontrol(struct uio_info *info, s32 irq_on)
>  {
>  	struct tcmu_dev *tcmu_dev = container_of(info, struct tcmu_dev, uio_info);
>  
> +	mutex_lock(&tcmu_dev->cmdr_lock);
>  	tcmu_handle_completions(tcmu_dev);
> +	mutex_unlock(&tcmu_dev->cmdr_lock);
>  
>  	return 0;
>  }
>  
> -static void tcmu_blocks_release(struct tcmu_dev *udev, bool release_pending)
> -{
> -	uint32_t dbi, end;
> -	void *addr;
> -
> -	spin_lock_irq(&udev->cmdr_lock);
> -
> -	end = udev->dbi_max + 1;
> -
> -	/* try to release all unused blocks */
> -	dbi = find_first_zero_bit(udev->data_bitmap, end);
> -	if (dbi >= end) {
> -		spin_unlock_irq(&udev->cmdr_lock);
> -		return;
> -	}
> -	do {
> -		addr = radix_tree_delete(&udev->data_blocks, dbi);
> -		kfree(addr);
> -
> -		dbi = find_next_zero_bit(udev->data_bitmap, end, dbi + 1);
> -	} while (dbi < end);
> -
> -	if (!release_pending)
> -		return;
> -
> -	/* try to release all pending blocks */
> -	dbi = find_first_bit(udev->data_bitmap, end);
> -	if (dbi >= end) {
> -		spin_unlock_irq(&udev->cmdr_lock);
> -		return;
> -	}
> -	do {
> -		addr = radix_tree_delete(&udev->data_blocks, dbi);
> -		kfree(addr);
> -
> -		dbi = find_next_bit(udev->data_bitmap, end, dbi + 1);
> -	} while (dbi < end);
> -
> -	spin_unlock_irq(&udev->cmdr_lock);
> -}
> -
> -static void tcmu_vma_close(struct vm_area_struct *vma)
> -{
> -	struct tcmu_dev *udev = vma->vm_private_data;
> -
> -	tcmu_blocks_release(udev, false);
> -}
> -
>  /*
>   * mmap code from uio.c. Copied here because we want to hook mmap()
>   * and this stuff must come along.
> @@ -943,6 +986,57 @@ static int tcmu_find_mem_index(struct vm_area_struct *vma)
>  	return -1;
>  }
>  
> +static struct page *tcmu_try_get_block_page(struct tcmu_dev *udev, uint32_t dbi)
> +{
> +	struct page *page;
> +	int ret;
> +
> +	mutex_lock(&udev->cmdr_lock);
> +	page = tcmu_get_block_page(udev, dbi);
> +	if (likely(page)) {
> +		mutex_unlock(&udev->cmdr_lock);
> +		return page;
> +	}
> +
> +	/*
> +	 * Normally it shouldn't be here:
> +	 * Only when the userspace has touched the blocks which
> +	 * are out of the tcmu_cmd's data iov[], and will return
> +	 * one zeroed page.


Is it a userspace bug when this happens? Do you know when it is occcuring?


> +	 */
> +	if (dbi >= udev->dbi_thresh) {
> +		/* Extern the udev->dbi_thresh to dbi + 1 */
> +		udev->dbi_thresh = dbi + 1;
> +		udev->dbi_max = dbi;
> +	}
> +
> +	page = radix_tree_lookup(&udev->data_blocks, dbi);
> +	if (!page) {
> +		page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> +		if (!page) {
> +			mutex_unlock(&udev->cmdr_lock);
> +			return NULL;
> +		}
> +
> +		ret = radix_tree_insert(&udev->data_blocks, dbi, page);
> +		if (ret) {
> +			mutex_unlock(&udev->cmdr_lock);
> +			__free_page(page);
> +			return NULL;
> +		}
> +
> +		/*
> +		 * Since this case is rare in page fault routine, here we
> +		 * will allow the global_db_count >= TCMU_GLOBAL_MAX_BLOCKS
> +		 * to reduce possible page fault call trace.
> +		 */
> +		atomic_inc(&global_db_count);
> +	}
> +	mutex_unlock(&udev->cmdr_lock);
> +
> +	return page;
> +}
> +
>  static int tcmu_vma_fault(struct vm_fault *vmf)
>  {
>  	struct tcmu_dev *udev = vmf->vma->vm_private_data;
> @@ -966,14 +1060,13 @@ static int tcmu_vma_fault(struct vm_fault *vmf)
>  		addr = (void *)(unsigned long)info->mem[mi].addr + offset;
>  		page = vmalloc_to_page(addr);
>  	} else {
> -		/* For the dynamically growing data area pages */
>  		uint32_t dbi;
>  
> +		/* For the dynamically growing data area pages */
>  		dbi = (offset - udev->data_off) / DATA_BLOCK_SIZE;
> -		addr = tcmu_get_block_addr(udev, dbi);
> -		if (!addr)
> +		page = tcmu_try_get_block_page(udev, dbi);
> +		if (!page)
>  			return VM_FAULT_NOPAGE;
> -		page = virt_to_page(addr);
>  	}
>  
>  	get_page(page);
> @@ -982,7 +1075,6 @@ static int tcmu_vma_fault(struct vm_fault *vmf)
>  }
>  
>  static const struct vm_operations_struct tcmu_vm_ops = {
> -	.close = tcmu_vma_close,
>  	.fault = tcmu_vma_fault,
>  };
>  
> @@ -1010,6 +1102,8 @@ static int tcmu_open(struct uio_info *info, struct inode *inode)
>  	if (test_and_set_bit(TCMU_DEV_BIT_OPEN, &udev->flags))
>  		return -EBUSY;
>  
> +	udev->inode = inode;
> +
>  	pr_debug("open\n");
>  
>  	return 0;
> @@ -1100,6 +1194,8 @@ static int tcmu_configure_device(struct se_device *dev)
>  	udev->cmdr_size = CMDR_SIZE - CMDR_OFF;
>  	udev->data_off = CMDR_SIZE;
>  	udev->data_size = DATA_SIZE;
> +	udev->dbi_thresh = 0; /* Default in Idle state */
> +	udev->waiting_global = false;
>  
>  	/* Initialise the mailbox of the ring buffer */
>  	mb = udev->mb_addr;
> @@ -1112,7 +1208,7 @@ static int tcmu_configure_device(struct se_device *dev)
>  	WARN_ON(udev->data_size % PAGE_SIZE);
>  	WARN_ON(udev->data_size % DATA_BLOCK_SIZE);
>  
> -	INIT_RADIX_TREE(&udev->data_blocks, GFP_ATOMIC);
> +	INIT_RADIX_TREE(&udev->data_blocks, GFP_KERNEL);
>  
>  	info->version = __stringify(TCMU_MAILBOX_VERSION);
>  
> @@ -1145,6 +1241,10 @@ static int tcmu_configure_device(struct se_device *dev)
>  	if (ret)
>  		goto err_netlink;
>  
> +	mutex_lock(&root_udev_mutex);
> +	list_add(&udev->node, &root_udev);
> +	mutex_unlock(&root_udev_mutex);
> +
>  	return 0;
>  
>  err_netlink:
> @@ -1179,6 +1279,23 @@ static bool tcmu_dev_configured(struct tcmu_dev *udev)
>  	return udev->uio_info.uio_dev ? true : false;
>  }
>  
> +static void tcmu_blocks_release(struct tcmu_dev *udev)
> +{
> +	int i;
> +	struct page *page;
> +
> +	/* Try to release all block pages */
> +	mutex_lock(&udev->cmdr_lock);
> +	for (i = 0; i <= udev->dbi_max; i++) {
> +		page = radix_tree_delete(&udev->data_blocks, i);
> +		if (page) {
> +			__free_page(page);
> +			atomic_dec(&global_db_count);
> +		}
> +	}
> +	mutex_unlock(&udev->cmdr_lock);
> +}
> +
>  static void tcmu_free_device(struct se_device *dev)
>  {
>  	struct tcmu_dev *udev = TCMU_DEV(dev);
> @@ -1188,6 +1305,10 @@ static void tcmu_free_device(struct se_device *dev)
>  
>  	del_timer_sync(&udev->timeout);
>  
> +	mutex_lock(&root_udev_mutex);
> +	list_del(&udev->node);
> +	mutex_unlock(&root_udev_mutex);
> +
>  	vfree(udev->mb_addr);
>  
>  	/* Upper layer should drain all requests before calling this */
> @@ -1200,7 +1321,7 @@ static void tcmu_free_device(struct se_device *dev)
>  	spin_unlock_irq(&udev->commands_lock);
>  	WARN_ON(!all_expired);
>  
> -	tcmu_blocks_release(udev, true);
> +	tcmu_blocks_release(udev);
>  
>  	if (tcmu_dev_configured(udev)) {
>  		tcmu_netlink_event(TCMU_CMD_REMOVED_DEVICE, udev->uio_info.name,
> @@ -1388,6 +1509,81 @@ static ssize_t tcmu_cmd_time_out_store(struct config_item *item, const char *pag
>  	.tb_dev_attrib_attrs	= NULL,
>  };
>  
> +static int unmap_thread_fn(void *data)
> +{
> +	struct tcmu_dev *udev;
> +	loff_t off;
> +	uint32_t start, end, block;
> +	struct page *page;
> +	int i;
> +
> +	while (1) {
> +		DEFINE_WAIT(__wait);
> +
> +		prepare_to_wait(&unmap_wait, &__wait, TASK_INTERRUPTIBLE);
> +		schedule();
> +		finish_wait(&unmap_wait, &__wait);
> +
> +		mutex_lock(&root_udev_mutex);
> +		list_for_each_entry(udev, &root_udev, node) {
> +			mutex_lock(&udev->cmdr_lock);
> +
> +			/* Try to complete the finished commands first */
> +			tcmu_handle_completions(udev);
> +
> +			/* Skip the udevs waiting the global pool or in idle */
> +			if (udev->waiting_global || !udev->dbi_thresh) {
> +				mutex_unlock(&udev->cmdr_lock);
> +				continue;
> +			}
> +
> +			end = udev->dbi_max + 1;
> +			block = find_last_bit(udev->data_bitmap, end);
> +			if (block == udev->dbi_max) {
> +				/*
> +				 * The last bit is dbi_max, so there is
> +				 * no need to shrink any blocks.
> +				 */
> +				mutex_unlock(&udev->cmdr_lock);
> +				continue;
> +			} else if (block == end) {
> +				/* The current udev will goto idle state */
> +				udev->dbi_thresh = start = 0;
> +				udev->dbi_max = 0;
> +			} else {
> +				udev->dbi_thresh = start = block + 1;
> +				udev->dbi_max = block;
> +			}
> +
> +			/* Here will truncate the data area from off */
> +			off = udev->data_off + start * DATA_BLOCK_SIZE;
> +			unmap_mapping_range(udev->inode->i_mapping, off, 0, 1);
> +
> +			/* Release the block pages */
> +			for (i = start; i < end; i++) {
> +				page = radix_tree_delete(&udev->data_blocks, i);
> +				if (page) {
> +					__free_page(page);
> +					atomic_dec(&global_db_count);
> +				}
> +			}
> +			mutex_unlock(&udev->cmdr_lock);
> +		}
> +
> +		/*
> +		 * Try to wake up the udevs who are waiting
> +		 * for the global data pool.
> +		 */
> +		list_for_each_entry(udev, &root_udev, node) {
> +			if (udev->waiting_global)
> +				wake_up(&udev->wait_cmdr);
> +		}


To avoid starvation, I think you want a second list/fifo that holds the
watiers. In tcmu_get_empty_block if the list is not empty, record how
many pages we needed and then add the device to the list and wait in
tcmu_queue_cmd_ring.

Above if we freed enough pages for the device at head then wake up the
device.

I think you also need a wake_up call in the completion path in case the
initial call could not free enough pages. It could probably check if the
completion was going to free enough pages for a waiter and then call wake.


> +		mutex_unlock(&root_udev_mutex);
> +	}
> +
> +	return 0;
> +}
> +
>  static int __init tcmu_module_init(void)
>  {
>  	int ret, i, len = 0;
> @@ -1433,8 +1629,17 @@ static int __init tcmu_module_init(void)
>  	if (ret)
>  		goto out_attrs;
>  
> +	init_waitqueue_head(&unmap_wait);
> +	unmap_thread = kthread_run(unmap_thread_fn, NULL, "tcmu_unmap");
> +	if (IS_ERR(unmap_thread)) {
> +		unmap_thread = NULL;

No need to set unmap_thread to NULL since nothing will ever see it
again. You should set ret to an error value so module_init is failed
properly:

ret = PTR_ERR(unmap_thread);


> +		goto out_unreg_transport;
> +	}
> +
>  	return 0;
>  
> +out_unreg_transport:
> +	target_backend_unregister(&tcmu_ops);
>  out_attrs:
>  	kfree(tcmu_attrs);
>  out_unreg_genl:
> @@ -1449,6 +1654,9 @@ static int __init tcmu_module_init(void)
>  
>  static void __exit tcmu_module_exit(void)
>  {
> +	if (unmap_thread)


The module exit will only be called if init succeeded so you can drop
the check.

> +		kthread_stop(unmap_thread);
> +
>  	target_backend_unregister(&tcmu_ops);
>  	kfree(tcmu_attrs);
>  	genl_unregister_family(&tcmu_genl_family);
> 

Re: [PATCH v6 2/2] tcmu: Add global data block pool support

[Xiubo Li]

[...]

>> +
>> +static bool tcmu_get_empty_blocks(struct tcmu_dev *udev,
>> +				  struct tcmu_cmd *tcmu_cmd,
>> +				  uint32_t blocks_needed)
>
> Can drop blocks_needed.
>
Will fix it.

[...]
>>   
>> -static void *tcmu_get_block_addr(struct tcmu_dev *udev, uint32_t dbi)
>> +static inline struct page *
>> +tcmu_get_block_page(struct tcmu_dev *udev, uint32_t dbi)
>>   {
>>   	return radix_tree_lookup(&udev->data_blocks, dbi);
>>   }
>> @@ -365,17 +417,20 @@ static int alloc_and_scatter_data_area(struct tcmu_dev *udev,
>
> We don't actually alloc the area here any more. Could probably rename
> it to be similar to gather_data_area.
>
Yes, will rename to scatter_data_area.

[...]
>
>> @@ -616,6 +711,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
>>   			&iov, &iov_cnt, copy_to_data_area);
>>   	if (ret) {
>>   		pr_err("tcmu: alloc and scatter data failed\n");
>> +		mutex_unlock(&udev->cmdr_lock);
>
> I think here and in the error case below, you need to do a
> tcmu_cmd_free_data.
>
Yes, good catch, and the tcmu_cmd_free_data is needed here to free the 
bits in udev->data_bitmap.
And the unlock will be added in the first patch.

[...]
>> +static struct page *tcmu_try_get_block_page(struct tcmu_dev *udev, uint32_t dbi)
>> +{
>> +	struct page *page;
>> +	int ret;
>> +
>> +	mutex_lock(&udev->cmdr_lock);
>> +	page = tcmu_get_block_page(udev, dbi);
>> +	if (likely(page)) {
>> +		mutex_unlock(&udev->cmdr_lock);
>> +		return page;
>> +	}
>> +
>> +	/*
>> +	 * Normally it shouldn't be here:
>> +	 * Only when the userspace has touched the blocks which
>> +	 * are out of the tcmu_cmd's data iov[], and will return
>> +	 * one zeroed page.
>
> Is it a userspace bug when this happens? Do you know when it is occcuring?
Since the UIO will map the whole ring buffer to the user space at the 
beginning, and the userspace is allowed and legal to access any block 
within the limits of the mapped ring area.

But actually when this happens, it normally will be one bug of the 
userspace. Without this checking the kernel will output many page fault 
dump traces.

Maybe here outputing some warning message is a good idea, and will be 
easy to debug for userspace.


[...]
>> @@ -1388,6 +1509,81 @@ static ssize_t tcmu_cmd_time_out_store(struct config_item *item, const char *pag
>>   	.tb_dev_attrib_attrs	= NULL,
>>   };
>>   
>> +static int unmap_thread_fn(void *data)
>> +{
>> +	struct tcmu_dev *udev;
>> +	loff_t off;
>> +	uint32_t start, end, block;
>> +	struct page *page;
>> +	int i;
>> +
>> +	while (1) {
>> +		DEFINE_WAIT(__wait);
>> +
>> +		prepare_to_wait(&unmap_wait, &__wait, TASK_INTERRUPTIBLE);
>> +		schedule();
>> +		finish_wait(&unmap_wait, &__wait);
>> +
>> +		mutex_lock(&root_udev_mutex);
>> +		list_for_each_entry(udev, &root_udev, node) {
>> +			mutex_lock(&udev->cmdr_lock);
>> +
>> +			/* Try to complete the finished commands first */
>> +			tcmu_handle_completions(udev);
>> +
>> +			/* Skip the udevs waiting the global pool or in idle */
>> +			if (udev->waiting_global || !udev->dbi_thresh) {
>> +				mutex_unlock(&udev->cmdr_lock);
>> +				continue;
>> +			}
>> +
>> +			end = udev->dbi_max + 1;
>> +			block = find_last_bit(udev->data_bitmap, end);
>> +			if (block == udev->dbi_max) {
>> +				/*
>> +				 * The last bit is dbi_max, so there is
>> +				 * no need to shrink any blocks.
>> +				 */
>> +				mutex_unlock(&udev->cmdr_lock);
>> +				continue;
>> +			} else if (block == end) {
>> +				/* The current udev will goto idle state */
>> +				udev->dbi_thresh = start = 0;
>> +				udev->dbi_max = 0;
>> +			} else {
>> +				udev->dbi_thresh = start = block + 1;
>> +				udev->dbi_max = block;
>> +			}
>> +
>> +			/* Here will truncate the data area from off */
>> +			off = udev->data_off + start * DATA_BLOCK_SIZE;
>> +			unmap_mapping_range(udev->inode->i_mapping, off, 0, 1);
>> +
>> +			/* Release the block pages */
>> +			for (i = start; i < end; i++) {
>> +				page = radix_tree_delete(&udev->data_blocks, i);
>> +				if (page) {
>> +					__free_page(page);
>> +					atomic_dec(&global_db_count);
>> +				}
>> +			}
>> +			mutex_unlock(&udev->cmdr_lock);
>> +		}
>> +
>> +		/*
>> +		 * Try to wake up the udevs who are waiting
>> +		 * for the global data pool.
>> +		 */
>> +		list_for_each_entry(udev, &root_udev, node) {
>> +			if (udev->waiting_global)
>> +				wake_up(&udev->wait_cmdr);
>> +		}
>
> To avoid starvation, I think you want a second list/fifo that holds the
> watiers. In tcmu_get_empty_block if the list is not empty, record how
> many pages we needed and then add the device to the list and wait in
> tcmu_queue_cmd_ring.
>
> Above if we freed enough pages for the device at head then wake up the
> device.
>
> I think you also need a wake_up call in the completion path in case the
> initial call could not free enough pages. It could probably check if the
> completion was going to free enough pages for a waiter and then call wake.
>
Yes, I meant to introduce this later after this series to not let the 
patches too
complex to review.

If you agree I will do this later, or in V7 series ?

>> +		mutex_unlock(&root_udev_mutex);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int __init tcmu_module_init(void)
>>   {
>>   	int ret, i, len = 0;
>> @@ -1433,8 +1629,17 @@ static int __init tcmu_module_init(void)
>>   	if (ret)
>>   		goto out_attrs;
>>   
>> +	init_waitqueue_head(&unmap_wait);
>> +	unmap_thread = kthread_run(unmap_thread_fn, NULL, "tcmu_unmap");
>> +	if (IS_ERR(unmap_thread)) {
>> +		unmap_thread = NULL;
> No need to set unmap_thread to NULL since nothing will ever see it
> again. You should set ret to an error value so module_init is failed
> properly:
>
> ret = PTR_ERR(unmap_thread);
>
>
Will fix it.

>> +		goto out_unreg_transport;
>> +	}
>> +
>>   	return 0;
>>   
>> +out_unreg_transport:
>> +	target_backend_unregister(&tcmu_ops);
>>   out_attrs:
>>   	kfree(tcmu_attrs);
>>   out_unreg_genl:
>> @@ -1449,6 +1654,9 @@ static int __init tcmu_module_init(void)
>>   
>>   static void __exit tcmu_module_exit(void)
>>   {
>> +	if (unmap_thread)
>
> The module exit will only be called if init succeeded so you can drop
> the check.
Will fix it.

Thank very much,

BRs
Xiubo

>> +		kthread_stop(unmap_thread);
>> +
>>   	target_backend_unregister(&tcmu_ops);
>>   	kfree(tcmu_attrs);
>>   	genl_unregister_family(&tcmu_genl_family);
>>

Re: [PATCH v6 2/2] tcmu: Add global data block pool support

[Mike Christie]

On 04/30/2017 06:29 AM, Xiubo Li wrote:
> [...]
>>> +static struct page *tcmu_try_get_block_page(struct tcmu_dev *udev,
>>> uint32_t dbi)
>>> +{
>>> +    struct page *page;
>>> +    int ret;
>>> +
>>> +    mutex_lock(&udev->cmdr_lock);
>>> +    page = tcmu_get_block_page(udev, dbi);
>>> +    if (likely(page)) {
>>> +        mutex_unlock(&udev->cmdr_lock);
>>> +        return page;
>>> +    }
>>> +
>>> +    /*
>>> +     * Normally it shouldn't be here:
>>> +     * Only when the userspace has touched the blocks which
>>> +     * are out of the tcmu_cmd's data iov[], and will return
>>> +     * one zeroed page.
>>
>> Is it a userspace bug when this happens? Do you know when it is
>> occcuring?
> Since the UIO will map the whole ring buffer to the user space at the
> beginning, and the userspace is allowed and legal to access any block
> within the limits of the mapped ring area.
> 
> But actually when this happens, it normally will be one bug of the
> userspace. Without this checking the kernel will output many page fault
> dump traces.
> 
> Maybe here outputing some warning message is a good idea, and will be
> easy to debug for userspace.

Yeah.

> 
> 
> [...]
>>> @@ -1388,6 +1509,81 @@ static ssize_t tcmu_cmd_time_out_store(struct
>>> config_item *item, const char *pag
>>>       .tb_dev_attrib_attrs    = NULL,
>>>   };
>>>   +static int unmap_thread_fn(void *data)
>>> +{
>>> +    struct tcmu_dev *udev;
>>> +    loff_t off;
>>> +    uint32_t start, end, block;
>>> +    struct page *page;
>>> +    int i;
>>> +
>>> +    while (1) {
>>> +        DEFINE_WAIT(__wait);
>>> +
>>> +        prepare_to_wait(&unmap_wait, &__wait, TASK_INTERRUPTIBLE);
>>> +        schedule();
>>> +        finish_wait(&unmap_wait, &__wait);
>>> +
>>> +        mutex_lock(&root_udev_mutex);
>>> +        list_for_each_entry(udev, &root_udev, node) {
>>> +            mutex_lock(&udev->cmdr_lock);
>>> +
>>> +            /* Try to complete the finished commands first */
>>> +            tcmu_handle_completions(udev);
>>> +
>>> +            /* Skip the udevs waiting the global pool or in idle */
>>> +            if (udev->waiting_global || !udev->dbi_thresh) {
>>> +                mutex_unlock(&udev->cmdr_lock);
>>> +                continue;
>>> +            }
>>> +
>>> +            end = udev->dbi_max + 1;
>>> +            block = find_last_bit(udev->data_bitmap, end);
>>> +            if (block == udev->dbi_max) {
>>> +                /*
>>> +                 * The last bit is dbi_max, so there is
>>> +                 * no need to shrink any blocks.
>>> +                 */
>>> +                mutex_unlock(&udev->cmdr_lock);
>>> +                continue;
>>> +            } else if (block == end) {
>>> +                /* The current udev will goto idle state */
>>> +                udev->dbi_thresh = start = 0;
>>> +                udev->dbi_max = 0;
>>> +            } else {
>>> +                udev->dbi_thresh = start = block + 1;
>>> +                udev->dbi_max = block;
>>> +            }
>>> +
>>> +            /* Here will truncate the data area from off */
>>> +            off = udev->data_off + start * DATA_BLOCK_SIZE;
>>> +            unmap_mapping_range(udev->inode->i_mapping, off, 0, 1);
>>> +
>>> +            /* Release the block pages */
>>> +            for (i = start; i < end; i++) {
>>> +                page = radix_tree_delete(&udev->data_blocks, i);
>>> +                if (page) {
>>> +                    __free_page(page);
>>> +                    atomic_dec(&global_db_count);
>>> +                }
>>> +            }
>>> +            mutex_unlock(&udev->cmdr_lock);
>>> +        }
>>> +
>>> +        /*
>>> +         * Try to wake up the udevs who are waiting
>>> +         * for the global data pool.
>>> +         */
>>> +        list_for_each_entry(udev, &root_udev, node) {
>>> +            if (udev->waiting_global)
>>> +                wake_up(&udev->wait_cmdr);
>>> +        }
>>
>> To avoid starvation, I think you want a second list/fifo that holds the
>> watiers. In tcmu_get_empty_block if the list is not empty, record how
>> many pages we needed and then add the device to the list and wait in
>> tcmu_queue_cmd_ring.
>>
>> Above if we freed enough pages for the device at head then wake up the
>> device.
>>
>> I think you also need a wake_up call in the completion path in case the
>> initial call could not free enough pages. It could probably check if the
>> completion was going to free enough pages for a waiter and then call
>> wake.
>>
> Yes, I meant to introduce this later after this series to not let the
> patches too
> complex to review.
> 
> If you agree I will do this later, or in V7 series ?


Yeah, I am ok with adding it after the initial patches go in.

Re: [PATCH v6 2/2] tcmu: Add global data block pool support

[Nicholas A. Bellinger]

On Mon, 2017-05-01 at 13:40 -0500, Mike Christie wrote:
> On 04/30/2017 06:29 AM, Xiubo Li wrote:
> > [...]

<SNIP>

> >> To avoid starvation, I think you want a second list/fifo that holds the
> >> watiers. In tcmu_get_empty_block if the list is not empty, record how
> >> many pages we needed and then add the device to the list and wait in
> >> tcmu_queue_cmd_ring.
> >>
> >> Above if we freed enough pages for the device at head then wake up the
> >> device.
> >>
> >> I think you also need a wake_up call in the completion path in case the
> >> initial call could not free enough pages. It could probably check if the
> >> completion was going to free enough pages for a waiter and then call
> >> wake.
> >>
> > Yes, I meant to introduce this later after this series to not let the
> > patches too
> > complex to review.
> > 
> > If you agree I will do this later, or in V7 series ?
> 
> 
> Yeah, I am ok with adding it after the initial patches go in.

Btw, adding your Acked-by for the initial merge of these.

If that's a problem, please make some noise.  ;)

Re: [PATCH v6 0/2] tcmu: Dynamic growing data area support

[Nicholas A. Bellinger]

Hi Xiubo & Co,

On Wed, 2017-04-26 at 14:25 +0800, lixiubo@cmss.chinamobile.com wrote:
> From: Xiubo Li <lixiubo@cmss.chinamobile.com>
> 
> Changed for V6:
> - Remove the tcmu_vma_close(). Since the unmap thread will do the same for it 
> - The unmap thread will skip the busy devices.
> - Using and testing the V5 version 3 weeks and the V6 for about 1 week, all in a higher IOPS environment:
>   * using fio and dd commands
>   * using about 4 targets based user:rbd/user:file backend
>   * set the global pool size to 512 * 1024 blocks * block_size, for 4K page size, the size is 2G.
>   * each target here needs more than 1100 blocks.
>   * fio: -iodepth 16 -thread -rw=[read write] -bs=[1M] -size=20G -numjobs=10 -runtime=1000  ...
>   * restart the tcmu-runner at any time.
> 
> Changed for V5:
> - Rebase to the newest target-pending repository.
> - Add as many comments as possbile to make the patch more readable.
> - Move tcmu_handle_completions() in timeout handler to unmap thread
>   and then replace the spin lock with mutex lock(because the unmap_*
>   or zap_* may goto sleep) to simplify the patch and the code.
> - Thanks very much for Mike's tips and suggestions.
> - Tested this for more than 3 days by:
>   * using fio and dd commands
>   * using about 1~5 targets
>   * set the global pool size to [512 1024 2048 512 * 1024] blocks * block_size
>   * each target here needs more than 450 blocks when running in my environments.
>   * fio: -iodepth [1 2 4 8 16] -thread -rw=[read write] -bs=[1K 2K 3K 5K 7K 16K 64K 1M] -size=20G -numjobs=10 -runtime=1000  ...
>   * in the tcmu-runner, try to touch blocks out of tcmu_cmds' iov[] manually
>   * restart the tcmu-runner at any time.
>   * in my environment for the low IOPS case: the read throughput goes from about 5200KB/s to 6700KB/s; the write throughput goes from about 3000KB/s to 3700KB/s.
> 
> Xiubo Li (2):
>   tcmu: Add dynamic growing data area feature support
>   tcmu: Add global data block pool support
> 
>  drivers/target/target_core_user.c | 598 ++++++++++++++++++++++++++++++--------
>  1 file changed, 469 insertions(+), 129 deletions(-)
> 

So based upon the feedback from MNC, this looks OK to merge.

It looks like there is one bug as reported by MNC introduced by patch
#1.

Please go ahead and post an incremental patch to address this at your
earliest convenience.

Thank you.

答复: [PATCH v6 0/2] tcmu: Dynamic growing data area support

[lixiubo]

> > Xiubo Li (2):
> >   tcmu: Add dynamic growing data area feature support
> >   tcmu: Add global data block pool support
> >
> >  drivers/target/target_core_user.c | 598
> > ++++++++++++++++++++++++++++++--------
> >  1 file changed, 469 insertions(+), 129 deletions(-)
> >
> 
> So based upon the feedback from MNC, this looks OK to merge.
> 
> It looks like there is one bug as reported by MNC introduced by patch 
> #1.
> 

It's actually one enhancement.

> Please go ahead and post an incremental patch to address this at your 
> earliest convenience.
> 

And I will address this later.

Thank,

BRs
Xiubo