Re: [PATCH 2/8] target: remove iblock WRITE_SAME passthrough support

2017-11-14 Thread Bryant G. Ly 
On 4/12/17 12:30 AM, Nicholas A. Bellinger wrote:

> On Mon, 2017-04-10 at 18:08 +0200, Christoph Hellwig wrote:
>> Use the pscsi driver to support arbitrary command passthrough
>> instead.
>>
> The people who are actively using iblock_execute_write_same_direct() are
> doing so in the context of ESX VAAI BlockZero, together with
> EXTENDED_COPY and COMPARE_AND_WRITE primitives.  Just using PSCSI is not
> an option for them.
>
> In practice though I've not seen any users of IBLOCK WRITE_SAME for
> anything other than VAAI BlockZero, so just using blkdev_issue_zeroout()
> when available, and falling back to iblock_execute_write_same() if the
> WRITE_SAME buffer contains anything other than zeros should be OK.
>
> How about something like the following below..?
>
> This would bring parity to how blkdev_issue_write_same() works atm wrt
> to synchronous bio completions.  However, most folks with a raw
> make_request or blk-mq backend driver that supports multiple GB/sec of
> zero bandwidth end up changing IBLOCK to support asynchronous
> REQ_WRITE_SAME completions anyways.
>
> I'd be happy to add support for that using __blkdev_issue_zeroout() once
> the basic conversion is in place.
>
> From ff74012eaff38f9fa0d74aca60507b9964f484ce Mon Sep 17 00:00:00 2001
> From: Nicholas Bellinger 
> Date: Tue, 11 Apr 2017 22:21:47 -0700
> Subject: [PATCH] target/iblock: Convert WRITE_SAME to blkdev_issue_zeroout
>
> Signed-off-by: Nicholas Bellinger 
> ---
>  drivers/target/target_core_iblock.c | 44 
> +++--
>  1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/target/target_core_iblock.c 
> b/drivers/target/target_core_iblock.c
> index d316ed5..5bfde20 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -86,6 +86,7 @@ static int iblock_configure_device(struct se_device *dev)
>   struct block_device *bd = NULL;
>   struct blk_integrity *bi;
>   fmode_t mode;
> + unsigned int max_write_zeroes_sectors;
>   int ret = -ENOMEM;
>
>   if (!(ib_dev->ibd_flags & IBDF_HAS_UDEV_PATH)) {
> @@ -129,7 +130,11 @@ static int iblock_configure_device(struct se_device *dev)
>* Enable write same emulation for IBLOCK and use 0x as
>* the smaller WRITE_SAME(10) only has a two-byte block count.
>*/
> - dev->dev_attrib.max_write_same_len = 0x;
> + max_write_zeroes_sectors = bdev_write_zeroes_sectors(bd);
> + if (max_write_zeroes_sectors)
> + dev->dev_attrib.max_write_same_len = max_write_zeroes_sectors;
> + else
> + dev->dev_attrib.max_write_same_len = 0x;
>
>   if (blk_queue_nonrot(q))
>   dev->dev_attrib.is_nonrot = 1;
> @@ -415,28 +420,31 @@ static void iblock_end_io_flush(struct bio *bio)
>  }
>
>  static sense_reason_t
> -iblock_execute_write_same_direct(struct block_device *bdev, struct se_cmd 
> *cmd)
> +iblock_execute_zero_out(struct block_device *bdev, struct se_cmd *cmd)
>  {
>   struct se_device *dev = cmd->se_dev;
>   struct scatterlist *sg = >t_data_sg[0];
> - struct page *page = NULL;
> - int ret;
> + unsigned char *buf, zero = 0x00, *p = 
> + int rc, ret;
>
> - if (sg->offset) {
> - page = alloc_page(GFP_KERNEL);
> - if (!page)
> - return TCM_OUT_OF_RESOURCES;
> - sg_copy_to_buffer(sg, cmd->t_data_nents, page_address(page),
> -   dev->dev_attrib.block_size);
> - }
> + buf = kmap(sg_page(sg)) + sg->offset;
> + if (!buf)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> + /*
> +  * Fall back to block_execute_write_same() slow-path if
> +  * incoming WRITE_SAME payload does not contain zeros.
> +  */
> + rc = memcmp(buf, p, cmd->data_length);
> + kunmap(sg_page(sg));
> +

I recently pulled in this patch and I am getting:

[  716.756756] [ cut here ]
[  716.756757] kernel BUG at 
/build/linux-hwe-edge-F6_Smd/linux-hwe-edge-4.13.0/lib/string.c:985!
[  716.756762] Oops: Exception in kernel mode, sig: 5 [#1]
[  716.756764] SMP NR_CPUS=2048 
[  716.756765] NUMA 
[  716.756767] pSeries
[  716.756769] Modules linked in: hvcs(OE) hvcserver dm_snapshot dm_bufio 
ip6table_raw xt_CT xt_mac xt_tcpudp xt_comment xt_physdev xt_set 
ip_set_hash_net ip_set rpadlpar_io rpaphp iptable_raw target_core_pscsi(OE) 
target_core_file(OE) target_core_iblock(OE) iscsi_target_mod(OE) dccp_diag dccp 
tcp_diag udp_diag inet_diag unix_diag af_packet_diag netlink_diag vport_vxlan 
vxlan ip6_udp_tunnel udp_tunnel openvswitch nf_nat_ipv6 target_core_user(OE) 
uio binfmt_misc ibmvmc(OE) pseries_rng vmx_crypto crct10dif_vpmsum xt_conntrack 
nf_nat_ftp nf_conntrack_ftp nf_conntrack_netlink nfnetlink 
nf_conntrack_netbios_ns nf_conntrack_broadcast nf_conntrack_ipv6 nf_defrag_ipv6 
nbd ipt_REJECT nf_reject_ipv4 ipt_MASQUERADE

Re: [PATCH 2/8] target: remove iblock WRITE_SAME passthrough support

2017-06-01 Thread Christoph Hellwig 
On Wed, May 31, 2017 at 11:27:01PM -0700, Nicholas A. Bellinger wrote:
> Hey HCH & Jens,
> 
> Is this already queued up for v4.13 to address the missing LBPRZ feature
> bit..?
> 
> If not, I'll happy to take it via target-pending along with the
> following to re-enable it via max_write_zeroes_sectors.

It's not queued up.  Feel free to take it through the target tree.

Re: [PATCH 2/8] target: remove iblock WRITE_SAME passthrough support

2017-06-01 Thread Nicholas A. Bellinger 
Hey HCH & Jens,

Is this already queued up for v4.13 to address the missing LBPRZ feature
bit..?

If not, I'll happy to take it via target-pending along with the
following to re-enable it via max_write_zeroes_sectors.

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index d2f089c..e7caf78 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 = 0;
+   attrib->unmap_zeroes_data = (q->limits.max_write_zeroes_sectors);
return true;
 }
 EXPORT_SYMBOL(target_configure_unmap_from_queue);

Any objections..?

On Tue, 2017-04-11 at 22:30 -0700, Nicholas A. Bellinger wrote:
> On Mon, 2017-04-10 at 18:08 +0200, Christoph Hellwig wrote:
> > Use the pscsi driver to support arbitrary command passthrough
> > instead.
> > 
> 
> The people who are actively using iblock_execute_write_same_direct() are
> doing so in the context of ESX VAAI BlockZero, together with
> EXTENDED_COPY and COMPARE_AND_WRITE primitives.  Just using PSCSI is not
> an option for them.
> 
> In practice though I've not seen any users of IBLOCK WRITE_SAME for
> anything other than VAAI BlockZero, so just using blkdev_issue_zeroout()
> when available, and falling back to iblock_execute_write_same() if the
> WRITE_SAME buffer contains anything other than zeros should be OK.
> 
> How about something like the following below..?
> 
> This would bring parity to how blkdev_issue_write_same() works atm wrt
> to synchronous bio completions.  However, most folks with a raw
> make_request or blk-mq backend driver that supports multiple GB/sec of
> zero bandwidth end up changing IBLOCK to support asynchronous
> REQ_WRITE_SAME completions anyways.
> 
> I'd be happy to add support for that using __blkdev_issue_zeroout() once
> the basic conversion is in place.
> 
> From ff74012eaff38f9fa0d74aca60507b9964f484ce Mon Sep 17 00:00:00 2001
> From: Nicholas Bellinger 
> Date: Tue, 11 Apr 2017 22:21:47 -0700
> Subject: [PATCH] target/iblock: Convert WRITE_SAME to blkdev_issue_zeroout
> 
> Signed-off-by: Nicholas Bellinger 
> ---
>  drivers/target/target_core_iblock.c | 44 
> +++--
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/target/target_core_iblock.c 
> b/drivers/target/target_core_iblock.c
> index d316ed5..5bfde20 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -86,6 +86,7 @@ static int iblock_configure_device(struct se_device *dev)
>   struct block_device *bd = NULL;
>   struct blk_integrity *bi;
>   fmode_t mode;
> + unsigned int max_write_zeroes_sectors;
>   int ret = -ENOMEM;
>  
>   if (!(ib_dev->ibd_flags & IBDF_HAS_UDEV_PATH)) {
> @@ -129,7 +130,11 @@ static int iblock_configure_device(struct se_device *dev)
>* Enable write same emulation for IBLOCK and use 0x as
>* the smaller WRITE_SAME(10) only has a two-byte block count.
>*/
> - dev->dev_attrib.max_write_same_len = 0x;
> + max_write_zeroes_sectors = bdev_write_zeroes_sectors(bd);
> + if (max_write_zeroes_sectors)
> + dev->dev_attrib.max_write_same_len = max_write_zeroes_sectors;
> + else
> + dev->dev_attrib.max_write_same_len = 0x;
>  
>   if (blk_queue_nonrot(q))
>   dev->dev_attrib.is_nonrot = 1;
> @@ -415,28 +420,31 @@ static void iblock_end_io_flush(struct bio *bio)
>  }
>  
>  static sense_reason_t
> -iblock_execute_write_same_direct(struct block_device *bdev, struct se_cmd 
> *cmd)
> +iblock_execute_zero_out(struct block_device *bdev, struct se_cmd *cmd)
>  {
>   struct se_device *dev = cmd->se_dev;
>   struct scatterlist *sg = >t_data_sg[0];
> - struct page *page = NULL;
> - int ret;
> + unsigned char *buf, zero = 0x00, *p = 
> + int rc, ret;
>  
> - if (sg->offset) {
> - page = alloc_page(GFP_KERNEL);
> - if (!page)
> - return TCM_OUT_OF_RESOURCES;
> - sg_copy_to_buffer(sg, cmd->t_data_nents, page_address(page),
> -   dev->dev_attrib.block_size);
> - }
> + buf = kmap(sg_page(sg)) + sg->offset;
> + if (!buf)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> + /*
> +  * Fall back to block_execute_write_same() slow-path if
> +  * incoming WRITE_SAME payload does not contain zeros.
> +  */
> + rc = memcmp(buf, p, cmd->data_length);
> + kunmap(sg_page(sg));
> +
> + if (rc)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;

Re: [PATCH 2/8] target: remove iblock WRITE_SAME passthrough support

2017-04-11 Thread Christoph Hellwig 
Hi Nic,

this patch looks fine, and I'll include it for the next post.  I'll
move some of the explanation in this mail into the patch, though.

Re: [PATCH 2/8] target: remove iblock WRITE_SAME passthrough support

2017-04-11 Thread Nicholas A. Bellinger 
On Mon, 2017-04-10 at 18:08 +0200, Christoph Hellwig wrote:
> Use the pscsi driver to support arbitrary command passthrough
> instead.
> 

The people who are actively using iblock_execute_write_same_direct() are
doing so in the context of ESX VAAI BlockZero, together with
EXTENDED_COPY and COMPARE_AND_WRITE primitives.  Just using PSCSI is not
an option for them.

In practice though I've not seen any users of IBLOCK WRITE_SAME for
anything other than VAAI BlockZero, so just using blkdev_issue_zeroout()
when available, and falling back to iblock_execute_write_same() if the
WRITE_SAME buffer contains anything other than zeros should be OK.

How about something like the following below..?

This would bring parity to how blkdev_issue_write_same() works atm wrt
to synchronous bio completions.  However, most folks with a raw
make_request or blk-mq backend driver that supports multiple GB/sec of
zero bandwidth end up changing IBLOCK to support asynchronous
REQ_WRITE_SAME completions anyways.

I'd be happy to add support for that using __blkdev_issue_zeroout() once
the basic conversion is in place.

>From ff74012eaff38f9fa0d74aca60507b9964f484ce Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger 
Date: Tue, 11 Apr 2017 22:21:47 -0700
Subject: [PATCH] target/iblock: Convert WRITE_SAME to blkdev_issue_zeroout

Signed-off-by: Nicholas Bellinger 
---
 drivers/target/target_core_iblock.c | 44 +++--
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/target/target_core_iblock.c 
b/drivers/target/target_core_iblock.c
index d316ed5..5bfde20 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -86,6 +86,7 @@ static int iblock_configure_device(struct se_device *dev)
struct block_device *bd = NULL;
struct blk_integrity *bi;
fmode_t mode;
+   unsigned int max_write_zeroes_sectors;
int ret = -ENOMEM;
 
if (!(ib_dev->ibd_flags & IBDF_HAS_UDEV_PATH)) {
@@ -129,7 +130,11 @@ static int iblock_configure_device(struct se_device *dev)
 * Enable write same emulation for IBLOCK and use 0x as
 * the smaller WRITE_SAME(10) only has a two-byte block count.
 */
-   dev->dev_attrib.max_write_same_len = 0x;
+   max_write_zeroes_sectors = bdev_write_zeroes_sectors(bd);
+   if (max_write_zeroes_sectors)
+   dev->dev_attrib.max_write_same_len = max_write_zeroes_sectors;
+   else
+   dev->dev_attrib.max_write_same_len = 0x;
 
if (blk_queue_nonrot(q))
dev->dev_attrib.is_nonrot = 1;
@@ -415,28 +420,31 @@ static void iblock_end_io_flush(struct bio *bio)
 }
 
 static sense_reason_t
-iblock_execute_write_same_direct(struct block_device *bdev, struct se_cmd *cmd)
+iblock_execute_zero_out(struct block_device *bdev, struct se_cmd *cmd)
 {
struct se_device *dev = cmd->se_dev;
struct scatterlist *sg = >t_data_sg[0];
-   struct page *page = NULL;
-   int ret;
+   unsigned char *buf, zero = 0x00, *p = 
+   int rc, ret;
 
-   if (sg->offset) {
-   page = alloc_page(GFP_KERNEL);
-   if (!page)
-   return TCM_OUT_OF_RESOURCES;
-   sg_copy_to_buffer(sg, cmd->t_data_nents, page_address(page),
- dev->dev_attrib.block_size);
-   }
+   buf = kmap(sg_page(sg)) + sg->offset;
+   if (!buf)
+   return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+   /*
+* Fall back to block_execute_write_same() slow-path if
+* incoming WRITE_SAME payload does not contain zeros.
+*/
+   rc = memcmp(buf, p, cmd->data_length);
+   kunmap(sg_page(sg));
+
+   if (rc)
+   return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
-   ret = blkdev_issue_write_same(bdev,
+   ret = blkdev_issue_zeroout(bdev,
target_to_linux_sector(dev, cmd->t_task_lba),
target_to_linux_sector(dev,
sbc_get_write_same_sectors(cmd)),
-   GFP_KERNEL, page ? page : sg_page(sg));
-   if (page)
-   __free_page(page);
+   GFP_KERNEL, false);
if (ret)
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
@@ -472,8 +480,10 @@ static void iblock_end_io_flush(struct bio *bio)
return TCM_INVALID_CDB_FIELD;
}
 
-   if (bdev_write_same(bdev))
-   return iblock_execute_write_same_direct(bdev, cmd);
+   if (bdev_write_zeroes_sectors(bdev)) {
+   if (!iblock_execute_zero_out(bdev, cmd))
+   return 0;
+   }
 
ibr = kzalloc(sizeof(struct iblock_req), GFP_KERNEL);
if (!ibr)

[PATCH 2/8] target: remove iblock WRITE_SAME passthrough support

2017-04-10 Thread Christoph Hellwig 
Use the pscsi driver to support arbitrary command passthrough
instead.

Signed-off-by: Christoph Hellwig 
---
 drivers/target/target_core_iblock.c | 34 --
 1 file changed, 34 deletions(-)

diff --git a/drivers/target/target_core_iblock.c 
b/drivers/target/target_core_iblock.c
index d316ed537d59..9da31970a004 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -415,39 +415,8 @@ iblock_execute_unmap(struct se_cmd *cmd, sector_t lba, 
sector_t nolb)
 }
 
 static sense_reason_t
-iblock_execute_write_same_direct(struct block_device *bdev, struct se_cmd *cmd)
-{
-   struct se_device *dev = cmd->se_dev;
-   struct scatterlist *sg = >t_data_sg[0];
-   struct page *page = NULL;
-   int ret;
-
-   if (sg->offset) {
-   page = alloc_page(GFP_KERNEL);
-   if (!page)
-   return TCM_OUT_OF_RESOURCES;
-   sg_copy_to_buffer(sg, cmd->t_data_nents, page_address(page),
- dev->dev_attrib.block_size);
-   }
-
-   ret = blkdev_issue_write_same(bdev,
-   target_to_linux_sector(dev, cmd->t_task_lba),
-   target_to_linux_sector(dev,
-   sbc_get_write_same_sectors(cmd)),
-   GFP_KERNEL, page ? page : sg_page(sg));
-   if (page)
-   __free_page(page);
-   if (ret)
-   return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-
-   target_complete_cmd(cmd, GOOD);
-   return 0;
-}
-
-static sense_reason_t
 iblock_execute_write_same(struct se_cmd *cmd)
 {
-   struct block_device *bdev = IBLOCK_DEV(cmd->se_dev)->ibd_bd;
struct iblock_req *ibr;
struct scatterlist *sg;
struct bio *bio;
@@ -472,9 +441,6 @@ iblock_execute_write_same(struct se_cmd *cmd)
return TCM_INVALID_CDB_FIELD;
}
 
-   if (bdev_write_same(bdev))
-   return iblock_execute_write_same_direct(bdev, cmd);
-
ibr = kzalloc(sizeof(struct iblock_req), GFP_KERNEL);
if (!ibr)
goto fail;
-- 
2.11.0