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

[Nicholas A. Bellinger]

On Tue, 2017-05-02 at 11:38 +0800, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> Changed for V7:
> - #1 fix two issues.
> - #2 fix kbuild warning and some issues.
> 
> 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 | 602 
> ++
>  1 file changed, 473 insertions(+), 129 deletions(-)
> 

Ah, just missed the v7.  ;)

Applied.

Thanks 

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:
> > [...]



> >> 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, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> 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.

Re: [PATCH] target: fixup error message in target_tg_pt_gp_tg_pt_gp_id_store()

[Nicholas A. Bellinger]

On Fri, 2017-04-28 at 10:04 +0200, Hannes Reinecke wrote:
> When setting up an ALUA target port group with an invalid ID the
> error message
> 
> kstrtoul() returned -22 for tg_pt_gp_id
> 
> is displayed, which is not really helpful.
> Convert it to something sane.
> And while we're at it, join the messages onto a single line.
> 
> Signed-by: Hannes Reinecke 
> ---
>  drivers/target/target_core_configfs.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 

Applied.

Thanks Hannes.

Re: [PATCH] target: fixup error message in target_tg_pt_gp_alua_access_type_store()

[Nicholas A. Bellinger]

On Fri, 2017-04-28 at 10:03 +0200, Hannes Reinecke wrote:
> When setting up a target the error message:
> 
> Unable to do set ##_name ALUA state on non valid tg_pt_gp ID: 0
> 
> is displayed.
> Apparently concatenation doesn't work in a string; one should be using
> implicit string concatenation here.
> 
> Signed-off-by: Hannes Reinecke 
> Reviewed-by: Bart van Assche 
> ---
>  drivers/target/target_core_configfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied.

Thanks Hannes.

Re: [PATCH v3] target/user: PGR Support

[Nicholas A. Bellinger]

On Fri, 2017-04-21 at 20:40 -0500, Bryant G. Ly wrote:
> This adds initial PGR support for just TCMU, since tcmu doesn't
> have the necessary IT_NEXUS info to process PGR in userspace,
> so have those commands be processed in kernel.
> 
> HA support is not available yet, we will work on it if this patch
> is acceptable.
> 
> Signed-off-by: Bryant G. Ly 
> ---
>  drivers/target/target_core_configfs.c | 10 -
>  drivers/target/target_core_device.c   | 38 
> +++
>  drivers/target/target_core_pr.c   |  2 +-
>  drivers/target/target_core_pscsi.c|  3 ++-
>  include/target/target_core_backend.h  |  1 +
>  5 files changed, 47 insertions(+), 7 deletions(-)

Applied.

Thanks alot for adding the target_cmd_size_check() into
passthrough_parse_cdb(), btw.  :)

Re: [PATCH] target: Add WRITE_VERIFY_16

[Nicholas A. Bellinger]

On Tue, 2017-04-18 at 14:10 -0500, Bryant G. Ly wrote:
> This patch addresses clients who needs write_verify_16 for
> large volume groups such as AIX.
> 
> Signed-off-by: Bryant G. Ly 
> ---
>  drivers/target/target_core_sbc.c | 2 ++
>  include/scsi/scsi_proto.h| 1 +
>  2 files changed, 3 insertions(+)

Applied.

Thanks Bryant.

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Nicholas A. Bellinger]

Hi Bryant & Co,

Apologies for the delayed follow up.

Comments below.

On Mon, 2017-04-10 at 13:52 -0500, Bryant G. Ly wrote:
> The driver is sending a response to the aborted task response
> along with LIO sending the tmr response.
> ibmvscsis_tgt does not send the response to the client until
> release_cmd time. The reason for this was because if we did it
> at queue_status time, then the client would be free to reuse the
> tag for that command, but we're still using the tag until the
> command is released at release_cmd time, so we chose to delay
> sending the response until then. That then caused this issue, because
> release_cmd is always called, even if queue_status is not.
> 
> SCSI spec says that the initiator that sends the abort task
> TM NEVER gets a response to the aborted op and with the current
> code it will send a response. Thus this fix will remove that response
> if the TAS bit is set.
> 
> Cc:  # v4.8+
> Signed-off-by: Bryant G. Ly 
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 60 
> +---
>  1 file changed, 40 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
> b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 4bb5635..f75948a 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -1758,33 +1758,42 @@ static void ibmvscsis_send_messages(struct scsi_info 
> *vscsi)
>  
>   if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
>   list_for_each_entry_safe(cmd, nxt, >waiting_rsp, list) {
> - iue = cmd->iue;
> + /*
> +  * If Task Abort Status Bit is set, then dont send a
> +  * response.
> +  */
> + if (cmd->se_cmd.transport_state & CMD_T_TAS) {
> + list_del(>list);
> + ibmvscsis_free_cmd_resources(vscsi, cmd);
> + } else {
> + iue = cmd->iue;

Unless I'm mistaken, this should be a check for CMD_T_ABORTED &&
!CMD_T_TAS to avoid generating an explicit response + immediately free,
and not a check for CMD_T_TAS when a command still needs a explicit
response..

That is, CMD_T_TAS is the only scenario where target-core is supposed to
generate an explicit response of SAM_STAT_TASK_ABORTED, which means
h_send_crq() should be called for those se_cmd descriptors.

However for CMD_T_ABORTED w/o CMD_T_TAS scenarios (like TMR TASK_ABORT
for example) where target-core does not call ->queue_status(), this is
the case where ibmvscsis_send_messages() should be ignoring calls to
h_send_crq(), because se_cmd descriptors must not be generating response
after they have been aborted.

> @@ -3581,9 +3590,20 @@ static int ibmvscsis_write_pending(struct se_cmd 
> *se_cmd)
>  {
>   struct ibmvscsis_cmd *cmd = container_of(se_cmd, struct ibmvscsis_cmd,
>se_cmd);
> + struct scsi_info *vscsi = cmd->adapter;
>   struct iu_entry *iue = cmd->iue;
>   int rc;
>  
> + /*
> +  * If CLIENT_FAILED OR RESPONSE_Q_DOWN, then just return success
> +  * since LIO can't do anything about it, and we dont want to
> +  * attempt an srp_transfer_data.
> +  */
> + if ((vscsi->flags & (CLIENT_FAILED | RESPONSE_Q_DOWN))) {
> + pr_err("write_pending failed since: %d\n", vscsi->flags);
> + return 0;
> + }
> +
>   rc = srp_transfer_data(cmd, _iu(iue)->srp.cmd, ibmvscsis_rdma,
>  1, 1);
>   if (rc) {

AFAICT, returning '0' here is correct to avoid generating an explicit
CHECK_CONDITION for non -EAGAIN or -ENOMEM return values.

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

[lixiubo]

From: Xiubo Li 

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 
Signed-off-by: Jianfei Hu 
---
 drivers/target/target_core_user.c | 338 ++
 1 file changed, 237 insertions(+), 101 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index f615c3b..02cf543 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -2,6 +2,7 @@
  * Copyright (C) 2013 Shaohua Li 
  * 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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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(>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(>data_blocks, dbi, p);
+   if (ret) {
+   kfree(p);
+   

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

[lixiubo]

From: Xiubo Li 

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 
Signed-off-by: Jianfei Hu 
---
 drivers/target/target_core_user.c | 464 +++---
 1 file changed, 336 insertions(+), 128 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 02cf543..0b29e4f 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -31,6 +31,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -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,78 @@ 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 == 

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

[lixiubo]

From: Xiubo Li 

Changed for V7:
- #1 fix two issues.
- #2 fix kbuild warning and some issues.

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 | 602 ++
 1 file changed, 473 insertions(+), 129 deletions(-)

-- 
1.8.3.1

Re: [PATCH 0/5] target: Fine-tuning for some function implementations

[Nicholas A. Bellinger]

Hi Markus,

Apologies for the delayed follow-up here as well.

On Sun, 2017-04-09 at 21:43 +0200, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Sun, 9 Apr 2017 21:33:21 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (5):
>   rd: Use kcalloc() in two functions
>   rd: Delete error messages for failed memory allocations
>   rd: Improve size determinations in two functions
>   sbc: Use kmalloc_array() in compare_and_write_callback()
>   transport: Use kmalloc_array() in transport_kmap_data_sg()
> 
>  drivers/target/target_core_rd.c| 33 +
>  drivers/target/target_core_sbc.c   |  4 ++--
>  drivers/target/target_core_transport.c |  2 +-
>  3 files changed, 12 insertions(+), 27 deletions(-)
> 

Applied.

Thank you.

Re: [PATCH 0/3] iSCSI-target: Fine-tuning for some function implementations

[Nicholas A. Bellinger]

Hi Markus,

Apologies for the extended delayed response.

On Sun, 2017-04-09 at 16:20 +0200, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Sun, 9 Apr 2017 16:11:23 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (3):
>   Use kcalloc() in iscsit_allocate_iovecs()
>   Delete error messages for failed memory allocations
>   Improve size determinations in four functions
> 
>  drivers/target/iscsi/iscsi_target.c | 50 
> +++--
>  1 file changed, 15 insertions(+), 35 deletions(-)
> 

Applied.

Thank you.

Re: [PATCH 25/27] block: remove the discard_zeroes_data flag

[Bart Van Assche]

On Wed, 2017-04-05 at 19:21 +0200, Christoph Hellwig wrote:
> Now that we use the proper REQ_OP_WRITE_ZEROES operation everywhere we can
> kill this hack.
> 
> Signed-off-by: Christoph Hellwig 
> Reviewed-by: Martin K. Petersen 
> Reviewed-by: Hannes Reinecke 
> [ ... ]
> diff --git a/drivers/target/target_core_device.c 
> b/drivers/target/target_core_device.c
> index c754ae33bf7b..d2f089cfa9ae 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -851,7 +851,7 @@ bool target_configure_unmap_from_queue(struct 
> se_dev_attrib *attrib,
>   attrib->unmap_granularity = q->limits.discard_granularity / block_size;
>   attrib->unmap_granularity_alignment = q->limits.discard_alignment /
>   block_size;
> - attrib->unmap_zeroes_data = q->limits.discard_zeroes_data;
> + attrib->unmap_zeroes_data = 0;
>   return true;
>  }
>  EXPORT_SYMBOL(target_configure_unmap_from_queue);

Hello Christoph,

Sorry that I hadn't noticed this before but I think that this patch
introduces a significant performance regressions for LIO users. Before
this patch the LBPRZ flag was reported correctly to initiator systems
through the thin provisioning VPD. With this patch applied that flag
will always be reported as zero, forcing initiators to submit WRITE
commands with zeroed data buffers instead of submitting the SCSI UNMAP
command to block devices for which discard_zeroes_data was set. From
target_core_spc.c:

/* Thin Provisioning VPD */
static sense_reason_t spc_emulate_evpd_b2(struct se_cmd *cmd, unsigned char 
*buf)
{
[ ... ]
/*
 * The unmap_zeroes_data set means that the underlying device supports
 * REQ_DISCARD and has the discard_zeroes_data bit set. This satisfies
 * the SBC requirements for LBPRZ, meaning that a subsequent read
 * will return zeroes after an UNMAP or WRITE SAME (16) to an LBA
 * See sbc4r36 6.6.4.
 */
if (((dev->dev_attrib.emulate_tpu != 0) ||
 (dev->dev_attrib.emulate_tpws != 0)) &&
 (dev->dev_attrib.unmap_zeroes_data != 0))
buf[5] |= 0x04;
[ ... ]
}

Please advise how to proceed.

Thanks,

Bart.

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(>cmdr_lock);
>>> +page = tcmu_get_block_page(udev, dbi);
>>> +if (likely(page)) {
>>> +mutex_unlock(>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(_wait, &__wait, TASK_INTERRUPTIBLE);
>>> +schedule();
>>> +finish_wait(_wait, &__wait);
>>> +
>>> +mutex_lock(_udev_mutex);
>>> +list_for_each_entry(udev, _udev, node) {
>>> +mutex_lock(>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(>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(>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(>data_blocks, i);
>>> +if (page) {
>>> +__free_page(page);
>>> +atomic_dec(_db_count);
>>> +}
>>> +}
>>> +mutex_unlock(>cmdr_lock);
>>> +}
>>> +
>>> +/*
>>> + * Try to wake up the udevs who are waiting
>>> + * for the global data pool.
>>> + */
>>> +list_for_each_entry(udev, _udev, node) {
>>> +if (udev->waiting_global)
>>> +wake_up(>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 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, lixi...@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, );
>>> +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.

Update Your Webmail Account Within 24 Hours

[Brigitte Kätzler]

Dear E-mail User

Please follow the link to update your webmail account  
http://ets1n4w20.ulcraft.com


Thank You