[Qemu-devel] [PATCHv4 00/17] block: logical block provisioning enhancements

[Qemu-devel] [PATCHv4 00/17] 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.

v3->v4:
 - changed BlockLimits struct to typedef (Stefan, Eric)
 - renamed bdrv_zeroize to bdrv_make_zero (Stefan)
 - added comment about the -S flag of qemu-img convert in
   qemu-img.texi (Eric)
 - used struct assignment for bs->bl in raw_open (Stefan, Eric)
 - dropped 3 get_block_status fixes that are independent of
   this series and already partly merged.

v2->v3:
 - fix merge conflict in block/qcow2_cluster.c
 - changed return type of bdrv_has_discard_zeroes and
   bdrv_has_discard_write_zeroes to bool.
 - moved alignment and limits info to a BlockLimits struct (Paolo).
 - added magic constanst for default maximum in bdrv_co_do_write_zeroes
   and bdrv_co_discard (Eric).
 - bdrv_co_do_write_zeroes: allocating the bounce buffer only once (Eric),
   fixed bounce iov_len in the fall back path.
 - bdrv_zeroize: added inline docu (Eric) and do not mask flags passed
   to bdrv_write_zeroes (Eric).
 - qemu-img: changed the default hint for -S (min_sparse) in the usage
   help to 4k. not changing the default as it is unclear why this default
   was set. size suffixes are already supported (Eric).

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 (17):
  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 BlockLimits structure to BlockDriverState
  block: honour BlockLimits in bdrv_co_do_write_zeroes
  block: honour BlockLimits 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_make_zero
  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 BlockLimits on raw_open

 block-migration.c         |    3 +-
 block.c                   |  199 +++++++++++++++++++++++++++++++++++++--------
 block/backup.c            |    3 +-
 block/iscsi.c             |  152 ++++++++++++++++++++++++----------
 block/qcow2-cluster.c     |    2 +-
 block/qcow2.c             |    2 +-
 block/qed.c               |    3 +-
 block/raw_bsd.c           |   62 ++++++++------
 block/vmdk.c              |    3 +-
 include/block/block.h     |   19 ++++-
 include/block/block_int.h |   32 +++++++-
 qemu-img.c                |   18 +++-
 qemu-img.texi             |    5 ++
 qemu-io-cmds.c            |    2 +-
 14 files changed, 394 insertions(+), 111 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCHv4 01/17] 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 93e113a..08cba1e 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 f808550..9fb0c3d 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] [PATCHv4 02/17] 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-cluster.c     |    2 +-
 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 +-
 11 files changed, 27 insertions(+), 21 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 08cba1e..8ed53a2 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);
@@ -2375,10 +2375,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,
@@ -2560,7 +2561,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.
@@ -2694,7 +2695,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;
@@ -2706,7 +2707,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;
         }
@@ -2761,7 +2762,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);
     }
@@ -2795,12 +2796,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-cluster.c b/block/qcow2-cluster.c
index 39323ac..ce3c5d9 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1608,7 +1608,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
             }
 
             ret = bdrv_write_zeroes(bs->file, offset / BDRV_SECTOR_SIZE,
-                                    s->cluster_sectors);
+                                    s->cluster_sectors, 0);
             if (ret < 0) {
                 if (!preallocated) {
                     qcow2_free_clusters(bs, offset, s->cluster_size,
diff --git a/block/qcow2.c b/block/qcow2.c
index 4a9888c..9e28a1a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1600,7 +1600,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 6c0cba0..adc2736 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1397,7 +1397,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 d4ace60..d5ab295 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 5d56e31..75e926e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1391,7 +1391,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 9fb0c3d..47c6475 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -188,7 +188,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);
@@ -210,7 +210,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 211087a..f95dba7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -124,7 +124,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] [PATCHv4 03/17] block: introduce BDRV_REQ_MAY_UNMAP request flag

[Peter Lieven]

Reviewed-by: Eric Blake <eblake@redhat.com>
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 8ed53a2..0675257 100644
--- a/block.c
+++ b/block.c
@@ -2801,6 +2801,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 47c6475..b72c81a 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 the 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

[Qemu-devel] [PATCHv4 04/17] block: introduce bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes

[Peter Lieven]

Reviewed-by: Eric Blake <eblake@redhat.com>
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 0675257..6a46bc2 100644
--- a/block.c
+++ b/block.c
@@ -3085,6 +3085,35 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     return 0;
 }
 
+bool bdrv_has_discard_zeroes(BlockDriverState *bs)
+{
+    assert(bs->drv);
+
+    if (bs->backing_hd) {
+        return false;
+    }
+    if (bs->drv->bdrv_has_discard_zeroes) {
+        return bs->drv->bdrv_has_discard_zeroes(bs);
+    }
+
+    return false;
+}
+
+bool bdrv_has_discard_write_zeroes(BlockDriverState *bs)
+{
+    assert(bs->drv);
+
+    if (bs->backing_hd || !(bs->open_flags & BDRV_O_UNMAP)) {
+        return false;
+    }
+
+    if (bs->drv->bdrv_has_discard_write_zeroes) {
+        return bs->drv->bdrv_has_discard_write_zeroes(bs);
+    }
+
+    return false;
+}
+
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
diff --git a/include/block/block.h b/include/block/block.h
index b72c81a..2de226f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -310,6 +310,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);
+bool bdrv_has_discard_zeroes(BlockDriverState *bs);
+bool 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 f95dba7..ce5c62e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -217,6 +217,19 @@ struct BlockDriver {
      */
     int (*bdrv_has_zero_init)(BlockDriverState *bs);
 
+    /*
+     * Returns true if discarded blocks read back as zeroes.
+     */
+    bool (*bdrv_has_discard_zeroes)(BlockDriverState *bs);
+
+    /*
+     * Returns true 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.
+     */
+    bool (*bdrv_has_discard_write_zeroes)(BlockDriverState *bs);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 05/17] block/raw: add bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes

[Peter Lieven]

Reviewed-by: Eric Blake <eblake@redhat.com>
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 d5ab295..8dc7bba 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 bool raw_has_discard_zeroes(BlockDriverState *bs)
+{
+    return bdrv_has_discard_zeroes(bs->file);
+}
+
+static bool raw_has_discard_write_zeroes(BlockDriverState *bs)
+{
+    return bdrv_has_discard_write_zeroes(bs->file);
+}
+
 static int raw_create(const char *filename, QEMUOptionParameter *options,
                       Error **errp)
 {
@@ -165,28 +175,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

[Qemu-devel] [PATCHv4 06/17] block: add BlockLimits structure to BlockDriverState

[Peter Lieven]

this patch adds BlockLimits which introduces discard and write_zeroes
limits and alignment information to the BlockDriverState.

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

diff --git a/include/block/block_int.h b/include/block/block_int.h
index ce5c62e..775b7ab 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -233,6 +233,20 @@ struct BlockDriver {
     QLIST_ENTRY(BlockDriver) list;
 };
 
+typedef struct BlockLimits {
+    /* 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;
+} BlockLimits;
+
 /*
  * Note: the function bdrv_append() copies and swaps contents of
  * BlockDriverStates, so if you add new fields to this struct, please
@@ -286,6 +300,9 @@ struct BlockDriverState {
     uint64_t total_time_ns[BDRV_MAX_IOTYPE];
     uint64_t wr_highest_sector;
 
+    /* I/O Limits */
+    BlockLimits bl;
+
     /* Whether the disk can expand beyond total_sectors */
     int growable;
 
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 07/17] block: honour BlockLimits in bdrv_co_do_write_zeroes

[Peter Lieven]

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

diff --git a/block.c b/block.c
index 6a46bc2..7551751 100644
--- a/block.c
+++ b/block.c
@@ -2694,32 +2694,65 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
                             BDRV_REQ_COPY_ON_READ);
 }
 
+/* if no limit is specified in the BlockLimits use a default
+ * of 32768 512-byte sectors (16 MiB) per request.
+ */
+#define MAX_WRITE_ZEROES_DEFAULT 32768
+
 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
 {
     BlockDriver *drv = bs->drv;
     QEMUIOVector qiov;
-    struct iovec iov;
-    int ret;
+    struct iovec iov = {0};
+    int ret = 0;
 
-    /* TODO Emulate only part of misaligned requests instead of letting block
-     * drivers return -ENOTSUP and emulate everything */
+    int max_write_zeroes = bs->bl.max_write_zeroes ?
+                           bs->bl.max_write_zeroes : MAX_WRITE_ZEROES_DEFAULT;
 
-    /* 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->bl.write_zeroes_alignment &&
+            num >= bs->bl.write_zeroes_alignment &&
+            sector_num % bs->bl.write_zeroes_alignment) {
+            if (num > bs->bl.write_zeroes_alignment) {
+                num = bs->bl.write_zeroes_alignment;
+            }
+            num -= sector_num % bs->bl.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 = -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 = num * BDRV_SECTOR_SIZE;
+            if (iov.iov_base == NULL) {
+                /* allocate bounce buffer only once and ensure that it
+                 * is big enough for this and all future requests.
+                 */
+                size_t bufsize = num <= nb_sectors ? num : max_write_zeroes;
+                iov.iov_base = qemu_blockalign(bs, bufsize * BDRV_SECTOR_SIZE);
+                memset(iov.iov_base, 0, bufsize * BDRV_SECTOR_SIZE);
+            }
+            qemu_iovec_init_external(&qiov, &iov, 1);
 
-    ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, &qiov);
+            ret = drv->bdrv_co_writev(bs, sector_num, num, &qiov);
+        }
+
+        sector_num += num;
+        nb_sectors -= num;
+    }
 
     qemu_vfree(iov.iov_base);
     return ret;
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 08/17] block: honour BlockLimits in bdrv_co_discard

[Peter Lieven]

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

diff --git a/block.c b/block.c
index 7551751..43d5f46 100644
--- a/block.c
+++ b/block.c
@@ -4209,6 +4209,11 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
     rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
 }
 
+/* if no limit is specified in the BlockLimits use a default
+ * of 32768 512-byte sectors (16 MiB) per request.
+ */
+#define MAX_DISCARD_DEFAULT 32768
+
 int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
                                  int nb_sectors)
 {
@@ -4230,7 +4235,37 @@ 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);
+        int max_discard = bs->bl.max_discard ?
+                          bs->bl.max_discard : MAX_DISCARD_DEFAULT;
+
+        while (nb_sectors > 0) {
+            int ret;
+            int num = nb_sectors;
+
+            /* align request */
+            if (bs->bl.discard_alignment &&
+                num >= bs->bl.discard_alignment &&
+                sector_num % bs->bl.discard_alignment) {
+                if (num > bs->bl.discard_alignment) {
+                    num = bs->bl.discard_alignment;
+                }
+                num -= sector_num % bs->bl.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

[Qemu-devel] [PATCHv4 09/17] 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.

Reviewed-by: Eric Blake <eblake@redhat.com>
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 6152ef1..be70ced 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)
@@ -912,8 +911,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;
@@ -925,52 +922,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

[Qemu-devel] [PATCHv4 10/17] iscsi: set limits in BlockDriverState

[Peter Lieven]

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

diff --git a/block/iscsi.c b/block/iscsi.c
index be70ced..8ed2274 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1367,6 +1367,20 @@ 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->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
+                                                 iscsilun);
+        }
+        bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
+                                                   iscsilun);
+
+        if (iscsilun->bl.max_ws_len < 0xffffffff) {
+            bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len,
+                                                      iscsilun);
+        }
+        bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
+                                                        iscsilun);
     }
 
 #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 11/17] iscsi: add bdrv_has_discard_zeroes and bdrv_has_discard_write_zeroes

[Peter Lieven]

Reviewed-by: Eric Blake <eblake@redhat.com>
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 8ed2274..f0ac620 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1449,6 +1449,18 @@ static int iscsi_has_zero_init(BlockDriverState *bs)
     return 0;
 }
 
+static bool iscsi_has_discard_zeroes(BlockDriverState *bs)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    return !!iscsilun->lbprz;
+}
+
+static bool 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,
                         Error **errp)
 {
@@ -1535,7 +1547,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

[Qemu-devel] [PATCHv4 12/17] iscsi: add bdrv_co_write_zeroes

[Peter Lieven]

Reviewed-by: Eric Blake <eblake@redhat.com>
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 f0ac620..16510c2 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 {
@@ -959,6 +960,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;
@@ -1421,6 +1478,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));
 }
 
@@ -1542,6 +1600,7 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_co_get_block_status = iscsi_co_get_block_status,
 #endif
     .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

[Qemu-devel] [PATCHv4 13/17] block: introduce bdrv_make_zero

[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               |   37 +++++++++++++++++++++++++++++++++++++
 include/block/block.h |    1 +
 2 files changed, 38 insertions(+)

diff --git a/block.c b/block.c
index 43d5f46..fc931e3 100644
--- a/block.c
+++ b/block.c
@@ -2382,6 +2382,43 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
                       BDRV_REQ_ZERO_WRITE | flags);
 }
 
+/*
+ * Completely zero out a block device with the help of bdrv_write_zeroes.
+ * The operation is sped up by checking the block status and only writing
+ * zeroes to the device if they currently do not return zeroes. Optional
+ * flags are passed through to bdrv_write_zeroes (e.g. BDRV_REQ_MAY_UNMAP).
+ *
+ * Returns < 0 on error, 0 on success. For error codes see bdrv_write().
+ */
+int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
+{
+    int64_t target_size = bdrv_getlength(bs) / BDRV_SECTOR_SIZE;
+    int64_t ret, nb_sectors, sector_num = 0;
+    int n;
+
+    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);
+        if (ret < 0) {
+            error_report("error writing zeroes at sector %" PRId64 ": %s",
+                         sector_num, strerror(-ret));
+            return ret;
+        }
+        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 2de226f..f466953 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -196,6 +196,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_make_zero(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

[Qemu-devel] [PATCHv4 14/17] 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.

Reviewed-by: Eric Blake <eblake@redhat.com>
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 fc931e3..1be4418 100644
--- a/block.c
+++ b/block.c
@@ -3247,8 +3247,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

[Qemu-devel] [PATCHv4 15/17] qemu-img: add support for fully allocated images

[Peter Lieven]

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

diff --git a/qemu-img.c b/qemu-img.c
index 926f0a0..c6eff15 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 4k) 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"
@@ -1465,7 +1467,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;
diff --git a/qemu-img.texi b/qemu-img.texi
index 768054e..51a1ee5 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -193,6 +193,11 @@ Image conversion is also useful to get smaller image when using a
 growable format such as @code{qcow} or @code{cow}: the empty sectors
 are detected and suppressed from the destination image.
 
+@var{sparse_size} indicates the consecutive number of bytes (defaults to 4k)
+that must contain only zeros for qemu-img to create a sparse image during
+conversion. If the number of bytes is 0 sparse files are disabled and
+images will always be fully allocated.
+
 You can use the @var{backing_file} option to force the output image to be
 created as a copy on write image of the specified base image; the
 @var{backing_file} should have the same content as the input's base image,
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 16/17] 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.

Reviewed-by: Eric Blake <eblake@redhat.com>
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 c6eff15..2d46de8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1353,7 +1353,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);
@@ -1469,6 +1469,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_make_zero(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

[Qemu-devel] [PATCHv4 17/17] block/raw: copy BlockLimits on raw_open

[Peter Lieven]

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

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 8dc7bba..2c26b79 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -159,6 +159,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
     bs->sg = bs->file->sg;
+    bs->bl = bs->file->bl;
     return 0;
 }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv4 16/17] qemu-img: conditionally zero out target on convert

[Stefan Hajnoczi]

On Tue, Oct 08, 2013 at 01:58:10PM +0200, Peter Lieven wrote:
> If the target has_zero_init = 0, but supports efficiently
> writing zeroes by unmapping we call bdrv_zeroize to

s/bdrv_zeroize/bdrv_make_zero/

No need to respin.

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

[Stefan Hajnoczi]

On Tue, Oct 08, 2013 at 01:58:08PM +0200, 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.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 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 fc931e3..1be4418 100644
> --- a/block.c
> +++ b/block.c
> @@ -3247,8 +3247,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)) {

I'm a little unclear about the semantics of bdrv_has_discard_zeroes().
Originally I thought it just meant any blocks discarded will read back
as zeroes.  But here it implies that any unallocated block reads
back as zeroes too?

In other words, this patch assumes unallocated blocks behave the same as
discarded blocks wrt to zeroes.

Stefan

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

[Paolo Bonzini]

Il 18/10/2013 14:38, Stefan Hajnoczi ha scritto:
> On Tue, Oct 08, 2013 at 01:58:08PM +0200, 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.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 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 fc931e3..1be4418 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3247,8 +3247,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)) {
> 
> I'm a little unclear about the semantics of bdrv_has_discard_zeroes().
> Originally I thought it just meant any blocks discarded will read back
> as zeroes.  But here it implies that any unallocated block reads
> back as zeroes too?
> 
> In other words, this patch assumes unallocated blocks behave the same as
> discarded blocks wrt to zeroes.

Note that earlier patches introduce both bdrv_has_discard_zeroes and
bdrv_has_discard_write_zeroes.  There is no documentation, but the iscsi
implementation let us understand the meaning:


+static bool iscsi_has_discard_zeroes(BlockDriverState *bs)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    return !!iscsilun->lbprz;
+}

That is, unallocated block reads back as zeroes

+static bool iscsi_has_discard_write_zeroes(BlockDriverState *bs)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    return iscsilun->lbprz && iscsilun->lbp.lbpws;
+}

That is, discarded blocks read back zeroes.  This is because:

- UNMAP is not guaranteeing that blocks are discarded, and thus not
really guaranteeing anything on its contents.

- but WRITE SAME is guaranteeing that blocks you "write same" read with
the payload of the command.  This means that in practice for !LBPRZ the
WRITE SAME command will not discard (unless the firmware has bugs).

- so for !LBPRZ you must use UNMAP, but for LBPRZ you can use WRITE SAME
and guarantee that the block reads as zero


Perhaps better names could be

- bdrv_discard_zeroes for bdrv_has_discard_write_zeroes

- bdrv_unallocated_blocks_are_zero for bdrv_has_discard_zeroes

But I'm not sure why we have different BlockDriver APIs.  I'd rather put
the new flags in BlockDriverInfo, and make the new functions simple
wrappers around bdrv_get_info.  I think I proposed that before, maybe I
wasn't clear or I was misunderstood.

Paolo

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

[Peter Lieven]

On 18.10.2013 14:38, Stefan Hajnoczi wrote:
> On Tue, Oct 08, 2013 at 01:58:08PM +0200, 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.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 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 fc931e3..1be4418 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3247,8 +3247,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)) {
> I'm a little unclear about the semantics of bdrv_has_discard_zeroes().
> Originally I thought it just meant any blocks discarded will read back
> as zeroes.  But here it implies that any unallocated block reads
> back as zeroes too?
>
> In other words, this patch assumes unallocated blocks behave the same as
> discarded blocks wrt to zeroes.
>
> Stefan
bdrv_has_discard_zeroes() indicates if unallocated blocks read back as zeroes.
as a discard may silently fail this is no guarantee that a discard will result
in a properly zeroed area.

bdrv_has_discard_write_zeroes() indicates that the driver is able to
honour the BDRV_REQ_MAY_UNMAP flag on bdrv_write_zeroes().

see documentation in block_int.h

     /*
      * Returns true if discarded blocks read back as zeroes.
      */
     bool (*bdrv_has_discard_zeroes)(BlockDriverState *bs);

     /*
      * Returns true 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.
      */
     bool (*bdrv_has_discard_write_zeroes)(BlockDriverState *bs);


Peter

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

[Stefan Hajnoczi]

On Fri, Oct 18, 2013 at 02:49:11PM +0200, Paolo Bonzini wrote:
> Il 18/10/2013 14:38, Stefan Hajnoczi ha scritto:
> > On Tue, Oct 08, 2013 at 01:58:08PM +0200, 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.
> >>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> 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 fc931e3..1be4418 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3247,8 +3247,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)) {
> > 
> > I'm a little unclear about the semantics of bdrv_has_discard_zeroes().
> > Originally I thought it just meant any blocks discarded will read back
> > as zeroes.  But here it implies that any unallocated block reads
> > back as zeroes too?
> > 
> > In other words, this patch assumes unallocated blocks behave the same as
> > discarded blocks wrt to zeroes.
> 
> Note that earlier patches introduce both bdrv_has_discard_zeroes and
> bdrv_has_discard_write_zeroes.  There is no documentation, but the iscsi
> implementation let us understand the meaning:

There are doc comments but they differ from what you've posted:

+    /*
+     * Returns true if discarded blocks read back as zeroes.
+     */
+    bool (*bdrv_has_discard_zeroes)(BlockDriverState *bs);

> +static bool iscsi_has_discard_zeroes(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    return !!iscsilun->lbprz;
> +}
> 
> That is, unallocated block reads back as zeroes

Okay, your semantics make sense.  With them the later patches are correct.

Peter: Please update the doc comments although and consider Paolo's comments
about block driver info.

Stefan

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

[Peter Lieven]

On 18.10.2013 14:49, Paolo Bonzini wrote:
> Il 18/10/2013 14:38, Stefan Hajnoczi ha scritto:
>> On Tue, Oct 08, 2013 at 01:58:08PM +0200, 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.
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> 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 fc931e3..1be4418 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -3247,8 +3247,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)) {
>> I'm a little unclear about the semantics of bdrv_has_discard_zeroes().
>> Originally I thought it just meant any blocks discarded will read back
>> as zeroes.  But here it implies that any unallocated block reads
>> back as zeroes too?
>>
>> In other words, this patch assumes unallocated blocks behave the same as
>> discarded blocks wrt to zeroes.
> Note that earlier patches introduce both bdrv_has_discard_zeroes and
> bdrv_has_discard_write_zeroes.  There is no documentation, but the iscsi
> implementation let us understand the meaning:
>
>
> +static bool iscsi_has_discard_zeroes(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    return !!iscsilun->lbprz;
> +}
>
> That is, unallocated block reads back as zeroes
>
> +static bool iscsi_has_discard_write_zeroes(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    return iscsilun->lbprz && iscsilun->lbp.lbpws;
> +}
>
> That is, discarded blocks read back zeroes.  This is because:
>
> - UNMAP is not guaranteeing that blocks are discarded, and thus not
> really guaranteeing anything on its contents.
>
> - but WRITE SAME is guaranteeing that blocks you "write same" read with
> the payload of the command.  This means that in practice for !LBPRZ the
> WRITE SAME command will not discard (unless the firmware has bugs).
>
> - so for !LBPRZ you must use UNMAP, but for LBPRZ you can use WRITE SAME
> and guarantee that the block reads as zero
>
>
> Perhaps better names could be
>
> - bdrv_discard_zeroes for bdrv_has_discard_write_zeroes
This would conform to the linux ioctl BLKDISCARDZEROES.
However, we need the write_zeroes operation for a guarantee
that zeroes are return.

>
> - bdrv_unallocated_blocks_are_zero for bdrv_has_discard_zeroes
>
> But I'm not sure why we have different BlockDriver APIs.  I'd rather put
> the new flags in BlockDriverInfo, and make the new functions simple
> wrappers around bdrv_get_info.  I think I proposed that before, maybe I
> wasn't clear or I was misunderstood.
I think Kevin wanted to have special functions for this.

Peter

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

[Paolo Bonzini]

Il 18/10/2013 15:26, Peter Lieven ha scritto:
>>
>>
>> - bdrv_discard_zeroes for bdrv_has_discard_write_zeroes
> This would conform to the linux ioctl BLKDISCARDZEROES.
> However, we need the write_zeroes operation for a guarantee
> that zeroes are return.

Yes.  I'm fine with the current names actually, just thinking loudly.

>> - bdrv_unallocated_blocks_are_zero for bdrv_has_discard_zeroes
>>
>> But I'm not sure why we have different BlockDriver APIs.  I'd rather put
>> the new flags in BlockDriverInfo, and make the new functions simple
>> wrappers around bdrv_get_info.  I think I proposed that before, maybe I
>> wasn't clear or I was misunderstood.
> I think Kevin wanted to have special functions for this.

Yes, but I think he referred to block.c functions not BlockDriver functions.

Paolo

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

[Peter Lieven]

On 18.10.2013 15:24, Stefan Hajnoczi wrote:
> On Fri, Oct 18, 2013 at 02:49:11PM +0200, Paolo Bonzini wrote:
>> Il 18/10/2013 14:38, Stefan Hajnoczi ha scritto:
>>> On Tue, Oct 08, 2013 at 01:58:08PM +0200, 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.
>>>>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> 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 fc931e3..1be4418 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -3247,8 +3247,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)) {
>>> I'm a little unclear about the semantics of bdrv_has_discard_zeroes().
>>> Originally I thought it just meant any blocks discarded will read back
>>> as zeroes.  But here it implies that any unallocated block reads
>>> back as zeroes too?
>>>
>>> In other words, this patch assumes unallocated blocks behave the same as
>>> discarded blocks wrt to zeroes.
>> Note that earlier patches introduce both bdrv_has_discard_zeroes and
>> bdrv_has_discard_write_zeroes.  There is no documentation, but the iscsi
>> implementation let us understand the meaning:
> There are doc comments but they differ from what you've posted:
>
> +    /*
> +     * Returns true if discarded blocks read back as zeroes.
> +     */
> +    bool (*bdrv_has_discard_zeroes)(BlockDriverState *bs);
>
>> +static bool iscsi_has_discard_zeroes(BlockDriverState *bs)
>> +{
>> +    IscsiLun *iscsilun = bs->opaque;
>> +    return !!iscsilun->lbprz;
>> +}
>>
>> That is, unallocated block reads back as zeroes
> Okay, your semantics make sense.  With them the later patches are correct.
>
> Peter: Please update the doc comments although and consider Paolo's comments
> about block driver info.
Ok, we have 2 features, but we need better names for them.

a) unallocated blocks read back as zeroes. I would suggest to rename
bdrv_has_discard_zeroes() to bdrv_unallocated_blocks_return_zero() and
update the comment to:

  /*
   * Returns true if unallocated blocks read back as zeroes.
   */

b) the driver has an efficient way of speeding up bdrv_write_zeroes by unmapping
blocks. for iscsi this is done through WRITESAME16 with the UNMAP flag. for other
drivers this could be a similar approach as long as it is guaranteed that its not
writing actual zeroes to disk and that zeroes are returned in any case.
I would suggest to rename bdrv_has_discard_write_zeroes to bdrv_can_write_zeroes_by_unmap().

     /*
      * Returns true if the driver can optimize writing zeroes by unmapping
      * without actually writing real zeroes to disk. However, it must be guaranteed
      * that all blocks read back as zeroes afterwards. It is additionally required that
      * the block device is opened with BDRV_O_UNMAP and the that the
      * bdrv_write_zeroes request carries the BDRV_REQ_MAY_UNMAP flag for
      * this to work.
      */

Regarding putting this info into the BDI I am fine with that, but I would keep the wrapper functions.
On the other hand, bdrv_has_zero_init is also not in the BDI... I had it in the BDI and got the request
to move it to separate functions. To finish this series I need a definitive decision where to put it.

Peter

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

[Paolo Bonzini]

Il 18/10/2013 15:52, Peter Lieven ha scritto:
> 
> Regarding putting this info into the BDI I am fine with that, but I
> would keep the wrapper functions.
> On the other hand, bdrv_has_zero_init is also not in the BDI... I had it
> in the BDI and got the request
> to move it to separate functions. To finish this series I need a
> definitive decision where to put it.

Moving bdrv_has_zero_init to BDI is also a good idea, but it can be done
later.

Paolo

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

[Peter Lieven]

> Am 18.10.2013 um 15:50 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
> Il 18/10/2013 15:26, Peter Lieven ha scritto:
>>> 
>>> 
>>> - bdrv_discard_zeroes for bdrv_has_discard_write_zeroes
>> This would conform to the linux ioctl BLKDISCARDZEROES.
>> However, we need the write_zeroes operation for a guarantee
>> that zeroes are return.
> 
> Yes.  I'm fine with the current names actually, just thinking loudly.
> 
>>> - bdrv_unallocated_blocks_are_zero for bdrv_has_discard_zeroes
>>> 
>>> But I'm not sure why we have different BlockDriver APIs.  I'd rather put
>>> the new flags in BlockDriverInfo, and make the new functions simple
>>> wrappers around bdrv_get_info.  I think I proposed that before, maybe I
>>> wasn't clear or I was misunderstood.
>> I think Kevin wanted to have special functions for this.
> 
> Yes, but I think he referred to block.c functions not BlockDriver functions.

Ok, if Stefan and Kevin agree i will change it once more. I Would also like some Feedback on the new names for the functions and changed description. I can send a respin next week then.

Peter

> 
> Paolo

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

[Stefan Hajnoczi]

On Fri, Oct 18, 2013 at 09:10:43PM +0200, Peter Lieven wrote:
> 
> 
> > Am 18.10.2013 um 15:50 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> > 
> > Il 18/10/2013 15:26, Peter Lieven ha scritto:
> >>> 
> >>> 
> >>> - bdrv_discard_zeroes for bdrv_has_discard_write_zeroes
> >> This would conform to the linux ioctl BLKDISCARDZEROES.
> >> However, we need the write_zeroes operation for a guarantee
> >> that zeroes are return.
> > 
> > Yes.  I'm fine with the current names actually, just thinking loudly.
> > 
> >>> - bdrv_unallocated_blocks_are_zero for bdrv_has_discard_zeroes
> >>> 
> >>> But I'm not sure why we have different BlockDriver APIs.  I'd rather put
> >>> the new flags in BlockDriverInfo, and make the new functions simple
> >>> wrappers around bdrv_get_info.  I think I proposed that before, maybe I
> >>> wasn't clear or I was misunderstood.
> >> I think Kevin wanted to have special functions for this.
> > 
> > Yes, but I think he referred to block.c functions not BlockDriver functions.
> 
> Ok, if Stefan and Kevin agree i will change it once more. I Would also like some Feedback on the new names for the functions and changed description. I can send a respin next week then.

(Catching up with old mails)

Fine here.

Stefan

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

[Peter Lieven]

Hi Stefan, please have a Look at v7 of this series. Hopefully the final one.

Thx,

> Am 30.10.2013 um 09:28 schrieb Stefan Hajnoczi <stefanha@redhat.com>:
> 
>> On Fri, Oct 18, 2013 at 09:10:43PM +0200, Peter Lieven wrote:
>> 
>> 
>>> Am 18.10.2013 um 15:50 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>>> 
>>> Il 18/10/2013 15:26, Peter Lieven ha scritto:
>>>>> 
>>>>> 
>>>>> - bdrv_discard_zeroes for bdrv_has_discard_write_zeroes
>>>> This would conform to the linux ioctl BLKDISCARDZEROES.
>>>> However, we need the write_zeroes operation for a guarantee
>>>> that zeroes are return.
>>> 
>>> Yes.  I'm fine with the current names actually, just thinking loudly.
>>> 
>>>>> - bdrv_unallocated_blocks_are_zero for bdrv_has_discard_zeroes
>>>>> 
>>>>> But I'm not sure why we have different BlockDriver APIs.  I'd rather put
>>>>> the new flags in BlockDriverInfo, and make the new functions simple
>>>>> wrappers around bdrv_get_info.  I think I proposed that before, maybe I
>>>>> wasn't clear or I was misunderstood.
>>>> I think Kevin wanted to have special functions for this.
>>> 
>>> Yes, but I think he referred to block.c functions not BlockDriver functions.
>> 
>> Ok, if Stefan and Kevin agree i will change it once more. I Would also like some Feedback on the new names for the functions and changed description. I can send a respin next week then.
> 
> (Catching up with old mails)
> 
> Fine here.
> 
> Stefan