[Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Kevin Wolf]

This series converts all I/O function in the core block layer up to
bdrv_co_preadv/pwritev() to taking a BdrvChild as their first parameter
instead of a BlockDriverState.

The original motivation for this change were op blockers, where one of
the biggest problems is making sure that every user of block devices
actually registers correctly with the op blockers system. If the I/O
functions know which parent a request comes from (BdrvChild basically
corresponds to an edge in our block device graph), it can use assertions
to make sure that that parent has actually registered its activities and
thereby ensured that it doesn't conflict with other users.

There are, however, more benefits we get from this change. The most
important one is probably that it enforces important aspects of the
block layer design like that external users go through a BlockBackend
and request are internally routed along the edges of the graph. Accesses
to random BDSes are no longer possible, you need to own an actual child
reference so you can make a request.

The work on this series already led to a few cleanups and BlockBackend
conversions in master, and this series contains a few more.

As a bonus, all the block drivers using bs->file->bs everywhere can now
go back to bs->file, which is a little nicer to read.

Kevin Wolf (17):
  vvfat: Use BdrvChild for s->qcow
  blkreplay: Convert to byte-based I/O
  vhdx: Some more BlockBackend use in vhdx_create()
  block: Convert bdrv_co_readv() to BdrvChild
  block: Convert bdrv_co_writev() to BdrvChild
  block: Convert bdrv_aio_readv() to BdrvChild
  block: Convert bdrv_aio_writev() to BdrvChild
  block: Convert bdrv_co_do_readv/writev to BdrvChild
  block: Move bdrv_commit() to block/commit.c
  block: Use BlockBackend for I/O in bdrv_commit()
  block: Convert bdrv_read() to BdrvChild
  block: Convert bdrv_write() to BdrvChild
  block: Convert bdrv_pread(v) to BdrvChild
  block: Convert bdrv_pwrite(v/_sync) to BdrvChild
  block: Convert bdrv_pwrite_zeroes() to BdrvChild
  block: Convert bdrv_prwv_co() to BdrvChild
  block: Convert bdrv_co_preadv/pwritev to BdrvChild

 block.c                        | 117 ++----------------------------------
 block/Makefile.objs            |   3 +-
 block/blkdebug.c               |   4 +-
 block/blkreplay.c              |  18 +++---
 block/blkverify.c              |   8 +--
 block/block-backend.c          |   9 ++-
 block/bochs.c                  |   8 +--
 block/cloop.c                  |   8 +--
 block/commit.c                 | 121 +++++++++++++++++++++++++++++++++++++
 block/crypto.c                 |   6 +-
 block/dmg.c                    |  21 +++----
 block/io.c                     | 132 ++++++++++++++++++++++-------------------
 block/parallels.c              |  16 ++---
 block/qcow.c                   |  41 +++++++------
 block/qcow2-cache.c            |   4 +-
 block/qcow2-cluster.c          |  18 +++---
 block/qcow2-refcount.c         |  36 +++++------
 block/qcow2-snapshot.c         |  26 ++++----
 block/qcow2.c                  |  50 +++++++++-------
 block/qed-table.c              |   4 +-
 block/qed.c                    |  22 +++----
 block/quorum.c                 |   8 +--
 block/raw_bsd.c                |   6 +-
 block/vdi.c                    |  14 ++---
 block/vhdx-log.c               |  12 ++--
 block/vhdx.c                   |  85 ++++++++++++++------------
 block/vmdk.c                   |  54 ++++++++---------
 block/vpc.c                    |  24 ++++----
 block/vvfat.c                  |  61 ++++++++++++-------
 include/block/block.h          |  38 ++++++------
 include/block/block_int.h      |   4 +-
 include/sysemu/block-backend.h |   1 +
 qemu-img.c                     |   2 +-
 33 files changed, 522 insertions(+), 459 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/17] vvfat: Use BdrvChild for s->qcow

[Kevin Wolf]

vvfat uses a temporary qcow file to cache written data in read-write
mode. In order to do things properly, this should show up in the BDS
graph and I/O should go through BdrvChild like for every other node.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 66 ++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 6d2e21c..2eb2536 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -344,9 +344,8 @@ typedef struct BDRVVVFATState {
     unsigned int current_cluster;
 
     /* write support */
-    BlockDriverState* write_target;
     char* qcow_filename;
-    BlockDriverState* qcow;
+    BdrvChild* qcow;
     void* fat2;
     char* used_clusters;
     array_t commits;
@@ -984,7 +983,7 @@ static int init_directories(BDRVVVFATState* s,
 static BDRVVVFATState *vvv = NULL;
 #endif
 
-static int enable_write_target(BDRVVVFATState *s, Error **errp);
+static int enable_write_target(BlockDriverState *bs, Error **errp);
 static int is_consistent(BDRVVVFATState *s);
 
 static QemuOptsList runtime_opts = {
@@ -1162,7 +1161,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* read only is the default for safety */
     bs->read_only = 1;
-    s->qcow = s->write_target = NULL;
+    s->qcow = NULL;
     s->qcow_filename = NULL;
     s->fat2 = NULL;
     s->downcase_short_names = 1;
@@ -1173,7 +1172,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
 
     if (qemu_opt_get_bool(opts, "rw", false)) {
-        ret = enable_write_target(s, errp);
+        ret = enable_write_target(bs, errp);
         if (ret < 0) {
             goto fail;
         }
@@ -1390,9 +1389,10 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
 	   return -1;
 	if (s->qcow) {
 	    int n;
-            if (bdrv_is_allocated(s->qcow, sector_num, nb_sectors-i, &n)) {
-DLOG(fprintf(stderr, "sectors %d+%d allocated\n", (int)sector_num, n));
-                if (bdrv_read(s->qcow, sector_num, buf + i*0x200, n)) {
+            if (bdrv_is_allocated(s->qcow->bs, sector_num, nb_sectors-i, &n)) {
+                DLOG(fprintf(stderr, "sectors %d+%d allocated\n",
+                             (int)sector_num, n));
+                if (bdrv_read(s->qcow->bs, sector_num, buf + i*0x200, n)) {
                     return -1;
                 }
                 i += n - 1;
@@ -1668,12 +1668,15 @@ static inline int cluster_was_modified(BDRVVVFATState* s, uint32_t cluster_num)
     int was_modified = 0;
     int i, dummy;
 
-    if (s->qcow == NULL)
-	return 0;
+    if (s->qcow == NULL) {
+        return 0;
+    }
 
-    for (i = 0; !was_modified && i < s->sectors_per_cluster; i++)
-	was_modified = bdrv_is_allocated(s->qcow,
-		cluster2sector(s, cluster_num) + i, 1, &dummy);
+    for (i = 0; !was_modified && i < s->sectors_per_cluster; i++) {
+        was_modified = bdrv_is_allocated(s->qcow->bs,
+	                                     cluster2sector(s, cluster_num) + i,
+                                         1, &dummy);
+    }
 
     return was_modified;
 }
@@ -1822,11 +1825,17 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
 
 		vvfat_close_current_file(s);
                 for (i = 0; i < s->sectors_per_cluster; i++) {
-                    if (!bdrv_is_allocated(s->qcow, offset + i, 1, &dummy)) {
-                        if (vvfat_read(s->bs, offset, s->cluster_buffer, 1)) {
+                    int res;
+
+                    res = bdrv_is_allocated(s->qcow->bs, offset + i, 1, &dummy);
+                    if (!res) {
+                        res = vvfat_read(s->bs, offset, s->cluster_buffer, 1);
+                        if (res) {
                             return -1;
                         }
-                        if (bdrv_write(s->qcow, offset, s->cluster_buffer, 1)) {
+                        res = bdrv_write(s->qcow->bs, offset,
+                                         s->cluster_buffer, 1);
+                        if (res) {
                             return -2;
                         }
                     }
@@ -2782,8 +2791,8 @@ static int do_commit(BDRVVVFATState* s)
 	return ret;
     }
 
-    if (s->qcow->drv->bdrv_make_empty) {
-        s->qcow->drv->bdrv_make_empty(s->qcow);
+    if (s->qcow->bs->drv->bdrv_make_empty) {
+        s->qcow->bs->drv->bdrv_make_empty(s->qcow->bs);
     }
 
     memset(s->used_clusters, 0, sector2cluster(s, s->sector_count));
@@ -2879,7 +2888,7 @@ DLOG(checkpoint());
      * Use qcow backend. Commit later.
      */
 DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
-    ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
+    ret = bdrv_write(s->qcow->bs, sector_num, buf, nb_sectors);
     if (ret < 0) {
 	fprintf(stderr, "Error writing to qcow backend\n");
 	return ret;
@@ -2949,7 +2958,7 @@ write_target_commit(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
 
 static void write_target_close(BlockDriverState *bs) {
     BDRVVVFATState* s = *((BDRVVVFATState**) bs->opaque);
-    bdrv_unref(s->qcow);
+    bdrv_unref_child(NULL, s->qcow);
     g_free(s->qcow_filename);
 }
 
@@ -2959,8 +2968,19 @@ static BlockDriver vvfat_write_target = {
     .bdrv_close         = write_target_close,
 };
 
-static int enable_write_target(BDRVVVFATState *s, Error **errp)
+static void vvfat_qcow_options(int *child_flags, QDict *child_options,
+                               int parent_flags, QDict *parent_options)
 {
+    *child_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH;
+}
+
+const BdrvChildRole child_vvfat_qcow = {
+    .inherit_options    = vvfat_qcow_options,
+};
+
+static int enable_write_target(BlockDriverState *bs, Error **errp)
+{
+    BDRVVVFATState *s = bs->opaque;
     BlockDriver *bdrv_qcow = NULL;
     BlockDriverState *backing;
     QemuOpts *opts = NULL;
@@ -2999,8 +3019,8 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
 
     options = qdict_new();
     qdict_put(options, "driver", qstring_from_str("qcow"));
-    s->qcow = bdrv_open(s->qcow_filename, NULL, options,
-                        BDRV_O_RDWR | BDRV_O_NO_FLUSH, errp);
+    s->qcow = bdrv_open_child(s->qcow_filename, options, "qcow", bs,
+                              &child_vvfat_qcow, false, errp);
     if (!s->qcow) {
         ret = -EINVAL;
         goto err;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 01/17] vvfat: Use BdrvChild for s->qcow

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> vvfat uses a temporary qcow file to cache written data in read-write
> mode. In order to do things properly, this should show up in the BDS
> graph and I/O should go through BdrvChild like for every other node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vvfat.c | 66 ++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 23 deletions(-)

checkpatch.pl has some complaints:

ERROR: "foo* bar" should be "foo *bar"
#43: FILE: block/vvfat.c:348:
+    BdrvChild* qcow;

ERROR: spaces required around that '-' (ctx:VxV)
#81: FILE: block/vvfat.c:1392:
+            if (bdrv_is_allocated(s->qcow->bs, sector_num,
nb_sectors-i, &n)) {
                                                                      ^

ERROR: spaces required around that '*' (ctx:VxV)
#84: FILE: block/vvfat.c:1395:
+                if (bdrv_read(s->qcow->bs, sector_num, buf + i*0x200, n)) {
                                                               ^

ERROR: code indent should never use tabs
#103: FILE: block/vvfat.c:1677:
+^I                                     cluster2sector(s, cluster_num) + i,$

The fourth is actually something you introduced (more or less, at least
all of the surrounding code uses spaces), so that must be fixed.

The rest is preexisting, but I think some of those are worth fixing
still; namely all but the first. You know how I think about the first
and I know how you think about it, so I won't complain if it's
surrounded by code in the same style.

> diff --git a/block/vvfat.c b/block/vvfat.c
> index 6d2e21c..2eb2536 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c

[...]

> @@ -2949,7 +2958,7 @@ write_target_commit(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
>  
>  static void write_target_close(BlockDriverState *bs) {
>      BDRVVVFATState* s = *((BDRVVVFATState**) bs->opaque);
> -    bdrv_unref(s->qcow);
> +    bdrv_unref_child(NULL, s->qcow);

Not that it matters, but why not s->bs instead of NULL?

>      g_free(s->qcow_filename);
>  }
>  
> @@ -2959,8 +2968,19 @@ static BlockDriver vvfat_write_target = {
>      .bdrv_close         = write_target_close,
>  };
>  
> -static int enable_write_target(BDRVVVFATState *s, Error **errp)
> +static void vvfat_qcow_options(int *child_flags, QDict *child_options,
> +                               int parent_flags, QDict *parent_options)
>  {
> +    *child_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH;
> +}
> +
> +const BdrvChildRole child_vvfat_qcow = {

Any reason for not making this static?

> +    .inherit_options    = vvfat_qcow_options,
> +};
> +
> +static int enable_write_target(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVVVFATState *s = bs->opaque;
>      BlockDriver *bdrv_qcow = NULL;
>      BlockDriverState *backing;
>      QemuOpts *opts = NULL;
> @@ -2999,8 +3019,8 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
>  
>      options = qdict_new();
>      qdict_put(options, "driver", qstring_from_str("qcow"));
> -    s->qcow = bdrv_open(s->qcow_filename, NULL, options,
> -                        BDRV_O_RDWR | BDRV_O_NO_FLUSH, errp);
> +    s->qcow = bdrv_open_child(s->qcow_filename, options, "qcow", bs,

I'd have called it "write_target", but, well.

Max

> +                              &child_vvfat_qcow, false, errp);
>      if (!s->qcow) {
>          ret = -EINVAL;
>          goto err;
> 



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

[Qemu-devel] [PATCH 02/17] blkreplay: Convert to byte-based I/O

[Kevin Wolf]

The blkreplay driver only forwards the requests it gets, so converting
it to byte granularity is trivial.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkreplay.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/blkreplay.c b/block/blkreplay.c
index 525c2d5..196b8d0 100755
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -81,22 +81,22 @@ static void block_request_create(uint64_t reqid, BlockDriverState *bs,
     replay_block_event(req->bh, reqid);
 }
 
-static int coroutine_fn blkreplay_co_readv(BlockDriverState *bs,
-    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
+static int coroutine_fn blkreplay_co_preadv(BlockDriverState *bs,
+    uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
     uint64_t reqid = request_id++;
-    int ret = bdrv_co_readv(bs->file->bs, sector_num, nb_sectors, qiov);
+    int ret = bdrv_co_preadv(bs->file->bs, offset, bytes, qiov, flags);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
 
     return ret;
 }
 
-static int coroutine_fn blkreplay_co_writev(BlockDriverState *bs,
-    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
+static int coroutine_fn blkreplay_co_pwritev(BlockDriverState *bs,
+    uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
     uint64_t reqid = request_id++;
-    int ret = bdrv_co_writev(bs->file->bs, sector_num, nb_sectors, qiov);
+    int ret = bdrv_co_pwritev(bs->file->bs, offset, bytes, qiov, flags);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
 
@@ -144,8 +144,8 @@ static BlockDriver bdrv_blkreplay = {
     .bdrv_close             = blkreplay_close,
     .bdrv_getlength         = blkreplay_getlength,
 
-    .bdrv_co_readv          = blkreplay_co_readv,
-    .bdrv_co_writev         = blkreplay_co_writev,
+    .bdrv_co_preadv         = blkreplay_co_preadv,
+    .bdrv_co_pwritev        = blkreplay_co_pwritev,
 
     .bdrv_co_pwrite_zeroes  = blkreplay_co_pwrite_zeroes,
     .bdrv_co_discard        = blkreplay_co_discard,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 02/17] blkreplay: Convert to byte-based I/O

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> The blkreplay driver only forwards the requests it gets, so converting
> it to byte granularity is trivial.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkreplay.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH 02/17] blkreplay: Convert to byte-based I/O

[Eric Blake]

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

On 06/21/2016 03:21 AM, Kevin Wolf wrote:
> The blkreplay driver only forwards the requests it gets, so converting
> it to byte granularity is trivial.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkreplay.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 

>  
> -static int coroutine_fn blkreplay_co_writev(BlockDriverState *bs,
> -    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> +static int coroutine_fn blkreplay_co_pwritev(BlockDriverState *bs,
> +    uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
>  {
>      uint64_t reqid = request_id++;
> -    int ret = bdrv_co_writev(bs->file->bs, sector_num, nb_sectors, qiov);
> +    int ret = bdrv_co_pwritev(bs->file->bs, offset, bytes, qiov, flags);

This would pass flags through, but fails to set .supported_write_flags,
so the caller will always pass flags=0. I think you want to set
.supported_write_flags during open; but see my recent patch about the
raw driver only setting flags according to the underlying device:

https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg05823.html

-- 
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: 604 bytes --]

[Qemu-devel] [PATCH 03/17] vhdx: Some more BlockBackend use in vhdx_create()

[Kevin Wolf]

This does some easy conversions from bdrv_* to blk_* functions in
vhdx_create(). We should avoid bypassing the BlockBackend layer whenever
possible.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vhdx.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index f5605a2..33b81e2 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1387,9 +1387,10 @@ exit:
  * There are 2 headers, and the highest sequence number will represent
  * the active header
  */
-static int vhdx_create_new_headers(BlockDriverState *bs, uint64_t image_size,
+static int vhdx_create_new_headers(BlockBackend *blk, uint64_t image_size,
                                    uint32_t log_size)
 {
+    BlockDriverState *bs = blk_bs(blk);
     int ret = 0;
     VHDXHeader *hdr = NULL;
 
@@ -1442,7 +1443,7 @@ exit:
  * The first 64KB of the Metadata section is reserved for the metadata
  * header and entries; beyond that, the metadata items themselves reside.
  */
-static int vhdx_create_new_metadata(BlockDriverState *bs,
+static int vhdx_create_new_metadata(BlockBackend *blk,
                                     uint64_t image_size,
                                     uint32_t block_size,
                                     uint32_t sector_size,
@@ -1538,13 +1539,13 @@ static int vhdx_create_new_metadata(BlockDriverState *bs,
                                    VHDX_META_FLAGS_IS_VIRTUAL_DISK;
     vhdx_metadata_entry_le_export(&md_table_entry[4]);
 
-    ret = bdrv_pwrite(bs, metadata_offset, buffer, VHDX_HEADER_BLOCK_SIZE);
+    ret = blk_pwrite(blk, metadata_offset, buffer, VHDX_HEADER_BLOCK_SIZE, 0);
     if (ret < 0) {
         goto exit;
     }
 
-    ret = bdrv_pwrite(bs, metadata_offset + (64 * KiB), entry_buffer,
-                      VHDX_METADATA_ENTRY_BUFFER_SIZE);
+    ret = blk_pwrite(blk, metadata_offset + (64 * KiB), entry_buffer,
+                     VHDX_METADATA_ENTRY_BUFFER_SIZE, 0);
     if (ret < 0) {
         goto exit;
     }
@@ -1564,7 +1565,7 @@ exit:
  *  Fixed images: default state of the BAT is fully populated, with
  *                file offsets and state PAYLOAD_BLOCK_FULLY_PRESENT.
  */
-static int vhdx_create_bat(BlockDriverState *bs, BDRVVHDXState *s,
+static int vhdx_create_bat(BlockBackend *blk, BDRVVHDXState *s,
                            uint64_t image_size, VHDXImageType type,
                            bool use_zero_blocks, uint64_t file_offset,
                            uint32_t length)
@@ -1588,12 +1589,12 @@ static int vhdx_create_bat(BlockDriverState *bs, BDRVVHDXState *s,
     if (type == VHDX_TYPE_DYNAMIC) {
         /* All zeroes, so we can just extend the file - the end of the BAT
          * is the furthest thing we have written yet */
-        ret = bdrv_truncate(bs, data_file_offset);
+        ret = blk_truncate(blk, data_file_offset);
         if (ret < 0) {
             goto exit;
         }
     } else if (type == VHDX_TYPE_FIXED) {
-        ret = bdrv_truncate(bs, data_file_offset + image_size);
+        ret = blk_truncate(blk, data_file_offset + image_size);
         if (ret < 0) {
             goto exit;
         }
@@ -1604,7 +1605,7 @@ static int vhdx_create_bat(BlockDriverState *bs, BDRVVHDXState *s,
 
     if (type == VHDX_TYPE_FIXED ||
                 use_zero_blocks ||
-                bdrv_has_zero_init(bs) == 0) {
+                bdrv_has_zero_init(blk_bs(blk)) == 0) {
         /* for a fixed file, the default BAT entry is not zero */
         s->bat = g_try_malloc0(length);
         if (length && s->bat == NULL) {
@@ -1620,12 +1621,12 @@ static int vhdx_create_bat(BlockDriverState *bs, BDRVVHDXState *s,
             sinfo.file_offset = data_file_offset +
                                 (sector_num << s->logical_sector_size_bits);
             sinfo.file_offset = ROUND_UP(sinfo.file_offset, MiB);
-            vhdx_update_bat_table_entry(bs, s, &sinfo, &unused, &unused,
+            vhdx_update_bat_table_entry(blk_bs(blk), s, &sinfo, &unused, &unused,
                                         block_state);
             cpu_to_le64s(&s->bat[sinfo.bat_idx]);
             sector_num += s->sectors_per_block;
         }
-        ret = bdrv_pwrite(bs, file_offset, s->bat, length);
+        ret = blk_pwrite(blk, file_offset, s->bat, length, 0);
         if (ret < 0) {
             goto exit;
         }
@@ -1645,7 +1646,7 @@ exit:
  * to create the BAT itself, we will also cause the BAT to be
  * created.
  */
-static int vhdx_create_new_region_table(BlockDriverState *bs,
+static int vhdx_create_new_region_table(BlockBackend *blk,
                                         uint64_t image_size,
                                         uint32_t block_size,
                                         uint32_t sector_size,
@@ -1720,21 +1721,21 @@ static int vhdx_create_new_region_table(BlockDriverState *bs,
 
     /* The region table gives us the data we need to create the BAT,
      * so do that now */
-    ret = vhdx_create_bat(bs, s, image_size, type, use_zero_blocks,
+    ret = vhdx_create_bat(blk, s, image_size, type, use_zero_blocks,
                           bat_file_offset, bat_length);
     if (ret < 0) {
         goto exit;
     }
 
     /* Now write out the region headers to disk */
-    ret = bdrv_pwrite(bs, VHDX_REGION_TABLE_OFFSET, buffer,
-                      VHDX_HEADER_BLOCK_SIZE);
+    ret = blk_pwrite(blk, VHDX_REGION_TABLE_OFFSET, buffer,
+                     VHDX_HEADER_BLOCK_SIZE, 0);
     if (ret < 0) {
         goto exit;
     }
 
-    ret = bdrv_pwrite(bs, VHDX_REGION_TABLE2_OFFSET, buffer,
-                      VHDX_HEADER_BLOCK_SIZE);
+    ret = blk_pwrite(blk, VHDX_REGION_TABLE2_OFFSET, buffer,
+                     VHDX_HEADER_BLOCK_SIZE, 0);
     if (ret < 0) {
         goto exit;
     }
@@ -1871,13 +1872,13 @@ static int vhdx_create(const char *filename, QemuOpts *opts, Error **errp)
 
 
     /* Creates (B),(C) */
-    ret = vhdx_create_new_headers(blk_bs(blk), image_size, log_size);
+    ret = vhdx_create_new_headers(blk, image_size, log_size);
     if (ret < 0) {
         goto delete_and_exit;
     }
 
     /* Creates (D),(E),(G) explicitly. (F) created as by-product */
-    ret = vhdx_create_new_region_table(blk_bs(blk), image_size, block_size, 512,
+    ret = vhdx_create_new_region_table(blk, image_size, block_size, 512,
                                        log_size, use_zero_blocks, image_type,
                                        &metadata_offset);
     if (ret < 0) {
@@ -1885,7 +1886,7 @@ static int vhdx_create(const char *filename, QemuOpts *opts, Error **errp)
     }
 
     /* Creates (H) */
-    ret = vhdx_create_new_metadata(blk_bs(blk), image_size, block_size, 512,
+    ret = vhdx_create_new_metadata(blk, image_size, block_size, 512,
                                    metadata_offset, image_type);
     if (ret < 0) {
         goto delete_and_exit;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 03/17] vhdx: Some more BlockBackend use in vhdx_create()

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> This does some easy conversions from bdrv_* to blk_* functions in
> vhdx_create(). We should avoid bypassing the BlockBackend layer whenever
> possible.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vhdx.c | 41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 04/17] block: Convert bdrv_co_readv() to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/crypto.c        | 2 +-
 block/io.c            | 8 ++++----
 block/parallels.c     | 2 +-
 block/qcow.c          | 5 ++---
 block/raw_bsd.c       | 2 +-
 block/vhdx.c          | 2 +-
 include/block/block.h | 4 ++--
 7 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 758e14e..3f0000d 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -428,7 +428,7 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
         qemu_iovec_reset(&hd_qiov);
         qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512);
 
-        ret = bdrv_co_readv(bs->file->bs,
+        ret = bdrv_co_readv(bs->file,
                             payload_offset + sector_num,
                             cur_nr_sectors, &hd_qiov);
         if (ret < 0) {
diff --git a/block/io.c b/block/io.c
index ebdb9d8..07e74b9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1137,12 +1137,12 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
                           nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
 }
 
-int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
-    int nb_sectors, QEMUIOVector *qiov)
+int coroutine_fn bdrv_co_readv(BdrvChild *child, int64_t sector_num,
+                               int nb_sectors, QEMUIOVector *qiov)
 {
-    trace_bdrv_co_readv(bs, sector_num, nb_sectors);
+    trace_bdrv_co_readv(child->bs, sector_num, nb_sectors);
 
-    return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov, 0);
+    return bdrv_co_do_readv(child->bs, sector_num, nb_sectors, qiov, 0);
 }
 
 #define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
diff --git a/block/parallels.c b/block/parallels.c
index d6a1a61..7da01fb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -351,7 +351,7 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
             qemu_iovec_reset(&hd_qiov);
             qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
 
-            ret = bdrv_co_readv(bs->file->bs, position, n, &hd_qiov);
+            ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
             if (ret < 0) {
                 break;
             }
diff --git a/block/qcow.c b/block/qcow.c
index 312af52..38a6ac0 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -619,8 +619,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
                 hd_iov.iov_len = n * 512;
                 qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
                 qemu_co_mutex_unlock(&s->lock);
-                ret = bdrv_co_readv(bs->backing->bs, sector_num,
-                                    n, &hd_qiov);
+                ret = bdrv_co_readv(bs->backing, sector_num, n, &hd_qiov);
                 qemu_co_mutex_lock(&s->lock);
                 if (ret < 0) {
                     goto fail;
@@ -644,7 +643,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
             hd_iov.iov_len = n * 512;
             qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
             qemu_co_mutex_unlock(&s->lock);
-            ret = bdrv_co_readv(bs->file->bs,
+            ret = bdrv_co_readv(bs->file,
                                 (cluster_offset >> 9) + index_in_cluster,
                                 n, &hd_qiov);
             qemu_co_mutex_lock(&s->lock);
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index b1d5237..8a943a4 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -54,7 +54,7 @@ static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
                                      int nb_sectors, QEMUIOVector *qiov)
 {
     BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
-    return bdrv_co_readv(bs->file->bs, sector_num, nb_sectors, qiov);
+    return bdrv_co_readv(bs->file, sector_num, nb_sectors, qiov);
 }
 
 static int coroutine_fn
diff --git a/block/vhdx.c b/block/vhdx.c
index 33b81e2..cca2540 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1117,7 +1117,7 @@ static coroutine_fn int vhdx_co_readv(BlockDriverState *bs, int64_t sector_num,
                 break;
             case PAYLOAD_BLOCK_FULLY_PRESENT:
                 qemu_co_mutex_unlock(&s->lock);
-                ret = bdrv_co_readv(bs->file->bs,
+                ret = bdrv_co_readv(bs->file,
                                     sinfo.file_offset >> BDRV_SECTOR_BITS,
                                     sinfo.sectors_avail, &hd_qiov);
                 qemu_co_mutex_lock(&s->lock);
diff --git a/include/block/block.h b/include/block/block.h
index 733a8ec..18236a6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -241,8 +241,8 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
 int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov);
 int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
     const void *buf, int count);
-int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
-    int nb_sectors, QEMUIOVector *qiov);
+int coroutine_fn bdrv_co_readv(BdrvChild *child, int64_t sector_num,
+                               int nb_sectors, QEMUIOVector *qiov);
 int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
     int nb_sectors, QEMUIOVector *qiov);
 /*
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 04/17] block: Convert bdrv_co_readv() to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/crypto.c        | 2 +-
>  block/io.c            | 8 ++++----
>  block/parallels.c     | 2 +-
>  block/qcow.c          | 5 ++---
>  block/raw_bsd.c       | 2 +-
>  block/vhdx.c          | 2 +-
>  include/block/block.h | 4 ++--
>  7 files changed, 12 insertions(+), 13 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 05/17] block: Convert bdrv_co_writev() to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/crypto.c        | 2 +-
 block/io.c            | 6 +++---
 block/parallels.c     | 2 +-
 block/qcow.c          | 2 +-
 block/vhdx.c          | 2 +-
 include/block/block.h | 4 ++--
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 3f0000d..844ed5c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -507,7 +507,7 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
         qemu_iovec_reset(&hd_qiov);
         qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512);
 
-        ret = bdrv_co_writev(bs->file->bs,
+        ret = bdrv_co_writev(bs->file,
                              payload_offset + sector_num,
                              cur_nr_sectors, &hd_qiov);
         if (ret < 0) {
diff --git a/block/io.c b/block/io.c
index 07e74b9..2696dd7 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1539,12 +1539,12 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
                            nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
 }
 
-int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
+int coroutine_fn bdrv_co_writev(BdrvChild *child, int64_t sector_num,
     int nb_sectors, QEMUIOVector *qiov)
 {
-    trace_bdrv_co_writev(bs, sector_num, nb_sectors);
+    trace_bdrv_co_writev(child->bs, sector_num, nb_sectors);
 
-    return bdrv_co_do_writev(bs, sector_num, nb_sectors, qiov, 0);
+    return bdrv_co_do_writev(child->bs, sector_num, nb_sectors, qiov, 0);
 }
 
 int coroutine_fn bdrv_co_pwrite_zeroes(BlockDriverState *bs,
diff --git a/block/parallels.c b/block/parallels.c
index 7da01fb..91ab61f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -311,7 +311,7 @@ static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
         qemu_iovec_reset(&hd_qiov);
         qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
 
-        ret = bdrv_co_writev(bs->file->bs, position, n, &hd_qiov);
+        ret = bdrv_co_writev(bs->file, position, n, &hd_qiov);
         if (ret < 0) {
             break;
         }
diff --git a/block/qcow.c b/block/qcow.c
index 38a6ac0..e09827b 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -745,7 +745,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
         hd_iov.iov_len = n * 512;
         qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
         qemu_co_mutex_unlock(&s->lock);
-        ret = bdrv_co_writev(bs->file->bs,
+        ret = bdrv_co_writev(bs->file,
                              (cluster_offset >> 9) + index_in_cluster,
                              n, &hd_qiov);
         qemu_co_mutex_lock(&s->lock);
diff --git a/block/vhdx.c b/block/vhdx.c
index cca2540..b0f66de 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1326,7 +1326,7 @@ static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t sector_num,
                 }
                 /* block exists, so we can just overwrite it */
                 qemu_co_mutex_unlock(&s->lock);
-                ret = bdrv_co_writev(bs->file->bs,
+                ret = bdrv_co_writev(bs->file,
                                     sinfo.file_offset >> BDRV_SECTOR_BITS,
                                     sectors_to_write, &hd_qiov);
                 qemu_co_mutex_lock(&s->lock);
diff --git a/include/block/block.h b/include/block/block.h
index 18236a6..d39c6bee 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -243,8 +243,8 @@ int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
     const void *buf, int count);
 int coroutine_fn bdrv_co_readv(BdrvChild *child, int64_t sector_num,
                                int nb_sectors, QEMUIOVector *qiov);
-int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
-    int nb_sectors, QEMUIOVector *qiov);
+int coroutine_fn bdrv_co_writev(BdrvChild *child, int64_t sector_num,
+                               int nb_sectors, QEMUIOVector *qiov);
 /*
  * Efficiently zero a region of the disk image.  Note that this is a regular
  * I/O request like read or write and should have a reasonable size.  This
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 05/17] block: Convert bdrv_co_writev() to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/crypto.c        | 2 +-
>  block/io.c            | 6 +++---
>  block/parallels.c     | 2 +-
>  block/qcow.c          | 2 +-
>  block/vhdx.c          | 2 +-
>  include/block/block.h | 4 ++--
>  6 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Side note: I'd be completely happy with renaming these functions, as
proposed by Paolo. But I'm fine with them staying as they are, too.

Max


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

Re: [Qemu-devel] [PATCH 05/17] block: Convert bdrv_co_writev() to BdrvChild

[Kevin Wolf]

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

Am 25.06.2016 um 17:14 hat Max Reitz geschrieben:
> On 21.06.2016 11:21, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/crypto.c        | 2 +-
> >  block/io.c            | 6 +++---
> >  block/parallels.c     | 2 +-
> >  block/qcow.c          | 2 +-
> >  block/vhdx.c          | 2 +-
> >  include/block/block.h | 4 ++--
> >  6 files changed, 9 insertions(+), 9 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> Side note: I'd be completely happy with renaming these functions, as
> proposed by Paolo. But I'm fine with them staying as they are, too.

If someone has a good suggestion, I'm not opposed to renaming them.
Otherwise, I'd just leave them.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[Qemu-devel] [PATCH 06/17] block: Convert bdrv_aio_readv() to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c      | 2 +-
 block/blkverify.c     | 4 ++--
 block/io.c            | 6 +++---
 block/qed-table.c     | 2 +-
 block/qed.c           | 6 +++---
 block/quorum.c        | 4 ++--
 include/block/block.h | 2 +-
 7 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 20d25bd..35e5a24 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -456,7 +456,7 @@ static BlockAIOCB *blkdebug_aio_readv(BlockDriverState *bs,
         return inject_error(bs, cb, opaque, rule);
     }
 
-    return bdrv_aio_readv(bs->file->bs, sector_num, qiov, nb_sectors,
+    return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors,
                           cb, opaque);
 }
 
diff --git a/block/blkverify.c b/block/blkverify.c
index 4045396..4672fda 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -247,9 +247,9 @@ static BlockAIOCB *blkverify_aio_readv(BlockDriverState *bs,
     qemu_iovec_init(&acb->raw_qiov, acb->qiov->niov);
     qemu_iovec_clone(&acb->raw_qiov, qiov, acb->buf);
 
-    bdrv_aio_readv(s->test_file->bs, sector_num, qiov, nb_sectors,
+    bdrv_aio_readv(s->test_file, sector_num, qiov, nb_sectors,
                    blkverify_aio_cb, acb);
-    bdrv_aio_readv(bs->file->bs, sector_num, &acb->raw_qiov, nb_sectors,
+    bdrv_aio_readv(bs->file, sector_num, &acb->raw_qiov, nb_sectors,
                    blkverify_aio_cb, acb);
     return &acb->common;
 }
diff --git a/block/io.c b/block/io.c
index 2696dd7..b514b4d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1954,13 +1954,13 @@ int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
 /**************************************************************/
 /* async I/Os */
 
-BlockAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
+BlockAIOCB *bdrv_aio_readv(BdrvChild *child, int64_t sector_num,
                            QEMUIOVector *qiov, int nb_sectors,
                            BlockCompletionFunc *cb, void *opaque)
 {
-    trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
+    trace_bdrv_aio_readv(child->bs, sector_num, nb_sectors, opaque);
 
-    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, 0,
+    return bdrv_co_aio_rw_vector(child->bs, sector_num, qiov, nb_sectors, 0,
                                  cb, opaque, false);
 }
 
diff --git a/block/qed-table.c b/block/qed-table.c
index c841ad1..2db0a33 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -65,7 +65,7 @@ static void qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
     read_table_cb->iov.iov_len = s->header.cluster_size * s->header.table_size,
 
     qemu_iovec_init_external(qiov, &read_table_cb->iov, 1);
-    bdrv_aio_readv(s->bs->file->bs, offset / BDRV_SECTOR_SIZE, qiov,
+    bdrv_aio_readv(s->bs->file, offset / BDRV_SECTOR_SIZE, qiov,
                    qiov->size / BDRV_SECTOR_SIZE,
                    qed_read_table_cb, read_table_cb);
 }
diff --git a/block/qed.c b/block/qed.c
index 1206806..7f71007 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -155,7 +155,7 @@ static void qed_write_header(BDRVQEDState *s, BlockCompletionFunc cb,
     write_header_cb->iov.iov_len = len;
     qemu_iovec_init_external(&write_header_cb->qiov, &write_header_cb->iov, 1);
 
-    bdrv_aio_readv(s->bs->file->bs, 0, &write_header_cb->qiov, nsectors,
+    bdrv_aio_readv(s->bs->file, 0, &write_header_cb->qiov, nsectors,
                    qed_write_header_read_cb, write_header_cb);
 }
 
@@ -800,7 +800,7 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
     qemu_iovec_concat(*backing_qiov, qiov, 0, size);
 
     BLKDBG_EVENT(s->bs->file, BLKDBG_READ_BACKING_AIO);
-    bdrv_aio_readv(s->bs->backing->bs, pos / BDRV_SECTOR_SIZE,
+    bdrv_aio_readv(s->bs->backing, pos / BDRV_SECTOR_SIZE,
                    *backing_qiov, size / BDRV_SECTOR_SIZE, cb, opaque);
 }
 
@@ -1319,7 +1319,7 @@ static void qed_aio_read_data(void *opaque, int ret,
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
-    bdrv_aio_readv(bs->file->bs, offset / BDRV_SECTOR_SIZE,
+    bdrv_aio_readv(bs->file, offset / BDRV_SECTOR_SIZE,
                    &acb->cur_qiov, acb->cur_qiov.size / BDRV_SECTOR_SIZE,
                    qed_aio_next_io, acb);
     return;
diff --git a/block/quorum.c b/block/quorum.c
index ec6f3b9..044d744 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -660,7 +660,7 @@ static BlockAIOCB *read_quorum_children(QuorumAIOCB *acb)
     }
 
     for (i = 0; i < s->num_children; i++) {
-        acb->qcrs[i].aiocb = bdrv_aio_readv(s->children[i]->bs, acb->sector_num,
+        acb->qcrs[i].aiocb = bdrv_aio_readv(s->children[i], acb->sector_num,
                                             &acb->qcrs[i].qiov, acb->nb_sectors,
                                             quorum_aio_cb, &acb->qcrs[i]);
     }
@@ -678,7 +678,7 @@ static BlockAIOCB *read_fifo_child(QuorumAIOCB *acb)
     qemu_iovec_clone(&acb->qcrs[acb->child_iter].qiov, acb->qiov,
                      acb->qcrs[acb->child_iter].buf);
     acb->qcrs[acb->child_iter].aiocb =
-        bdrv_aio_readv(s->children[acb->child_iter]->bs, acb->sector_num,
+        bdrv_aio_readv(s->children[acb->child_iter], acb->sector_num,
                        &acb->qcrs[acb->child_iter].qiov, acb->nb_sectors,
                        quorum_aio_cb, &acb->qcrs[acb->child_iter]);
 
diff --git a/include/block/block.h b/include/block/block.h
index d39c6bee..8aa77fb 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -310,7 +310,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
                                         const char *node_name, Error **errp);
 
 /* async block I/O */
-BlockAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
+BlockAIOCB *bdrv_aio_readv(BdrvChild *child, int64_t sector_num,
                            QEMUIOVector *iov, int nb_sectors,
                            BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 06/17] block: Convert bdrv_aio_readv() to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkdebug.c      | 2 +-
>  block/blkverify.c     | 4 ++--
>  block/io.c            | 6 +++---
>  block/qed-table.c     | 2 +-
>  block/qed.c           | 6 +++---
>  block/quorum.c        | 4 ++--
>  include/block/block.h | 2 +-
>  7 files changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 07/17] block: Convert bdrv_aio_writev() to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c      | 2 +-
 block/blkverify.c     | 4 ++--
 block/io.c            | 6 +++---
 block/qed-table.c     | 2 +-
 block/qed.c           | 6 +++---
 block/quorum.c        | 4 ++--
 include/block/block.h | 2 +-
 7 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 35e5a24..084fe5b 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -479,7 +479,7 @@ static BlockAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
         return inject_error(bs, cb, opaque, rule);
     }
 
-    return bdrv_aio_writev(bs->file->bs, sector_num, qiov, nb_sectors,
+    return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors,
                            cb, opaque);
 }
 
diff --git a/block/blkverify.c b/block/blkverify.c
index 4672fda..da62d75 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -262,9 +262,9 @@ static BlockAIOCB *blkverify_aio_writev(BlockDriverState *bs,
     BlkverifyAIOCB *acb = blkverify_aio_get(bs, true, sector_num, qiov,
                                             nb_sectors, cb, opaque);
 
-    bdrv_aio_writev(s->test_file->bs, sector_num, qiov, nb_sectors,
+    bdrv_aio_writev(s->test_file, sector_num, qiov, nb_sectors,
                     blkverify_aio_cb, acb);
-    bdrv_aio_writev(bs->file->bs, sector_num, qiov, nb_sectors,
+    bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors,
                     blkverify_aio_cb, acb);
     return &acb->common;
 }
diff --git a/block/io.c b/block/io.c
index b514b4d..27721ce 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1964,13 +1964,13 @@ BlockAIOCB *bdrv_aio_readv(BdrvChild *child, int64_t sector_num,
                                  cb, opaque, false);
 }
 
-BlockAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
+BlockAIOCB *bdrv_aio_writev(BdrvChild *child, int64_t sector_num,
                             QEMUIOVector *qiov, int nb_sectors,
                             BlockCompletionFunc *cb, void *opaque)
 {
-    trace_bdrv_aio_writev(bs, sector_num, nb_sectors, opaque);
+    trace_bdrv_aio_writev(child->bs, sector_num, nb_sectors, opaque);
 
-    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, 0,
+    return bdrv_co_aio_rw_vector(child->bs, sector_num, qiov, nb_sectors, 0,
                                  cb, opaque, true);
 }
 
diff --git a/block/qed-table.c b/block/qed-table.c
index 2db0a33..1a731df 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -154,7 +154,7 @@ static void qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
     /* Adjust for offset into table */
     offset += start * sizeof(uint64_t);
 
-    bdrv_aio_writev(s->bs->file->bs, offset / BDRV_SECTOR_SIZE,
+    bdrv_aio_writev(s->bs->file, offset / BDRV_SECTOR_SIZE,
                     &write_table_cb->qiov,
                     write_table_cb->qiov.size / BDRV_SECTOR_SIZE,
                     qed_write_table_cb, write_table_cb);
diff --git a/block/qed.c b/block/qed.c
index 7f71007..9d3d588 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -123,7 +123,7 @@ static void qed_write_header_read_cb(void *opaque, int ret)
     /* Update header */
     qed_header_cpu_to_le(&s->header, (QEDHeader *)write_header_cb->buf);
 
-    bdrv_aio_writev(s->bs->file->bs, 0, &write_header_cb->qiov,
+    bdrv_aio_writev(s->bs->file, 0, &write_header_cb->qiov,
                     write_header_cb->nsectors, qed_write_header_cb,
                     write_header_cb);
 }
@@ -837,7 +837,7 @@ static void qed_copy_from_backing_file_write(void *opaque, int ret)
     }
 
     BLKDBG_EVENT(s->bs->file, BLKDBG_COW_WRITE);
-    bdrv_aio_writev(s->bs->file->bs, copy_cb->offset / BDRV_SECTOR_SIZE,
+    bdrv_aio_writev(s->bs->file, copy_cb->offset / BDRV_SECTOR_SIZE,
                     &copy_cb->qiov, copy_cb->qiov.size / BDRV_SECTOR_SIZE,
                     qed_copy_from_backing_file_cb, copy_cb);
 }
@@ -1087,7 +1087,7 @@ static void qed_aio_write_main(void *opaque, int ret)
     }
 
     BLKDBG_EVENT(s->bs->file, BLKDBG_WRITE_AIO);
-    bdrv_aio_writev(s->bs->file->bs, offset / BDRV_SECTOR_SIZE,
+    bdrv_aio_writev(s->bs->file, offset / BDRV_SECTOR_SIZE,
                     &acb->cur_qiov, acb->cur_qiov.size / BDRV_SECTOR_SIZE,
                     next_fn, acb);
 }
diff --git a/block/quorum.c b/block/quorum.c
index 044d744..8c886b8 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -383,7 +383,7 @@ static bool quorum_rewrite_bad_versions(BDRVQuorumState *s, QuorumAIOCB *acb,
             continue;
         }
         QLIST_FOREACH(item, &version->items, next) {
-            bdrv_aio_writev(s->children[item->index]->bs, acb->sector_num,
+            bdrv_aio_writev(s->children[item->index], acb->sector_num,
                             acb->qiov, acb->nb_sectors, quorum_rewrite_aio_cb,
                             acb);
         }
@@ -719,7 +719,7 @@ static BlockAIOCB *quorum_aio_writev(BlockDriverState *bs,
     int i;
 
     for (i = 0; i < s->num_children; i++) {
-        acb->qcrs[i].aiocb = bdrv_aio_writev(s->children[i]->bs, sector_num,
+        acb->qcrs[i].aiocb = bdrv_aio_writev(s->children[i], sector_num,
                                              qiov, nb_sectors, &quorum_aio_cb,
                                              &acb->qcrs[i]);
     }
diff --git a/include/block/block.h b/include/block/block.h
index 8aa77fb..c6891d7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -313,7 +313,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
 BlockAIOCB *bdrv_aio_readv(BdrvChild *child, int64_t sector_num,
                            QEMUIOVector *iov, int nb_sectors,
                            BlockCompletionFunc *cb, void *opaque);
-BlockAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
+BlockAIOCB *bdrv_aio_writev(BdrvChild *child, int64_t sector_num,
                             QEMUIOVector *iov, int nb_sectors,
                             BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *bdrv_aio_flush(BlockDriverState *bs,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 07/17] block: Convert bdrv_aio_writev() to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkdebug.c      | 2 +-
>  block/blkverify.c     | 4 ++--
>  block/io.c            | 6 +++---
>  block/qed-table.c     | 2 +-
>  block/qed.c           | 6 +++---
>  block/quorum.c        | 4 ++--
>  include/block/block.h | 2 +-
>  7 files changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 08/17] block: Convert bdrv_co_do_readv/writev to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/block/io.c b/block/io.c
index 27721ce..299e9ec 100644
--- a/block/io.c
+++ b/block/io.c
@@ -33,7 +33,7 @@
 
 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
 
-static BlockAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
+static BlockAIOCB *bdrv_co_aio_rw_vector(BdrvChild *child,
                                          int64_t sector_num,
                                          QEMUIOVector *qiov,
                                          int nb_sectors,
@@ -1125,7 +1125,7 @@ int coroutine_fn bdrv_co_preadv(BlockDriverState *bs,
     return ret;
 }
 
-static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
+static int coroutine_fn bdrv_co_do_readv(BdrvChild *child,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
     BdrvRequestFlags flags)
 {
@@ -1133,7 +1133,7 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
         return -EINVAL;
     }
 
-    return bdrv_co_preadv(bs, sector_num << BDRV_SECTOR_BITS,
+    return bdrv_co_preadv(child->bs, sector_num << BDRV_SECTOR_BITS,
                           nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
 }
 
@@ -1142,7 +1142,7 @@ int coroutine_fn bdrv_co_readv(BdrvChild *child, int64_t sector_num,
 {
     trace_bdrv_co_readv(child->bs, sector_num, nb_sectors);
 
-    return bdrv_co_do_readv(child->bs, sector_num, nb_sectors, qiov, 0);
+    return bdrv_co_do_readv(child, sector_num, nb_sectors, qiov, 0);
 }
 
 #define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
@@ -1527,7 +1527,7 @@ out:
     return ret;
 }
 
-static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
+static int coroutine_fn bdrv_co_do_writev(BdrvChild *child,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
     BdrvRequestFlags flags)
 {
@@ -1535,7 +1535,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
         return -EINVAL;
     }
 
-    return bdrv_co_pwritev(bs, sector_num << BDRV_SECTOR_BITS,
+    return bdrv_co_pwritev(child->bs, sector_num << BDRV_SECTOR_BITS,
                            nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
 }
 
@@ -1544,7 +1544,7 @@ int coroutine_fn bdrv_co_writev(BdrvChild *child, int64_t sector_num,
 {
     trace_bdrv_co_writev(child->bs, sector_num, nb_sectors);
 
-    return bdrv_co_do_writev(child->bs, sector_num, nb_sectors, qiov, 0);
+    return bdrv_co_do_writev(child, sector_num, nb_sectors, qiov, 0);
 }
 
 int coroutine_fn bdrv_co_pwrite_zeroes(BlockDriverState *bs,
@@ -1960,7 +1960,7 @@ BlockAIOCB *bdrv_aio_readv(BdrvChild *child, int64_t sector_num,
 {
     trace_bdrv_aio_readv(child->bs, sector_num, nb_sectors, opaque);
 
-    return bdrv_co_aio_rw_vector(child->bs, sector_num, qiov, nb_sectors, 0,
+    return bdrv_co_aio_rw_vector(child, sector_num, qiov, nb_sectors, 0,
                                  cb, opaque, false);
 }
 
@@ -1970,7 +1970,7 @@ BlockAIOCB *bdrv_aio_writev(BdrvChild *child, int64_t sector_num,
 {
     trace_bdrv_aio_writev(child->bs, sector_num, nb_sectors, opaque);
 
-    return bdrv_co_aio_rw_vector(child->bs, sector_num, qiov, nb_sectors, 0,
+    return bdrv_co_aio_rw_vector(child, sector_num, qiov, nb_sectors, 0,
                                  cb, opaque, true);
 }
 
@@ -2026,6 +2026,7 @@ typedef struct BlockRequest {
 
 typedef struct BlockAIOCBCoroutine {
     BlockAIOCB common;
+    BdrvChild *child;
     BlockRequest req;
     bool is_write;
     bool need_bh;
@@ -2069,20 +2070,19 @@ static void bdrv_co_maybe_schedule_bh(BlockAIOCBCoroutine *acb)
 static void coroutine_fn bdrv_co_do_rw(void *opaque)
 {
     BlockAIOCBCoroutine *acb = opaque;
-    BlockDriverState *bs = acb->common.bs;
 
     if (!acb->is_write) {
-        acb->req.error = bdrv_co_do_readv(bs, acb->req.sector,
+        acb->req.error = bdrv_co_do_readv(acb->child, acb->req.sector,
             acb->req.nb_sectors, acb->req.qiov, acb->req.flags);
     } else {
-        acb->req.error = bdrv_co_do_writev(bs, acb->req.sector,
+        acb->req.error = bdrv_co_do_writev(acb->child, acb->req.sector,
             acb->req.nb_sectors, acb->req.qiov, acb->req.flags);
     }
 
     bdrv_co_complete(acb);
 }
 
-static BlockAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
+static BlockAIOCB *bdrv_co_aio_rw_vector(BdrvChild *child,
                                          int64_t sector_num,
                                          QEMUIOVector *qiov,
                                          int nb_sectors,
@@ -2094,7 +2094,8 @@ static BlockAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
     Coroutine *co;
     BlockAIOCBCoroutine *acb;
 
-    acb = qemu_aio_get(&bdrv_em_co_aiocb_info, bs, cb, opaque);
+    acb = qemu_aio_get(&bdrv_em_co_aiocb_info, child->bs, cb, opaque);
+    acb->child = child;
     acb->need_bh = true;
     acb->req.error = -EINPROGRESS;
     acb->req.sector = sector_num;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 08/17] block: Convert bdrv_co_do_readv/writev to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 09/17] block: Move bdrv_commit() to block/commit.c

[Kevin Wolf]

No code changes, just moved from one file to another.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c             | 110 ---------------------------------------------------
 block/Makefile.objs |   3 +-
 block/commit.c      | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 112 insertions(+), 112 deletions(-)

diff --git a/block.c b/block.c
index b331eb9..160324a 100644
--- a/block.c
+++ b/block.c
@@ -2331,116 +2331,6 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
     return bs->drv->bdrv_check(bs, res, fix);
 }
 
-#define COMMIT_BUF_SECTORS 2048
-
-/* commit COW file into the raw image */
-int bdrv_commit(BlockDriverState *bs)
-{
-    BlockDriver *drv = bs->drv;
-    int64_t sector, total_sectors, length, backing_length;
-    int n, ro, open_flags;
-    int ret = 0;
-    uint8_t *buf = NULL;
-
-    if (!drv)
-        return -ENOMEDIUM;
-
-    if (!bs->backing) {
-        return -ENOTSUP;
-    }
-
-    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, NULL) ||
-        bdrv_op_is_blocked(bs->backing->bs, BLOCK_OP_TYPE_COMMIT_TARGET, NULL)) {
-        return -EBUSY;
-    }
-
-    ro = bs->backing->bs->read_only;
-    open_flags =  bs->backing->bs->open_flags;
-
-    if (ro) {
-        if (bdrv_reopen(bs->backing->bs, open_flags | BDRV_O_RDWR, NULL)) {
-            return -EACCES;
-        }
-    }
-
-    length = bdrv_getlength(bs);
-    if (length < 0) {
-        ret = length;
-        goto ro_cleanup;
-    }
-
-    backing_length = bdrv_getlength(bs->backing->bs);
-    if (backing_length < 0) {
-        ret = backing_length;
-        goto ro_cleanup;
-    }
-
-    /* If our top snapshot is larger than the backing file image,
-     * grow the backing file image if possible.  If not possible,
-     * we must return an error */
-    if (length > backing_length) {
-        ret = bdrv_truncate(bs->backing->bs, length);
-        if (ret < 0) {
-            goto ro_cleanup;
-        }
-    }
-
-    total_sectors = length >> BDRV_SECTOR_BITS;
-
-    /* qemu_try_blockalign() for bs will choose an alignment that works for
-     * bs->backing->bs as well, so no need to compare the alignment manually. */
-    buf = qemu_try_blockalign(bs, COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
-    if (buf == NULL) {
-        ret = -ENOMEM;
-        goto ro_cleanup;
-    }
-
-    for (sector = 0; sector < total_sectors; sector += n) {
-        ret = bdrv_is_allocated(bs, sector, COMMIT_BUF_SECTORS, &n);
-        if (ret < 0) {
-            goto ro_cleanup;
-        }
-        if (ret) {
-            ret = bdrv_read(bs, sector, buf, n);
-            if (ret < 0) {
-                goto ro_cleanup;
-            }
-
-            ret = bdrv_write(bs->backing->bs, sector, buf, n);
-            if (ret < 0) {
-                goto ro_cleanup;
-            }
-        }
-    }
-
-    if (drv->bdrv_make_empty) {
-        ret = drv->bdrv_make_empty(bs);
-        if (ret < 0) {
-            goto ro_cleanup;
-        }
-        bdrv_flush(bs);
-    }
-
-    /*
-     * Make sure all data we wrote to the backing device is actually
-     * stable on disk.
-     */
-    if (bs->backing) {
-        bdrv_flush(bs->backing->bs);
-    }
-
-    ret = 0;
-ro_cleanup:
-    qemu_vfree(buf);
-
-    if (ro) {
-        /* ignoring error return here */
-        bdrv_reopen(bs->backing->bs, open_flags & ~BDRV_O_RDWR, NULL);
-    }
-
-    return ret;
-}
-
 /*
  * Return values:
  * 0        - success
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 44a5416..2593a2f 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -9,7 +9,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o
 block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
 block-obj-$(CONFIG_POSIX) += raw-posix.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
-block-obj-y += null.o mirror.o io.o
+block-obj-y += null.o mirror.o commit.o io.o
 block-obj-y += throttle-groups.o
 
 block-obj-y += nbd.o nbd-client.o sheepdog.o
@@ -26,7 +26,6 @@ block-obj-y += write-threshold.o
 block-obj-y += crypto.o
 
 common-obj-y += stream.o
-common-obj-y += commit.o
 common-obj-y += backup.o
 
 iscsi.o-cflags     := $(LIBISCSI_CFLAGS)
diff --git a/block/commit.c b/block/commit.c
index 444333b..4ac3df3 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -282,3 +282,114 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
     trace_commit_start(bs, base, top, s, s->common.co, opaque);
     qemu_coroutine_enter(s->common.co, s);
 }
+
+
+#define COMMIT_BUF_SECTORS 2048
+
+/* commit COW file into the raw image */
+int bdrv_commit(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    int64_t sector, total_sectors, length, backing_length;
+    int n, ro, open_flags;
+    int ret = 0;
+    uint8_t *buf = NULL;
+
+    if (!drv)
+        return -ENOMEDIUM;
+
+    if (!bs->backing) {
+        return -ENOTSUP;
+    }
+
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, NULL) ||
+        bdrv_op_is_blocked(bs->backing->bs, BLOCK_OP_TYPE_COMMIT_TARGET, NULL)) {
+        return -EBUSY;
+    }
+
+    ro = bs->backing->bs->read_only;
+    open_flags =  bs->backing->bs->open_flags;
+
+    if (ro) {
+        if (bdrv_reopen(bs->backing->bs, open_flags | BDRV_O_RDWR, NULL)) {
+            return -EACCES;
+        }
+    }
+
+    length = bdrv_getlength(bs);
+    if (length < 0) {
+        ret = length;
+        goto ro_cleanup;
+    }
+
+    backing_length = bdrv_getlength(bs->backing->bs);
+    if (backing_length < 0) {
+        ret = backing_length;
+        goto ro_cleanup;
+    }
+
+    /* If our top snapshot is larger than the backing file image,
+     * grow the backing file image if possible.  If not possible,
+     * we must return an error */
+    if (length > backing_length) {
+        ret = bdrv_truncate(bs->backing->bs, length);
+        if (ret < 0) {
+            goto ro_cleanup;
+        }
+    }
+
+    total_sectors = length >> BDRV_SECTOR_BITS;
+
+    /* qemu_try_blockalign() for bs will choose an alignment that works for
+     * bs->backing->bs as well, so no need to compare the alignment manually. */
+    buf = qemu_try_blockalign(bs, COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
+    if (buf == NULL) {
+        ret = -ENOMEM;
+        goto ro_cleanup;
+    }
+
+    for (sector = 0; sector < total_sectors; sector += n) {
+        ret = bdrv_is_allocated(bs, sector, COMMIT_BUF_SECTORS, &n);
+        if (ret < 0) {
+            goto ro_cleanup;
+        }
+        if (ret) {
+            ret = bdrv_read(bs, sector, buf, n);
+            if (ret < 0) {
+                goto ro_cleanup;
+            }
+
+            ret = bdrv_write(bs->backing->bs, sector, buf, n);
+            if (ret < 0) {
+                goto ro_cleanup;
+            }
+        }
+    }
+
+    if (drv->bdrv_make_empty) {
+        ret = drv->bdrv_make_empty(bs);
+        if (ret < 0) {
+            goto ro_cleanup;
+        }
+        bdrv_flush(bs);
+    }
+
+    /*
+     * Make sure all data we wrote to the backing device is actually
+     * stable on disk.
+     */
+    if (bs->backing) {
+        bdrv_flush(bs->backing->bs);
+    }
+
+    ret = 0;
+ro_cleanup:
+    qemu_vfree(buf);
+
+    if (ro) {
+        /* ignoring error return here */
+        bdrv_reopen(bs->backing->bs, open_flags & ~BDRV_O_RDWR, NULL);
+    }
+
+    return ret;
+}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 09/17] block: Move bdrv_commit() to block/commit.c

[Eric Blake]

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

On 06/21/2016 03:21 AM, Kevin Wolf wrote:
> No code changes, just moved from one file to another.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c             | 110 ---------------------------------------------------
>  block/Makefile.objs |   3 +-
>  block/commit.c      | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 112 insertions(+), 112 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: 604 bytes --]

[Qemu-devel] [PATCH 10/17] block: Use BlockBackend for I/O in bdrv_commit()

[Kevin Wolf]

Just like block jobs, the HMP commit command should use its own
BlockBackend for doing I/O on BlockDriverStates.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 4ac3df3..17f365f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -289,6 +289,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
 /* commit COW file into the raw image */
 int bdrv_commit(BlockDriverState *bs)
 {
+    BlockBackend *src, *backing;
     BlockDriver *drv = bs->drv;
     int64_t sector, total_sectors, length, backing_length;
     int n, ro, open_flags;
@@ -316,13 +317,19 @@ int bdrv_commit(BlockDriverState *bs)
         }
     }
 
-    length = bdrv_getlength(bs);
+    src = blk_new();
+    blk_insert_bs(src, bs);
+
+    backing = blk_new();
+    blk_insert_bs(backing, bs->backing->bs);
+
+    length = blk_getlength(src);
     if (length < 0) {
         ret = length;
         goto ro_cleanup;
     }
 
-    backing_length = bdrv_getlength(bs->backing->bs);
+    backing_length = blk_getlength(backing);
     if (backing_length < 0) {
         ret = backing_length;
         goto ro_cleanup;
@@ -332,7 +339,7 @@ int bdrv_commit(BlockDriverState *bs)
      * grow the backing file image if possible.  If not possible,
      * we must return an error */
     if (length > backing_length) {
-        ret = bdrv_truncate(bs->backing->bs, length);
+        ret = blk_truncate(backing, length);
         if (ret < 0) {
             goto ro_cleanup;
         }
@@ -342,7 +349,7 @@ int bdrv_commit(BlockDriverState *bs)
 
     /* qemu_try_blockalign() for bs will choose an alignment that works for
      * bs->backing->bs as well, so no need to compare the alignment manually. */
-    buf = qemu_try_blockalign(bs, COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
+    buf = blk_try_blockalign(src, COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
     if (buf == NULL) {
         ret = -ENOMEM;
         goto ro_cleanup;
@@ -354,12 +361,14 @@ int bdrv_commit(BlockDriverState *bs)
             goto ro_cleanup;
         }
         if (ret) {
-            ret = bdrv_read(bs, sector, buf, n);
+            ret = blk_pread(src, sector * BDRV_SECTOR_SIZE, buf,
+                            n * BDRV_SECTOR_SIZE);
             if (ret < 0) {
                 goto ro_cleanup;
             }
 
-            ret = bdrv_write(bs->backing->bs, sector, buf, n);
+            ret = blk_pwrite(backing, sector * BDRV_SECTOR_SIZE, buf,
+                             n * BDRV_SECTOR_SIZE, 0);
             if (ret < 0) {
                 goto ro_cleanup;
             }
@@ -371,21 +380,22 @@ int bdrv_commit(BlockDriverState *bs)
         if (ret < 0) {
             goto ro_cleanup;
         }
-        bdrv_flush(bs);
+        blk_flush(src);
     }
 
     /*
      * Make sure all data we wrote to the backing device is actually
      * stable on disk.
      */
-    if (bs->backing) {
-        bdrv_flush(bs->backing->bs);
-    }
+    blk_flush(backing);
 
     ret = 0;
 ro_cleanup:
     qemu_vfree(buf);
 
+    blk_unref(src);
+    blk_unref(backing);
+
     if (ro) {
         /* ignoring error return here */
         bdrv_reopen(bs->backing->bs, open_flags & ~BDRV_O_RDWR, NULL);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 10/17] block: Use BlockBackend for I/O in bdrv_commit()

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Just like block jobs, the HMP commit command should use its own
> BlockBackend for doing I/O on BlockDriverStates.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/commit.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 4ac3df3..17f365f 100644
> --- a/block/commit.c
> +++ b/block/commit.c

[...]

> @@ -342,7 +349,7 @@ int bdrv_commit(BlockDriverState *bs)
>  
>      /* qemu_try_blockalign() for bs will choose an alignment that works for
>       * bs->backing->bs as well, so no need to compare the alignment manually. */

This comment will need a little change, too.

With that done: Reviewed-by: Max Reitz <mreitz@redhat.com>

> -    buf = qemu_try_blockalign(bs, COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
> +    buf = blk_try_blockalign(src, COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
>      if (buf == NULL) {
>          ret = -ENOMEM;
>          goto ro_cleanup;


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

[Qemu-devel] [PATCH 11/17] block: Convert bdrv_read() to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c            | 4 ++--
 block/qcow2-cluster.c | 6 +++---
 block/vdi.c           | 4 ++--
 block/vvfat.c         | 2 +-
 include/block/block.h | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/io.c b/block/io.c
index 299e9ec..d14c982 100644
--- a/block/io.c
+++ b/block/io.c
@@ -634,10 +634,10 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
 }
 
 /* return < 0 if error. See bdrv_write() for the return codes */
-int bdrv_read(BlockDriverState *bs, int64_t sector_num,
+int bdrv_read(BdrvChild *child, int64_t sector_num,
               uint8_t *buf, int nb_sectors)
 {
-    return bdrv_rw_co(bs, sector_num, buf, nb_sectors, false, 0);
+    return bdrv_rw_co(child->bs, sector_num, buf, nb_sectors, false, 0);
 }
 
 /* Return < 0 if error. Important errors are:
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 893ddf6..ed9832a 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1410,7 +1410,7 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
         sector_offset = coffset & 511;
         csize = nb_csectors * 512 - sector_offset;
         BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
-        ret = bdrv_read(bs->file->bs, coffset >> 9, s->cluster_data,
+        ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
                         nb_csectors);
         if (ret < 0) {
             return ret;
@@ -1679,7 +1679,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                     (void **)&l2_table);
         } else {
             /* load inactive L2 tables from disk */
-            ret = bdrv_read(bs->file->bs, l2_offset / BDRV_SECTOR_SIZE,
+            ret = bdrv_read(bs->file, l2_offset / BDRV_SECTOR_SIZE,
                             (void *)l2_table, s->cluster_sectors);
         }
         if (ret < 0) {
@@ -1861,7 +1861,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
 
         l1_table = g_realloc(l1_table, l1_sectors * BDRV_SECTOR_SIZE);
 
-        ret = bdrv_read(bs->file->bs,
+        ret = bdrv_read(bs->file,
                         s->snapshots[i].l1_table_offset / BDRV_SECTOR_SIZE,
                         (void *)l1_table, l1_sectors);
         if (ret < 0) {
diff --git a/block/vdi.c b/block/vdi.c
index 7d9ab9c..46a3436 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -403,7 +403,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
 
     logout("\n");
 
-    ret = bdrv_read(bs->file->bs, 0, (uint8_t *)&header, 1);
+    ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
     if (ret < 0) {
         goto fail;
     }
@@ -500,7 +500,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_read(bs->file->bs, s->bmap_sector, (uint8_t *)s->bmap,
+    ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
                     bmap_size);
     if (ret < 0) {
         goto fail_free_bmap;
diff --git a/block/vvfat.c b/block/vvfat.c
index 2eb2536..1d287a2 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1392,7 +1392,7 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
             if (bdrv_is_allocated(s->qcow->bs, sector_num, nb_sectors-i, &n)) {
                 DLOG(fprintf(stderr, "sectors %d+%d allocated\n",
                              (int)sector_num, n));
-                if (bdrv_read(s->qcow->bs, sector_num, buf + i*0x200, n)) {
+                if (bdrv_read(s->qcow, sector_num, buf + i*0x200, n)) {
                     return -1;
                 }
                 i += n - 1;
diff --git a/include/block/block.h b/include/block/block.h
index c6891d7..47c0177 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -226,7 +226,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                         BlockReopenQueue *queue, Error **errp);
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
-int bdrv_read(BlockDriverState *bs, int64_t sector_num,
+int bdrv_read(BdrvChild *child, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
 int bdrv_write(BlockDriverState *bs, int64_t sector_num,
                const uint8_t *buf, int nb_sectors);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 11/17] block: Convert bdrv_read() to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c            | 4 ++--
>  block/qcow2-cluster.c | 6 +++---
>  block/vdi.c           | 4 ++--
>  block/vvfat.c         | 2 +-
>  include/block/block.h | 2 +-
>  5 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c             |  5 +++--
 block/qcow.c           | 10 +++++++++-
 block/qcow2-cluster.c  |  2 +-
 block/qcow2-refcount.c |  2 +-
 block/qcow2.c          | 10 +++++++++-
 block/vdi.c            |  4 ++--
 block/vvfat.c          |  5 ++---
 include/block/block.h  |  2 +-
 8 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/block/io.c b/block/io.c
index d14c982..80ecfdc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -646,10 +646,11 @@ int bdrv_read(BdrvChild *child, int64_t sector_num,
   -EINVAL      Invalid sector number or nb_sectors
   -EACCES      Trying to write a read-only device
 */
-int bdrv_write(BlockDriverState *bs, int64_t sector_num,
+int bdrv_write(BdrvChild *child, int64_t sector_num,
                const uint8_t *buf, int nb_sectors)
 {
-    return bdrv_rw_co(bs, sector_num, (uint8_t *)buf, nb_sectors, true, 0);
+    return bdrv_rw_co(child->bs, sector_num, (uint8_t *)buf, nb_sectors,
+                      true, 0);
 }
 
 int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
diff --git a/block/qcow.c b/block/qcow.c
index e09827b..c80df78 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -969,7 +969,15 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
 
     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
-        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
+        QEMUIOVector qiov;
+        struct iovec iov = {
+            .iov_base   = (uint8_t*) buf,
+            .iov_len    = nb_sectors * BDRV_SECTOR_SIZE,
+        };
+        qemu_iovec_init_external(&qiov, &iov, 1);
+
+        ret = bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_sectors,
+                                      &qiov);
         if (ret < 0) {
             goto fail;
         }
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ed9832a..c2b78ad 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1786,7 +1786,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                     goto fail;
                 }
 
-                ret = bdrv_write(bs->file->bs, l2_offset / BDRV_SECTOR_SIZE,
+                ret = bdrv_write(bs->file, l2_offset / BDRV_SECTOR_SIZE,
                                  (void *)l2_table, s->cluster_sectors);
                 if (ret < 0) {
                     goto fail;
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 66f187a..7d391a5 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2101,7 +2101,7 @@ write_refblocks:
         on_disk_refblock = (void *)((char *) *refcount_table +
                                     refblock_index * s->cluster_size);
 
-        ret = bdrv_write(bs->file->bs, refblock_offset / BDRV_SECTOR_SIZE,
+        ret = bdrv_write(bs->file, refblock_offset / BDRV_SECTOR_SIZE,
                          on_disk_refblock, s->cluster_sectors);
         if (ret < 0) {
             fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
diff --git a/block/qcow2.c b/block/qcow2.c
index 4718f82..bc78af3 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2597,7 +2597,15 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
 
     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
-        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
+        QEMUIOVector qiov;
+        struct iovec iov = {
+            .iov_base   = (uint8_t*) buf,
+            .iov_len    = nb_sectors * BDRV_SECTOR_SIZE,
+        };
+        qemu_iovec_init_external(&qiov, &iov, 1);
+
+        ret = bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_sectors,
+                                      &qiov);
         if (ret < 0) {
             goto fail;
         }
diff --git a/block/vdi.c b/block/vdi.c
index 46a3436..b2871ca 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -719,7 +719,7 @@ vdi_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         assert(VDI_IS_ALLOCATED(bmap_first));
         *header = s->header;
         vdi_header_to_le(header);
-        ret = bdrv_write(bs->file->bs, 0, block, 1);
+        ret = bdrv_write(bs->file, 0, block, 1);
         g_free(block);
         block = NULL;
 
@@ -737,7 +737,7 @@ vdi_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
         logout("will write %u block map sectors starting from entry %u\n",
                n_sectors, bmap_first);
-        ret = bdrv_write(bs->file->bs, offset, base, n_sectors);
+        ret = bdrv_write(bs->file, offset, base, n_sectors);
     }
 
     return ret;
diff --git a/block/vvfat.c b/block/vvfat.c
index 1d287a2..a81e040 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1833,8 +1833,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
                         if (res) {
                             return -1;
                         }
-                        res = bdrv_write(s->qcow->bs, offset,
-                                         s->cluster_buffer, 1);
+                        res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1);
                         if (res) {
                             return -2;
                         }
@@ -2888,7 +2887,7 @@ DLOG(checkpoint());
      * Use qcow backend. Commit later.
      */
 DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
-    ret = bdrv_write(s->qcow->bs, sector_num, buf, nb_sectors);
+    ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
     if (ret < 0) {
 	fprintf(stderr, "Error writing to qcow backend\n");
 	return ret;
diff --git a/include/block/block.h b/include/block/block.h
index 47c0177..7eadfaf 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -228,7 +228,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 int bdrv_read(BdrvChild *child, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
-int bdrv_write(BlockDriverState *bs, int64_t sector_num,
+int bdrv_write(BdrvChild *child, int64_t sector_num,
                const uint8_t *buf, int nb_sectors);
 int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
                        int count, BdrvRequestFlags flags);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c             |  5 +++--
>  block/qcow.c           | 10 +++++++++-
>  block/qcow2-cluster.c  |  2 +-
>  block/qcow2-refcount.c |  2 +-
>  block/qcow2.c          | 10 +++++++++-
>  block/vdi.c            |  4 ++--
>  block/vvfat.c          |  5 ++---
>  include/block/block.h  |  2 +-
>  8 files changed, 28 insertions(+), 12 deletions(-)

Because I think this is better than saying "I won't give an R-b because
of (1), (2)..." at the bottom:

Review key:
[o] -- completely optional suggestion
[r] -- request, optional, but I'll wait with an R-b until I get feedback
[x] -- requires a fix

[...]

> diff --git a/block/qcow.c b/block/qcow.c
> index e09827b..c80df78 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -969,7 +969,15 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
>  
>      if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
>          /* could not compress: write normal cluster */
> -        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
> +        QEMUIOVector qiov;
> +        struct iovec iov = {
> +            .iov_base   = (uint8_t*) buf,

[o] ERROR: "(foo*)" should be "(foo *)"

!!11!1elf

> +            .iov_len    = nb_sectors * BDRV_SECTOR_SIZE,
> +        };
> +        qemu_iovec_init_external(&qiov, &iov, 1);
> +
> +        ret = bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_sectors,
> +                                      &qiov);

[r] I'd use nb_sectors instead of s->cluster_sectors (or use
s->cluster_sectors above), I think it should be the same variable in
both places.

[o] Also, but this is a much weaker proposal, I'd have preferred a
direct call to qcow_co_writev(). But maybe that's just me.

>          if (ret < 0) {
>              goto fail;
>          }

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4718f82..bc78af3 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2597,7 +2597,15 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>  
>      if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
>          /* could not compress: write normal cluster */
> -        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
> +        QEMUIOVector qiov;
> +        struct iovec iov = {
> +            .iov_base   = (uint8_t*) buf,

[o] ERROR: "(foo*)" should be "(foo *)"

> +            .iov_len    = nb_sectors * BDRV_SECTOR_SIZE,
> +        };
> +        qemu_iovec_init_external(&qiov, &iov, 1);
> +
> +        ret = bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_sectors,
> +                                      &qiov);

[x] I don't think qcow2 has a bdrv_co_writev().

($ dd if=/dev/urandom of=foo.raw bs=64k count=1
 $ ./qemu-img convert -c -O qcow2 foo.raw foo.qcow2
[1]    11808 segmentation fault (core dumped))

[r] Same as in qcow.c, I'd rather use nb_sectors instead of
s->cluster_sectors.

[o] Same as in qcow.c, I'd call qcow2_co_pwritev() directly. Doing so
would have prevented [x].

Max

>          if (ret < 0) {
>              goto fail;
>          }


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

Re: [Qemu-devel] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild

[Max Reitz]

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

On 27.06.2016 15:44, Max Reitz wrote:
> On 21.06.2016 11:21, Kevin Wolf wrote:
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block/io.c             |  5 +++--
>>  block/qcow.c           | 10 +++++++++-
>>  block/qcow2-cluster.c  |  2 +-
>>  block/qcow2-refcount.c |  2 +-
>>  block/qcow2.c          | 10 +++++++++-
>>  block/vdi.c            |  4 ++--
>>  block/vvfat.c          |  5 ++---
>>  include/block/block.h  |  2 +-
>>  8 files changed, 28 insertions(+), 12 deletions(-)
> 
> Because I think this is better than saying "I won't give an R-b because
> of (1), (2)..." at the bottom:
> 
> Review key:
> [o] -- completely optional suggestion
> [r] -- request, optional, but I'll wait with an R-b until I get feedback
> [x] -- requires a fix
> 
> [...]
> 
>> diff --git a/block/qcow.c b/block/qcow.c
>> index e09827b..c80df78 100644
>> --- a/block/qcow.c
>> +++ b/block/qcow.c
>> @@ -969,7 +969,15 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>  
>>      if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
>>          /* could not compress: write normal cluster */
>> -        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
>> +        QEMUIOVector qiov;
>> +        struct iovec iov = {
>> +            .iov_base   = (uint8_t*) buf,
> 
> [o] ERROR: "(foo*)" should be "(foo *)"
> 
> !!11!1elf
> 
>> +            .iov_len    = nb_sectors * BDRV_SECTOR_SIZE,
>> +        };
>> +        qemu_iovec_init_external(&qiov, &iov, 1);
>> +
>> +        ret = bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_sectors,
>> +                                      &qiov);
> 
> [r] I'd use nb_sectors instead of s->cluster_sectors (or use
> s->cluster_sectors above), I think it should be the same variable in
> both places.
> 
> [o] Also, but this is a much weaker proposal, I'd have preferred a
> direct call to qcow_co_writev(). But maybe that's just me.

[x] qcow_write_compressed() is not a coroutine_fn, resulting in:

qemu-img: ./util/qemu-coroutine-lock.c:143: qemu_co_mutex_unlock:
Assertion `qemu_in_coroutine()' failed.

>>          if (ret < 0) {
>>              goto fail;
>>          }
> 
> [...]
> 
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 4718f82..bc78af3 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2597,7 +2597,15 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>  
>>      if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
>>          /* could not compress: write normal cluster */
>> -        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
>> +        QEMUIOVector qiov;
>> +        struct iovec iov = {
>> +            .iov_base   = (uint8_t*) buf,
> 
> [o] ERROR: "(foo*)" should be "(foo *)"
> 
>> +            .iov_len    = nb_sectors * BDRV_SECTOR_SIZE,
>> +        };
>> +        qemu_iovec_init_external(&qiov, &iov, 1);
>> +
>> +        ret = bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_sectors,
>> +                                      &qiov);
> 
> [x] I don't think qcow2 has a bdrv_co_writev().
> 
> ($ dd if=/dev/urandom of=foo.raw bs=64k count=1
>  $ ./qemu-img convert -c -O qcow2 foo.raw foo.qcow2
> [1]    11808 segmentation fault (core dumped))
> 
> [r] Same as in qcow.c, I'd rather use nb_sectors instead of
> s->cluster_sectors.
> 
> [o] Same as in qcow.c, I'd call qcow2_co_pwritev() directly. Doing so
> would have prevented [x].

[x] Same as in qcow.c, qcow2_write_compressed() is not a coroutine_fn
either.

> Max
> 
>>          if (ret < 0) {
>>              goto fail;
>>          }
> 



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

[Qemu-devel] [PATCH v2 12/17] block: Convert bdrv_write() to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
---

This patch contains non-trivial fixes, so I think it's worth sending out a v2
for it even though I already applied the series. I added a coroutine entry
wrapper qcow(2)_write that can be used from .bdrv_write_compressed. These
wrappers will soon disappear again when .bdrv_write_compressed is changed into
.bdrv_co_pwritev_compressed (Pavel Butsykin's backup compression series).

 block/io.c             |  5 +++--
 block/qcow.c           | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 block/qcow2-cluster.c  |  2 +-
 block/qcow2-refcount.c |  2 +-
 block/qcow2.c          | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 block/vdi.c            |  4 ++--
 block/vvfat.c          |  5 ++---
 include/block/block.h  |  2 +-
 8 files changed, 100 insertions(+), 12 deletions(-)

diff --git a/block/io.c b/block/io.c
index 6dfc0eb..2e04a80 100644
--- a/block/io.c
+++ b/block/io.c
@@ -642,10 +642,11 @@ int bdrv_read(BdrvChild *child, int64_t sector_num,
   -EINVAL      Invalid sector number or nb_sectors
   -EACCES      Trying to write a read-only device
 */
-int bdrv_write(BlockDriverState *bs, int64_t sector_num,
+int bdrv_write(BdrvChild *child, int64_t sector_num,
                const uint8_t *buf, int nb_sectors)
 {
-    return bdrv_rw_co(bs, sector_num, (uint8_t *)buf, nb_sectors, true, 0);
+    return bdrv_rw_co(child->bs, sector_num, (uint8_t *)buf, nb_sectors,
+                      true, 0);
 }
 
 int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
diff --git a/block/qcow.c b/block/qcow.c
index 0db43f8..674595e 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -913,6 +913,49 @@ static int qcow_make_empty(BlockDriverState *bs)
     return 0;
 }
 
+typedef struct QcowWriteCo {
+    BlockDriverState *bs;
+    int64_t sector_num;
+    const uint8_t *buf;
+    int nb_sectors;
+    int ret;
+} QcowWriteCo;
+
+static void qcow_write_co_entry(void *opaque)
+{
+    QcowWriteCo *co = opaque;
+    QEMUIOVector qiov;
+
+    struct iovec iov = (struct iovec) {
+        .iov_base   = (uint8_t*) co->buf,
+        .iov_len    = co->nb_sectors * BDRV_SECTOR_SIZE,
+    };
+    qemu_iovec_init_external(&qiov, &iov, 1);
+
+    co->ret = qcow_co_writev(co->bs, co->sector_num, co->nb_sectors, &qiov);
+}
+
+/* Wrapper for non-coroutine contexts */
+static int qcow_write(BlockDriverState *bs, int64_t sector_num,
+                      const uint8_t *buf, int nb_sectors)
+{
+    Coroutine *co;
+    AioContext *aio_context = bdrv_get_aio_context(bs);
+    QcowWriteCo data = {
+        .bs         = bs,
+        .sector_num = sector_num,
+        .buf        = buf,
+        .nb_sectors = nb_sectors,
+        .ret        = -EINPROGRESS,
+    };
+    co = qemu_coroutine_create(qcow_write_co_entry);
+    qemu_coroutine_enter(co, &data);
+    while (data.ret == -EINPROGRESS) {
+        aio_poll(aio_context, true);
+    }
+    return data.ret;
+}
+
 /* XXX: put compressed sectors first, then all the cluster aligned
    tables to avoid losing bytes in alignment */
 static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
@@ -969,7 +1012,7 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
 
     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
-        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
+        ret = qcow_write(bs, sector_num, buf, s->cluster_sectors);
         if (ret < 0) {
             goto fail;
         }
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index c1e9eee..a2490d7 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1784,7 +1784,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                     goto fail;
                 }
 
-                ret = bdrv_write(bs->file->bs, l2_offset / BDRV_SECTOR_SIZE,
+                ret = bdrv_write(bs->file, l2_offset / BDRV_SECTOR_SIZE,
                                  (void *)l2_table, s->cluster_sectors);
                 if (ret < 0) {
                     goto fail;
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3bef410..12e7e6b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2098,7 +2098,7 @@ write_refblocks:
         on_disk_refblock = (void *)((char *) *refcount_table +
                                     refblock_index * s->cluster_size);
 
-        ret = bdrv_write(bs->file->bs, refblock_offset / BDRV_SECTOR_SIZE,
+        ret = bdrv_write(bs->file, refblock_offset / BDRV_SECTOR_SIZE,
                          on_disk_refblock, s->cluster_sectors);
         if (ret < 0) {
             fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
diff --git a/block/qcow2.c b/block/qcow2.c
index 0178931..cd9c27b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2533,6 +2533,51 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
     return 0;
 }
 
+typedef struct Qcow2WriteCo {
+    BlockDriverState *bs;
+    int64_t sector_num;
+    const uint8_t *buf;
+    int nb_sectors;
+    int ret;
+} Qcow2WriteCo;
+
+static void qcow2_write_co_entry(void *opaque)
+{
+    Qcow2WriteCo *co = opaque;
+    QEMUIOVector qiov;
+    uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE;
+    uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE;
+
+    struct iovec iov = (struct iovec) {
+        .iov_base   = (uint8_t*) co->buf,
+        .iov_len    = bytes,
+    };
+    qemu_iovec_init_external(&qiov, &iov, 1);
+
+    co->ret = qcow2_co_pwritev(co->bs, offset, bytes, &qiov, 0);
+}
+
+/* Wrapper for non-coroutine contexts */
+static int qcow2_write(BlockDriverState *bs, int64_t sector_num,
+                       const uint8_t *buf, int nb_sectors)
+{
+    Coroutine *co;
+    AioContext *aio_context = bdrv_get_aio_context(bs);
+    Qcow2WriteCo data = {
+        .bs         = bs,
+        .sector_num = sector_num,
+        .buf        = buf,
+        .nb_sectors = nb_sectors,
+        .ret        = -EINPROGRESS,
+    };
+    co = qemu_coroutine_create(qcow2_write_co_entry);
+    qemu_coroutine_enter(co, &data);
+    while (data.ret == -EINPROGRESS) {
+        aio_poll(aio_context, true);
+    }
+    return data.ret;
+}
+
 /* XXX: put compressed sectors first, then all the cluster aligned
    tables to avoid losing bytes in alignment */
 static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
@@ -2596,7 +2641,7 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
 
     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
-        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
+        ret = qcow2_write(bs, sector_num, buf, s->cluster_sectors);
         if (ret < 0) {
             goto fail;
         }
diff --git a/block/vdi.c b/block/vdi.c
index 46a3436..b2871ca 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -719,7 +719,7 @@ vdi_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         assert(VDI_IS_ALLOCATED(bmap_first));
         *header = s->header;
         vdi_header_to_le(header);
-        ret = bdrv_write(bs->file->bs, 0, block, 1);
+        ret = bdrv_write(bs->file, 0, block, 1);
         g_free(block);
         block = NULL;
 
@@ -737,7 +737,7 @@ vdi_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
         logout("will write %u block map sectors starting from entry %u\n",
                n_sectors, bmap_first);
-        ret = bdrv_write(bs->file->bs, offset, base, n_sectors);
+        ret = bdrv_write(bs->file, offset, base, n_sectors);
     }
 
     return ret;
diff --git a/block/vvfat.c b/block/vvfat.c
index 5f980bb..c3f24c6 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1834,8 +1834,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
                         if (res) {
                             return -1;
                         }
-                        res = bdrv_write(s->qcow->bs, offset,
-                                         s->cluster_buffer, 1);
+                        res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1);
                         if (res) {
                             return -2;
                         }
@@ -2889,7 +2888,7 @@ DLOG(checkpoint());
      * Use qcow backend. Commit later.
      */
 DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
-    ret = bdrv_write(s->qcow->bs, sector_num, buf, nb_sectors);
+    ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
     if (ret < 0) {
 	fprintf(stderr, "Error writing to qcow backend\n");
 	return ret;
diff --git a/include/block/block.h b/include/block/block.h
index b6744ab..ea17936 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -228,7 +228,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 int bdrv_read(BdrvChild *child, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
-int bdrv_write(BlockDriverState *bs, int64_t sector_num,
+int bdrv_write(BdrvChild *child, int64_t sector_num,
                const uint8_t *buf, int nb_sectors);
 int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
                        int count, BdrvRequestFlags flags);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 12/17] block: Convert bdrv_write() to BdrvChild

[Max Reitz]

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

On 29.06.2016 14:11, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> 
> This patch contains non-trivial fixes, so I think it's worth sending out a v2
> for it even though I already applied the series. I added a coroutine entry
> wrapper qcow(2)_write that can be used from .bdrv_write_compressed. These
> wrappers will soon disappear again when .bdrv_write_compressed is changed into
> .bdrv_co_pwritev_compressed (Pavel Butsykin's backup compression series).
> 
>  block/io.c             |  5 +++--
>  block/qcow.c           | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  block/qcow2-cluster.c  |  2 +-
>  block/qcow2-refcount.c |  2 +-
>  block/qcow2.c          | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  block/vdi.c            |  4 ++--
>  block/vvfat.c          |  5 ++---
>  include/block/block.h  |  2 +-
>  8 files changed, 100 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0178931..cd9c27b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2533,6 +2533,51 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>      return 0;
>  }
>  
> +typedef struct Qcow2WriteCo {
> +    BlockDriverState *bs;
> +    int64_t sector_num;
> +    const uint8_t *buf;
> +    int nb_sectors;
> +    int ret;
> +} Qcow2WriteCo;
> +
> +static void qcow2_write_co_entry(void *opaque)
> +{
> +    Qcow2WriteCo *co = opaque;
> +    QEMUIOVector qiov;
> +    uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE;
> +    uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE;

It doesn't make much sense to make this a uint64_t, and I'm afraid
Coverity will complain about it... It's not wrong, though, but an int
would have been more "honest".

Max

> +
> +    struct iovec iov = (struct iovec) {
> +        .iov_base   = (uint8_t*) co->buf,
> +        .iov_len    = bytes,
> +    };
> +    qemu_iovec_init_external(&qiov, &iov, 1);
> +
> +    co->ret = qcow2_co_pwritev(co->bs, offset, bytes, &qiov, 0);
> +}


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

Re: [Qemu-devel] [PATCH v2 12/17] block: Convert bdrv_write() to BdrvChild

[Kevin Wolf]

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

Am 29.06.2016 um 17:22 hat Max Reitz geschrieben:
> On 29.06.2016 14:11, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > 
> > This patch contains non-trivial fixes, so I think it's worth sending out a v2
> > for it even though I already applied the series. I added a coroutine entry
> > wrapper qcow(2)_write that can be used from .bdrv_write_compressed. These
> > wrappers will soon disappear again when .bdrv_write_compressed is changed into
> > .bdrv_co_pwritev_compressed (Pavel Butsykin's backup compression series).
> > 
> >  block/io.c             |  5 +++--
> >  block/qcow.c           | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> >  block/qcow2-cluster.c  |  2 +-
> >  block/qcow2-refcount.c |  2 +-
> >  block/qcow2.c          | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  block/vdi.c            |  4 ++--
> >  block/vvfat.c          |  5 ++---
> >  include/block/block.h  |  2 +-
> >  8 files changed, 100 insertions(+), 12 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> [...]
> 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 0178931..cd9c27b 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2533,6 +2533,51 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
> >      return 0;
> >  }
> >  
> > +typedef struct Qcow2WriteCo {
> > +    BlockDriverState *bs;
> > +    int64_t sector_num;
> > +    const uint8_t *buf;
> > +    int nb_sectors;
> > +    int ret;
> > +} Qcow2WriteCo;
> > +
> > +static void qcow2_write_co_entry(void *opaque)
> > +{
> > +    Qcow2WriteCo *co = opaque;
> > +    QEMUIOVector qiov;
> > +    uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE;
> > +    uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE;
> 
> It doesn't make much sense to make this a uint64_t, and I'm afraid
> Coverity will complain about it... It's not wrong, though, but an int
> would have been more "honest".

Hm, just copied from vmdk... Anyway, you right that we don't really need
uint64_t here because of BDRV_REQUEST_MAX_SECTORS, but BDRV_SECTOR_SIZE
is unsigned long long, so at least this is a proper 64 bit calculation
and Coverity should stay silent.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v2 12/17] block: Convert bdrv_write() to BdrvChild

[Max Reitz]

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

On 29.06.2016 17:33, Kevin Wolf wrote:
> Am 29.06.2016 um 17:22 hat Max Reitz geschrieben:
>> On 29.06.2016 14:11, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>
>>> This patch contains non-trivial fixes, so I think it's worth sending out a v2
>>> for it even though I already applied the series. I added a coroutine entry
>>> wrapper qcow(2)_write that can be used from .bdrv_write_compressed. These
>>> wrappers will soon disappear again when .bdrv_write_compressed is changed into
>>> .bdrv_co_pwritev_compressed (Pavel Butsykin's backup compression series).
>>>
>>>  block/io.c             |  5 +++--
>>>  block/qcow.c           | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>>  block/qcow2-cluster.c  |  2 +-
>>>  block/qcow2-refcount.c |  2 +-
>>>  block/qcow2.c          | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>>>  block/vdi.c            |  4 ++--
>>>  block/vvfat.c          |  5 ++---
>>>  include/block/block.h  |  2 +-
>>>  8 files changed, 100 insertions(+), 12 deletions(-)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> [...]
>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 0178931..cd9c27b 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -2533,6 +2533,51 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>>>      return 0;
>>>  }
>>>  
>>> +typedef struct Qcow2WriteCo {
>>> +    BlockDriverState *bs;
>>> +    int64_t sector_num;
>>> +    const uint8_t *buf;
>>> +    int nb_sectors;
>>> +    int ret;
>>> +} Qcow2WriteCo;
>>> +
>>> +static void qcow2_write_co_entry(void *opaque)
>>> +{
>>> +    Qcow2WriteCo *co = opaque;
>>> +    QEMUIOVector qiov;
>>> +    uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE;
>>> +    uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE;
>>
>> It doesn't make much sense to make this a uint64_t, and I'm afraid
>> Coverity will complain about it... It's not wrong, though, but an int
>> would have been more "honest".
> 
> Hm, just copied from vmdk... Anyway, you right that we don't really need
> uint64_t here because of BDRV_REQUEST_MAX_SECTORS, but BDRV_SECTOR_SIZE
> is unsigned long long, so at least this is a proper 64 bit calculation
> and Coverity should stay silent.

Oh, right, I missed that. That is actually very clever, I should
remember that.

Max


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

[Qemu-devel] [PATCH 13/17] block: Convert bdrv_pread(v) to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                |  7 ++++---
 block/bochs.c          |  6 +++---
 block/cloop.c          |  8 ++++----
 block/crypto.c         |  2 +-
 block/dmg.c            | 21 +++++++++++----------
 block/io.c             |  8 ++++----
 block/parallels.c      |  4 ++--
 block/qcow.c           | 10 +++++-----
 block/qcow2-cache.c    |  2 +-
 block/qcow2-refcount.c | 12 ++++++------
 block/qcow2-snapshot.c | 12 ++++++------
 block/qcow2.c          | 16 ++++++++--------
 block/qed.c            |  6 +++---
 block/vhdx-log.c       |  8 ++++----
 block/vhdx.c           | 38 +++++++++++++++++++++++---------------
 block/vmdk.c           | 36 +++++++++++++++++-------------------
 block/vpc.c            |  8 ++++----
 include/block/block.h  |  5 ++---
 18 files changed, 108 insertions(+), 101 deletions(-)

diff --git a/block.c b/block.c
index 160324a..31e781f 100644
--- a/block.c
+++ b/block.c
@@ -540,9 +540,10 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
     return drv;
 }
 
-static int find_image_format(BlockDriverState *bs, const char *filename,
+static int find_image_format(BdrvChild *file, const char *filename,
                              BlockDriver **pdrv, Error **errp)
 {
+    BlockDriverState *bs = file->bs;
     BlockDriver *drv;
     uint8_t buf[BLOCK_PROBE_BUF_SIZE];
     int ret = 0;
@@ -553,7 +554,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
         return ret;
     }
 
-    ret = bdrv_pread(bs, 0, buf, sizeof(buf));
+    ret = bdrv_pread(file, 0, buf, sizeof(buf));
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read image for determining its "
                          "format");
@@ -1657,7 +1658,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
     /* Image format probing */
     bs->probed = !drv;
     if (!drv && file) {
-        ret = find_image_format(file->bs, filename, &drv, &local_err);
+        ret = find_image_format(file, filename, &drv, &local_err);
         if (ret < 0) {
             goto fail;
         }
diff --git a/block/bochs.c b/block/bochs.c
index 6c8d0f3..e9323ec7 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -107,7 +107,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
     bs->read_only = 1; // no write support yet
     bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */
 
-    ret = bdrv_pread(bs->file->bs, 0, &bochs, sizeof(bochs));
+    ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
         return ret;
     }
@@ -141,7 +141,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return -ENOMEM;
     }
 
-    ret = bdrv_pread(bs->file->bs, le32_to_cpu(bochs.header), s->catalog_bitmap,
+    ret = bdrv_pread(bs->file, le32_to_cpu(bochs.header), s->catalog_bitmap,
                      s->catalog_size * 4);
     if (ret < 0) {
         goto fail;
@@ -210,7 +210,7 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
         (s->extent_blocks + s->bitmap_blocks));
 
     /* read in bitmap for current extent */
-    ret = bdrv_pread(bs->file->bs, bitmap_offset + (extent_offset / 8),
+    ret = bdrv_pread(bs->file, bitmap_offset + (extent_offset / 8),
                      &bitmap_entry, 1);
     if (ret < 0) {
         return ret;
diff --git a/block/cloop.c b/block/cloop.c
index ea5a92b..3ddad96 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -70,7 +70,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
     bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */
 
     /* read header */
-    ret = bdrv_pread(bs->file->bs, 128, &s->block_size, 4);
+    ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
     if (ret < 0) {
         return ret;
     }
@@ -96,7 +96,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    ret = bdrv_pread(bs->file->bs, 128 + 4, &s->n_blocks, 4);
+    ret = bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4);
     if (ret < 0) {
         return ret;
     }
@@ -127,7 +127,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return -ENOMEM;
     }
 
-    ret = bdrv_pread(bs->file->bs, 128 + 4 + 4, s->offsets, offsets_size);
+    ret = bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size);
     if (ret < 0) {
         goto fail;
     }
@@ -207,7 +207,7 @@ static inline int cloop_read_block(BlockDriverState *bs, int block_num)
         int ret;
         uint32_t bytes = s->offsets[block_num + 1] - s->offsets[block_num];
 
-        ret = bdrv_pread(bs->file->bs, s->offsets[block_num],
+        ret = bdrv_pread(bs->file, s->offsets[block_num],
                          s->compressed_block, bytes);
         if (ret != bytes) {
             return -1;
diff --git a/block/crypto.c b/block/crypto.c
index 844ed5c..a127649 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -64,7 +64,7 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
     BlockDriverState *bs = opaque;
     ssize_t ret;
 
-    ret = bdrv_pread(bs->file->bs, offset, buf, buflen);
+    ret = bdrv_pread(bs->file, offset, buf, buflen);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read encryption header");
         return ret;
diff --git a/block/dmg.c b/block/dmg.c
index 06eb513..9ced2c4 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -86,7 +86,7 @@ static int read_uint64(BlockDriverState *bs, int64_t offset, uint64_t *result)
     uint64_t buffer;
     int ret;
 
-    ret = bdrv_pread(bs->file->bs, offset, &buffer, 8);
+    ret = bdrv_pread(bs->file, offset, &buffer, 8);
     if (ret < 0) {
         return ret;
     }
@@ -100,7 +100,7 @@ static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result)
     uint32_t buffer;
     int ret;
 
-    ret = bdrv_pread(bs->file->bs, offset, &buffer, 4);
+    ret = bdrv_pread(bs->file, offset, &buffer, 4);
     if (ret < 0) {
         return ret;
     }
@@ -153,8 +153,9 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
     }
 }
 
-static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
+static int64_t dmg_find_koly_offset(BdrvChild *file, Error **errp)
 {
+    BlockDriverState *file_bs = file->bs;
     int64_t length;
     int64_t offset = 0;
     uint8_t buffer[515];
@@ -178,7 +179,7 @@ static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
         offset = length - 511 - 512;
     }
     length = length < 515 ? length : 515;
-    ret = bdrv_pread(file_bs, offset, buffer, length);
+    ret = bdrv_pread(file, offset, buffer, length);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Failed while reading UDIF trailer");
         return ret;
@@ -355,7 +356,7 @@ static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
         offset += 4;
 
         buffer = g_realloc(buffer, count);
-        ret = bdrv_pread(bs->file->bs, offset, buffer, count);
+        ret = bdrv_pread(bs->file, offset, buffer, count);
         if (ret < 0) {
             goto fail;
         }
@@ -392,7 +393,7 @@ static int dmg_read_plist_xml(BlockDriverState *bs, DmgHeaderState *ds,
 
     buffer = g_malloc(info_length + 1);
     buffer[info_length] = '\0';
-    ret = bdrv_pread(bs->file->bs, info_begin, buffer, info_length);
+    ret = bdrv_pread(bs->file, info_begin, buffer, info_length);
     if (ret != info_length) {
         ret = -EINVAL;
         goto fail;
@@ -449,7 +450,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     ds.max_sectors_per_chunk = 1;
 
     /* locate the UDIF trailer */
-    offset = dmg_find_koly_offset(bs->file->bs, errp);
+    offset = dmg_find_koly_offset(bs->file, errp);
     if (offset < 0) {
         ret = offset;
         goto fail;
@@ -595,7 +596,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
         case 0x80000005: { /* zlib compressed */
             /* we need to buffer, because only the chunk as whole can be
              * inflated. */
-            ret = bdrv_pread(bs->file->bs, s->offsets[chunk],
+            ret = bdrv_pread(bs->file, s->offsets[chunk],
                              s->compressed_chunk, s->lengths[chunk]);
             if (ret != s->lengths[chunk]) {
                 return -1;
@@ -619,7 +620,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
         case 0x80000006: /* bzip2 compressed */
             /* we need to buffer, because only the chunk as whole can be
              * inflated. */
-            ret = bdrv_pread(bs->file->bs, s->offsets[chunk],
+            ret = bdrv_pread(bs->file, s->offsets[chunk],
                              s->compressed_chunk, s->lengths[chunk]);
             if (ret != s->lengths[chunk]) {
                 return -1;
@@ -644,7 +645,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
             break;
 #endif /* CONFIG_BZIP2 */
         case 1: /* copy */
-            ret = bdrv_pread(bs->file->bs, s->offsets[chunk],
+            ret = bdrv_pread(bs->file, s->offsets[chunk],
                              s->uncompressed_chunk, s->lengths[chunk]);
             if (ret != s->lengths[chunk]) {
                 return -1;
diff --git a/block/io.c b/block/io.c
index 80ecfdc..1256151 100644
--- a/block/io.c
+++ b/block/io.c
@@ -713,11 +713,11 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
     }
 }
 
-int bdrv_preadv(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
+int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
 {
     int ret;
 
-    ret = bdrv_prwv_co(bs, offset, qiov, false, 0);
+    ret = bdrv_prwv_co(child->bs, offset, qiov, false, 0);
     if (ret < 0) {
         return ret;
     }
@@ -725,7 +725,7 @@ int bdrv_preadv(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
     return qiov->size;
 }
 
-int bdrv_pread(BlockDriverState *bs, int64_t offset, void *buf, int bytes)
+int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
 {
     QEMUIOVector qiov;
     struct iovec iov = {
@@ -738,7 +738,7 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset, void *buf, int bytes)
     }
 
     qemu_iovec_init_external(&qiov, &iov, 1);
-    return bdrv_preadv(bs, offset, &qiov);
+    return bdrv_preadv(child, offset, &qiov);
 }
 
 int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
diff --git a/block/parallels.c b/block/parallels.c
index 91ab61f..85c7cdc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -576,7 +576,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     Error *local_err = NULL;
     char *buf;
 
-    ret = bdrv_pread(bs->file->bs, 0, &ph, sizeof(ph));
+    ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
     if (ret < 0) {
         goto fail;
     }
@@ -631,7 +631,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         s->header_size = size;
     }
 
-    ret = bdrv_pread(bs->file->bs, 0, s->header, s->header_size);
+    ret = bdrv_pread(bs->file, 0, s->header, s->header_size);
     if (ret < 0) {
         goto fail;
     }
diff --git a/block/qcow.c b/block/qcow.c
index c80df78..8b1d70b 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -105,7 +105,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
     QCowHeader header;
 
-    ret = bdrv_pread(bs->file->bs, 0, &header, sizeof(header));
+    ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
     if (ret < 0) {
         goto fail;
     }
@@ -208,7 +208,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_pread(bs->file->bs, s->l1_table_offset, s->l1_table,
+    ret = bdrv_pread(bs->file, s->l1_table_offset, s->l1_table,
                s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
         goto fail;
@@ -239,7 +239,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
             ret = -EINVAL;
             goto fail;
         }
-        ret = bdrv_pread(bs->file->bs, header.backing_file_offset,
+        ret = bdrv_pread(bs->file, header.backing_file_offset,
                    bs->backing_file, len);
         if (ret < 0) {
             goto fail;
@@ -424,7 +424,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
                 s->l2_size * sizeof(uint64_t)) < 0)
             return 0;
     } else {
-        if (bdrv_pread(bs->file->bs, l2_offset, l2_table,
+        if (bdrv_pread(bs->file, l2_offset, l2_table,
                        s->l2_size * sizeof(uint64_t)) !=
             s->l2_size * sizeof(uint64_t))
             return 0;
@@ -565,7 +565,7 @@ static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
     if (s->cluster_cache_offset != coffset) {
         csize = cluster_offset >> (63 - s->cluster_bits);
         csize &= (s->cluster_size - 1);
-        ret = bdrv_pread(bs->file->bs, coffset, s->cluster_data, csize);
+        ret = bdrv_pread(bs->file, coffset, s->cluster_data, csize);
         if (ret != csize)
             return -1;
         if (decompress_buffer(s->cluster_cache, s->cluster_size,
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 580631c..5eba75f 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -357,7 +357,7 @@ static int qcow2_cache_do_get(BlockDriverState *bs, Qcow2Cache *c,
             BLKDBG_EVENT(bs->file, BLKDBG_L2_LOAD);
         }
 
-        ret = bdrv_pread(bs->file->bs, offset,
+        ret = bdrv_pread(bs->file, offset,
                          qcow2_cache_get_table_addr(bs, c, i),
                          s->cluster_size);
         if (ret < 0) {
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7d391a5..2d2bf8a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -104,7 +104,7 @@ int qcow2_refcount_init(BlockDriverState *bs)
             goto fail;
         }
         BLKDBG_EVENT(bs->file, BLKDBG_REFTABLE_LOAD);
-        ret = bdrv_pread(bs->file->bs, s->refcount_table_offset,
+        ret = bdrv_pread(bs->file, s->refcount_table_offset,
                          s->refcount_table, refcount_table_size2);
         if (ret < 0) {
             goto fail;
@@ -1073,7 +1073,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
         }
         l1_allocated = true;
 
-        ret = bdrv_pread(bs->file->bs, l1_table_offset, l1_table, l1_size2);
+        ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
         if (ret < 0) {
             goto fail;
         }
@@ -1385,7 +1385,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
     l2_size = s->l2_size * sizeof(uint64_t);
     l2_table = g_malloc(l2_size);
 
-    ret = bdrv_pread(bs->file->bs, l2_offset, l2_table, l2_size);
+    ret = bdrv_pread(bs->file, l2_offset, l2_table, l2_size);
     if (ret < 0) {
         fprintf(stderr, "ERROR: I/O error in check_refcounts_l2\n");
         res->check_errors++;
@@ -1517,7 +1517,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
             res->check_errors++;
             goto fail;
         }
-        ret = bdrv_pread(bs->file->bs, l1_table_offset, l1_table, l1_size2);
+        ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
         if (ret < 0) {
             fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
             res->check_errors++;
@@ -1615,7 +1615,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
             }
         }
 
-        ret = bdrv_pread(bs->file->bs, l2_offset, l2_table,
+        ret = bdrv_pread(bs->file, l2_offset, l2_table,
                          s->l2_size * sizeof(uint64_t));
         if (ret < 0) {
             fprintf(stderr, "ERROR: Could not read L2 table: %s\n",
@@ -2410,7 +2410,7 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
                 return -ENOMEM;
             }
 
-            ret = bdrv_pread(bs->file->bs, l1_ofs, l1, l1_sz2);
+            ret = bdrv_pread(bs->file, l1_ofs, l1, l1_sz2);
             if (ret < 0) {
                 g_free(l1);
                 return ret;
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 242fb21..2693ebf 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -67,7 +67,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
     for(i = 0; i < s->nb_snapshots; i++) {
         /* Read statically sized part of the snapshot header */
         offset = align_offset(offset, 8);
-        ret = bdrv_pread(bs->file->bs, offset, &h, sizeof(h));
+        ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
         if (ret < 0) {
             goto fail;
         }
@@ -86,7 +86,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
         name_size = be16_to_cpu(h.name_size);
 
         /* Read extra data */
-        ret = bdrv_pread(bs->file->bs, offset, &extra,
+        ret = bdrv_pread(bs->file, offset, &extra,
                          MIN(sizeof(extra), extra_data_size));
         if (ret < 0) {
             goto fail;
@@ -105,7 +105,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 
         /* Read snapshot ID */
         sn->id_str = g_malloc(id_str_size + 1);
-        ret = bdrv_pread(bs->file->bs, offset, sn->id_str, id_str_size);
+        ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size);
         if (ret < 0) {
             goto fail;
         }
@@ -114,7 +114,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 
         /* Read snapshot name */
         sn->name = g_malloc(name_size + 1);
-        ret = bdrv_pread(bs->file->bs, offset, sn->name, name_size);
+        ret = bdrv_pread(bs->file, offset, sn->name, name_size);
         if (ret < 0) {
             goto fail;
         }
@@ -512,7 +512,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
         goto fail;
     }
 
-    ret = bdrv_pread(bs->file->bs, sn->l1_table_offset,
+    ret = bdrv_pread(bs->file, sn->l1_table_offset,
                      sn_l1_table, sn_l1_bytes);
     if (ret < 0) {
         goto fail;
@@ -716,7 +716,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
         return -ENOMEM;
     }
 
-    ret = bdrv_pread(bs->file->bs, sn->l1_table_offset,
+    ret = bdrv_pread(bs->file, sn->l1_table_offset,
                      new_l1_table, new_l1_bytes);
     if (ret < 0) {
         error_setg(errp, "Failed to read l1 table for snapshot");
diff --git a/block/qcow2.c b/block/qcow2.c
index bc78af3..559f28e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -107,7 +107,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
         printf("attempting to read extended header in offset %lu\n", offset);
 #endif
 
-        ret = bdrv_pread(bs->file->bs, offset, &ext, sizeof(ext));
+        ret = bdrv_pread(bs->file, offset, &ext, sizeof(ext));
         if (ret < 0) {
             error_setg_errno(errp, -ret, "qcow2_read_extension: ERROR: "
                              "pread fail from offset %" PRIu64, offset);
@@ -135,7 +135,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                            sizeof(bs->backing_format));
                 return 2;
             }
-            ret = bdrv_pread(bs->file->bs, offset, bs->backing_format, ext.len);
+            ret = bdrv_pread(bs->file, offset, bs->backing_format, ext.len);
             if (ret < 0) {
                 error_setg_errno(errp, -ret, "ERROR: ext_backing_format: "
                                  "Could not read format name");
@@ -151,7 +151,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
         case QCOW2_EXT_MAGIC_FEATURE_TABLE:
             if (p_feature_table != NULL) {
                 void* feature_table = g_malloc0(ext.len + 2 * sizeof(Qcow2Feature));
-                ret = bdrv_pread(bs->file->bs, offset , feature_table, ext.len);
+                ret = bdrv_pread(bs->file, offset , feature_table, ext.len);
                 if (ret < 0) {
                     error_setg_errno(errp, -ret, "ERROR: ext_feature_table: "
                                      "Could not read table");
@@ -172,7 +172,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                 uext->len = ext.len;
                 QLIST_INSERT_HEAD(&s->unknown_header_ext, uext, next);
 
-                ret = bdrv_pread(bs->file->bs, offset , uext->data, uext->len);
+                ret = bdrv_pread(bs->file, offset , uext->data, uext->len);
                 if (ret < 0) {
                     error_setg_errno(errp, -ret, "ERROR: unknown extension: "
                                      "Could not read data");
@@ -817,7 +817,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     uint64_t ext_end;
     uint64_t l1_vm_state_index;
 
-    ret = bdrv_pread(bs->file->bs, 0, &header, sizeof(header));
+    ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read qcow2 header");
         goto fail;
@@ -892,7 +892,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     if (header.header_length > sizeof(header)) {
         s->unknown_header_fields_size = header.header_length - sizeof(header);
         s->unknown_header_fields = g_malloc(s->unknown_header_fields_size);
-        ret = bdrv_pread(bs->file->bs, sizeof(header), s->unknown_header_fields,
+        ret = bdrv_pread(bs->file, sizeof(header), s->unknown_header_fields,
                          s->unknown_header_fields_size);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not read unknown qcow2 header "
@@ -1069,7 +1069,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
             ret = -ENOMEM;
             goto fail;
         }
-        ret = bdrv_pread(bs->file->bs, s->l1_table_offset, s->l1_table,
+        ret = bdrv_pread(bs->file, s->l1_table_offset, s->l1_table,
                          s->l1_size * sizeof(uint64_t));
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not read L1 table");
@@ -1125,7 +1125,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
             ret = -EINVAL;
             goto fail;
         }
-        ret = bdrv_pread(bs->file->bs, header.backing_file_offset,
+        ret = bdrv_pread(bs->file, header.backing_file_offset,
                          bs->backing_file, len);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not read backing file name");
diff --git a/block/qed.c b/block/qed.c
index 9d3d588..d6d5e33 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -218,7 +218,7 @@ static bool qed_is_image_size_valid(uint64_t image_size, uint32_t cluster_size,
  *
  * The string is NUL-terminated.
  */
-static int qed_read_string(BlockDriverState *file, uint64_t offset, size_t n,
+static int qed_read_string(BdrvChild *file, uint64_t offset, size_t n,
                            char *buf, size_t buflen)
 {
     int ret;
@@ -389,7 +389,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
     s->bs = bs;
     QSIMPLEQ_INIT(&s->allocating_write_reqs);
 
-    ret = bdrv_pread(bs->file->bs, 0, &le_header, sizeof(le_header));
+    ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
     if (ret < 0) {
         return ret;
     }
@@ -446,7 +446,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
             return -EINVAL;
         }
 
-        ret = qed_read_string(bs->file->bs, s->header.backing_filename_offset,
+        ret = qed_read_string(bs->file, s->header.backing_filename_offset,
                               s->header.backing_filename_size, bs->backing_file,
                               sizeof(bs->backing_file));
         if (ret < 0) {
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 8ab7d22..1e3928f 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -84,7 +84,7 @@ static int vhdx_log_peek_hdr(BlockDriverState *bs, VHDXLogEntries *log,
 
     offset = log->offset + read;
 
-    ret = bdrv_pread(bs->file->bs, offset, hdr, sizeof(VHDXLogEntryHeader));
+    ret = bdrv_pread(bs->file, offset, hdr, sizeof(VHDXLogEntryHeader));
     if (ret < 0) {
         goto exit;
     }
@@ -144,7 +144,7 @@ static int vhdx_log_read_sectors(BlockDriverState *bs, VHDXLogEntries *log,
         }
         offset = log->offset + read;
 
-        ret = bdrv_pread(bs->file->bs, offset, buffer, VHDX_LOG_SECTOR_SIZE);
+        ret = bdrv_pread(bs->file, offset, buffer, VHDX_LOG_SECTOR_SIZE);
         if (ret < 0) {
             goto exit;
         }
@@ -945,7 +945,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
 
         if (i == 0 && leading_length) {
             /* partial sector at the front of the buffer */
-            ret = bdrv_pread(bs->file->bs, file_offset, merged_sector,
+            ret = bdrv_pread(bs->file, file_offset, merged_sector,
                              VHDX_LOG_SECTOR_SIZE);
             if (ret < 0) {
                 goto exit;
@@ -955,7 +955,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
             sector_write = merged_sector;
         } else if (i == sectors - 1 && trailing_length) {
             /* partial sector at the end of the buffer */
-            ret = bdrv_pread(bs->file->bs,
+            ret = bdrv_pread(bs->file,
                             file_offset,
                             merged_sector + trailing_length,
                             VHDX_LOG_SECTOR_SIZE - trailing_length);
diff --git a/block/vhdx.c b/block/vhdx.c
index b0f66de..c5ec608 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -298,9 +298,10 @@ static int vhdx_probe(const uint8_t *buf, int buf_size, const char *filename)
  * and then update the header checksum.  Header is converted to proper
  * endianness before being written to the specified file offset
  */
-static int vhdx_write_header(BlockDriverState *bs_file, VHDXHeader *hdr,
+static int vhdx_write_header(BdrvChild *file, VHDXHeader *hdr,
                              uint64_t offset, bool read)
 {
+    BlockDriverState *bs_file = file->bs;
     uint8_t *buffer = NULL;
     int ret;
     VHDXHeader *header_le;
@@ -315,7 +316,7 @@ static int vhdx_write_header(BlockDriverState *bs_file, VHDXHeader *hdr,
     buffer = qemu_blockalign(bs_file, VHDX_HEADER_SIZE);
     if (read) {
         /* if true, we can't assume the extra reserved bytes are 0 */
-        ret = bdrv_pread(bs_file, offset, buffer, VHDX_HEADER_SIZE);
+        ret = bdrv_pread(file, offset, buffer, VHDX_HEADER_SIZE);
         if (ret < 0) {
             goto exit;
         }
@@ -378,7 +379,7 @@ static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s,
         inactive_header->log_guid = *log_guid;
     }
 
-    ret = vhdx_write_header(bs->file->bs, inactive_header, header_offset, true);
+    ret = vhdx_write_header(bs->file, inactive_header, header_offset, true);
     if (ret < 0) {
         goto exit;
     }
@@ -430,7 +431,7 @@ static void vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s,
     /* We have to read the whole VHDX_HEADER_SIZE instead of
      * sizeof(VHDXHeader), because the checksum is over the whole
      * region */
-    ret = bdrv_pread(bs->file->bs, VHDX_HEADER1_OFFSET, buffer,
+    ret = bdrv_pread(bs->file, VHDX_HEADER1_OFFSET, buffer,
                      VHDX_HEADER_SIZE);
     if (ret < 0) {
         goto fail;
@@ -447,7 +448,7 @@ static void vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s,
         }
     }
 
-    ret = bdrv_pread(bs->file->bs, VHDX_HEADER2_OFFSET, buffer,
+    ret = bdrv_pread(bs->file, VHDX_HEADER2_OFFSET, buffer,
                      VHDX_HEADER_SIZE);
     if (ret < 0) {
         goto fail;
@@ -521,7 +522,7 @@ static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
      * whole block */
     buffer = qemu_blockalign(bs, VHDX_HEADER_BLOCK_SIZE);
 
-    ret = bdrv_pread(bs->file->bs, VHDX_REGION_TABLE_OFFSET, buffer,
+    ret = bdrv_pread(bs->file, VHDX_REGION_TABLE_OFFSET, buffer,
                      VHDX_HEADER_BLOCK_SIZE);
     if (ret < 0) {
         goto fail;
@@ -634,7 +635,7 @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
 
     buffer = qemu_blockalign(bs, VHDX_METADATA_TABLE_MAX_SIZE);
 
-    ret = bdrv_pread(bs->file->bs, s->metadata_rt.file_offset, buffer,
+    ret = bdrv_pread(bs->file, s->metadata_rt.file_offset, buffer,
                      VHDX_METADATA_TABLE_MAX_SIZE);
     if (ret < 0) {
         goto exit;
@@ -737,7 +738,7 @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
         goto exit;
     }
 
-    ret = bdrv_pread(bs->file->bs,
+    ret = bdrv_pread(bs->file,
                      s->metadata_entries.file_parameters_entry.offset
                                          + s->metadata_rt.file_offset,
                      &s->params,
@@ -772,7 +773,7 @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
     /* determine virtual disk size, logical sector size,
      * and phys sector size */
 
-    ret = bdrv_pread(bs->file->bs,
+    ret = bdrv_pread(bs->file,
                      s->metadata_entries.virtual_disk_size_entry.offset
                                            + s->metadata_rt.file_offset,
                      &s->virtual_disk_size,
@@ -780,7 +781,7 @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
     if (ret < 0) {
         goto exit;
     }
-    ret = bdrv_pread(bs->file->bs,
+    ret = bdrv_pread(bs->file,
                      s->metadata_entries.logical_sector_size_entry.offset
                                              + s->metadata_rt.file_offset,
                      &s->logical_sector_size,
@@ -788,7 +789,7 @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
     if (ret < 0) {
         goto exit;
     }
-    ret = bdrv_pread(bs->file->bs,
+    ret = bdrv_pread(bs->file,
                      s->metadata_entries.phys_sector_size_entry.offset
                                           + s->metadata_rt.file_offset,
                      &s->physical_sector_size,
@@ -905,7 +906,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
     QLIST_INIT(&s->regions);
 
     /* validate the file signature */
-    ret = bdrv_pread(bs->file->bs, 0, &signature, sizeof(uint64_t));
+    ret = bdrv_pread(bs->file, 0, &signature, sizeof(uint64_t));
     if (ret < 0) {
         goto fail;
     }
@@ -964,7 +965,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_pread(bs->file->bs, s->bat_offset, s->bat, s->bat_rt.length);
+    ret = bdrv_pread(bs->file, s->bat_offset, s->bat, s->bat_rt.length);
     if (ret < 0) {
         goto fail;
     }
@@ -1391,6 +1392,7 @@ static int vhdx_create_new_headers(BlockBackend *blk, uint64_t image_size,
                                    uint32_t log_size)
 {
     BlockDriverState *bs = blk_bs(blk);
+    BdrvChild *child;
     int ret = 0;
     VHDXHeader *hdr = NULL;
 
@@ -1405,12 +1407,18 @@ static int vhdx_create_new_headers(BlockBackend *blk, uint64_t image_size,
     vhdx_guid_generate(&hdr->file_write_guid);
     vhdx_guid_generate(&hdr->data_write_guid);
 
-    ret = vhdx_write_header(bs, hdr, VHDX_HEADER1_OFFSET, false);
+    /* XXX Ugly way to get blk->root, but that's a feature, not a bug. This
+     * hack makes it obvious that vhdx_write_header() bypasses the BlockBackend
+     * here, which it really shouldn't be doing. */
+    child = QLIST_FIRST(&bs->parents);
+    assert(!QLIST_NEXT(child, next_parent));
+
+    ret = vhdx_write_header(child, hdr, VHDX_HEADER1_OFFSET, false);
     if (ret < 0) {
         goto exit;
     }
     hdr->sequence_number++;
-    ret = vhdx_write_header(bs, hdr, VHDX_HEADER2_OFFSET, false);
+    ret = vhdx_write_header(child, hdr, VHDX_HEADER2_OFFSET, false);
     if (ret < 0) {
         goto exit;
     }
diff --git a/block/vmdk.c b/block/vmdk.c
index ee09423..a695e95 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -252,7 +252,7 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
     int ret;
 
     desc = g_malloc0(DESC_SIZE);
-    ret = bdrv_pread(bs->file->bs, s->desc_offset, desc, DESC_SIZE);
+    ret = bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE);
     if (ret < 0) {
         g_free(desc);
         return 0;
@@ -286,7 +286,7 @@ static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
 
     desc = g_malloc0(DESC_SIZE);
     tmp_desc = g_malloc0(DESC_SIZE);
-    ret = bdrv_pread(bs->file->bs, s->desc_offset, desc, DESC_SIZE);
+    ret = bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE);
     if (ret < 0) {
         goto out;
     }
@@ -350,7 +350,7 @@ static int vmdk_parent_open(BlockDriverState *bs)
     int ret;
 
     desc = g_malloc0(DESC_SIZE + 1);
-    ret = bdrv_pread(bs->file->bs, s->desc_offset, desc, DESC_SIZE);
+    ret = bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE);
     if (ret < 0) {
         goto out;
     }
@@ -454,7 +454,7 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
         return -ENOMEM;
     }
 
-    ret = bdrv_pread(extent->file->bs,
+    ret = bdrv_pread(extent->file,
                      extent->l1_table_offset,
                      extent->l1_table,
                      l1_size);
@@ -474,7 +474,7 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
             ret = -ENOMEM;
             goto fail_l1;
         }
-        ret = bdrv_pread(extent->file->bs,
+        ret = bdrv_pread(extent->file,
                          extent->l1_backup_table_offset,
                          extent->l1_backup_table,
                          l1_size);
@@ -508,7 +508,7 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
     VMDK3Header header;
     VmdkExtent *extent;
 
-    ret = bdrv_pread(file->bs, sizeof(magic), &header, sizeof(header));
+    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
         error_setg_errno(errp, -ret,
                          "Could not read header from file '%s'",
@@ -538,14 +538,13 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
 static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
                                QDict *options, Error **errp);
 
-static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
-                            Error **errp)
+static char *vmdk_read_desc(BdrvChild *file, uint64_t desc_offset, Error **errp)
 {
     int64_t size;
     char *buf;
     int ret;
 
-    size = bdrv_getlength(file);
+    size = bdrv_getlength(file->bs);
     if (size < 0) {
         error_setg_errno(errp, -size, "Could not access file");
         return NULL;
@@ -586,7 +585,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     int64_t l1_backup_offset = 0;
     bool compressed;
 
-    ret = bdrv_pread(file->bs, sizeof(magic), &header, sizeof(header));
+    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
         error_setg_errno(errp, -ret,
                          "Could not read header from file '%s'",
@@ -596,7 +595,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     if (header.capacity == 0) {
         uint64_t desc_offset = le64_to_cpu(header.desc_offset);
         if (desc_offset) {
-            char *buf = vmdk_read_desc(file->bs, desc_offset << 9, errp);
+            char *buf = vmdk_read_desc(file, desc_offset << 9, errp);
             if (!buf) {
                 return -EINVAL;
             }
@@ -636,7 +635,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
             } QEMU_PACKED eos_marker;
         } QEMU_PACKED footer;
 
-        ret = bdrv_pread(file->bs,
+        ret = bdrv_pread(file,
             bs->file->bs->total_sectors * 512 - 1536,
             &footer, sizeof(footer));
         if (ret < 0) {
@@ -874,7 +873,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             extent->flat_start_offset = flat_offset << 9;
         } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
             /* SPARSE extent and VMFSSPARSE extent are both "COWD" sparse file*/
-            char *buf = vmdk_read_desc(extent_file->bs, 0, errp);
+            char *buf = vmdk_read_desc(extent_file, 0, errp);
             if (!buf) {
                 ret = -EINVAL;
             } else {
@@ -943,7 +942,7 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVVmdkState *s = bs->opaque;
     uint32_t magic;
 
-    buf = vmdk_read_desc(bs->file->bs, 0, errp);
+    buf = vmdk_read_desc(bs->file, 0, errp);
     if (!buf) {
         return -EINVAL;
     }
@@ -1046,7 +1045,7 @@ static int get_whole_cluster(BlockDriverState *bs,
     /* Read backing data before skip range */
     if (skip_start_bytes > 0) {
         if (bs->backing) {
-            ret = bdrv_pread(bs->backing->bs, offset, whole_grain,
+            ret = bdrv_pread(bs->backing, offset, whole_grain,
                              skip_start_bytes);
             if (ret < 0) {
                 ret = VMDK_ERROR;
@@ -1063,7 +1062,7 @@ static int get_whole_cluster(BlockDriverState *bs,
     /* Read backing data after skip range */
     if (skip_end_bytes < cluster_bytes) {
         if (bs->backing) {
-            ret = bdrv_pread(bs->backing->bs, offset + skip_end_bytes,
+            ret = bdrv_pread(bs->backing, offset + skip_end_bytes,
                              whole_grain + skip_end_bytes,
                              cluster_bytes - skip_end_bytes);
             if (ret < 0) {
@@ -1191,8 +1190,7 @@ static int get_cluster_offset(BlockDriverState *bs,
         }
     }
     l2_table = extent->l2_cache + (min_index * extent->l2_size);
-    if (bdrv_pread(
-                extent->file->bs,
+    if (bdrv_pread(extent->file,
                 (int64_t)l2_offset * 512,
                 l2_table,
                 extent->l2_size * sizeof(uint32_t)
@@ -1426,7 +1424,7 @@ static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
     buf_bytes = cluster_bytes * 2;
     cluster_buf = g_malloc(buf_bytes);
     uncomp_buf = g_malloc(cluster_bytes);
-    ret = bdrv_pread(extent->file->bs,
+    ret = bdrv_pread(extent->file,
                 cluster_offset,
                 cluster_buf, buf_bytes);
     if (ret < 0) {
diff --git a/block/vpc.c b/block/vpc.c
index 076a7ce..9528637 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -237,7 +237,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_pread(bs->file->bs, 0, s->footer_buf, HEADER_SIZE);
+    ret = bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE);
     if (ret < 0) {
         error_setg(errp, "Unable to read VHD header");
         goto fail;
@@ -257,7 +257,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         }
 
         /* If a fixed disk, the footer is found only at the end of the file */
-        ret = bdrv_pread(bs->file->bs, offset-HEADER_SIZE, s->footer_buf,
+        ret = bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf,
                          HEADER_SIZE);
         if (ret < 0) {
             goto fail;
@@ -328,7 +328,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     if (disk_type == VHD_DYNAMIC) {
-        ret = bdrv_pread(bs->file->bs, be64_to_cpu(footer->data_offset), buf,
+        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
                          HEADER_SIZE);
         if (ret < 0) {
             error_setg(errp, "Error reading dynamic VHD header");
@@ -385,7 +385,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 
         s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
 
-        ret = bdrv_pread(bs->file->bs, s->bat_offset, s->pagetable,
+        ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable,
                          pagetable_size);
         if (ret < 0) {
             error_setg(errp, "Error reading pagetable");
diff --git a/include/block/block.h b/include/block/block.h
index 7eadfaf..a7735d8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -233,9 +233,8 @@ int bdrv_write(BdrvChild *child, int64_t sector_num,
 int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
                        int count, BdrvRequestFlags flags);
 int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags);
-int bdrv_pread(BlockDriverState *bs, int64_t offset,
-               void *buf, int count);
-int bdrv_preadv(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov);
+int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes);
+int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
 int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
                 const void *buf, int count);
 int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 13/17] block: Convert bdrv_pread(v) to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                |  7 ++++---
>  block/bochs.c          |  6 +++---
>  block/cloop.c          |  8 ++++----
>  block/crypto.c         |  2 +-
>  block/dmg.c            | 21 +++++++++++----------
>  block/io.c             |  8 ++++----
>  block/parallels.c      |  4 ++--
>  block/qcow.c           | 10 +++++-----
>  block/qcow2-cache.c    |  2 +-
>  block/qcow2-refcount.c | 12 ++++++------
>  block/qcow2-snapshot.c | 12 ++++++------
>  block/qcow2.c          | 16 ++++++++--------
>  block/qed.c            |  6 +++---
>  block/vhdx-log.c       |  8 ++++----
>  block/vhdx.c           | 38 +++++++++++++++++++++++---------------
>  block/vmdk.c           | 36 +++++++++++++++++-------------------
>  block/vpc.c            |  8 ++++----
>  include/block/block.h  |  5 ++---
>  18 files changed, 108 insertions(+), 101 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

But I do take issue with a comment in this patch, see below.

[...]

> diff --git a/block/vhdx.c b/block/vhdx.c
> index b0f66de..c5ec608 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c

[...]

> @@ -1405,12 +1407,18 @@ static int vhdx_create_new_headers(BlockBackend *blk, uint64_t image_size,
>      vhdx_guid_generate(&hdr->file_write_guid);
>      vhdx_guid_generate(&hdr->data_write_guid);
>  
> -    ret = vhdx_write_header(bs, hdr, VHDX_HEADER1_OFFSET, false);
> +    /* XXX Ugly way to get blk->root, but that's a feature, not a bug. This
> +     * hack makes it obvious that vhdx_write_header() bypasses the BlockBackend
> +     * here, which it really shouldn't be doing. */

Short question: Why not?


Long stuff:

The question I'm asking myself is why do we have a BB here at all? We
don't need it, really. Actually, the whole file creation business is
pretty shady. See bdrv_create(), that thing takes a filename for
whatever reason (OK, historical ones, I know). This is fine on the
protocol level, but strange on the format level.

Ideally we'd want to seperate the protocol creation from the format
creation, and the format level just gets some (indirect?) handle to the
protocol BDS.

So that's where the BB comes from, because the format level has to open
the file (on the protocol level) itself somehow, and we decided to frown
upon naked accesses to a BDS. (But I feel like I'm forgetting something
here, I think there was another reason, too...)

My intuitive and personal distinction between BDS and BB accesses is the
following:
- A BB is somehow related to an external user. External users always
  have to go through a BB to access a BDS tree. It's VM-y (by that I
  mean it's related to what the whole qemu program does).
- A BDS is just a way to access a host image. It's not VM-y.

So in the case of image creation, I personally would be completely fine
with accesses to the naked BDS, because those are not external users and
they just want to access a host image directly.

As long as there's no real difference between how accessing an image
through a BB or without a BB works, I don't particularly care which is used.

But there's a catch: Some format drivers reuse code they use for
existing images for image creation, for instance, writing out the image
header. If image creation works on a BB while code for existing images
does not (it works on a BdrvChild), sharing this code does not work
trivially. Previously, this was worked around by just bypassing the BB,
and I think this is completely fine and very reasonable, because, as I
said above, I don't see why code for new images should be treated
differently than code for existing images.

However, that no longer works after this series, as we can see here.
Bypassing the BB is not trivial and you say it's not intended. However,
that would mean that code for creating a new image has to be completely
separate from code for an existing image, because one takes a BB and the
other takes a BdrvChild, and I don't agree on that.


I think the issue is another: At this point, the current idea of image
creation and the premise of this series clash. The premise of this
series is that you should only be allowed to access a BDS if it's your
child, and this mostly works because any BDS is someone's child, and
actually only that someone wants to perform accesses on this (one
exception being the qcow{2,}_write_compressed() code).

However, this premise breaks here. Since all format drivers have to
create the protocol file and the protocol BDS themselves, these are
suddenly not their children. We work around that by putting an anonymous
BB on top of them, so that they are someone's children; but that someone
is the BB, not the image creator.

I think you should be able to access any BDS if you own it, not just if
it's your child. I'd argue that any BDS you own should be your child,
actually. And the issue with our image creation is that the format
drivers do own the protocol BDS, but it's not their child.


Your answer to this is: Image creation code should just use the BB.
Technically a valid answer, but I think this leads to unnecessary code
duplication, and I also don't understand the reasoning behind this.

I see two other ways:

The first one is to introduce a way to retrieve the BdrvChild from a BB.
Technically very simple. I do agree that this is not how the BB is
supposed to be used, so I would definitely restrict this to anonymous BBs.
The idea behind this is: If you own a BB, you should transitively own
the BDS behind it. Thus, it should be your child, too, and you should be
able to access it as such.
Other argument: We have blk_bs(). I don't see the difference, frankly.

The second one would be to have a way to introduce a very lightweight
BDS parent object. You can just put this on top of an existing BDS to
claim your parenthood, basically, and this one gives you then the
BdrvChild handle.
Technically not so simple. And also, I'd claim that we already have such
a lightweight parent object which is called "anonymous BB".


In my mind, the use of anonymous BBs is really just to claim parenthood
to a some BDS. Thus, I think it should be very much fine to have a
BdrvChild *blk_child(BlockBackend *blk) function which asserts that the
BB is indeed anonymous.


Max

> +    child = QLIST_FIRST(&bs->parents);
> +    assert(!QLIST_NEXT(child, next_parent));
> +
> +    ret = vhdx_write_header(child, hdr, VHDX_HEADER1_OFFSET, false);
>      if (ret < 0) {
>          goto exit;
>      }
>      hdr->sequence_number++;
> -    ret = vhdx_write_header(bs, hdr, VHDX_HEADER2_OFFSET, false);
> +    ret = vhdx_write_header(child, hdr, VHDX_HEADER2_OFFSET, false);
>      if (ret < 0) {
>          goto exit;
>      }



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

[Qemu-devel] [PATCH 14/17] block: Convert bdrv_pwrite(v/_sync) to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c             | 17 ++++++++---------
 block/parallels.c      |  6 +++---
 block/qcow.c           | 14 +++++++-------
 block/qcow2-cache.c    |  2 +-
 block/qcow2-cluster.c  |  6 +++---
 block/qcow2-refcount.c | 22 +++++++++++-----------
 block/qcow2-snapshot.c | 14 +++++++-------
 block/qcow2.c          | 14 +++++++-------
 block/qed.c            |  4 ++--
 block/vdi.c            |  2 +-
 block/vhdx-log.c       |  4 ++--
 block/vhdx.c           |  2 +-
 block/vmdk.c           | 12 +++++-------
 block/vpc.c            |  8 ++++----
 include/block/block.h  |  9 ++++-----
 15 files changed, 66 insertions(+), 70 deletions(-)

diff --git a/block/io.c b/block/io.c
index 1256151..ac6f3a4 100644
--- a/block/io.c
+++ b/block/io.c
@@ -741,11 +741,11 @@ int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
     return bdrv_preadv(child, offset, &qiov);
 }
 
-int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
+int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
 {
     int ret;
 
-    ret = bdrv_prwv_co(bs, offset, qiov, true, 0);
+    ret = bdrv_prwv_co(child->bs, offset, qiov, true, 0);
     if (ret < 0) {
         return ret;
     }
@@ -753,8 +753,7 @@ int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
     return qiov->size;
 }
 
-int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
-                const void *buf, int bytes)
+int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
 {
     QEMUIOVector qiov;
     struct iovec iov = {
@@ -767,7 +766,7 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
     }
 
     qemu_iovec_init_external(&qiov, &iov, 1);
-    return bdrv_pwritev(bs, offset, &qiov);
+    return bdrv_pwritev(child, offset, &qiov);
 }
 
 /*
@@ -776,17 +775,17 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
  *
  * Returns 0 on success, -errno in error cases.
  */
-int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
-    const void *buf, int count)
+int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
+                     const void *buf, int count)
 {
     int ret;
 
-    ret = bdrv_pwrite(bs, offset, buf, count);
+    ret = bdrv_pwrite(child, offset, buf, count);
     if (ret < 0) {
         return ret;
     }
 
-    ret = bdrv_flush(bs);
+    ret = bdrv_flush(child->bs);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/parallels.c b/block/parallels.c
index 85c7cdc..4542eb8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -250,7 +250,7 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs)
         if (off + to_write > s->header_size) {
             to_write = s->header_size - off;
         }
-        ret = bdrv_pwrite(bs->file->bs, off, (uint8_t *)s->header + off,
+        ret = bdrv_pwrite(bs->file, off, (uint8_t *)s->header + off,
                           to_write);
         if (ret < 0) {
             qemu_co_mutex_unlock(&s->lock);
@@ -432,7 +432,7 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
     }
 
     if (flush_bat) {
-        ret = bdrv_pwrite_sync(bs->file->bs, 0, s->header, s->header_size);
+        ret = bdrv_pwrite_sync(bs->file, 0, s->header, s->header_size);
         if (ret < 0) {
             res->check_errors++;
             return ret;
@@ -563,7 +563,7 @@ static int parallels_update_header(BlockDriverState *bs)
     if (size > s->header_size) {
         size = s->header_size;
     }
-    return bdrv_pwrite_sync(bs->file->bs, 0, s->header, size);
+    return bdrv_pwrite_sync(bs->file, 0, s->header, size);
 }
 
 static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
diff --git a/block/qcow.c b/block/qcow.c
index 8b1d70b..4c4b94a 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -390,7 +390,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
         /* update the L1 entry */
         s->l1_table[l1_index] = l2_offset;
         tmp = cpu_to_be64(l2_offset);
-        if (bdrv_pwrite_sync(bs->file->bs,
+        if (bdrv_pwrite_sync(bs->file,
                 s->l1_table_offset + l1_index * sizeof(tmp),
                 &tmp, sizeof(tmp)) < 0)
             return 0;
@@ -420,7 +420,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
     l2_table = s->l2_cache + (min_index << s->l2_bits);
     if (new_l2_table) {
         memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
-        if (bdrv_pwrite_sync(bs->file->bs, l2_offset, l2_table,
+        if (bdrv_pwrite_sync(bs->file, l2_offset, l2_table,
                 s->l2_size * sizeof(uint64_t)) < 0)
             return 0;
     } else {
@@ -450,7 +450,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
             cluster_offset = (cluster_offset + s->cluster_size - 1) &
                 ~(s->cluster_size - 1);
             /* write the cluster content */
-            if (bdrv_pwrite(bs->file->bs, cluster_offset, s->cluster_cache,
+            if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
                             s->cluster_size) !=
                 s->cluster_size)
                 return -1;
@@ -480,7 +480,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
                                 errno = EIO;
                                 return -1;
                             }
-                            if (bdrv_pwrite(bs->file->bs,
+                            if (bdrv_pwrite(bs->file,
                                             cluster_offset + i * 512,
                                             s->cluster_data, 512) != 512)
                                 return -1;
@@ -495,7 +495,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
         /* update L2 table */
         tmp = cpu_to_be64(cluster_offset);
         l2_table[l2_index] = tmp;
-        if (bdrv_pwrite_sync(bs->file->bs, l2_offset + l2_index * sizeof(tmp),
+        if (bdrv_pwrite_sync(bs->file, l2_offset + l2_index * sizeof(tmp),
                 &tmp, sizeof(tmp)) < 0)
             return 0;
     }
@@ -899,7 +899,7 @@ static int qcow_make_empty(BlockDriverState *bs)
     int ret;
 
     memset(s->l1_table, 0, l1_length);
-    if (bdrv_pwrite_sync(bs->file->bs, s->l1_table_offset, s->l1_table,
+    if (bdrv_pwrite_sync(bs->file, s->l1_table_offset, s->l1_table,
             l1_length) < 0)
         return -1;
     ret = bdrv_truncate(bs->file->bs, s->l1_table_offset + l1_length);
@@ -990,7 +990,7 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
         }
 
         cluster_offset &= s->cluster_offset_mask;
-        ret = bdrv_pwrite(bs->file->bs, cluster_offset, out_buf, out_len);
+        ret = bdrv_pwrite(bs->file, cluster_offset, out_buf, out_len);
         if (ret < 0) {
             goto fail;
         }
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 5eba75f..6eaefed 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -210,7 +210,7 @@ static int qcow2_cache_entry_flush(BlockDriverState *bs, Qcow2Cache *c, int i)
         BLKDBG_EVENT(bs->file, BLKDBG_L2_UPDATE);
     }
 
-    ret = bdrv_pwrite(bs->file->bs, c->entries[i].offset,
+    ret = bdrv_pwrite(bs->file, c->entries[i].offset,
                       qcow2_cache_get_table_addr(bs, c, i), s->cluster_size);
     if (ret < 0) {
         return ret;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index c2b78ad..a219e68 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -108,7 +108,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
     BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_WRITE_TABLE);
     for(i = 0; i < s->l1_size; i++)
         new_l1_table[i] = cpu_to_be64(new_l1_table[i]);
-    ret = bdrv_pwrite_sync(bs->file->bs, new_l1_table_offset,
+    ret = bdrv_pwrite_sync(bs->file, new_l1_table_offset,
                            new_l1_table, new_l1_size2);
     if (ret < 0)
         goto fail;
@@ -119,7 +119,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
     BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ACTIVATE_TABLE);
     cpu_to_be32w((uint32_t*)data, new_l1_size);
     stq_be_p(data + 4, new_l1_table_offset);
-    ret = bdrv_pwrite_sync(bs->file->bs, offsetof(QCowHeader, l1_size),
+    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_size),
                            data, sizeof(data));
     if (ret < 0) {
         goto fail;
@@ -187,7 +187,7 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_L1_UPDATE);
-    ret = bdrv_pwrite_sync(bs->file->bs,
+    ret = bdrv_pwrite_sync(bs->file,
                            s->l1_table_offset + 8 * l1_start_index,
                            buf, sizeof(buf));
     if (ret < 0) {
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 2d2bf8a..0373d32 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -434,7 +434,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
     if (refcount_table_index < s->refcount_table_size) {
         uint64_t data64 = cpu_to_be64(new_block);
         BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_ALLOC_HOOKUP);
-        ret = bdrv_pwrite_sync(bs->file->bs,
+        ret = bdrv_pwrite_sync(bs->file,
             s->refcount_table_offset + refcount_table_index * sizeof(uint64_t),
             &data64, sizeof(data64));
         if (ret < 0) {
@@ -536,7 +536,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
 
     /* Write refcount blocks to disk */
     BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_ALLOC_WRITE_BLOCKS);
-    ret = bdrv_pwrite_sync(bs->file->bs, meta_offset, new_blocks,
+    ret = bdrv_pwrite_sync(bs->file, meta_offset, new_blocks,
         blocks_clusters * s->cluster_size);
     g_free(new_blocks);
     new_blocks = NULL;
@@ -550,7 +550,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_ALLOC_WRITE_TABLE);
-    ret = bdrv_pwrite_sync(bs->file->bs, table_offset, new_table,
+    ret = bdrv_pwrite_sync(bs->file, table_offset, new_table,
         table_size * sizeof(uint64_t));
     if (ret < 0) {
         goto fail_table;
@@ -568,7 +568,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
     cpu_to_be64w(&data.d64, table_offset);
     cpu_to_be32w(&data.d32, table_clusters);
     BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_ALLOC_SWITCH_TABLE);
-    ret = bdrv_pwrite_sync(bs->file->bs,
+    ret = bdrv_pwrite_sync(bs->file,
                            offsetof(QCowHeader, refcount_table_offset),
                            &data, sizeof(data));
     if (ret < 0) {
@@ -1226,7 +1226,7 @@ fail:
             cpu_to_be64s(&l1_table[i]);
         }
 
-        ret = bdrv_pwrite_sync(bs->file->bs, l1_table_offset,
+        ret = bdrv_pwrite_sync(bs->file, l1_table_offset,
                                l1_table, l1_size2);
 
         for (i = 0; i < l1_size; i++) {
@@ -1667,7 +1667,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
                 goto fail;
             }
 
-            ret = bdrv_pwrite(bs->file->bs, l2_offset, l2_table,
+            ret = bdrv_pwrite(bs->file, l2_offset, l2_table,
                               s->cluster_size);
             if (ret < 0) {
                 fprintf(stderr, "ERROR: Could not write L2 table: %s\n",
@@ -2150,7 +2150,7 @@ write_refblocks:
     }
 
     assert(reftable_size < INT_MAX / sizeof(uint64_t));
-    ret = bdrv_pwrite(bs->file->bs, reftable_offset, on_disk_reftable,
+    ret = bdrv_pwrite(bs->file, reftable_offset, on_disk_reftable,
                       reftable_size * sizeof(uint64_t));
     if (ret < 0) {
         fprintf(stderr, "ERROR writing reftable: %s\n", strerror(-ret));
@@ -2162,8 +2162,8 @@ write_refblocks:
                  reftable_offset);
     cpu_to_be32w(&reftable_offset_and_clusters.reftable_clusters,
                  size_to_clusters(s, reftable_size * sizeof(uint64_t)));
-    ret = bdrv_pwrite_sync(bs->file->bs, offsetof(QCowHeader,
-                                                  refcount_table_offset),
+    ret = bdrv_pwrite_sync(bs->file,
+                           offsetof(QCowHeader, refcount_table_offset),
                            &reftable_offset_and_clusters,
                            sizeof(reftable_offset_and_clusters));
     if (ret < 0) {
@@ -2563,7 +2563,7 @@ static int flush_refblock(BlockDriverState *bs, uint64_t **reftable,
             return ret;
         }
 
-        ret = bdrv_pwrite(bs->file->bs, offset, refblock, s->cluster_size);
+        ret = bdrv_pwrite(bs->file, offset, refblock, s->cluster_size);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Failed to write refblock");
             return ret;
@@ -2833,7 +2833,7 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
         cpu_to_be64s(&new_reftable[i]);
     }
 
-    ret = bdrv_pwrite(bs->file->bs, new_reftable_offset, new_reftable,
+    ret = bdrv_pwrite(bs->file, new_reftable_offset, new_reftable,
                       new_reftable_size * sizeof(uint64_t));
 
     for (i = 0; i < new_reftable_size; i++) {
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 2693ebf..0324243 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -217,25 +217,25 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
         h.name_size = cpu_to_be16(name_size);
         offset = align_offset(offset, 8);
 
-        ret = bdrv_pwrite(bs->file->bs, offset, &h, sizeof(h));
+        ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
         if (ret < 0) {
             goto fail;
         }
         offset += sizeof(h);
 
-        ret = bdrv_pwrite(bs->file->bs, offset, &extra, sizeof(extra));
+        ret = bdrv_pwrite(bs->file, offset, &extra, sizeof(extra));
         if (ret < 0) {
             goto fail;
         }
         offset += sizeof(extra);
 
-        ret = bdrv_pwrite(bs->file->bs, offset, sn->id_str, id_str_size);
+        ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size);
         if (ret < 0) {
             goto fail;
         }
         offset += id_str_size;
 
-        ret = bdrv_pwrite(bs->file->bs, offset, sn->name, name_size);
+        ret = bdrv_pwrite(bs->file, offset, sn->name, name_size);
         if (ret < 0) {
             goto fail;
         }
@@ -257,7 +257,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     header_data.nb_snapshots        = cpu_to_be32(s->nb_snapshots);
     header_data.snapshots_offset    = cpu_to_be64(snapshots_offset);
 
-    ret = bdrv_pwrite_sync(bs->file->bs, offsetof(QCowHeader, nb_snapshots),
+    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
                            &header_data, sizeof(header_data));
     if (ret < 0) {
         goto fail;
@@ -399,7 +399,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         goto fail;
     }
 
-    ret = bdrv_pwrite(bs->file->bs, sn->l1_table_offset, l1_table,
+    ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
                       s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
         goto fail;
@@ -530,7 +530,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
         goto fail;
     }
 
-    ret = bdrv_pwrite_sync(bs->file->bs, s->l1_table_offset, sn_l1_table,
+    ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table,
                            cur_l1_bytes);
     if (ret < 0) {
         goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index 559f28e..8b8f730 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -249,7 +249,7 @@ int qcow2_mark_dirty(BlockDriverState *bs)
     }
 
     val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY);
-    ret = bdrv_pwrite(bs->file->bs, offsetof(QCowHeader, incompatible_features),
+    ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features),
                       &val, sizeof(val));
     if (ret < 0) {
         return ret;
@@ -1975,7 +1975,7 @@ int qcow2_update_header(BlockDriverState *bs)
     }
 
     /* Write the new header */
-    ret = bdrv_pwrite(bs->file->bs, 0, header, s->cluster_size);
+    ret = bdrv_pwrite(bs->file, 0, header, s->cluster_size);
     if (ret < 0) {
         goto fail;
     }
@@ -2058,7 +2058,7 @@ static int preallocate(BlockDriverState *bs)
      */
     if (host_offset != 0) {
         uint8_t data = 0;
-        ret = bdrv_pwrite(bs->file->bs, (host_offset + cur_bytes) - 1,
+        ret = bdrv_pwrite(bs->file, (host_offset + cur_bytes) - 1,
                           &data, 1);
         if (ret < 0) {
             return ret;
@@ -2524,7 +2524,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
 
     /* write updated header.size */
     offset = cpu_to_be64(offset);
-    ret = bdrv_pwrite_sync(bs->file->bs, offsetof(QCowHeader, size),
+    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
                            &offset, sizeof(uint64_t));
     if (ret < 0) {
         return ret;
@@ -2624,7 +2624,7 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
         }
 
         BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED);
-        ret = bdrv_pwrite(bs->file->bs, cluster_offset, out_buf, out_len);
+        ret = bdrv_pwrite(bs->file, cluster_offset, out_buf, out_len);
         if (ret < 0) {
             goto fail;
         }
@@ -2706,7 +2706,7 @@ static int make_completely_empty(BlockDriverState *bs)
     cpu_to_be64w(&l1_ofs_rt_ofs_cls.l1_offset, 3 * s->cluster_size);
     cpu_to_be64w(&l1_ofs_rt_ofs_cls.reftable_offset, s->cluster_size);
     cpu_to_be32w(&l1_ofs_rt_ofs_cls.reftable_clusters, 1);
-    ret = bdrv_pwrite_sync(bs->file->bs, offsetof(QCowHeader, l1_table_offset),
+    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_table_offset),
                            &l1_ofs_rt_ofs_cls, sizeof(l1_ofs_rt_ofs_cls));
     if (ret < 0) {
         goto fail_broken_refcounts;
@@ -2737,7 +2737,7 @@ static int make_completely_empty(BlockDriverState *bs)
 
     /* Enter the first refblock into the reftable */
     rt_entry = cpu_to_be64(2 * s->cluster_size);
-    ret = bdrv_pwrite_sync(bs->file->bs, s->cluster_size,
+    ret = bdrv_pwrite_sync(bs->file, s->cluster_size,
                            &rt_entry, sizeof(rt_entry));
     if (ret < 0) {
         goto fail_broken_refcounts;
diff --git a/block/qed.c b/block/qed.c
index d6d5e33..f619d82 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -86,7 +86,7 @@ int qed_write_header_sync(BDRVQEDState *s)
     int ret;
 
     qed_header_cpu_to_le(&s->header, &le);
-    ret = bdrv_pwrite(s->bs->file->bs, 0, &le, sizeof(le));
+    ret = bdrv_pwrite(s->bs->file, 0, &le, sizeof(le));
     if (ret != sizeof(le)) {
         return ret;
     }
@@ -1575,7 +1575,7 @@ static int bdrv_qed_change_backing_file(BlockDriverState *bs,
     }
 
     /* Write new header */
-    ret = bdrv_pwrite_sync(bs->file->bs, 0, buffer, buffer_len);
+    ret = bdrv_pwrite_sync(bs->file, 0, buffer, buffer_len);
     g_free(buffer);
     if (ret == 0) {
         memcpy(&s->header, &new_header, sizeof(new_header));
diff --git a/block/vdi.c b/block/vdi.c
index b2871ca..a1a12d9 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -670,7 +670,7 @@ vdi_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
              * acquire the lock and thus the padded cluster is written before
              * the other coroutines can write to the affected area. */
             qemu_co_mutex_lock(&s->write_lock);
-            ret = bdrv_pwrite(bs->file->bs, data_offset, block, s->block_size);
+            ret = bdrv_pwrite(bs->file, data_offset, block, s->block_size);
             qemu_co_mutex_unlock(&s->write_lock);
         } else {
             uint64_t data_offset = s->header.offset_data +
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 1e3928f..02eb104 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -194,7 +194,7 @@ static int vhdx_log_write_sectors(BlockDriverState *bs, VHDXLogEntries *log,
             /* full */
             break;
         }
-        ret = bdrv_pwrite(bs->file->bs, offset, buffer_tmp,
+        ret = bdrv_pwrite(bs->file, offset, buffer_tmp,
                           VHDX_LOG_SECTOR_SIZE);
         if (ret < 0) {
             goto exit;
@@ -466,7 +466,7 @@ static int vhdx_log_flush_desc(BlockDriverState *bs, VHDXLogDescriptor *desc,
 
     /* count is only > 1 if we are writing zeroes */
     for (i = 0; i < count; i++) {
-        ret = bdrv_pwrite_sync(bs->file->bs, file_offset, buffer,
+        ret = bdrv_pwrite_sync(bs->file, file_offset, buffer,
                                VHDX_LOG_SECTOR_SIZE);
         if (ret < 0) {
             goto exit;
diff --git a/block/vhdx.c b/block/vhdx.c
index c5ec608..75ef2b1 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -330,7 +330,7 @@ static int vhdx_write_header(BdrvChild *file, VHDXHeader *hdr,
     vhdx_header_le_export(hdr, header_le);
     vhdx_update_checksum(buffer, VHDX_HEADER_SIZE,
                          offsetof(VHDXHeader, checksum));
-    ret = bdrv_pwrite_sync(bs_file, offset, header_le, sizeof(VHDXHeader));
+    ret = bdrv_pwrite_sync(file, offset, header_le, sizeof(VHDXHeader));
 
 exit:
     qemu_vfree(buffer);
diff --git a/block/vmdk.c b/block/vmdk.c
index a695e95..094013f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -306,7 +306,7 @@ static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
         pstrcat(desc, DESC_SIZE, tmp_desc);
     }
 
-    ret = bdrv_pwrite_sync(bs->file->bs, s->desc_offset, desc, DESC_SIZE);
+    ret = bdrv_pwrite_sync(bs->file, s->desc_offset, desc, DESC_SIZE);
 
 out:
     g_free(desc);
@@ -1052,7 +1052,7 @@ static int get_whole_cluster(BlockDriverState *bs,
                 goto exit;
             }
         }
-        ret = bdrv_pwrite(extent->file->bs, cluster_offset, whole_grain,
+        ret = bdrv_pwrite(extent->file, cluster_offset, whole_grain,
                           skip_start_bytes);
         if (ret < 0) {
             ret = VMDK_ERROR;
@@ -1070,7 +1070,7 @@ static int get_whole_cluster(BlockDriverState *bs,
                 goto exit;
             }
         }
-        ret = bdrv_pwrite(extent->file->bs, cluster_offset + skip_end_bytes,
+        ret = bdrv_pwrite(extent->file, cluster_offset + skip_end_bytes,
                           whole_grain + skip_end_bytes,
                           cluster_bytes - skip_end_bytes);
         if (ret < 0) {
@@ -1090,8 +1090,7 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
 {
     offset = cpu_to_le32(offset);
     /* update L2 table */
-    if (bdrv_pwrite_sync(
-                extent->file->bs,
+    if (bdrv_pwrite_sync(extent->file,
                 ((int64_t)m_data->l2_offset * 512)
                     + (m_data->l2_index * sizeof(offset)),
                 &offset, sizeof(offset)) < 0) {
@@ -1100,8 +1099,7 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
     /* update backup L2 table */
     if (extent->l1_backup_table_offset != 0) {
         m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
-        if (bdrv_pwrite_sync(
-                    extent->file->bs,
+        if (bdrv_pwrite_sync(extent->file,
                     ((int64_t)m_data->l2_offset * 512)
                         + (m_data->l2_index * sizeof(offset)),
                     &offset, sizeof(offset)) < 0) {
diff --git a/block/vpc.c b/block/vpc.c
index 9528637..dcb5312 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -481,7 +481,7 @@ static inline int64_t get_image_offset(BlockDriverState *bs, uint64_t offset,
 
         s->last_bitmap_offset = bitmap_offset;
         memset(bitmap, 0xff, s->bitmap_size);
-        bdrv_pwrite_sync(bs->file->bs, bitmap_offset, bitmap, s->bitmap_size);
+        bdrv_pwrite_sync(bs->file, bitmap_offset, bitmap, s->bitmap_size);
     }
 
     return block_offset;
@@ -505,7 +505,7 @@ static int rewrite_footer(BlockDriverState* bs)
     BDRVVPCState *s = bs->opaque;
     int64_t offset = s->free_data_block_offset;
 
-    ret = bdrv_pwrite_sync(bs->file->bs, offset, s->footer_buf, HEADER_SIZE);
+    ret = bdrv_pwrite_sync(bs->file, offset, s->footer_buf, HEADER_SIZE);
     if (ret < 0)
         return ret;
 
@@ -539,7 +539,7 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t offset)
 
     /* Initialize the block's bitmap */
     memset(bitmap, 0xff, s->bitmap_size);
-    ret = bdrv_pwrite_sync(bs->file->bs, s->free_data_block_offset, bitmap,
+    ret = bdrv_pwrite_sync(bs->file, s->free_data_block_offset, bitmap,
         s->bitmap_size);
     if (ret < 0) {
         return ret;
@@ -554,7 +554,7 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t offset)
     /* Write BAT entry to disk */
     bat_offset = s->bat_offset + (4 * index);
     bat_value = cpu_to_be32(s->pagetable[index]);
-    ret = bdrv_pwrite_sync(bs->file->bs, bat_offset, &bat_value, 4);
+    ret = bdrv_pwrite_sync(bs->file, bat_offset, &bat_value, 4);
     if (ret < 0)
         goto fail;
 
diff --git a/include/block/block.h b/include/block/block.h
index a7735d8..25978b1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -235,11 +235,10 @@ int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
 int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags);
 int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes);
 int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
-int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
-                const void *buf, int count);
-int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov);
-int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
-    const void *buf, int count);
+int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes);
+int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
+int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
+                     const void *buf, int count);
 int coroutine_fn bdrv_co_readv(BdrvChild *child, int64_t sector_num,
                                int nb_sectors, QEMUIOVector *qiov);
 int coroutine_fn bdrv_co_writev(BdrvChild *child, int64_t sector_num,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 14/17] block: Convert bdrv_pwrite(v/_sync) to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c             | 17 ++++++++---------
>  block/parallels.c      |  6 +++---
>  block/qcow.c           | 14 +++++++-------
>  block/qcow2-cache.c    |  2 +-
>  block/qcow2-cluster.c  |  6 +++---
>  block/qcow2-refcount.c | 22 +++++++++++-----------
>  block/qcow2-snapshot.c | 14 +++++++-------
>  block/qcow2.c          | 14 +++++++-------
>  block/qed.c            |  4 ++--
>  block/vdi.c            |  2 +-
>  block/vhdx-log.c       |  4 ++--
>  block/vhdx.c           |  2 +-
>  block/vmdk.c           | 12 +++++-------
>  block/vpc.c            |  8 ++++----
>  include/block/block.h  |  9 ++++-----
>  15 files changed, 66 insertions(+), 70 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 15/17] block: Convert bdrv_pwrite_zeroes() to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 5 +++++
 block/io.c                     | 9 +++++----
 block/parallels.c              | 2 +-
 block/qcow2-cluster.c          | 2 +-
 block/qcow2.c                  | 4 ++--
 include/block/block.h          | 4 ++--
 include/sysemu/block-backend.h | 1 +
 qemu-img.c                     | 2 +-
 8 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 34500e6..389b36b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -870,6 +870,11 @@ int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                    flags | BDRV_REQ_ZERO_WRITE);
 }
 
+int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags)
+{
+    return bdrv_make_zero(blk->root, flags);
+}
+
 static void error_callback_bh(void *opaque)
 {
     struct BlockBackendAIOCB *acb = opaque;
diff --git a/block/io.c b/block/io.c
index ac6f3a4..641cd49 100644
--- a/block/io.c
+++ b/block/io.c
@@ -653,7 +653,7 @@ int bdrv_write(BdrvChild *child, int64_t sector_num,
                       true, 0);
 }
 
-int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
+int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int count, BdrvRequestFlags flags)
 {
     QEMUIOVector qiov;
@@ -663,7 +663,7 @@ int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
     };
 
     qemu_iovec_init_external(&qiov, &iov, 1);
-    return bdrv_prwv_co(bs, offset, &qiov, true,
+    return bdrv_prwv_co(child->bs, offset, &qiov, true,
                         BDRV_REQ_ZERO_WRITE | flags);
 }
 
@@ -676,9 +676,10 @@ int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
  *
  * Returns < 0 on error, 0 on success. For error codes see bdrv_write().
  */
-int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
+int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
 {
     int64_t target_sectors, ret, nb_sectors, sector_num = 0;
+    BlockDriverState *bs = child->bs;
     BlockDriverState *file;
     int n;
 
@@ -702,7 +703,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
             sector_num += n;
             continue;
         }
-        ret = bdrv_pwrite_zeroes(bs, sector_num << BDRV_SECTOR_BITS,
+        ret = bdrv_pwrite_zeroes(child, sector_num << BDRV_SECTOR_BITS,
                                  n << BDRV_SECTOR_BITS, flags);
         if (ret < 0) {
             error_report("error writing zeroes at sector %" PRId64 ": %s",
diff --git a/block/parallels.c b/block/parallels.c
index 4542eb8..807a801 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -210,7 +210,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
         int ret;
         space += s->prealloc_size;
         if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-            ret = bdrv_pwrite_zeroes(bs->file->bs,
+            ret = bdrv_pwrite_zeroes(bs->file,
                                      s->data_end << BDRV_SECTOR_BITS,
                                      space << BDRV_SECTOR_BITS, 0);
         } else {
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index a219e68..20ac99e 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1754,7 +1754,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                 goto fail;
             }
 
-            ret = bdrv_pwrite_zeroes(bs->file->bs, offset, s->cluster_size, 0);
+            ret = bdrv_pwrite_zeroes(bs->file, offset, s->cluster_size, 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 8b8f730..4aab822 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2673,7 +2673,7 @@ static int make_completely_empty(BlockDriverState *bs)
     /* After this call, neither the in-memory nor the on-disk refcount
      * information accurately describe the actual references */
 
-    ret = bdrv_pwrite_zeroes(bs->file->bs, s->l1_table_offset,
+    ret = bdrv_pwrite_zeroes(bs->file, s->l1_table_offset,
                              l1_clusters * s->cluster_size, 0);
     if (ret < 0) {
         goto fail_broken_refcounts;
@@ -2687,7 +2687,7 @@ static int make_completely_empty(BlockDriverState *bs)
      * overwrite parts of the existing refcount and L1 table, which is not
      * an issue because the dirty flag is set, complete data loss is in fact
      * desired and partial data loss is consequently fine as well */
-    ret = bdrv_pwrite_zeroes(bs->file->bs, s->cluster_size,
+    ret = bdrv_pwrite_zeroes(bs->file, s->cluster_size,
                              (2 + l1_clusters) * s->cluster_size, 0);
     /* This call (even if it failed overall) may have overwritten on-disk
      * refcount structures; in that case, the in-memory refcount information
diff --git a/include/block/block.h b/include/block/block.h
index 25978b1..abc5f26 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -230,9 +230,9 @@ int bdrv_read(BdrvChild *child, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
 int bdrv_write(BdrvChild *child, int64_t sector_num,
                const uint8_t *buf, int nb_sectors);
-int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
+int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int count, BdrvRequestFlags flags);
-int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags);
+int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
 int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes);
 int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
 int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c04af8e..4a8d850 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -124,6 +124,7 @@ int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
 BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                                   int count, BdrvRequestFlags flags,
                                   BlockCompletionFunc *cb, void *opaque);
+int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags);
 int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int count);
 int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count,
                BdrvRequestFlags flags);
diff --git a/qemu-img.c b/qemu-img.c
index 14e2661..e50e167 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1641,7 +1641,7 @@ static int convert_do_copy(ImgConvertState *s)
     if (!s->has_zero_init && !s->target_has_backing &&
         bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)))
     {
-        ret = bdrv_make_zero(blk_bs(s->target), BDRV_REQ_MAY_UNMAP);
+        ret = blk_make_zero(s->target, BDRV_REQ_MAY_UNMAP);
         if (ret == 0) {
             s->has_zero_init = true;
         }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 15/17] block: Convert bdrv_pwrite_zeroes() to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 5 +++++
>  block/io.c                     | 9 +++++----
>  block/parallels.c              | 2 +-
>  block/qcow2-cluster.c          | 2 +-
>  block/qcow2.c                  | 4 ++--
>  include/block/block.h          | 4 ++--
>  include/sysemu/block-backend.h | 1 +
>  qemu-img.c                     | 2 +-
>  8 files changed, 18 insertions(+), 11 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 16/17] block: Convert bdrv_prwv_co() to BdrvChild

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/block/io.c b/block/io.c
index 641cd49..78529fc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -557,7 +557,7 @@ static int bdrv_check_request(BlockDriverState *bs, int64_t sector_num,
 }
 
 typedef struct RwCo {
-    BlockDriverState *bs;
+    BdrvChild *child;
     int64_t offset;
     QEMUIOVector *qiov;
     bool is_write;
@@ -570,11 +570,11 @@ static void coroutine_fn bdrv_rw_co_entry(void *opaque)
     RwCo *rwco = opaque;
 
     if (!rwco->is_write) {
-        rwco->ret = bdrv_co_preadv(rwco->bs, rwco->offset,
+        rwco->ret = bdrv_co_preadv(rwco->child->bs, rwco->offset,
                                    rwco->qiov->size, rwco->qiov,
                                    rwco->flags);
     } else {
-        rwco->ret = bdrv_co_pwritev(rwco->bs, rwco->offset,
+        rwco->ret = bdrv_co_pwritev(rwco->child->bs, rwco->offset,
                                     rwco->qiov->size, rwco->qiov,
                                     rwco->flags);
     }
@@ -583,13 +583,13 @@ static void coroutine_fn bdrv_rw_co_entry(void *opaque)
 /*
  * Process a vectored synchronous request using coroutines
  */
-static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset,
+static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
                         QEMUIOVector *qiov, bool is_write,
                         BdrvRequestFlags flags)
 {
     Coroutine *co;
     RwCo rwco = {
-        .bs = bs,
+        .child = child,
         .offset = offset,
         .qiov = qiov,
         .is_write = is_write,
@@ -601,7 +601,7 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset,
         /* Fast-path if already in coroutine context */
         bdrv_rw_co_entry(&rwco);
     } else {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
+        AioContext *aio_context = bdrv_get_aio_context(child->bs);
 
         co = qemu_coroutine_create(bdrv_rw_co_entry);
         qemu_coroutine_enter(co, &rwco);
@@ -615,7 +615,7 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset,
 /*
  * Process a synchronous request using coroutines
  */
-static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
+static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
                       int nb_sectors, bool is_write, BdrvRequestFlags flags)
 {
     QEMUIOVector qiov;
@@ -629,7 +629,7 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
     }
 
     qemu_iovec_init_external(&qiov, &iov, 1);
-    return bdrv_prwv_co(bs, sector_num << BDRV_SECTOR_BITS,
+    return bdrv_prwv_co(child, sector_num << BDRV_SECTOR_BITS,
                         &qiov, is_write, flags);
 }
 
@@ -637,7 +637,7 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
 int bdrv_read(BdrvChild *child, int64_t sector_num,
               uint8_t *buf, int nb_sectors)
 {
-    return bdrv_rw_co(child->bs, sector_num, buf, nb_sectors, false, 0);
+    return bdrv_rw_co(child, sector_num, buf, nb_sectors, false, 0);
 }
 
 /* Return < 0 if error. Important errors are:
@@ -649,8 +649,7 @@ int bdrv_read(BdrvChild *child, int64_t sector_num,
 int bdrv_write(BdrvChild *child, int64_t sector_num,
                const uint8_t *buf, int nb_sectors)
 {
-    return bdrv_rw_co(child->bs, sector_num, (uint8_t *)buf, nb_sectors,
-                      true, 0);
+    return bdrv_rw_co(child, sector_num, (uint8_t *)buf, nb_sectors, true, 0);
 }
 
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
@@ -663,7 +662,7 @@ int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
     };
 
     qemu_iovec_init_external(&qiov, &iov, 1);
-    return bdrv_prwv_co(child->bs, offset, &qiov, true,
+    return bdrv_prwv_co(child, offset, &qiov, true,
                         BDRV_REQ_ZERO_WRITE | flags);
 }
 
@@ -718,7 +717,7 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
 {
     int ret;
 
-    ret = bdrv_prwv_co(child->bs, offset, qiov, false, 0);
+    ret = bdrv_prwv_co(child, offset, qiov, false, 0);
     if (ret < 0) {
         return ret;
     }
@@ -746,7 +745,7 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
 {
     int ret;
 
-    ret = bdrv_prwv_co(child->bs, offset, qiov, true, 0);
+    ret = bdrv_prwv_co(child, offset, qiov, true, 0);
     if (ret < 0) {
         return ret;
     }
@@ -2202,9 +2201,15 @@ void qemu_aio_unref(void *p)
 /**************************************************************/
 /* Coroutine block device emulation */
 
+typedef struct FlushCo {
+    BlockDriverState *bs;
+    int ret;
+} FlushCo;
+
+
 static void coroutine_fn bdrv_flush_co_entry(void *opaque)
 {
-    RwCo *rwco = opaque;
+    FlushCo *rwco = opaque;
 
     rwco->ret = bdrv_co_flush(rwco->bs);
 }
@@ -2288,7 +2293,7 @@ out:
 int bdrv_flush(BlockDriverState *bs)
 {
     Coroutine *co;
-    RwCo rwco = {
+    FlushCo rwco = {
         .bs = bs,
         .ret = NOT_DONE,
     };
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 16/17] block: Convert bdrv_prwv_co() to BdrvChild

[Max Reitz]

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

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c | 37 +++++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 641cd49..78529fc 100644
> --- a/block/io.c
> +++ b/block/io.c

[...]

> @@ -2202,9 +2201,15 @@ void qemu_aio_unref(void *p)
>  /**************************************************************/
>  /* Coroutine block device emulation */
>  
> +typedef struct FlushCo {
> +    BlockDriverState *bs;
> +    int ret;
> +} FlushCo;
> +
> +
>  static void coroutine_fn bdrv_flush_co_entry(void *opaque)
>  {
> -    RwCo *rwco = opaque;
> +    FlushCo *rwco = opaque;
>  
>      rwco->ret = bdrv_co_flush(rwco->bs);
>  }
> @@ -2288,7 +2293,7 @@ out:
>  int bdrv_flush(BlockDriverState *bs)
>  {
>      Coroutine *co;
> -    RwCo rwco = {
> +    FlushCo rwco = {
>          .bs = bs,
>          .ret = NOT_DONE,
>      };
> 

Renaming the variable to something other than rwco would make sense, but
I'll leave that up to you.

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 17/17] block: Convert bdrv_co_preadv/pwritev to BdrvChild

[Kevin Wolf]

This is the final patch for converting the common I/O path to take
a BdrvChild parameter instead of BlockDriverState.

The completion of this conversion means that all users that perform I/O
on an image need to actually hold a reference (in the form of BdrvChild,
possible as part of a BlockBackend) to that image. This also protects
against inconsistent use of BlockBackend vs. BlockDriverState functions
because direct use of a BlockDriverState isn't possible any more and
blk->root is private for block-backends.c.

In addition, we can now distinguish different users in the I/O path,
and the future op blockers work is going to add assertions based on
permissions stored in BdrvChild.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkreplay.c         |  6 +++---
 block/block-backend.c     |  4 ++--
 block/bochs.c             |  2 +-
 block/io.c                | 25 +++++++++++++------------
 block/qcow2-cluster.c     |  2 +-
 block/qcow2.c             |  6 +++---
 block/raw_bsd.c           |  4 ++--
 block/vdi.c               |  4 ++--
 block/vmdk.c              |  6 +++---
 block/vpc.c               |  8 ++++----
 include/block/block.h     |  4 ++--
 include/block/block_int.h |  4 ++--
 12 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/block/blkreplay.c b/block/blkreplay.c
index 196b8d0..70650e4 100755
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -85,7 +85,7 @@ static int coroutine_fn blkreplay_co_preadv(BlockDriverState *bs,
     uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
     uint64_t reqid = request_id++;
-    int ret = bdrv_co_preadv(bs->file->bs, offset, bytes, qiov, flags);
+    int ret = bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
 
@@ -96,7 +96,7 @@ static int coroutine_fn blkreplay_co_pwritev(BlockDriverState *bs,
     uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
     uint64_t reqid = request_id++;
-    int ret = bdrv_co_pwritev(bs->file->bs, offset, bytes, qiov, flags);
+    int ret = bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
 
@@ -107,7 +107,7 @@ static int coroutine_fn blkreplay_co_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int count, BdrvRequestFlags flags)
 {
     uint64_t reqid = request_id++;
-    int ret = bdrv_co_pwrite_zeroes(bs->file->bs, offset, count, flags);
+    int ret = bdrv_co_pwrite_zeroes(bs->file, offset, count, flags);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
 
diff --git a/block/block-backend.c b/block/block-backend.c
index 389b36b..8b74780 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -760,7 +760,7 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
         throttle_group_co_io_limits_intercept(blk, bytes, false);
     }
 
-    return bdrv_co_preadv(blk_bs(blk), offset, bytes, qiov, flags);
+    return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
 }
 
 int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
@@ -785,7 +785,7 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
         flags |= BDRV_REQ_FUA;
     }
 
-    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
+    return bdrv_co_pwritev(blk->root, offset, bytes, qiov, flags);
 }
 
 typedef struct BlkRwCo {
diff --git a/block/bochs.c b/block/bochs.c
index e9323ec7..8125461 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -251,7 +251,7 @@ bochs_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         qemu_iovec_concat(&local_qiov, qiov, bytes_done, 512);
 
         if (block_offset > 0) {
-            ret = bdrv_co_preadv(bs->file->bs, block_offset, 512,
+            ret = bdrv_co_preadv(bs->file, block_offset, 512,
                                  &local_qiov, 0);
             if (ret < 0) {
                 goto fail;
diff --git a/block/io.c b/block/io.c
index 78529fc..96d2b15 100644
--- a/block/io.c
+++ b/block/io.c
@@ -570,11 +570,11 @@ static void coroutine_fn bdrv_rw_co_entry(void *opaque)
     RwCo *rwco = opaque;
 
     if (!rwco->is_write) {
-        rwco->ret = bdrv_co_preadv(rwco->child->bs, rwco->offset,
+        rwco->ret = bdrv_co_preadv(rwco->child, rwco->offset,
                                    rwco->qiov->size, rwco->qiov,
                                    rwco->flags);
     } else {
-        rwco->ret = bdrv_co_pwritev(rwco->child->bs, rwco->offset,
+        rwco->ret = bdrv_co_pwritev(rwco->child, rwco->offset,
                                     rwco->qiov->size, rwco->qiov,
                                     rwco->flags);
     }
@@ -1057,10 +1057,11 @@ out:
 /*
  * Handle a read request in coroutine context
  */
-int coroutine_fn bdrv_co_preadv(BlockDriverState *bs,
+int coroutine_fn bdrv_co_preadv(BdrvChild *child,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags)
 {
+    BlockDriverState *bs = child->bs;
     BlockDriver *drv = bs->drv;
     BdrvTrackedRequest req;
 
@@ -1133,7 +1134,7 @@ static int coroutine_fn bdrv_co_do_readv(BdrvChild *child,
         return -EINVAL;
     }
 
-    return bdrv_co_preadv(child->bs, sector_num << BDRV_SECTOR_BITS,
+    return bdrv_co_preadv(child, sector_num << BDRV_SECTOR_BITS,
                           nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
 }
 
@@ -1398,10 +1399,11 @@ fail:
 /*
  * Handle a write request in coroutine context
  */
-int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
+int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags)
 {
+    BlockDriverState *bs = child->bs;
     BdrvTrackedRequest req;
     uint64_t align = bs->request_alignment;
     uint8_t *head_buf = NULL;
@@ -1535,7 +1537,7 @@ static int coroutine_fn bdrv_co_do_writev(BdrvChild *child,
         return -EINVAL;
     }
 
-    return bdrv_co_pwritev(child->bs, sector_num << BDRV_SECTOR_BITS,
+    return bdrv_co_pwritev(child, sector_num << BDRV_SECTOR_BITS,
                            nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
 }
 
@@ -1547,17 +1549,16 @@ int coroutine_fn bdrv_co_writev(BdrvChild *child, int64_t sector_num,
     return bdrv_co_do_writev(child, sector_num, nb_sectors, qiov, 0);
 }
 
-int coroutine_fn bdrv_co_pwrite_zeroes(BlockDriverState *bs,
-                                       int64_t offset, int count,
-                                       BdrvRequestFlags flags)
+int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
+                                       int count, BdrvRequestFlags flags)
 {
-    trace_bdrv_co_pwrite_zeroes(bs, offset, count, flags);
+    trace_bdrv_co_pwrite_zeroes(child->bs, offset, count, flags);
 
-    if (!(bs->open_flags & BDRV_O_UNMAP)) {
+    if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
         flags &= ~BDRV_REQ_MAY_UNMAP;
     }
 
-    return bdrv_co_pwritev(bs, offset, count, NULL,
+    return bdrv_co_pwritev(child, offset, count, NULL,
                            BDRV_REQ_ZERO_WRITE | flags);
 }
 
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 20ac99e..361f166 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -448,7 +448,7 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
-    ret = bdrv_co_pwritev(bs->file->bs, cluster_offset + offset_in_cluster,
+    ret = bdrv_co_pwritev(bs->file, cluster_offset + offset_in_cluster,
                           bytes, &qiov, 0);
     if (ret < 0) {
         goto out;
diff --git a/block/qcow2.c b/block/qcow2.c
index 4aab822..1dbdbb6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1442,7 +1442,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
 
                     BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
                     qemu_co_mutex_unlock(&s->lock);
-                    ret = bdrv_co_preadv(bs->backing->bs, offset, n1,
+                    ret = bdrv_co_preadv(bs->backing, offset, n1,
                                          &local_qiov, 0);
                     qemu_co_mutex_lock(&s->lock);
 
@@ -1505,7 +1505,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
 
             BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
             qemu_co_mutex_unlock(&s->lock);
-            ret = bdrv_co_preadv(bs->file->bs,
+            ret = bdrv_co_preadv(bs->file,
                                  cluster_offset + offset_in_cluster,
                                  cur_bytes, &hd_qiov, 0);
             qemu_co_mutex_lock(&s->lock);
@@ -1636,7 +1636,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
         BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
         trace_qcow2_writev_data(qemu_coroutine_self(),
                                 cluster_offset + offset_in_cluster);
-        ret = bdrv_co_pwritev(bs->file->bs,
+        ret = bdrv_co_pwritev(bs->file,
                               cluster_offset + offset_in_cluster,
                               cur_bytes, &hd_qiov, 0);
         qemu_co_mutex_lock(&s->lock);
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 8a943a4..450bbb0 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -105,7 +105,7 @@ raw_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
-    ret = bdrv_co_pwritev(bs->file->bs, sector_num * BDRV_SECTOR_SIZE,
+    ret = bdrv_co_pwritev(bs->file, sector_num * BDRV_SECTOR_SIZE,
                           nb_sectors * BDRV_SECTOR_SIZE, qiov, flags);
 
 fail:
@@ -131,7 +131,7 @@ static int coroutine_fn raw_co_pwrite_zeroes(BlockDriverState *bs,
                                              int64_t offset, int count,
                                              BdrvRequestFlags flags)
 {
-    return bdrv_co_pwrite_zeroes(bs->file->bs, offset, count, flags);
+    return bdrv_co_pwrite_zeroes(bs->file, offset, count, flags);
 }
 
 static int coroutine_fn raw_co_discard(BlockDriverState *bs,
diff --git a/block/vdi.c b/block/vdi.c
index a1a12d9..8a1cf97 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -597,7 +597,7 @@ vdi_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
             qemu_iovec_reset(&local_qiov);
             qemu_iovec_concat(&local_qiov, qiov, bytes_done, n_bytes);
 
-            ret = bdrv_co_preadv(bs->file->bs, data_offset, n_bytes,
+            ret = bdrv_co_preadv(bs->file, data_offset, n_bytes,
                                  &local_qiov, 0);
         }
         logout("%u bytes read\n", n_bytes);
@@ -690,7 +690,7 @@ vdi_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
             qemu_iovec_reset(&local_qiov);
             qemu_iovec_concat(&local_qiov, qiov, bytes_done, n_bytes);
 
-            ret = bdrv_co_pwritev(bs->file->bs, data_offset, n_bytes,
+            ret = bdrv_co_pwritev(bs->file, data_offset, n_bytes,
                                   &local_qiov, 0);
         }
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 094013f..a37c85c 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1371,7 +1371,7 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
     }
 
     write_offset = cluster_offset + offset_in_cluster,
-    ret = bdrv_co_pwritev(extent->file->bs, write_offset, n_bytes,
+    ret = bdrv_co_pwritev(extent->file, write_offset, n_bytes,
                           &local_qiov, 0);
 
     write_end_sector = DIV_ROUND_UP(write_offset + n_bytes, BDRV_SECTOR_SIZE);
@@ -1409,7 +1409,7 @@ static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
 
 
     if (!extent->compressed) {
-        ret = bdrv_co_preadv(extent->file->bs,
+        ret = bdrv_co_preadv(extent->file,
                              cluster_offset + offset_in_cluster, bytes,
                              qiov, 0);
         if (ret < 0) {
@@ -1499,7 +1499,7 @@ vmdk_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
                 qemu_iovec_reset(&local_qiov);
                 qemu_iovec_concat(&local_qiov, qiov, bytes_done, n_bytes);
 
-                ret = bdrv_co_preadv(bs->backing->bs, offset, n_bytes,
+                ret = bdrv_co_preadv(bs->backing, offset, n_bytes,
                                      &local_qiov, 0);
                 if (ret < 0) {
                     goto fail;
diff --git a/block/vpc.c b/block/vpc.c
index dcb5312..43707ed 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -591,7 +591,7 @@ vpc_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     QEMUIOVector local_qiov;
 
     if (be32_to_cpu(footer->type) == VHD_FIXED) {
-        return bdrv_co_preadv(bs->file->bs, offset, bytes, qiov, 0);
+        return bdrv_co_preadv(bs->file, offset, bytes, qiov, 0);
     }
 
     qemu_co_mutex_lock(&s->lock);
@@ -607,7 +607,7 @@ vpc_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
             qemu_iovec_reset(&local_qiov);
             qemu_iovec_concat(&local_qiov, qiov, bytes_done, n_bytes);
 
-            ret = bdrv_co_preadv(bs->file->bs, image_offset, n_bytes,
+            ret = bdrv_co_preadv(bs->file, image_offset, n_bytes,
                                  &local_qiov, 0);
             if (ret < 0) {
                 goto fail;
@@ -640,7 +640,7 @@ vpc_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     QEMUIOVector local_qiov;
 
     if (be32_to_cpu(footer->type) == VHD_FIXED) {
-        return bdrv_co_pwritev(bs->file->bs, offset, bytes, qiov, 0);
+        return bdrv_co_pwritev(bs->file, offset, bytes, qiov, 0);
     }
 
     qemu_co_mutex_lock(&s->lock);
@@ -661,7 +661,7 @@ vpc_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         qemu_iovec_reset(&local_qiov);
         qemu_iovec_concat(&local_qiov, qiov, bytes_done, n_bytes);
 
-        ret = bdrv_co_pwritev(bs->file->bs, image_offset, n_bytes,
+        ret = bdrv_co_pwritev(bs->file, image_offset, n_bytes,
                               &local_qiov, 0);
         if (ret < 0) {
             goto fail;
diff --git a/include/block/block.h b/include/block/block.h
index abc5f26..12de4ea 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -249,8 +249,8 @@ int coroutine_fn bdrv_co_writev(BdrvChild *child, int64_t sector_num,
  * function is not suitable for zeroing the entire image in a single request
  * because it may allocate memory for the entire region.
  */
-int coroutine_fn bdrv_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
-    int count, BdrvRequestFlags flags);
+int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
+                                       int count, 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 688c6be..fc11732 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -544,10 +544,10 @@ extern BlockDriver bdrv_qcow2;
  */
 void bdrv_setup_io_funcs(BlockDriver *bdrv);
 
-int coroutine_fn bdrv_co_preadv(BlockDriverState *bs,
+int coroutine_fn bdrv_co_preadv(BdrvChild *child,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags);
-int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
+int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Paolo Bonzini]

On 21/06/2016 11:21, Kevin Wolf wrote:
> This series converts all I/O function in the core block layer up to
> bdrv_co_preadv/pwritev() to taking a BdrvChild as their first parameter
> instead of a BlockDriverState.
> 
> The original motivation for this change were op blockers, where one of
> the biggest problems is making sure that every user of block devices
> actually registers correctly with the op blockers system. If the I/O
> functions know which parent a request comes from (BdrvChild basically
> corresponds to an edge in our block device graph), it can use assertions
> to make sure that that parent has actually registered its activities and
> thereby ensured that it doesn't conflict with other users.
> 
> There are, however, more benefits we get from this change. The most
> important one is probably that it enforces important aspects of the
> block layer design like that external users go through a BlockBackend
> and request are internally routed along the edges of the graph. Accesses
> to random BDSes are no longer possible, you need to own an actual child
> reference so you can make a request.

I still fail to understand what is the rationale for this change.  The
API is weird; you read from a disk, not from an edge, and in fact the
first thing all the APIs do is dereference the BdrvChild...

The assertions are nice, but I'm not sure it's a good idea to design a
whole API around them.

Paolo

> The work on this series already led to a few cleanups and BlockBackend
> conversions in master, and this series contains a few more.
> 
> As a bonus, all the block drivers using bs->file->bs everywhere can now
> go back to bs->file, which is a little nicer to read.

Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Kevin Wolf]

Am 21.06.2016 um 11:47 hat Paolo Bonzini geschrieben:
> 
> 
> On 21/06/2016 11:21, Kevin Wolf wrote:
> > This series converts all I/O function in the core block layer up to
> > bdrv_co_preadv/pwritev() to taking a BdrvChild as their first parameter
> > instead of a BlockDriverState.
> > 
> > The original motivation for this change were op blockers, where one of
> > the biggest problems is making sure that every user of block devices
> > actually registers correctly with the op blockers system. If the I/O
> > functions know which parent a request comes from (BdrvChild basically
> > corresponds to an edge in our block device graph), it can use assertions
> > to make sure that that parent has actually registered its activities and
> > thereby ensured that it doesn't conflict with other users.
> > 
> > There are, however, more benefits we get from this change. The most
> > important one is probably that it enforces important aspects of the
> > block layer design like that external users go through a BlockBackend
> > and request are internally routed along the edges of the graph. Accesses
> > to random BDSes are no longer possible, you need to own an actual child
> > reference so you can make a request.
> 
> I still fail to understand what is the rationale for this change.  The
> API is weird; you read from a disk, not from an edge, and in fact the
> first thing all the APIs do is dereference the BdrvChild...
> 
> The assertions are nice, but I'm not sure it's a good idea to design a
> whole API around them.

Do you see a problem with such an API, though? If there is no reason not
to have the advantages, as small as they may seem, why not take them?

(By the way, I disagree that assertions for op blockers are just "nice
to have". I would never be confident that we covered everything without
them.)

Apart from that, I actually think the conversion has already done its
most important job, which is uncovering the places where we employed
hacks that violate our design. With this series compiling and working,
I'm even confident to say that everything that should be using
BlockBackend, does so by now (now = with the series applied).

But now that the patches exist, even if you dismiss the value of op
blocker assertions, applying them in order to keep us honest to the
design even in the future seems appealing to me.

Well, unless you show me an example of something that makes sense, is
compatible with the block layer design and would still be incorrectly
blocked by the requirement to have a BdrvChild. Then I might change my
opinion.

Kevin

Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Paolo Bonzini]

On 21/06/2016 12:56, Kevin Wolf wrote:
> Am 21.06.2016 um 11:47 hat Paolo Bonzini geschrieben:
>>
>>
>> On 21/06/2016 11:21, Kevin Wolf wrote:
>>> This series converts all I/O function in the core block layer up to
>>> bdrv_co_preadv/pwritev() to taking a BdrvChild as their first parameter
>>> instead of a BlockDriverState.
>>>
>>> The original motivation for this change were op blockers, where one of
>>> the biggest problems is making sure that every user of block devices
>>> actually registers correctly with the op blockers system. If the I/O
>>> functions know which parent a request comes from (BdrvChild basically
>>> corresponds to an edge in our block device graph), it can use assertions
>>> to make sure that that parent has actually registered its activities and
>>> thereby ensured that it doesn't conflict with other users.
>>>
>>> There are, however, more benefits we get from this change. The most
>>> important one is probably that it enforces important aspects of the
>>> block layer design like that external users go through a BlockBackend
>>> and request are internally routed along the edges of the graph. Accesses
>>> to random BDSes are no longer possible, you need to own an actual child
>>> reference so you can make a request.
>>
>> I still fail to understand what is the rationale for this change.  The
>> API is weird; you read from a disk, not from an edge, and in fact the
>> first thing all the APIs do is dereference the BdrvChild...
>>
>> The assertions are nice, but I'm not sure it's a good idea to design a
>> whole API around them.
> 
> Do you see a problem with such an API, though? If there is no reason not
> to have the advantages, as small as they may seem, why not take them?

I don't see a reason not to take them; I don't see any red flags, but
there are some yellow flags (the kinda weird API) that I don't
understand and I hope you can explain.

Thinking more about it, it's perfectly possible that this is just a
combination of block/io.c's growth by accretion and the well-known fact
"naming pseudo-OOP member functions in C sucks".

In other words, if you sell me this as "let's add some member functions
to BdrvChild and use them", I can buy it.  Perhaps the only thing to do
then is to rename functions and design a consistent naming.

Paolo

Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Kevin Wolf]

Am 21.06.2016 um 13:01 hat Paolo Bonzini geschrieben:
> On 21/06/2016 12:56, Kevin Wolf wrote:
> > Am 21.06.2016 um 11:47 hat Paolo Bonzini geschrieben:
> >> I still fail to understand what is the rationale for this change.  The
> >> API is weird; you read from a disk, not from an edge, and in fact the
> >> first thing all the APIs do is dereference the BdrvChild...
> >>
> >> The assertions are nice, but I'm not sure it's a good idea to design a
> >> whole API around them.
> > 
> > Do you see a problem with such an API, though? If there is no reason not
> > to have the advantages, as small as they may seem, why not take them?
> 
> I don't see a reason not to take them; I don't see any red flags, but
> there are some yellow flags (the kinda weird API) that I don't
> understand and I hope you can explain.
> 
> Thinking more about it, it's perfectly possible that this is just a
> combination of block/io.c's growth by accretion and the well-known fact
> "naming pseudo-OOP member functions in C sucks".
> 
> In other words, if you sell me this as "let's add some member functions
> to BdrvChild and use them", I can buy it.  Perhaps the only thing to do
> then is to rename functions and design a consistent naming.

Hm, I never thought about it this way, but I think it actually makes
sense.

As we want to represent a graph where both nodes and edges can have
attributes and methods, OOP-wise both of them are objects, namely BDS
and BdrvChild.

So we have some BDS A that has a Child B, and Child B in turn has a
BDS C. What we used to do is that A asks B for the node it points to
(C), and then directly calls a method of C. After the conversion, A
calls a method of B, which in turn forwards the request by calling a
method of C, which is much more straightforward and ideally even allows
the node that B points to to remain private (we're not quite there,
though).

Kevin

Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Fam Zheng]

On Tue, 06/21 13:31, Kevin Wolf wrote:
> Am 21.06.2016 um 13:01 hat Paolo Bonzini geschrieben:
> > On 21/06/2016 12:56, Kevin Wolf wrote:
> > > Am 21.06.2016 um 11:47 hat Paolo Bonzini geschrieben:
> > >> I still fail to understand what is the rationale for this change.  The
> > >> API is weird; you read from a disk, not from an edge, and in fact the
> > >> first thing all the APIs do is dereference the BdrvChild...
> > >>
> > >> The assertions are nice, but I'm not sure it's a good idea to design a
> > >> whole API around them.
> > > 
> > > Do you see a problem with such an API, though? If there is no reason not
> > > to have the advantages, as small as they may seem, why not take them?
> > 
> > I don't see a reason not to take them; I don't see any red flags, but
> > there are some yellow flags (the kinda weird API) that I don't
> > understand and I hope you can explain.
> > 
> > Thinking more about it, it's perfectly possible that this is just a
> > combination of block/io.c's growth by accretion and the well-known fact
> > "naming pseudo-OOP member functions in C sucks".

I think this interface makes a lot of sense to me - I have been looking forward
to this model since three or four years ago, just without knowing ahead that
the object will be called BdrvChild, and that they'll occupy the first
parameter of bdrv_* API. I'm glad we are here today.

> > 
> > In other words, if you sell me this as "let's add some member functions
> > to BdrvChild and use them", I can buy it.  Perhaps the only thing to do
> > then is to rename functions and design a consistent naming.

And so I agree on renaming functions. :)

Fam

> 
> Hm, I never thought about it this way, but I think it actually makes
> sense.
> 
> As we want to represent a graph where both nodes and edges can have
> attributes and methods, OOP-wise both of them are objects, namely BDS
> and BdrvChild.
> 
> So we have some BDS A that has a Child B, and Child B in turn has a
> BDS C. What we used to do is that A asks B for the node it points to
> (C), and then directly calls a method of C. After the conversion, A
> calls a method of B, which in turn forwards the request by calling a
> method of C, which is much more straightforward and ideally even allows
> the node that B points to to remain private (we're not quite there,
> though).

Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Stefan Hajnoczi]

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

On Tue, Jun 21, 2016 at 11:21:17AM +0200, Kevin Wolf wrote:
> This series converts all I/O function in the core block layer up to
> bdrv_co_preadv/pwritev() to taking a BdrvChild as their first parameter
> instead of a BlockDriverState.
> 
> The original motivation for this change were op blockers, where one of
> the biggest problems is making sure that every user of block devices
> actually registers correctly with the op blockers system. If the I/O
> functions know which parent a request comes from (BdrvChild basically
> corresponds to an edge in our block device graph), it can use assertions
> to make sure that that parent has actually registered its activities and
> thereby ensured that it doesn't conflict with other users.
> 
> There are, however, more benefits we get from this change. The most
> important one is probably that it enforces important aspects of the
> block layer design like that external users go through a BlockBackend
> and request are internally routed along the edges of the graph. Accesses
> to random BDSes are no longer possible, you need to own an actual child
> reference so you can make a request.
> 
> The work on this series already led to a few cleanups and BlockBackend
> conversions in master, and this series contains a few more.
> 
> As a bonus, all the block drivers using bs->file->bs everywhere can now
> go back to bs->file, which is a little nicer to read.
> 
> Kevin Wolf (17):
>   vvfat: Use BdrvChild for s->qcow
>   blkreplay: Convert to byte-based I/O
>   vhdx: Some more BlockBackend use in vhdx_create()
>   block: Convert bdrv_co_readv() to BdrvChild
>   block: Convert bdrv_co_writev() to BdrvChild
>   block: Convert bdrv_aio_readv() to BdrvChild
>   block: Convert bdrv_aio_writev() to BdrvChild
>   block: Convert bdrv_co_do_readv/writev to BdrvChild
>   block: Move bdrv_commit() to block/commit.c
>   block: Use BlockBackend for I/O in bdrv_commit()
>   block: Convert bdrv_read() to BdrvChild
>   block: Convert bdrv_write() to BdrvChild
>   block: Convert bdrv_pread(v) to BdrvChild
>   block: Convert bdrv_pwrite(v/_sync) to BdrvChild
>   block: Convert bdrv_pwrite_zeroes() to BdrvChild
>   block: Convert bdrv_prwv_co() to BdrvChild
>   block: Convert bdrv_co_preadv/pwritev to BdrvChild
> 
>  block.c                        | 117 ++----------------------------------
>  block/Makefile.objs            |   3 +-
>  block/blkdebug.c               |   4 +-
>  block/blkreplay.c              |  18 +++---
>  block/blkverify.c              |   8 +--
>  block/block-backend.c          |   9 ++-
>  block/bochs.c                  |   8 +--
>  block/cloop.c                  |   8 +--
>  block/commit.c                 | 121 +++++++++++++++++++++++++++++++++++++
>  block/crypto.c                 |   6 +-
>  block/dmg.c                    |  21 +++----
>  block/io.c                     | 132 ++++++++++++++++++++++-------------------
>  block/parallels.c              |  16 ++---
>  block/qcow.c                   |  41 +++++++------
>  block/qcow2-cache.c            |   4 +-
>  block/qcow2-cluster.c          |  18 +++---
>  block/qcow2-refcount.c         |  36 +++++------
>  block/qcow2-snapshot.c         |  26 ++++----
>  block/qcow2.c                  |  50 +++++++++-------
>  block/qed-table.c              |   4 +-
>  block/qed.c                    |  22 +++----
>  block/quorum.c                 |   8 +--
>  block/raw_bsd.c                |   6 +-
>  block/vdi.c                    |  14 ++---
>  block/vhdx-log.c               |  12 ++--
>  block/vhdx.c                   |  85 ++++++++++++++------------
>  block/vmdk.c                   |  54 ++++++++---------
>  block/vpc.c                    |  24 ++++----
>  block/vvfat.c                  |  61 ++++++++++++-------
>  include/block/block.h          |  38 ++++++------
>  include/block/block_int.h      |   4 +-
>  include/sysemu/block-backend.h |   1 +
>  qemu-img.c                     |   2 +-
>  33 files changed, 522 insertions(+), 459 deletions(-)

Do you want to take it through your tree to avoid
conflicts/dependencies?

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Kevin Wolf]

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

Am 28.06.2016 um 15:28 hat Stefan Hajnoczi geschrieben:
> Do you want to take it through your tree to avoid
> conflicts/dependencies?
> 
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

Yes, thanks. I've applied it to my tree now (with a few fixes addressing
the review comments).

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]