[Qemu-devel] [PATCHv2 00/20] block: logical block provisioning enhancements

[Qemu-devel] [PATCHv2 00/20] block: logical block provisioning enhancements

[Peter Lieven]

this patch adds the ability for targets to stay sparse during
block migration (if the zero_blocks capability is set) and qemu-img convert
even if the target does not have has_zero_init = 1.

the series was especially developed for iSCSI, but it should also work
with other drivers with little or no adjustments. these adjustments
should be limited to providing block provisioning information through
get_block_info and/or honouring BDRV_REQ_MAY_UNMAP on writing zeroes.

there are also 4 patches which are fixes/enhancements for the get_block_status API
discovered during development of this series.

v1->v2:
 - moved block max_discard and max_write_zeroes to BlockDriverState
 - added discard_alignment and write_zeroes_alignment to BlockDriverState
 - added bdrv_has_discard_zeroes() and bdrv_has_discard_write_zeroes()
 - added logic to bdrv_co_discard and bdrv_co_do_write_zeroes to honour
   limit and alignment info.
 - added support for -S 0 in qemu-img convert.

Peter Lieven (20):
  block: make BdrvRequestFlags public
  block: add flags to bdrv_*_write_zeroes
  block: introduce BDRV_REQ_MAY_UNMAP request flag
  block: introduce bdrv_has_discard_zeroes and
    bdrv_has_discard_write_zeroes
  block/raw: add bdrv_has_discard_zeroes and
    bdrv_has_discard_write_zeroes
  block: add discard and write_zeroes limits and alignment to
    BlockDriverState
  block: honour alignment and limit in bdrv_co_do_write_zeroes
  block: honour alignment and limit in bdrv_co_discard
  iscsi: simplify iscsi_co_discard
  iscsi: set limits in BlockDriverState
  iscsi: add bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes
  iscsi: add bdrv_co_write_zeroes
  block: introduce bdrv_zeroize
  block/get_block_status: set *pnum = 0 on error
  block/get_block_status: avoid segfault if there is no backing_hd
  block/get_block_status: avoid redundant callouts on raw devices
  block/get_block_status: fix BDRV_BLOCK_ZERO for unallocated blocks
  qemu-img: add support for fully allocated images
  qemu-img: conditionally zero out target on convert
  block/raw: copy block limits and alignment information on raw_open

 block-migration.c         |    3 +-
 block.c                   |  187 ++++++++++++++++++++++++++++++++++++---------
 block/backup.c            |    3 +-
 block/iscsi.c             |  148 +++++++++++++++++++++++++----------
 block/qcow2.c             |    2 +-
 block/qed.c               |    3 +-
 block/raw_bsd.c           |   65 ++++++++++------
 block/vmdk.c              |    3 +-
 include/block/block.h     |   19 ++++-
 include/block/block_int.h |   27 ++++++-
 qemu-img.c                |   18 ++++-
 qemu-io-cmds.c            |    2 +-
 12 files changed, 368 insertions(+), 112 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCHv2 01/20] block: make BdrvRequestFlags public

[Peter Lieven]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c               |    5 -----
 include/block/block.h |    5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index a325efc..878f365 100644
--- a/block.c
+++ b/block.c
@@ -51,11 +51,6 @@
 
 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
 
-typedef enum {
-    BDRV_REQ_COPY_ON_READ = 0x1,
-    BDRV_REQ_ZERO_WRITE   = 0x2,
-} BdrvRequestFlags;
-
 static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load);
 static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
diff --git a/include/block/block.h b/include/block/block.h
index 728ec1a..3249261 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -62,6 +62,11 @@ typedef struct BlockDevOps {
     void (*resize_cb)(void *opaque);
 } BlockDevOps;
 
+typedef enum {
+    BDRV_REQ_COPY_ON_READ = 0x1,
+    BDRV_REQ_ZERO_WRITE   = 0x2,
+} BdrvRequestFlags;
+
 #define BDRV_O_RDWR        0x0002
 #define BDRV_O_SNAPSHOT    0x0008 /* open the file read only and save writes in a snapshot */
 #define BDRV_O_NOCACHE     0x0020 /* do not use the host page cache */
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 02/20] block: add flags to bdrv_*_write_zeroes

[Peter Lieven]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block-migration.c         |    2 +-
 block.c                   |   20 +++++++++++---------
 block/backup.c            |    3 ++-
 block/qcow2.c             |    2 +-
 block/qed.c               |    3 ++-
 block/raw_bsd.c           |    5 +++--
 block/vmdk.c              |    3 ++-
 include/block/block.h     |    4 ++--
 include/block/block_int.h |    2 +-
 qemu-io-cmds.c            |    2 +-
 10 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index daf9ec1..713a8e3 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -780,7 +780,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
             }
 
             if (flags & BLK_MIG_FLAG_ZERO_BLOCK) {
-                ret = bdrv_write_zeroes(bs, addr, nr_sectors);
+                ret = bdrv_write_zeroes(bs, addr, nr_sectors, 0);
             } else {
                 buf = g_malloc(BLOCK_SIZE);
                 qemu_get_buffer(f, buf, BLOCK_SIZE);
diff --git a/block.c b/block.c
index 878f365..e5918a7 100644
--- a/block.c
+++ b/block.c
@@ -79,7 +79,7 @@ static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
                                                bool is_write);
 static void coroutine_fn bdrv_co_do_rw(void *opaque);
 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
-    int64_t sector_num, int nb_sectors);
+    int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
 
 static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
     QTAILQ_HEAD_INITIALIZER(bdrv_states);
@@ -2335,10 +2335,11 @@ int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov)
     return bdrv_rwv_co(bs, sector_num, qiov, true, 0);
 }
 
-int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
+int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
+                      int nb_sectors, BdrvRequestFlags flags)
 {
     return bdrv_rw_co(bs, sector_num, NULL, nb_sectors, true,
-                      BDRV_REQ_ZERO_WRITE);
+                      BDRV_REQ_ZERO_WRITE | flags);
 }
 
 int bdrv_pread(BlockDriverState *bs, int64_t offset,
@@ -2520,7 +2521,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BlockDriverState *bs,
     if (drv->bdrv_co_write_zeroes &&
         buffer_is_zero(bounce_buffer, iov.iov_len)) {
         ret = bdrv_co_do_write_zeroes(bs, cluster_sector_num,
-                                      cluster_nb_sectors);
+                                      cluster_nb_sectors, 0);
     } else {
         /* This does not change the data on the disk, it is not necessary
          * to flush even in cache=writethrough mode.
@@ -2654,7 +2655,7 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
 }
 
 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
-    int64_t sector_num, int nb_sectors)
+    int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
 {
     BlockDriver *drv = bs->drv;
     QEMUIOVector qiov;
@@ -2666,7 +2667,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
 
     /* First try the efficient write zeroes operation */
     if (drv->bdrv_co_write_zeroes) {
-        ret = drv->bdrv_co_write_zeroes(bs, sector_num, nb_sectors);
+        ret = drv->bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
         if (ret != -ENOTSUP) {
             return ret;
         }
@@ -2721,7 +2722,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
     if (ret < 0) {
         /* Do nothing, write notifier decided to fail this request */
     } else if (flags & BDRV_REQ_ZERO_WRITE) {
-        ret = bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors);
+        ret = bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors, flags);
     } else {
         ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
     }
@@ -2755,12 +2756,13 @@ int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
 }
 
 int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
-                                      int64_t sector_num, int nb_sectors)
+                                      int64_t sector_num, int nb_sectors,
+                                      BdrvRequestFlags flags)
 {
     trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors);
 
     return bdrv_co_do_writev(bs, sector_num, nb_sectors, NULL,
-                             BDRV_REQ_ZERO_WRITE);
+                             BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /**
diff --git a/block/backup.c b/block/backup.c
index 04c4b5c..5f6a642 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -138,7 +138,8 @@ static int coroutine_fn backup_do_cow(BlockDriverState *bs,
 
         if (buffer_is_zero(iov.iov_base, iov.iov_len)) {
             ret = bdrv_co_write_zeroes(job->target,
-                                       start * BACKUP_SECTORS_PER_CLUSTER, n);
+                                       start * BACKUP_SECTORS_PER_CLUSTER,
+                                       n, 0);
         } else {
             ret = bdrv_co_writev(job->target,
                                  start * BACKUP_SECTORS_PER_CLUSTER, n,
diff --git a/block/qcow2.c b/block/qcow2.c
index 578792f..2cb59a2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1555,7 +1555,7 @@ static int qcow2_make_empty(BlockDriverState *bs)
 }
 
 static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs,
-    int64_t sector_num, int nb_sectors)
+    int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
 {
     int ret;
     BDRVQcowState *s = bs->opaque;
diff --git a/block/qed.c b/block/qed.c
index 49b3a37..d6e42f3 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1389,7 +1389,8 @@ static void coroutine_fn qed_co_write_zeroes_cb(void *opaque, int ret)
 
 static int coroutine_fn bdrv_qed_co_write_zeroes(BlockDriverState *bs,
                                                  int64_t sector_num,
-                                                 int nb_sectors)
+                                                 int nb_sectors,
+                                                 BdrvRequestFlags flags)
 {
     BlockDriverAIOCB *blockacb;
     BDRVQEDState *s = bs->opaque;
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index a9060ca..bd4811b 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -66,9 +66,10 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
 }
 
 static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
-                                            int64_t sector_num, int nb_sectors)
+                                            int64_t sector_num, int nb_sectors,
+                                            BdrvRequestFlags flags)
 {
-    return bdrv_co_write_zeroes(bs->file, sector_num, nb_sectors);
+    return bdrv_co_write_zeroes(bs->file, sector_num, nb_sectors, flags);
 }
 
 static int coroutine_fn raw_co_discard(BlockDriverState *bs,
diff --git a/block/vmdk.c b/block/vmdk.c
index fb5b529..914a9b7 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1386,7 +1386,8 @@ static coroutine_fn int vmdk_co_write(BlockDriverState *bs, int64_t sector_num,
 
 static int coroutine_fn vmdk_co_write_zeroes(BlockDriverState *bs,
                                              int64_t sector_num,
-                                             int nb_sectors)
+                                             int nb_sectors,
+                                             BdrvRequestFlags flags)
 {
     int ret;
     BDRVVmdkState *s = bs->opaque;
diff --git a/include/block/block.h b/include/block/block.h
index 3249261..ddb6870 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -187,7 +187,7 @@ int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
 int bdrv_write(BlockDriverState *bs, int64_t sector_num,
                const uint8_t *buf, int nb_sectors);
 int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
-               int nb_sectors);
+               int nb_sectors, BdrvRequestFlags flags);
 int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
 int bdrv_pread(BlockDriverState *bs, int64_t offset,
                void *buf, int count);
@@ -209,7 +209,7 @@ int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
  * because it may allocate memory for the entire region.
  */
 int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
-    int nb_sectors);
+    int nb_sectors, BdrvRequestFlags flags);
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     const char *backing_file);
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7c35198..26bad36 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -115,7 +115,7 @@ struct BlockDriver {
      * instead.
      */
     int coroutine_fn (*bdrv_co_write_zeroes)(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors);
+        int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
     int coroutine_fn (*bdrv_co_discard)(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors);
     int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 8565d49..66c0b9e 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -441,7 +441,7 @@ static void coroutine_fn co_write_zeroes_entry(void *opaque)
     CoWriteZeroes *data = opaque;
 
     data->ret = bdrv_co_write_zeroes(data->bs, data->offset / BDRV_SECTOR_SIZE,
-                                     data->count / BDRV_SECTOR_SIZE);
+                                     data->count / BDRV_SECTOR_SIZE, 0);
     data->done = true;
     if (data->ret < 0) {
         *data->total = data->ret;
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 03/20] block: introduce BDRV_REQ_MAY_UNMAP request flag

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block-migration.c     |    3 ++-
 block.c               |    4 ++++
 block/backup.c        |    2 +-
 include/block/block.h |    7 +++++++
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 713a8e3..fc4ef93 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -780,7 +780,8 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
             }
 
             if (flags & BLK_MIG_FLAG_ZERO_BLOCK) {
-                ret = bdrv_write_zeroes(bs, addr, nr_sectors, 0);
+                ret = bdrv_write_zeroes(bs, addr, nr_sectors,
+                                        BDRV_REQ_MAY_UNMAP);
             } else {
                 buf = g_malloc(BLOCK_SIZE);
                 qemu_get_buffer(f, buf, BLOCK_SIZE);
diff --git a/block.c b/block.c
index e5918a7..6f498fc 100644
--- a/block.c
+++ b/block.c
@@ -2761,6 +2761,10 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
 {
     trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors);
 
+    if (!(bs->open_flags & BDRV_O_UNMAP)) {
+        flags &= ~BDRV_REQ_MAY_UNMAP;
+    }
+
     return bdrv_co_do_writev(bs, sector_num, nb_sectors, NULL,
                              BDRV_REQ_ZERO_WRITE | flags);
 }
diff --git a/block/backup.c b/block/backup.c
index 5f6a642..1ab080d 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -139,7 +139,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState *bs,
         if (buffer_is_zero(iov.iov_base, iov.iov_len)) {
             ret = bdrv_co_write_zeroes(job->target,
                                        start * BACKUP_SECTORS_PER_CLUSTER,
-                                       n, 0);
+                                       n, BDRV_REQ_MAY_UNMAP);
         } else {
             ret = bdrv_co_writev(job->target,
                                  start * BACKUP_SECTORS_PER_CLUSTER, n,
diff --git a/include/block/block.h b/include/block/block.h
index ddb6870..0328234 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -65,6 +65,13 @@ typedef struct BlockDevOps {
 typedef enum {
     BDRV_REQ_COPY_ON_READ = 0x1,
     BDRV_REQ_ZERO_WRITE   = 0x2,
+    /* the BDRV_REQ_MAY_UNMAP flag is used to indicate that block driver
+     * is allowed to optimize a write zeroes request by unmapping (discarding)
+     * blocks if it is guaranteed that the result will read back as
+     * zeroes. the flag is only passed to the driver if the block device is
+     * opened with BDRV_O_UNMAP.
+     */
+    BDRV_REQ_MAY_UNMAP    = 0x4,
 } BdrvRequestFlags;
 
 #define BDRV_O_RDWR        0x0002
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 03/20] block: introduce BDRV_REQ_MAY_UNMAP request flag

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block-migration.c     |    3 ++-
>  block.c               |    4 ++++
>  block/backup.c        |    2 +-
>  include/block/block.h |    7 +++++++
>  4 files changed, 14 insertions(+), 2 deletions(-)


> +++ b/include/block/block.h
> @@ -65,6 +65,13 @@ typedef struct BlockDevOps {
>  typedef enum {
>      BDRV_REQ_COPY_ON_READ = 0x1,
>      BDRV_REQ_ZERO_WRITE   = 0x2,
> +    /* the BDRV_REQ_MAY_UNMAP flag is used to indicate that block driver

Minor grammar improvements:

s/the/The/
s/that/that the/

> +     * is allowed to optimize a write zeroes request by unmapping (discarding)
> +     * blocks if it is guaranteed that the result will read back as
> +     * zeroes. the flag is only passed to the driver if the block device is

s/the flag/The flag/

> +     * opened with BDRV_O_UNMAP.
> +     */
> +    BDRV_REQ_MAY_UNMAP    = 0x4,

As those are trivial, feel free to add:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 04/20] block: introduce bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c                   |   29 +++++++++++++++++++++++++++++
 include/block/block.h     |    2 ++
 include/block/block_int.h |   13 +++++++++++++
 3 files changed, 44 insertions(+)

diff --git a/block.c b/block.c
index 6f498fc..177720e 100644
--- a/block.c
+++ b/block.c
@@ -3045,6 +3045,35 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     return 0;
 }
 
+int bdrv_has_discard_zeroes(BlockDriverState *bs)
+{
+    assert(bs->drv);
+
+    if (bs->backing_hd) {
+        return 0;
+    }
+    if (bs->drv->bdrv_has_discard_zeroes) {
+        return bs->drv->bdrv_has_discard_zeroes(bs);
+    }
+
+    return 0;
+}
+
+int bdrv_has_discard_write_zeroes(BlockDriverState *bs)
+{
+    assert(bs->drv);
+
+    if (bs->backing_hd || !(bs->open_flags & BDRV_O_UNMAP)) {
+        return 0;
+    }
+
+    if (bs->drv->bdrv_has_discard_write_zeroes) {
+        return bs->drv->bdrv_has_discard_write_zeroes(bs);
+    }
+
+    return 0;
+}
+
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
diff --git a/include/block/block.h b/include/block/block.h
index 0328234..8aa325a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -307,6 +307,8 @@ int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
 int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
+int bdrv_has_discard_zeroes(BlockDriverState *bs);
+int bdrv_has_discard_write_zeroes(BlockDriverState *bs);
 int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
                               int nb_sectors, int *pnum);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 26bad36..85c3474 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -202,6 +202,19 @@ struct BlockDriver {
      */
     int (*bdrv_has_zero_init)(BlockDriverState *bs);
 
+    /*
+     * Returns 1 if discarded blocks read back as zeroes, 0 otherwise.
+     */
+    int (*bdrv_has_discard_zeroes)(BlockDriverState *bs);
+
+    /*
+     * Returns 1 if the driver can optimize writing zeroes by discarding
+     * sectors. It is additionally required that the block device is
+     * opened with BDRV_O_UNMAP and the that write zeroes request carries
+     * the BDRV_REQ_MAY_UNMAP flag for this to work.
+     */
+    int (*bdrv_has_discard_write_zeroes)(BlockDriverState *bs);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 04/20] block: introduce bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block.c                   |   29 +++++++++++++++++++++++++++++
>  include/block/block.h     |    2 ++
>  include/block/block_int.h |   13 +++++++++++++
>  3 files changed, 44 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 05/20] block/raw: add bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/raw_bsd.c |   56 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index bd4811b..0bfa5fc 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -131,6 +131,16 @@ static int raw_has_zero_init(BlockDriverState *bs)
     return bdrv_has_zero_init(bs->file);
 }
 
+static int raw_has_discard_zeroes(BlockDriverState *bs)
+{
+    return bdrv_has_discard_zeroes(bs->file);
+}
+
+static int raw_has_discard_write_zeroes(BlockDriverState *bs)
+{
+    return bdrv_has_discard_write_zeroes(bs->file);
+}
+
 static int raw_create(const char *filename, QEMUOptionParameter *options)
 {
     return bdrv_create_file(filename, options);
@@ -155,28 +165,30 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
 }
 
 static BlockDriver bdrv_raw = {
-    .format_name          = "raw",
-    .bdrv_probe           = &raw_probe,
-    .bdrv_reopen_prepare  = &raw_reopen_prepare,
-    .bdrv_open            = &raw_open,
-    .bdrv_close           = &raw_close,
-    .bdrv_create          = &raw_create,
-    .bdrv_co_readv        = &raw_co_readv,
-    .bdrv_co_writev       = &raw_co_writev,
-    .bdrv_co_write_zeroes = &raw_co_write_zeroes,
-    .bdrv_co_discard      = &raw_co_discard,
-    .bdrv_co_get_block_status = &raw_co_get_block_status,
-    .bdrv_truncate        = &raw_truncate,
-    .bdrv_getlength       = &raw_getlength,
-    .bdrv_get_info        = &raw_get_info,
-    .bdrv_is_inserted     = &raw_is_inserted,
-    .bdrv_media_changed   = &raw_media_changed,
-    .bdrv_eject           = &raw_eject,
-    .bdrv_lock_medium     = &raw_lock_medium,
-    .bdrv_ioctl           = &raw_ioctl,
-    .bdrv_aio_ioctl       = &raw_aio_ioctl,
-    .create_options       = &raw_create_options[0],
-    .bdrv_has_zero_init   = &raw_has_zero_init
+    .format_name                   = "raw",
+    .bdrv_probe                    = &raw_probe,
+    .bdrv_reopen_prepare           = &raw_reopen_prepare,
+    .bdrv_open                     = &raw_open,
+    .bdrv_close                    = &raw_close,
+    .bdrv_create                   = &raw_create,
+    .bdrv_co_readv                 = &raw_co_readv,
+    .bdrv_co_writev                = &raw_co_writev,
+    .bdrv_co_write_zeroes          = &raw_co_write_zeroes,
+    .bdrv_co_discard               = &raw_co_discard,
+    .bdrv_co_get_block_status      = &raw_co_get_block_status,
+    .bdrv_truncate                 = &raw_truncate,
+    .bdrv_getlength                = &raw_getlength,
+    .bdrv_get_info                 = &raw_get_info,
+    .bdrv_is_inserted              = &raw_is_inserted,
+    .bdrv_media_changed            = &raw_media_changed,
+    .bdrv_eject                    = &raw_eject,
+    .bdrv_lock_medium              = &raw_lock_medium,
+    .bdrv_ioctl                    = &raw_ioctl,
+    .bdrv_aio_ioctl                = &raw_aio_ioctl,
+    .create_options                = &raw_create_options[0],
+    .bdrv_has_zero_init            = &raw_has_zero_init,
+    .bdrv_has_discard_zeroes       = &raw_has_discard_zeroes,
+    .bdrv_has_discard_write_zeroes = &raw_has_discard_write_zeroes,
 };
 
 static void bdrv_raw_init(void)
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 05/20] block/raw: add bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/raw_bsd.c |   56 +++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 22 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 06/20] block: add discard and write_zeroes limits and alignment to BlockDriverState

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 include/block/block_int.h |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 85c3474..692b9ed 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -280,6 +280,18 @@ struct BlockDriverState {
     /* the memory alignment required for the buffers handled by this driver */
     int buffer_alignment;
 
+    /* maximum number of sectors that can be discarded at once */
+    int max_discard;
+
+    /* optimal alignment for discard requests in sectors */
+    int64_t discard_alignment;
+
+    /* maximum number of sectors that can zeroized at once */
+    int max_write_zeroes;
+
+    /* optimal alignment for write zeroes requests in sectors */
+    int64_t write_zeroes_alignment;
+
     /* do we need to tell the quest if we have a volatile write cache? */
     int enable_write_cache;
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 06/20] block: add discard and write_zeroes limits and alignment to BlockDriverState

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  include/block/block_int.h |   12 ++++++++++++
>  1 file changed, 12 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> +
> +    /* optimal alignment for write zeroes requests in sectors */
> +    int64_t write_zeroes_alignment;
> +
>      /* do we need to tell the quest if we have a volatile write cache? */
>      int enable_write_cache;

Hmm, I just pointed out to Paolo that this ought to be bool.  But as it
is in the context and not your actual patch, it has no bearing on this
series, and even if that gets changed first, git's pretty good about
context-only conflict resolution.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCHv2 06/20] block: add discard and write_zeroes limits and alignment to BlockDriverState

[Peter Lieven]

Am 19.09.2013 20:53, schrieb Eric Blake:
> On 09/17/2013 07:48 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  include/block/block_int.h |   12 ++++++++++++
>>  1 file changed, 12 insertions(+)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +
>> +    /* optimal alignment for write zeroes requests in sectors */
>> +    int64_t write_zeroes_alignment;
>> +
Paolo voted for a BlockLimits struct to be nested into the BlockDriverState.
It would make it easier if the settings have to be copied (like in raw_open).
There might be more limits in the future so it might be good that its not necessary
to change the code everywhere.

>>      /* do we need to tell the quest if we have a volatile write cache? */
>>      int enable_write_cache;
> Hmm, I just pointed out to Paolo that this ought to be bool.  But as it
> is in the context and not your actual patch, it has no bearing on this
> series, and even if that gets changed first, git's pretty good about
> context-only conflict resolution.
>
If I put a v3 which is likely I can put a patch for that in as well.

Peter

[Qemu-devel] [PATCHv2 07/20] block: honour alignment and limit in bdrv_co_do_write_zeroes

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c |   57 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 177720e..74ec342 100644
--- a/block.c
+++ b/block.c
@@ -2660,28 +2660,53 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     BlockDriver *drv = bs->drv;
     QEMUIOVector qiov;
     struct iovec iov;
-    int ret;
+    int ret = 0;
 
-    /* TODO Emulate only part of misaligned requests instead of letting block
-     * drivers return -ENOTSUP and emulate everything */
+    /* if no limit is specified in the BlockDriverState use a default
+     * of 32768 512-byte sectors (16 MiB) per request.
+     */
+    int max_write_zeroes = bs->max_write_zeroes ? bs->max_write_zeroes : 32768;
 
-    /* First try the efficient write zeroes operation */
-    if (drv->bdrv_co_write_zeroes) {
-        ret = drv->bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
-        if (ret != -ENOTSUP) {
-            return ret;
+    while (nb_sectors > 0 && !ret) {
+        int num = nb_sectors;
+
+        /* align request */
+        if (bs->write_zeroes_alignment &&
+            num >= bs->write_zeroes_alignment &&
+            sector_num % bs->write_zeroes_alignment) {
+            if (num > bs->write_zeroes_alignment) {
+                num = bs->write_zeroes_alignment;
+            }
+            num -= sector_num % bs->write_zeroes_alignment;
         }
-    }
 
-    /* Fall back to bounce buffer if write zeroes is unsupported */
-    iov.iov_len  = nb_sectors * BDRV_SECTOR_SIZE;
-    iov.iov_base = qemu_blockalign(bs, iov.iov_len);
-    memset(iov.iov_base, 0, iov.iov_len);
-    qemu_iovec_init_external(&qiov, &iov, 1);
+        /* limit request size */
+        if (num > max_write_zeroes) {
+            num = max_write_zeroes;
+        }
 
-    ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, &qiov);
+        ret = -ENOTSUP;
+        /* First try the efficient write zeroes operation */
+        if (drv->bdrv_co_write_zeroes) {
+            ret = drv->bdrv_co_write_zeroes(bs, sector_num, num, flags);
+        }
+
+        if (ret == -ENOTSUP) {
+            /* Fall back to bounce buffer if write zeroes is unsupported */
+            iov.iov_len  = nb_sectors * BDRV_SECTOR_SIZE;
+            iov.iov_base = qemu_blockalign(bs, iov.iov_len);
+            memset(iov.iov_base, 0, iov.iov_len);
+            qemu_iovec_init_external(&qiov, &iov, 1);
+
+            ret = drv->bdrv_co_writev(bs, sector_num, num, &qiov);
+
+            qemu_vfree(iov.iov_base);
+        }
+
+        sector_num += num;
+        nb_sectors -= num;
+    }
 
-    qemu_vfree(iov.iov_base);
     return ret;
 }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 07/20] block: honour alignment and limit in bdrv_co_do_write_zeroes

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block.c |   57 +++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 41 insertions(+), 16 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 177720e..74ec342 100644
> --- a/block.c
> +++ b/block.c
> @@ -2660,28 +2660,53 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>      BlockDriver *drv = bs->drv;
>      QEMUIOVector qiov;
>      struct iovec iov;
> -    int ret;
> +    int ret = 0;
>  
> -    /* TODO Emulate only part of misaligned requests instead of letting block
> -     * drivers return -ENOTSUP and emulate everything */
> +    /* if no limit is specified in the BlockDriverState use a default
> +     * of 32768 512-byte sectors (16 MiB) per request.
> +     */
> +    int max_write_zeroes = bs->max_write_zeroes ? bs->max_write_zeroes : 32768;

Worth having a named constant instead of a magic number?

> +    while (nb_sectors > 0 && !ret) {

> +
> +        if (ret == -ENOTSUP) {
> +            /* Fall back to bounce buffer if write zeroes is unsupported */
> +            iov.iov_len  = nb_sectors * BDRV_SECTOR_SIZE;
> +            iov.iov_base = qemu_blockalign(bs, iov.iov_len);
> +            memset(iov.iov_base, 0, iov.iov_len);

This allocates, clears, and frees iov.iov_len bytes of 0 every iteration
through the loop.  Can you hoist that so you only allocate the bounce
buffer once, and then clean it up after the loop completes?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCHv2 07/20] block: honour alignment and limit in bdrv_co_do_write_zeroes

[Peter Lieven]

Am 19.09.2013 um 22:07 schrieb Eric Blake <eblake@redhat.com>:

> On 09/17/2013 07:48 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> block.c |   57 +++++++++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 41 insertions(+), 16 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 177720e..74ec342 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2660,28 +2660,53 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>>     BlockDriver *drv = bs->drv;
>>     QEMUIOVector qiov;
>>     struct iovec iov;
>> -    int ret;
>> +    int ret = 0;
>> 
>> -    /* TODO Emulate only part of misaligned requests instead of letting block
>> -     * drivers return -ENOTSUP and emulate everything */
>> +    /* if no limit is specified in the BlockDriverState use a default
>> +     * of 32768 512-byte sectors (16 MiB) per request.
>> +     */
>> +    int max_write_zeroes = bs->max_write_zeroes ? bs->max_write_zeroes : 32768;
> 
> Worth having a named constant instead of a magic number?

Its the only place where it is used. I can do that, but I would keep it in front of the
function rather than at the top of block.h or even in block_int.h

> 
>> +    while (nb_sectors > 0 && !ret) {
> 
>> +
>> +        if (ret == -ENOTSUP) {
>> +            /* Fall back to bounce buffer if write zeroes is unsupported */
>> +            iov.iov_len  = nb_sectors * BDRV_SECTOR_SIZE;
>> +            iov.iov_base = qemu_blockalign(bs, iov.iov_len);
>> +            memset(iov.iov_base, 0, iov.iov_len);
> 
> This allocates, clears, and frees iov.iov_len bytes of 0 every iteration
> through the loop.  Can you hoist that so you only allocate the bounce
> buffer once, and then clean it up after the loop completes?

I can do that, but in this case, I have to allocate max_write_zeroes * BDRV_SECTOR_SIZE
bytes because the iov_len might not be maximal in the first iteration due to the alignment
logic.

Peter

[Qemu-devel] [PATCHv2 08/20] block: honour alignment and limit in bdrv_co_discard

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c |   34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 74ec342..ecc5be4 100644
--- a/block.c
+++ b/block.c
@@ -4181,7 +4181,39 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
     }
 
     if (bs->drv->bdrv_co_discard) {
-        return bs->drv->bdrv_co_discard(bs, sector_num, nb_sectors);
+        /* if no limit is specified in the BlockDriverState use a default
+         * of 32768 512-byte sectors (16 MiB) per request.
+         */
+        int max_discard = bs->max_discard ? bs->max_discard : 32768;
+
+        while (nb_sectors > 0) {
+            int ret;
+            int num = nb_sectors;
+
+            /* align request */
+            if (bs->discard_alignment &&
+                num >= bs->discard_alignment &&
+                sector_num % bs->discard_alignment) {
+                if (num > bs->discard_alignment) {
+                    num = bs->discard_alignment;
+                }
+                num -= sector_num % bs->discard_alignment;
+            }
+
+            /* limit request size */
+            if (num > max_discard) {
+                num = max_discard;
+            }
+
+            ret = bs->drv->bdrv_co_discard(bs, sector_num, num);
+            if (ret) {
+                return ret;
+            }
+
+            sector_num += num;
+            nb_sectors -= num;
+        }
+        return 0;
     } else if (bs->drv->bdrv_aio_discard) {
         BlockDriverAIOCB *acb;
         CoroutineIOCompletion co = {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 08/20] block: honour alignment and limit in bdrv_co_discard

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block.c |   34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 74ec342..ecc5be4 100644
> --- a/block.c
> +++ b/block.c
> @@ -4181,7 +4181,39 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>      }
>  
>      if (bs->drv->bdrv_co_discard) {
> -        return bs->drv->bdrv_co_discard(bs, sector_num, nb_sectors);
> +        /* if no limit is specified in the BlockDriverState use a default
> +         * of 32768 512-byte sectors (16 MiB) per request.
> +         */
> +        int max_discard = bs->max_discard ? bs->max_discard : 32768;

Worth having a named constant instead of a magic number?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 09/20] iscsi: simplify iscsi_co_discard

[Peter Lieven]

now that bdrv_co_discard can handle limits we do not need
the request split logic here anymore.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c |   67 +++++++++++++++++++++------------------------------------
 1 file changed, 25 insertions(+), 42 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 68f99d3..aabcddb 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -87,7 +87,6 @@ typedef struct IscsiAIOCB {
 #define NOP_INTERVAL 5000
 #define MAX_NOP_FAILURES 3
 #define ISCSI_CMD_RETRIES 5
-#define ISCSI_MAX_UNMAP 131072
 
 static void
 iscsi_bh_cb(void *p)
@@ -908,8 +907,6 @@ coroutine_fn iscsi_co_discard(BlockDriverState *bs, int64_t sector_num,
     IscsiLun *iscsilun = bs->opaque;
     struct IscsiTask iTask;
     struct unmap_list list;
-    uint32_t nb_blocks;
-    uint32_t max_unmap;
 
     if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
         return -EINVAL;
@@ -921,52 +918,38 @@ coroutine_fn iscsi_co_discard(BlockDriverState *bs, int64_t sector_num,
     }
 
     list.lba = sector_qemu2lun(sector_num, iscsilun);
-    nb_blocks = sector_qemu2lun(nb_sectors, iscsilun);
+    list.num = sector_qemu2lun(nb_sectors, iscsilun);
 
-    max_unmap = iscsilun->bl.max_unmap;
-    if (max_unmap == 0xffffffff) {
-        max_unmap = ISCSI_MAX_UNMAP;
-    }
-
-    while (nb_blocks > 0) {
-        iscsi_co_init_iscsitask(iscsilun, &iTask);
-        list.num = nb_blocks;
-        if (list.num > max_unmap) {
-            list.num = max_unmap;
-        }
+    iscsi_co_init_iscsitask(iscsilun, &iTask);
 retry:
-        if (iscsi_unmap_task(iscsilun->iscsi, iscsilun->lun, 0, 0, &list, 1,
-                         iscsi_co_generic_cb, &iTask) == NULL) {
-            return -EIO;
-        }
-
-        while (!iTask.complete) {
-            iscsi_set_events(iscsilun);
-            qemu_coroutine_yield();
-        }
+    if (iscsi_unmap_task(iscsilun->iscsi, iscsilun->lun, 0, 0, &list, 1,
+                     iscsi_co_generic_cb, &iTask) == NULL) {
+        return -EIO;
+    }
 
-        if (iTask.task != NULL) {
-            scsi_free_scsi_task(iTask.task);
-            iTask.task = NULL;
-        }
+    while (!iTask.complete) {
+        iscsi_set_events(iscsilun);
+        qemu_coroutine_yield();
+    }
 
-        if (iTask.do_retry) {
-            goto retry;
-        }
+    if (iTask.task != NULL) {
+        scsi_free_scsi_task(iTask.task);
+        iTask.task = NULL;
+    }
 
-        if (iTask.status == SCSI_STATUS_CHECK_CONDITION) {
-            /* the target might fail with a check condition if it
-               is not happy with the alignment of the UNMAP request
-               we silently fail in this case */
-            return 0;
-        }
+    if (iTask.do_retry) {
+        goto retry;
+    }
 
-        if (iTask.status != SCSI_STATUS_GOOD) {
-            return -EIO;
-        }
+    if (iTask.status == SCSI_STATUS_CHECK_CONDITION) {
+        /* the target might fail with a check condition if it
+           is not happy with the alignment of the UNMAP request
+           we silently fail in this case */
+        return 0;
+    }
 
-        list.lba += list.num;
-        nb_blocks -= list.num;
+    if (iTask.status != SCSI_STATUS_GOOD) {
+        return -EIO;
     }
 
     return 0;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 09/20] iscsi: simplify iscsi_co_discard

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> now that bdrv_co_discard can handle limits we do not need
> the request split logic here anymore.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |   67 +++++++++++++++++++++------------------------------------
>  1 file changed, 25 insertions(+), 42 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 10/20] iscsi: set limits in BlockDriverState

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index aabcddb..21b1ecf 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1362,6 +1362,16 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
                sizeof(struct scsi_inquiry_block_limits));
         scsi_free_scsi_task(task);
         task = NULL;
+        
+        if (iscsilun->bl.max_unmap < 0xffffffff) {
+            bs->max_discard = sector_lun2qemu(iscsilun->bl.max_unmap, iscsilun);
+        }
+        bs->discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun);
+        
+        if (iscsilun->bl.max_ws_len < 0xffffffff) {
+            bs->max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len, iscsilun);
+        }
+        bs->write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun);
     }
 
 #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 10/20] iscsi: set limits in BlockDriverState

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index aabcddb..21b1ecf 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1362,6 +1362,16 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
>                 sizeof(struct scsi_inquiry_block_limits));
>          scsi_free_scsi_task(task);
>          task = NULL;
> +        
> +        if (iscsilun->bl.max_unmap < 0xffffffff) {
> +            bs->max_discard = sector_lun2qemu(iscsilun->bl.max_unmap, iscsilun);
> +        }
> +        bs->discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun);
> +        
> +        if (iscsilun->bl.max_ws_len < 0xffffffff) {
> +            bs->max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len, iscsilun);
> +        }
> +        bs->write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun);
>      }
>  
>  #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 11/20] iscsi: add bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 21b1ecf..46c7c8d 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1440,6 +1440,18 @@ static int iscsi_has_zero_init(BlockDriverState *bs)
     return 0;
 }
 
+static int iscsi_has_discard_zeroes(BlockDriverState *bs)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    return iscsilun->lbprz;
+}
+
+static int iscsi_has_discard_write_zeroes(BlockDriverState *bs)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    return iscsilun->lbprz && iscsilun->lbp.lbpws;
+}
+
 static int iscsi_create(const char *filename, QEMUOptionParameter *options)
 {
     int ret = 0;
@@ -1522,7 +1534,9 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_aio_writev = iscsi_aio_writev,
     .bdrv_aio_flush  = iscsi_aio_flush,
 
-    .bdrv_has_zero_init = iscsi_has_zero_init,
+    .bdrv_has_zero_init            = iscsi_has_zero_init,
+    .bdrv_has_discard_zeroes       = iscsi_has_discard_zeroes ,
+    .bdrv_has_discard_write_zeroes = iscsi_has_discard_write_zeroes,
 
 #ifdef __linux__
     .bdrv_ioctl       = iscsi_ioctl,
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 11/20] iscsi: add bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |   16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 

> @@ -1522,7 +1534,9 @@ static BlockDriver bdrv_iscsi = {
>      .bdrv_aio_writev = iscsi_aio_writev,
>      .bdrv_aio_flush  = iscsi_aio_flush,
>  
> -    .bdrv_has_zero_init = iscsi_has_zero_init,
> +    .bdrv_has_zero_init            = iscsi_has_zero_init,
> +    .bdrv_has_discard_zeroes       = iscsi_has_discard_zeroes ,

Spurious space before ','.  As that's trivial to fix, feel free to add:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 12/20] iscsi: add bdrv_co_write_zeroes

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 46c7c8d..4df33fc 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -56,6 +56,7 @@ typedef struct IscsiLun {
     uint8_t lbprz;
     struct scsi_inquiry_logical_block_provisioning lbp;
     struct scsi_inquiry_block_limits bl;
+    unsigned char *zeroblock;
 } IscsiLun;
 
 typedef struct IscsiTask {
@@ -955,6 +956,62 @@ retry:
     return 0;
 }
 
+
+static int
+coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
+                                   int nb_sectors, BdrvRequestFlags flags)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    struct IscsiTask iTask;
+    uint64_t lba;
+    uint32_t nb_blocks;
+
+    if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
+        return -EINVAL;
+    }
+
+    if (!iscsilun->lbp.lbpws) {
+        /* WRITE SAME is not supported by the target */
+        return -ENOTSUP;
+    }
+
+    lba = sector_qemu2lun(sector_num, iscsilun);
+    nb_blocks = sector_qemu2lun(nb_sectors, iscsilun);
+
+    if (iscsilun->zeroblock == NULL) {
+        iscsilun->zeroblock = g_malloc0(iscsilun->block_size);
+    }
+
+    iscsi_co_init_iscsitask(iscsilun, &iTask);
+retry:
+    if (iscsi_writesame16_task(iscsilun->iscsi, iscsilun->lun, lba,
+                               iscsilun->zeroblock, iscsilun->block_size,
+                               nb_blocks, 0, !!(flags & BDRV_REQ_MAY_UNMAP),
+                               0, 0, iscsi_co_generic_cb, &iTask) == NULL) {
+        return -EIO;
+    }
+
+    while (!iTask.complete) {
+        iscsi_set_events(iscsilun);
+        qemu_coroutine_yield();
+    }
+
+    if (iTask.task != NULL) {
+        scsi_free_scsi_task(iTask.task);
+        iTask.task = NULL;
+    }
+
+    if (iTask.do_retry) {
+        goto retry;
+    }
+
+    if (iTask.status != SCSI_STATUS_GOOD) {
+        return -EIO;
+    }
+
+    return 0;
+}
+
 static int parse_chap(struct iscsi_context *iscsi, const char *target)
 {
     QemuOptsList *list;
@@ -1412,6 +1469,7 @@ static void iscsi_close(BlockDriverState *bs)
     }
     qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL);
     iscsi_destroy_context(iscsi);
+    g_free(iscsilun->zeroblock);
     memset(iscsilun, 0, sizeof(IscsiLun));
 }
 
@@ -1529,6 +1587,7 @@ static BlockDriver bdrv_iscsi = {
 
     .bdrv_co_get_block_status = iscsi_co_get_block_status,
     .bdrv_co_discard      = iscsi_co_discard,
+    .bdrv_co_write_zeroes = iscsi_co_write_zeroes,
 
     .bdrv_aio_readv  = iscsi_aio_readv,
     .bdrv_aio_writev = iscsi_aio_writev,
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 12/20] iscsi: add bdrv_co_write_zeroes

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 13/20] block: introduce bdrv_zeroize

[Peter Lieven]

this patch adds a call to completely zero out a block device.
the operation is sped up by checking the block status and
only writing zeroes to the device if they currently do not
return zeroes. optionally the zero writing can be sped up
by setting the flag BDRV_REQ_MAY_UNMAP to emulate the zero
write by unmapping if the driver supports it.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c               |   30 ++++++++++++++++++++++++++++++
 include/block/block.h |    1 +
 2 files changed, 31 insertions(+)

diff --git a/block.c b/block.c
index ecc5be4..88b137c 100644
--- a/block.c
+++ b/block.c
@@ -2342,6 +2342,36 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
                       BDRV_REQ_ZERO_WRITE | flags);
 }
 
+int bdrv_zeroize(BlockDriverState *bs, BdrvRequestFlags flags)
+{
+    int64_t target_size = bdrv_getlength(bs) / BDRV_SECTOR_SIZE;
+    int64_t ret, nb_sectors, sector_num = 0;
+    int n;
+    /* split the write requests into 1MB chunks if the driver
+     * does not return a maximal size via bdi */
+    for (;;) {
+        nb_sectors = target_size - sector_num;
+        if (nb_sectors <= 0) {
+            return 0;
+        }
+        if (nb_sectors > INT_MAX) {
+            nb_sectors = INT_MAX;
+        }
+        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
+        if (ret & BDRV_BLOCK_ZERO) {
+            sector_num += n;
+            continue;
+        }
+        ret = bdrv_write_zeroes(bs, sector_num, n, flags & BDRV_REQ_MAY_UNMAP);
+        if (ret < 0) {
+            error_report("error writing zeroes at sector %" PRId64 ": %s",
+                         sector_num, strerror(-ret));
+            return -EIO;
+        }
+        sector_num += n;
+    }
+}
+
 int bdrv_pread(BlockDriverState *bs, int64_t offset,
                void *buf, int count1)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 8aa325a..a45166e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -195,6 +195,7 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
                const uint8_t *buf, int nb_sectors);
 int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
                int nb_sectors, BdrvRequestFlags flags);
+int bdrv_zeroize(BlockDriverState *bs, BdrvRequestFlags flags);
 int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
 int bdrv_pread(BlockDriverState *bs, int64_t offset,
                void *buf, int count);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 13/20] block: introduce bdrv_zeroize

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> this patch adds a call to completely zero out a block device.
> the operation is sped up by checking the block status and
> only writing zeroes to the device if they currently do not
> return zeroes. optionally the zero writing can be sped up
> by setting the flag BDRV_REQ_MAY_UNMAP to emulate the zero
> write by unmapping if the driver supports it.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block.c               |   30 ++++++++++++++++++++++++++++++
>  include/block/block.h |    1 +
>  2 files changed, 31 insertions(+)
> 
> diff --git a/block.c b/block.c
> index ecc5be4..88b137c 100644
> --- a/block.c
> +++ b/block.c
> @@ -2342,6 +2342,36 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>                        BDRV_REQ_ZERO_WRITE | flags);
>  }
>  
> +int bdrv_zeroize(BlockDriverState *bs, BdrvRequestFlags flags)

Please add documentation in the code base about what this function does,
and what return values mean.  (Bad practice in the past doesn't excuse
new patches from being more maintainer-friendly)

> +{
> +    int64_t target_size = bdrv_getlength(bs) / BDRV_SECTOR_SIZE;
> +    int64_t ret, nb_sectors, sector_num = 0;
> +    int n;
> +    /* split the write requests into 1MB chunks if the driver
> +     * does not return a maximal size via bdi */
> +    for (;;) {
> +        nb_sectors = target_size - sector_num;
> +        if (nb_sectors <= 0) {
> +            return 0;
> +        }
> +        if (nb_sectors > INT_MAX) {
> +            nb_sectors = INT_MAX;
> +        }
> +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
> +        if (ret & BDRV_BLOCK_ZERO) {
> +            sector_num += n;
> +            continue;
> +        }
> +        ret = bdrv_write_zeroes(bs, sector_num, n, flags & BDRV_REQ_MAY_UNMAP);

Is this intentionally throwing away all other flags except
BDRV_REQ_MAY_UNMAP?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCHv2 13/20] block: introduce bdrv_zeroize

[Peter Lieven]

Am 19.09.2013 um 22:26 schrieb Eric Blake <eblake@redhat.com>:

> On 09/17/2013 07:48 AM, Peter Lieven wrote:
>> this patch adds a call to completely zero out a block device.
>> the operation is sped up by checking the block status and
>> only writing zeroes to the device if they currently do not
>> return zeroes. optionally the zero writing can be sped up
>> by setting the flag BDRV_REQ_MAY_UNMAP to emulate the zero
>> write by unmapping if the driver supports it.
>> 
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> block.c               |   30 ++++++++++++++++++++++++++++++
>> include/block/block.h |    1 +
>> 2 files changed, 31 insertions(+)
>> 
>> diff --git a/block.c b/block.c
>> index ecc5be4..88b137c 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2342,6 +2342,36 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>>                       BDRV_REQ_ZERO_WRITE | flags);
>> }
>> 
>> +int bdrv_zeroize(BlockDriverState *bs, BdrvRequestFlags flags)
> 
> Please add documentation in the code base about what this function does,
> and what return values mean.  (Bad practice in the past doesn't excuse
> new patches from being more maintainer-friendly)

ok ;-)

> 
>> +{
>> +    int64_t target_size = bdrv_getlength(bs) / BDRV_SECTOR_SIZE;
>> +    int64_t ret, nb_sectors, sector_num = 0;
>> +    int n;
>> +    /* split the write requests into 1MB chunks if the driver
>> +     * does not return a maximal size via bdi */
>> +    for (;;) {
>> +        nb_sectors = target_size - sector_num;
>> +        if (nb_sectors <= 0) {
>> +            return 0;
>> +        }
>> +        if (nb_sectors > INT_MAX) {
>> +            nb_sectors = INT_MAX;
>> +        }
>> +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
>> +        if (ret & BDRV_BLOCK_ZERO) {
>> +            sector_num += n;
>> +            continue;
>> +        }
>> +        ret = bdrv_write_zeroes(bs, sector_num, n, flags & BDRV_REQ_MAY_UNMAP);
> 
> Is this intentionally throwing away all other flags except
> BDRV_REQ_MAY_UNMAP?

This is the only option that bdrv_write_zeroes currently expects, but as this might
change some day I don't mind to pass all flags.

Peter

[Qemu-devel] [PATCHv2 14/20] block/get_block_status: set *pnum = 0 on error

[Peter Lieven]

if the call is invoked through bdrv_is_allocated the caller might
expect *pnum = 0 on error. however, a new implementation of
bdrv_get_block_status might only return a negative exit value on
error while keeping *pnum untouched.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/block.c b/block.c
index 88b137c..805ee26 100644
--- a/block.c
+++ b/block.c
@@ -3188,6 +3188,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
 
     ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
     if (ret < 0) {
+        *pnum = 0;
         return ret;
     }
 
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 15/20] block/get_block_status: avoid segfault if there is no backing_hd

[Peter Lieven]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 805ee26..461a1d7 100644
--- a/block.c
+++ b/block.c
@@ -3195,7 +3195,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     if (!(ret & BDRV_BLOCK_DATA)) {
         if (bdrv_has_zero_init(bs)) {
             ret |= BDRV_BLOCK_ZERO;
-        } else {
+        } else if (bs->backing_hd) {
             BlockDriverState *bs2 = bs->backing_hd;
             int64_t length2 = bdrv_getlength(bs2);
             if (length2 >= 0 && sector_num >= (length2 >> BDRV_SECTOR_BITS)) {
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 16/20] block/get_block_status: avoid redundant callouts on raw devices

[Peter Lieven]

if a raw device like an iscsi target or host device is used
the current implementation makes a second call out to get
the block status of bs->file. however, the raw driver already
has called bdrv_get_block_status on bs->file.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 461a1d7..64fca1b 100644
--- a/block.c
+++ b/block.c
@@ -3206,7 +3206,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
 
     if (bs->file &&
         (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
-        (ret & BDRV_BLOCK_OFFSET_VALID)) {
+        (ret & BDRV_BLOCK_OFFSET_VALID) &&
+        strcmp(bs->drv->format_name, "raw")) {
         ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
                                         *pnum, pnum);
         if (ret2 >= 0) {
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 17/20] block/get_block_status: fix BDRV_BLOCK_ZERO for unallocated blocks

[Peter Lieven]

this patch does 2 things:
a) only do additional call outs if BDRV_BLOCK_ZERO is not already set.
b) use the newly introduced bdrv_has_discard_zeroes() to return the
   zero state of an unallocated block. the used callout to
   bdrv_has_zero_init() is only valid right after bdrv_create.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 64fca1b..7d7ee0f 100644
--- a/block.c
+++ b/block.c
@@ -3192,8 +3192,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
         return ret;
     }
 
-    if (!(ret & BDRV_BLOCK_DATA)) {
-        if (bdrv_has_zero_init(bs)) {
+    if (!(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO)) { 
+        if (bdrv_has_discard_zeroes(bs)) {
             ret |= BDRV_BLOCK_ZERO;
         } else if (bs->backing_hd) {
             BlockDriverState *bs2 = bs->backing_hd;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 17/20] block/get_block_status: fix BDRV_BLOCK_ZERO for unallocated blocks

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> this patch does 2 things:
> a) only do additional call outs if BDRV_BLOCK_ZERO is not already set.
> b) use the newly introduced bdrv_has_discard_zeroes() to return the
>    zero state of an unallocated block. the used callout to
>    bdrv_has_zero_init() is only valid right after bdrv_create.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block.c b/block.c
> index 64fca1b..7d7ee0f 100644
> --- a/block.c
> +++ b/block.c
> @@ -3192,8 +3192,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>          return ret;
>      }
>  
> -    if (!(ret & BDRV_BLOCK_DATA)) {
> -        if (bdrv_has_zero_init(bs)) {
> +    if (!(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO)) { 
> +        if (bdrv_has_discard_zeroes(bs)) {
>              ret |= BDRV_BLOCK_ZERO;
>          } else if (bs->backing_hd) {
>              BlockDriverState *bs2 = bs->backing_hd;
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 18/20] qemu-img: add support for fully allocated images

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 3e5e388..7600b58 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -100,8 +100,10 @@ static void help(void)
            "  '-h' with or without a command shows this help and lists the supported formats\n"
            "  '-p' show progress of command (only certain commands)\n"
            "  '-q' use Quiet mode - do not print any output (except errors)\n"
-           "  '-S' indicates the consecutive number of bytes that must contain only zeros\n"
-           "       for qemu-img to create a sparse image during conversion\n"
+           "  '-S' indicates the consecutive number of bytes (defaults to 4096) that must\n"
+           "       contain only zeros for qemu-img to create a sparse image during\n"
+           "       conversion. if the number of bytes is 0 sparse files are disabled and\n"
+           "       images will always be fully allocated\n"
            "  '--output' takes the format in which the output must be done (human or json)\n"
            "  '-n' skips the target volume creation (useful if the volume is created\n"
            "       prior to running qemu-img)\n"
@@ -1468,7 +1470,7 @@ static int img_convert(int argc, char **argv)
         /* signal EOF to align */
         bdrv_write_compressed(out_bs, 0, NULL, 0);
     } else {
-        int has_zero_init = bdrv_has_zero_init(out_bs);
+        int has_zero_init = min_sparse ? bdrv_has_zero_init(out_bs) : 0;
 
         sector_num = 0; // total number of sectors converted so far
         nb_sectors = total_sectors - sector_num;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 18/20] qemu-img: add support for fully allocated images

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 3e5e388..7600b58 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -100,8 +100,10 @@ static void help(void)
>             "  '-h' with or without a command shows this help and lists the supported formats\n"
>             "  '-p' show progress of command (only certain commands)\n"
>             "  '-q' use Quiet mode - do not print any output (except errors)\n"
> -           "  '-S' indicates the consecutive number of bytes that must contain only zeros\n"
> -           "       for qemu-img to create a sparse image during conversion\n"
> +           "  '-S' indicates the consecutive number of bytes (defaults to 4096) that must\n"
> +           "       contain only zeros for qemu-img to create a sparse image during\n"
> +           "       conversion. if the number of bytes is 0 sparse files are disabled and\n"

s/if/If/
s/0/0,/

> +           "       images will always be fully allocated\n"

IIRC correctly, on NTFS file system, the minimum hole size for a sparse
file is 64k.  Can you please make this option accept a suffixed number
(-S 64k) instead of forcing me to spell it out (-S 65536)?

>             "  '--output' takes the format in which the output must be done (human or json)\n"
>             "  '-n' skips the target volume creation (useful if the volume is created\n"
>             "       prior to running qemu-img)\n"
> @@ -1468,7 +1470,7 @@ static int img_convert(int argc, char **argv)
>          /* signal EOF to align */
>          bdrv_write_compressed(out_bs, 0, NULL, 0);
>      } else {
> -        int has_zero_init = bdrv_has_zero_init(out_bs);
> +        int has_zero_init = min_sparse ? bdrv_has_zero_init(out_bs) : 0;

Is this another function that should be returning bool instead of int?
But that's a question for a separate patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCHv2 18/20] qemu-img: add support for fully allocated images

[Peter Lieven]

Am 19.09.2013 um 22:36 schrieb Eric Blake <eblake@redhat.com>:

> On 09/17/2013 07:48 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> qemu-img.c |    8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 3e5e388..7600b58 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -100,8 +100,10 @@ static void help(void)
>>            "  '-h' with or without a command shows this help and lists the supported formats\n"
>>            "  '-p' show progress of command (only certain commands)\n"
>>            "  '-q' use Quiet mode - do not print any output (except errors)\n"
>> -           "  '-S' indicates the consecutive number of bytes that must contain only zeros\n"
>> -           "       for qemu-img to create a sparse image during conversion\n"
>> +           "  '-S' indicates the consecutive number of bytes (defaults to 4096) that must\n"
>> +           "       contain only zeros for qemu-img to create a sparse image during\n"
>> +           "       conversion. if the number of bytes is 0 sparse files are disabled and\n"
> 
> s/if/If/
> s/0/0,/
> 
>> +           "       images will always be fully allocated\n"
> 
> IIRC correctly, on NTFS file system, the minimum hole size for a sparse
> file is 64k.  Can you please make this option accept a suffixed number
> (-S 64k) instead of forcing me to spell it out (-S 65536)?

I can do that, of course. But the option is old it was not introduced by me ;-)
I only added the handling for -S 0 ;-)



> 
>>            "  '--output' takes the format in which the output must be done (human or json)\n"
>>            "  '-n' skips the target volume creation (useful if the volume is created\n"
>>            "       prior to running qemu-img)\n"
>> @@ -1468,7 +1470,7 @@ static int img_convert(int argc, char **argv)
>>         /* signal EOF to align */
>>         bdrv_write_compressed(out_bs, 0, NULL, 0);
>>     } else {
>> -        int has_zero_init = bdrv_has_zero_init(out_bs);
>> +        int has_zero_init = min_sparse ? bdrv_has_zero_init(out_bs) : 0;
> 
> Is this another function that should be returning bool instead of int?
> But that's a question for a separate patch.

If the answer is yes, bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes
should return bool as well.

Peter

[Qemu-devel] [PATCHv2 19/20] qemu-img: conditionally zero out target on convert

[Peter Lieven]

if the target has_zero_init = 0, but supports efficiently
writing zeroes by unmapping we call bdrv_zeroize to
avoid fully allocating the target. this currently
is designed especially for iscsi.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7600b58..d6af941 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1356,7 +1356,7 @@ static int img_convert(int argc, char **argv)
         }
     }
 
-    flags = BDRV_O_RDWR;
+    flags = min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
     ret = bdrv_parse_cache_flags(cache, &flags);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -1472,6 +1472,14 @@ static int img_convert(int argc, char **argv)
     } else {
         int has_zero_init = min_sparse ? bdrv_has_zero_init(out_bs) : 0;
 
+        if (!has_zero_init && bdrv_has_discard_write_zeroes(out_bs)) {
+            ret = bdrv_zeroize(out_bs, BDRV_REQ_MAY_UNMAP);
+            if (ret < 0) {
+                goto out;
+            }
+            has_zero_init = 1;
+        }
+
         sector_num = 0; // total number of sectors converted so far
         nb_sectors = total_sectors - sector_num;
         if (nb_sectors != 0) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 19/20] qemu-img: conditionally zero out target on convert

[Eric Blake]

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

On 09/17/2013 07:48 AM, Peter Lieven wrote:
> if the target has_zero_init = 0, but supports efficiently
> writing zeroes by unmapping we call bdrv_zeroize to
> avoid fully allocating the target. this currently

You don't like the shift key, do you? :)

s/this/This/ if you care about grammar in commit messages (I care about
it in user-visible strings, but am more than willing to let it slide in
commit messages)

> is designed especially for iscsi.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCHv2 20/20] block/raw: copy block limits and alignment information on raw_open

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/raw_bsd.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 0bfa5fc..dfdb375 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -149,6 +149,10 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
 static int raw_open(BlockDriverState *bs, QDict *options, int flags)
 {
     bs->sg = bs->file->sg;
+    bs->max_discard = bs->file->max_discard;
+    bs->discard_alignment = bs->file->discard_alignment;
+    bs->max_write_zeroes = bs->file->max_write_zeroes;
+    bs->write_zeroes_alignment = bs->file->write_zeroes_alignment;
     return 0;
 }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 20/20] block/raw: copy block limits and alignment information on raw_open

[Paolo Bonzini]

Il 17/09/2013 15:48, Peter Lieven ha scritto:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/raw_bsd.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 0bfa5fc..dfdb375 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -149,6 +149,10 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>  static int raw_open(BlockDriverState *bs, QDict *options, int flags)
>  {
>      bs->sg = bs->file->sg;
> +    bs->max_discard = bs->file->max_discard;
> +    bs->discard_alignment = bs->file->discard_alignment;
> +    bs->max_write_zeroes = bs->file->max_write_zeroes;
> +    bs->write_zeroes_alignment = bs->file->write_zeroes_alignment;
>      return 0;

What about introducing a BlockLimits struct?

Paolo

>  }
>  
> 

Re: [Qemu-devel] [PATCHv2 20/20] block/raw: copy block limits and alignment information on raw_open

[Peter Lieven]

On 18.09.2013 10:48, Paolo Bonzini wrote:
> Il 17/09/2013 15:48, Peter Lieven ha scritto:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/raw_bsd.c |    4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
>> index 0bfa5fc..dfdb375 100644
>> --- a/block/raw_bsd.c
>> +++ b/block/raw_bsd.c
>> @@ -149,6 +149,10 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>>   static int raw_open(BlockDriverState *bs, QDict *options, int flags)
>>   {
>>       bs->sg = bs->file->sg;
>> +    bs->max_discard = bs->file->max_discard;
>> +    bs->discard_alignment = bs->file->discard_alignment;
>> +    bs->max_write_zeroes = bs->file->max_write_zeroes;
>> +    bs->write_zeroes_alignment = bs->file->write_zeroes_alignment;
>>       return 0;
> What about introducing a BlockLimits struct?
Good idea.

Do you like the rest of the series?

Peter

Re: [Qemu-devel] [PATCHv2 20/20] block/raw: copy block limits and alignment information on raw_open

[Paolo Bonzini]

Il 19/09/2013 09:29, Peter Lieven ha scritto:
> On 18.09.2013 10:48, Paolo Bonzini wrote:
>> Il 17/09/2013 15:48, Peter Lieven ha scritto:
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>   block/raw_bsd.c |    4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
>>> index 0bfa5fc..dfdb375 100644
>>> --- a/block/raw_bsd.c
>>> +++ b/block/raw_bsd.c
>>> @@ -149,6 +149,10 @@ static int raw_create(const char *filename,
>>> QEMUOptionParameter *options)
>>>   static int raw_open(BlockDriverState *bs, QDict *options, int flags)
>>>   {
>>>       bs->sg = bs->file->sg;
>>> +    bs->max_discard = bs->file->max_discard;
>>> +    bs->discard_alignment = bs->file->discard_alignment;
>>> +    bs->max_write_zeroes = bs->file->max_write_zeroes;
>>> +    bs->write_zeroes_alignment = bs->file->write_zeroes_alignment;
>>>       return 0;
>> What about introducing a BlockLimits struct?
> Good idea.
> 
> Do you like the rest of the series?

Yes, I do.

Paolo