[Qemu-devel] [PATCH 0/6] block: bdrv_load/save_vmstate() cleanups

[Qemu-devel] [PATCH 0/6] block: bdrv_load/save_vmstate() cleanups

[Kevin Wolf]

This series contains a few cleanups with respect to the vmstate I/O functions.
Apart from making the interface more consistent (writes were already vectored,
but not reads), this makes use of the new byte-based .bdrv_co_preadv/pwritev
callbacks in qcow2 to get rid of a few hacks, including bs->zero_beyond_eof.

Kevin Wolf (6):
  block: Introduce bdrv_preadv()
  block: Make .bdrv_load_vmstate() vectored
  block: Allow .bdrv_load/save_vmstate() to return 0/-errno
  block: Make bdrv_load/save_vmstate coroutine_fns
  qcow2: Let vmstate call qcow2_co_preadv/pwrite directly
  block: Remove bs->zero_beyond_eof

 block.c                   |   2 -
 block/io.c                | 173 +++++++++++++++++++++++++++++++---------------
 block/qcow2.c             |  28 ++------
 block/sheepdog.c          |  13 +++-
 include/block/block.h     |   2 +
 include/block/block_int.h |  13 ++--
 6 files changed, 143 insertions(+), 88 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/6] block: Introduce bdrv_preadv()

[Kevin Wolf]

We already have a byte-based bdrv_pwritev(), but the read counterpart
was still missing. This commit adds it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c            | 20 +++++++++++++-------
 include/block/block.h |  1 +
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index b3d6228..11510cf 100644
--- a/block/io.c
+++ b/block/io.c
@@ -700,6 +700,18 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
     }
 }
 
+int bdrv_preadv(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
+{
+    int ret;
+
+    ret = bdrv_prwv_co(bs, offset, qiov, false, 0);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return qiov->size;
+}
+
 int bdrv_pread(BlockDriverState *bs, int64_t offset, void *buf, int bytes)
 {
     QEMUIOVector qiov;
@@ -707,19 +719,13 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset, void *buf, int bytes)
         .iov_base = (void *)buf,
         .iov_len = bytes,
     };
-    int ret;
 
     if (bytes < 0) {
         return -EINVAL;
     }
 
     qemu_iovec_init_external(&qiov, &iov, 1);
-    ret = bdrv_prwv_co(bs, offset, &qiov, false, 0);
-    if (ret < 0) {
-        return ret;
-    }
-
-    return bytes;
+    return bdrv_preadv(bs, offset, &qiov);
 }
 
 int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
diff --git a/include/block/block.h b/include/block/block.h
index fb0078f..aca7f23 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -232,6 +232,7 @@ int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
 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_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 1/6] block: Introduce bdrv_preadv()

[Eric Blake]

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

On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> We already have a byte-based bdrv_pwritev(), but the read counterpart
> was still missing. This commit adds it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c            | 20 +++++++++++++-------
>  include/block/block.h |  1 +
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 

Worth adding a flags argument while at it? But bdrv_pwritev() lacks one,
so for symmetry reasons, I'm okay if you don't bother.

> +int bdrv_preadv(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
> +{
> +    int ret;
> +
> +    ret = bdrv_prwv_co(bs, offset, qiov, false, 0);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return qiov->size;

This implies we never have a short read, it's an all-or-none error or
success.  Matches what we've done elsewhere, so I guess it's right.

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 2/6] block: Make .bdrv_load_vmstate() vectored

[Kevin Wolf]

This brings it in line with .bdrv_save_vmstate().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c                | 26 +++++++++++++++++++++-----
 block/qcow2.c             |  6 +++---
 block/sheepdog.c          | 13 ++++++++++---
 include/block/block.h     |  1 +
 include/block/block_int.h |  4 ++--
 5 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/block/io.c b/block/io.c
index 11510cf..602c7d3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1862,13 +1862,29 @@ int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
 int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
                       int64_t pos, int size)
 {
+    QEMUIOVector qiov;
+    struct iovec iov = {
+        .iov_base   = buf,
+        .iov_len    = size,
+    };
+    int ret;
+
+    qemu_iovec_init_external(&qiov, &iov, 1);
+    return bdrv_readv_vmstate(bs, &qiov, pos);
+}
+
+int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
+{
     BlockDriver *drv = bs->drv;
-    if (!drv)
+
+    if (!drv) {
         return -ENOMEDIUM;
-    if (drv->bdrv_load_vmstate)
-        return drv->bdrv_load_vmstate(bs, buf, pos, size);
-    if (bs->file)
-        return bdrv_load_vmstate(bs->file->bs, buf, pos, size);
+    } else if (drv->bdrv_load_vmstate) {
+        return drv->bdrv_load_vmstate(bs, qiov, pos);
+    } else if (bs->file) {
+        return bdrv_readv_vmstate(bs->file->bs, qiov, pos);
+    }
+
     return -ENOTSUP;
 }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index cb55e2d..72ae2bf 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2920,8 +2920,8 @@ static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
     return ret;
 }
 
-static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
-                              int64_t pos, int size)
+static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
+                              int64_t pos)
 {
     BDRVQcow2State *s = bs->opaque;
     bool zero_beyond_eof = bs->zero_beyond_eof;
@@ -2929,7 +2929,7 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
 
     BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD);
     bs->zero_beyond_eof = false;
-    ret = bdrv_pread(bs, qcow2_vm_state_offset(s) + pos, buf, size);
+    ret = bdrv_preadv(bs, qcow2_vm_state_offset(s) + pos, qiov);
     bs->zero_beyond_eof = zero_beyond_eof;
 
     return ret;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 23fbace..ef5d044 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2784,12 +2784,19 @@ static int sd_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
     return ret;
 }
 
-static int sd_load_vmstate(BlockDriverState *bs, uint8_t *data,
-                           int64_t pos, int size)
+static int sd_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
+                           int64_t pos)
 {
     BDRVSheepdogState *s = bs->opaque;
+    void *buf;
+    int ret;
 
-    return do_load_save_vmstate(s, data, pos, size, 1);
+    buf = qemu_blockalign(bs, qiov->size);
+    ret = do_load_save_vmstate(s, buf, pos, qiov->size, 1);
+    qemu_iovec_from_buf(qiov, 0, buf, qiov->size);
+    qemu_vfree(buf);
+
+    return ret;
 }
 
 
diff --git a/include/block/block.h b/include/block/block.h
index aca7f23..a158575 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -428,6 +428,7 @@ void path_combine(char *dest, int dest_size,
                   const char *base_path,
                   const char *filename);
 
+int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
 int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8a4963c..f9a32cc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -226,8 +226,8 @@ struct BlockDriver {
 
     int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
                              int64_t pos);
-    int (*bdrv_load_vmstate)(BlockDriverState *bs, uint8_t *buf,
-                             int64_t pos, int size);
+    int (*bdrv_load_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
+                             int64_t pos);
 
     int (*bdrv_change_backing_file)(BlockDriverState *bs,
         const char *backing_file, const char *backing_fmt);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/6] block: Make .bdrv_load_vmstate() vectored

[Eric Blake]

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

On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> This brings it in line with .bdrv_save_vmstate().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c                | 26 +++++++++++++++++++++-----
>  block/qcow2.c             |  6 +++---
>  block/sheepdog.c          | 13 ++++++++++---
>  include/block/block.h     |  1 +
>  include/block/block_int.h |  4 ++--
>  5 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 11510cf..602c7d3 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1862,13 +1862,29 @@ int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
>  int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
>                        int64_t pos, int size)
>  {
> +    QEMUIOVector qiov;
> +    struct iovec iov = {
> +        .iov_base   = buf,
> +        .iov_len    = size,
> +    };
> +    int ret;

Dead variable.

> +
> +    qemu_iovec_init_external(&qiov, &iov, 1);
> +    return bdrv_readv_vmstate(bs, &qiov, pos);
> +}
> +
> +int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
> +{
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> +
> +    if (!drv) {
>          return -ENOMEDIUM;
> -    if (drv->bdrv_load_vmstate)
> -        return drv->bdrv_load_vmstate(bs, buf, pos, size);
> -    if (bs->file)
> -        return bdrv_load_vmstate(bs->file->bs, buf, pos, size);
> +    } else if (drv->bdrv_load_vmstate) {
> +        return drv->bdrv_load_vmstate(bs, qiov, pos);
> +    } else if (bs->file) {
> +        return bdrv_readv_vmstate(bs->file->bs, qiov, pos);
> +    }

Don't know that I would have used 'else if' after a return, but it's not
wrong, so no need to change.

With the dead 'ret' gone,
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 3/6] block: Allow .bdrv_load/save_vmstate() to return 0/-errno

[Kevin Wolf]

The return value of .bdrv_load/save_vmstate() can be any non-negative
number in case of success now. It used to be bytes/-errno.

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

diff --git a/block/io.c b/block/io.c
index 602c7d3..bca244c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1839,9 +1839,16 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
         .iov_base   = (void *) buf,
         .iov_len    = size,
     };
+    int ret;
 
     qemu_iovec_init_external(&qiov, &iov, 1);
-    return bdrv_writev_vmstate(bs, &qiov, pos);
+
+    ret = bdrv_writev_vmstate(bs, &qiov, pos);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return size;
 }
 
 int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
@@ -1870,7 +1877,12 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
     int ret;
 
     qemu_iovec_init_external(&qiov, &iov, 1);
-    return bdrv_readv_vmstate(bs, &qiov, pos);
+    ret = bdrv_readv_vmstate(bs, &qiov, pos);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return size;
 }
 
 int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/6] block: Allow .bdrv_load/save_vmstate() to return 0/-errno

[Eric Blake]

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

On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> The return value of .bdrv_load/save_vmstate() can be any non-negative
> number in case of success now. It used to be bytes/-errno.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 602c7d3..bca244c 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1839,9 +1839,16 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
>          .iov_base   = (void *) buf,
>          .iov_len    = size,
>      };
> +    int ret;
>  
>      qemu_iovec_init_external(&qiov, &iov, 1);
> -    return bdrv_writev_vmstate(bs, &qiov, pos);
> +
> +    ret = bdrv_writev_vmstate(bs, &qiov, pos);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return size;
>  }
>  
>  int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
> @@ -1870,7 +1877,12 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
>      int ret;

Aha, my complaint in v2 about it being dead means you need to reinstate
it here.

>  
>      qemu_iovec_init_external(&qiov, &iov, 1);
> -    return bdrv_readv_vmstate(bs, &qiov, pos);
> +    ret = bdrv_readv_vmstate(bs, &qiov, pos);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return size;
>  }
>  
>  int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
> 

Matches the semantics we have elsewhere (I'm not sure if 'size' is the
best choice if we ever need to support short read/write, but doesn't
seem to hurt).

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 4/6] block: Make bdrv_load/save_vmstate coroutine_fns

[Kevin Wolf]

This allows drivers to share code between normal I/O and vmstate
accesses.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c                | 80 ++++++++++++++++++++++++++++++++++-------------
 include/block/block_int.h | 10 +++---
 2 files changed, 64 insertions(+), 26 deletions(-)

diff --git a/block/io.c b/block/io.c
index bca244c..adf2726 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1831,6 +1831,62 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
     return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
 }
 
+typedef struct BdrvVmstateCo {
+    BlockDriverState    *bs;
+    QEMUIOVector        *qiov;
+    int64_t             pos;
+    bool                is_read;
+    int                 ret;
+} BdrvVmstateCo;
+
+static int coroutine_fn
+bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
+                   bool is_read)
+{
+    BlockDriver *drv = bs->drv;
+
+    if (!drv) {
+        return -ENOMEDIUM;
+    } else if (drv->bdrv_load_vmstate) {
+        return is_read ? drv->bdrv_load_vmstate(bs, qiov, pos)
+                       : drv->bdrv_save_vmstate(bs, qiov, pos);
+    } else if (bs->file) {
+        return bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
+    }
+
+    return -ENOTSUP;
+}
+
+static void bdrv_co_rw_vmstate_entry(void *opaque)
+{
+    BdrvVmstateCo *co = opaque;
+    co->ret = bdrv_co_rw_vmstate(co->bs, co->qiov, co->pos, co->is_read);
+}
+
+static inline int
+bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
+                bool is_read)
+{
+    if (qemu_in_coroutine()) {
+        return bdrv_co_rw_vmstate(bs, qiov, pos, is_read);
+    } else {
+        BdrvVmstateCo data = {
+            .bs         = bs,
+            .qiov       = qiov,
+            .pos        = pos,
+            .is_read    = is_read,
+            .ret        = -EINPROGRESS,
+        };
+        Coroutine *co = qemu_coroutine_create(bdrv_co_rw_vmstate_entry);
+
+        qemu_coroutine_enter(co, &data);
+        while (data.ret == -EINPROGRESS) {
+            aio_poll(bdrv_get_aio_context(bs), true);
+        }
+        return data.ret;
+    }
+}
+
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size)
 {
@@ -1853,17 +1909,7 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
 
 int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
 {
-    BlockDriver *drv = bs->drv;
-
-    if (!drv) {
-        return -ENOMEDIUM;
-    } else if (drv->bdrv_save_vmstate) {
-        return drv->bdrv_save_vmstate(bs, qiov, pos);
-    } else if (bs->file) {
-        return bdrv_writev_vmstate(bs->file->bs, qiov, pos);
-    }
-
-    return -ENOTSUP;
+    return bdrv_rw_vmstate(bs, qiov, pos, false);
 }
 
 int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
@@ -1887,17 +1933,7 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
 
 int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
 {
-    BlockDriver *drv = bs->drv;
-
-    if (!drv) {
-        return -ENOMEDIUM;
-    } else if (drv->bdrv_load_vmstate) {
-        return drv->bdrv_load_vmstate(bs, qiov, pos);
-    } else if (bs->file) {
-        return bdrv_readv_vmstate(bs->file->bs, qiov, pos);
-    }
-
-    return -ENOTSUP;
+    return bdrv_rw_vmstate(bs, qiov, pos, true);
 }
 
 /**************************************************************/
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f9a32cc..1fe0811 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -224,10 +224,12 @@ struct BlockDriver {
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
     ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
 
-    int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
-                             int64_t pos);
-    int (*bdrv_load_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
-                             int64_t pos);
+    int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
+                                          QEMUIOVector *qiov,
+                                          int64_t pos);
+    int coroutine_fn (*bdrv_load_vmstate)(BlockDriverState *bs,
+                                          QEMUIOVector *qiov,
+                                          int64_t pos);
 
     int (*bdrv_change_backing_file)(BlockDriverState *bs,
         const char *backing_file, const char *backing_fmt);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/6] block: Make bdrv_load/save_vmstate coroutine_fns

[Eric Blake]

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

On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> This allows drivers to share code between normal I/O and vmstate
> accesses.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c                | 80 ++++++++++++++++++++++++++++++++++-------------
>  include/block/block_int.h | 10 +++---
>  2 files changed, 64 insertions(+), 26 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 --]

Re: [Qemu-devel] [Qemu-block] [PATCH 4/6] block: Make bdrv_load/save_vmstate coroutine_fns

[Stefan Hajnoczi]

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

On Fri, Jun 10, 2016 at 06:05:20PM +0200, Kevin Wolf wrote:
> +static int coroutine_fn
> +bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
> +                   bool is_read)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    if (!drv) {
> +        return -ENOMEDIUM;
> +    } else if (drv->bdrv_load_vmstate) {
> +        return is_read ? drv->bdrv_load_vmstate(bs, qiov, pos)
> +                       : drv->bdrv_save_vmstate(bs, qiov, pos);
> +    } else if (bs->file) {
> +        return bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
> +    }
> +
> +    return -ENOTSUP;
> +}
> +
> +static void bdrv_co_rw_vmstate_entry(void *opaque)

This should also be coroutine_fn.

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

[Qemu-devel] [PATCH 5/6] qcow2: Let vmstate call qcow2_co_preadv/pwrite directly

[Kevin Wolf]

We don't really want to go through the block layer in order to read from
or write to the vmstate in a qcow2 image. Doing so required a few ugly
hacks like saving and restoring the old image size (because writing to
vmstate offsets would increase the image size) or disabling the "reads
after EOF = zeroes" logic. When calling the right functions directly,
these hacks aren't necessary any more.

Note that .bdrv_vmstate_load/save() return 0 instead of the number of
bytes in case of success now.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 72ae2bf..c40baca 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2903,36 +2903,20 @@ static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
                               int64_t pos)
 {
     BDRVQcow2State *s = bs->opaque;
-    int64_t total_sectors = bs->total_sectors;
-    bool zero_beyond_eof = bs->zero_beyond_eof;
-    int ret;
 
     BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
-    bs->zero_beyond_eof = false;
-    ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
-    bs->zero_beyond_eof = zero_beyond_eof;
-
-    /* bdrv_co_do_writev will have increased the total_sectors value to include
-     * the VM state - the VM state is however not an actual part of the block
-     * device, therefore, we need to restore the old value. */
-    bs->total_sectors = total_sectors;
-
-    return ret;
+    return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos,
+                                    qiov->size, qiov, 0);
 }
 
 static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
                               int64_t pos)
 {
     BDRVQcow2State *s = bs->opaque;
-    bool zero_beyond_eof = bs->zero_beyond_eof;
-    int ret;
 
     BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD);
-    bs->zero_beyond_eof = false;
-    ret = bdrv_preadv(bs, qcow2_vm_state_offset(s) + pos, qiov);
-    bs->zero_beyond_eof = zero_beyond_eof;
-
-    return ret;
+    return bs->drv->bdrv_co_preadv(bs, qcow2_vm_state_offset(s) + pos,
+                                   qiov->size, qiov, 0);
 }
 
 /*
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/6] qcow2: Let vmstate call qcow2_co_preadv/pwrite directly

[Eric Blake]

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

On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> We don't really want to go through the block layer in order to read from
> or write to the vmstate in a qcow2 image. Doing so required a few ugly
> hacks like saving and restoring the old image size (because writing to
> vmstate offsets would increase the image size) or disabling the "reads
> after EOF = zeroes" logic. When calling the right functions directly,
> these hacks aren't necessary any more.
> 
> Note that .bdrv_vmstate_load/save() return 0 instead of the number of
> bytes in case of success now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 24 ++++--------------------
>  1 file changed, 4 insertions(+), 20 deletions(-)
> 

> +++ b/block/qcow2.c
> @@ -2903,36 +2903,20 @@ static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
>                                int64_t pos)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -    int64_t total_sectors = bs->total_sectors;
> -    bool zero_beyond_eof = bs->zero_beyond_eof;
> -    int ret;
>  
>      BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
> -    bs->zero_beyond_eof = false;
> -    ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
> -    bs->zero_beyond_eof = zero_beyond_eof;
> -
> -    /* bdrv_co_do_writev will have increased the total_sectors value to include
> -     * the VM state - the VM state is however not an actual part of the block
> -     * device, therefore, we need to restore the old value. */
> -    bs->total_sectors = total_sectors;
> -
> -    return ret;
> +    return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos,
> +                                    qiov->size, qiov, 0);
>  }

bs->drv->bdrv_co_pwritev() is an optional interface; not all the drivers
have it yet.  Should you be asserting that it exists, and/or returning
an error if it does not?

>  
>  static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
>                                int64_t pos)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -    bool zero_beyond_eof = bs->zero_beyond_eof;
> -    int ret;
>  
>      BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD);
> -    bs->zero_beyond_eof = false;
> -    ret = bdrv_preadv(bs, qcow2_vm_state_offset(s) + pos, qiov);
> -    bs->zero_beyond_eof = zero_beyond_eof;
> -
> -    return ret;
> +    return bs->drv->bdrv_co_preadv(bs, qcow2_vm_state_offset(s) + pos,
> +                                   qiov->size, qiov, 0);

Ditto.

-- 
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 --]

Re: [Qemu-devel] [PATCH 5/6] qcow2: Let vmstate call qcow2_co_preadv/pwrite directly

[Fam Zheng]

On Fri, 06/10 16:39, Eric Blake wrote:
> > +++ b/block/qcow2.c
> > @@ -2903,36 +2903,20 @@ static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
> >                                int64_t pos)
> >  {
> >      BDRVQcow2State *s = bs->opaque;
> > -    int64_t total_sectors = bs->total_sectors;
> > -    bool zero_beyond_eof = bs->zero_beyond_eof;
> > -    int ret;
> >  
> >      BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
> > -    bs->zero_beyond_eof = false;
> > -    ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
> > -    bs->zero_beyond_eof = zero_beyond_eof;
> > -
> > -    /* bdrv_co_do_writev will have increased the total_sectors value to include
> > -     * the VM state - the VM state is however not an actual part of the block
> > -     * device, therefore, we need to restore the old value. */
> > -    bs->total_sectors = total_sectors;
> > -
> > -    return ret;
> > +    return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos,
> > +                                    qiov->size, qiov, 0);
> >  }
> 
> bs->drv->bdrv_co_pwritev() is an optional interface; not all the drivers
> have it yet.  Should you be asserting that it exists, and/or returning
> an error if it does not?

bs->drv is always qcow2 driver here so this is correct.

> 
> >  
> >  static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
> >                                int64_t pos)
> >  {
> >      BDRVQcow2State *s = bs->opaque;
> > -    bool zero_beyond_eof = bs->zero_beyond_eof;
> > -    int ret;
> >  
> >      BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD);
> > -    bs->zero_beyond_eof = false;
> > -    ret = bdrv_preadv(bs, qcow2_vm_state_offset(s) + pos, qiov);
> > -    bs->zero_beyond_eof = zero_beyond_eof;
> > -
> > -    return ret;
> > +    return bs->drv->bdrv_co_preadv(bs, qcow2_vm_state_offset(s) + pos,
> > +                                   qiov->size, qiov, 0);
> 
> Ditto.
> 

Fam

Re: [Qemu-devel] [PATCH 5/6] qcow2: Let vmstate call qcow2_co_preadv/pwrite directly

[Eric Blake]

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

On 06/11/2016 08:58 PM, Fam Zheng wrote:

>>> -    return ret;
>>> +    return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos,
>>> +                                    qiov->size, qiov, 0);
>>>  }
>>
>> bs->drv->bdrv_co_pwritev() is an optional interface; not all the drivers
>> have it yet.  Should you be asserting that it exists, and/or returning
>> an error if it does not?
> 
> bs->drv is always qcow2 driver here so this is correct.

In that case,
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 6/6] block: Remove bs->zero_beyond_eof

[Kevin Wolf]

It is always true for open images now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   |  2 --
 block/io.c                | 51 +++++++++++++++++++++--------------------------
 include/block/block_int.h |  3 ---
 3 files changed, 23 insertions(+), 33 deletions(-)

diff --git a/block.c b/block.c
index 3d850a2..b350794 100644
--- a/block.c
+++ b/block.c
@@ -938,7 +938,6 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     }
 
     bs->request_alignment = drv->bdrv_co_preadv ? 1 : 512;
-    bs->zero_beyond_eof = true;
     bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
@@ -2192,7 +2191,6 @@ static void bdrv_close(BlockDriverState *bs)
         bs->encrypted = 0;
         bs->valid_key = 0;
         bs->sg = 0;
-        bs->zero_beyond_eof = false;
         QDECREF(bs->options);
         QDECREF(bs->explicit_options);
         bs->options = NULL;
diff --git a/block/io.c b/block/io.c
index adf2726..d504443 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1000,40 +1000,35 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
     }
 
     /* Forward the request to the BlockDriver */
-    if (!bs->zero_beyond_eof) {
-        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, 0);
-    } else {
-        /* Read zeros after EOF */
-        int64_t total_bytes, max_bytes;
+    int64_t total_bytes, max_bytes;
 
-        total_bytes = bdrv_getlength(bs);
-        if (total_bytes < 0) {
-            ret = total_bytes;
-            goto out;
-        }
+    total_bytes = bdrv_getlength(bs);
+    if (total_bytes < 0) {
+        ret = total_bytes;
+        goto out;
+    }
 
-        max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
-        if (bytes < max_bytes) {
-            ret = bdrv_driver_preadv(bs, offset, bytes, qiov, 0);
-        } else if (max_bytes > 0) {
-            QEMUIOVector local_qiov;
+    max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
+    if (bytes < max_bytes) {
+        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, 0);
+    } else if (max_bytes > 0) {
+        QEMUIOVector local_qiov;
 
-            qemu_iovec_init(&local_qiov, qiov->niov);
-            qemu_iovec_concat(&local_qiov, qiov, 0, max_bytes);
+        qemu_iovec_init(&local_qiov, qiov->niov);
+        qemu_iovec_concat(&local_qiov, qiov, 0, max_bytes);
 
-            ret = bdrv_driver_preadv(bs, offset, max_bytes, &local_qiov, 0);
+        ret = bdrv_driver_preadv(bs, offset, max_bytes, &local_qiov, 0);
 
-            qemu_iovec_destroy(&local_qiov);
-        } else {
-            ret = 0;
-        }
+        qemu_iovec_destroy(&local_qiov);
+    } else {
+        ret = 0;
+    }
 
-        /* Reading beyond end of file is supposed to produce zeroes */
-        if (ret == 0 && total_bytes < offset + bytes) {
-            uint64_t zero_offset = MAX(0, total_bytes - offset);
-            uint64_t zero_bytes = offset + bytes - zero_offset;
-            qemu_iovec_memset(qiov, zero_offset, 0, zero_bytes);
-        }
+    /* Reading beyond end of file is supposed to produce zeroes */
+    if (ret == 0 && total_bytes < offset + bytes) {
+        uint64_t zero_offset = MAX(0, total_bytes - offset);
+        uint64_t zero_bytes = offset + bytes - zero_offset;
+        qemu_iovec_memset(qiov, zero_offset, 0, zero_bytes);
     }
 
 out:
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1fe0811..16c43e2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -451,9 +451,6 @@ struct BlockDriverState {
     /* I/O Limits */
     BlockLimits bl;
 
-    /* Whether produces zeros when read beyond eof */
-    bool zero_beyond_eof;
-
     /* Alignment requirement for offset/length of I/O requests */
     unsigned int request_alignment;
     /* Flags honored during pwrite (so far: BDRV_REQ_FUA) */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 6/6] block: Remove bs->zero_beyond_eof

[Eric Blake]

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

On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> It is always true for open images now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   |  2 --
>  block/io.c                | 51 +++++++++++++++++++++--------------------------
>  include/block/block_int.h |  3 ---
>  3 files changed, 23 insertions(+), 33 deletions(-)
> 

> +++ b/block/io.c
> @@ -1000,40 +1000,35 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
>      }
>  
>      /* Forward the request to the BlockDriver */
> -    if (!bs->zero_beyond_eof) {
> -        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, 0);
> -    } else {
> -        /* Read zeros after EOF */
> -        int64_t total_bytes, max_bytes;
> +    int64_t total_bytes, max_bytes;

This declaration is now in the middle of the function.  Shouldn't you
hoist it to the beginning?

That's minor enough to fix on pull request, so:
Reviewed-by: Eric Blake <eblake@redhat.com>

I'll rebase my pending byte-based BlockLimits series on top of this.

-- 
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 --]

Re: [Qemu-devel] [PATCH 0/6] block: bdrv_load/save_vmstate() cleanups

[Fam Zheng]

On Fri, 06/10 18:05, Kevin Wolf wrote:
> This series contains a few cleanups with respect to the vmstate I/O functions.
> Apart from making the interface more consistent (writes were already vectored,
> but not reads), this makes use of the new byte-based .bdrv_co_preadv/pwritev
> callbacks in qcow2 to get rid of a few hacks, including bs->zero_beyond_eof.

Looks sane to me,

Reviewed-by: Fam Zheng <famz@redhat.com>

(Kevin, this is based on your "qcow2: Implement .bdrv_co_preadv/pwritev"
series, right?)

Fam

> 
> Kevin Wolf (6):
>   block: Introduce bdrv_preadv()
>   block: Make .bdrv_load_vmstate() vectored
>   block: Allow .bdrv_load/save_vmstate() to return 0/-errno
>   block: Make bdrv_load/save_vmstate coroutine_fns
>   qcow2: Let vmstate call qcow2_co_preadv/pwrite directly
>   block: Remove bs->zero_beyond_eof
> 
>  block.c                   |   2 -
>  block/io.c                | 173 +++++++++++++++++++++++++++++++---------------
>  block/qcow2.c             |  28 ++------
>  block/sheepdog.c          |  13 +++-
>  include/block/block.h     |   2 +
>  include/block/block_int.h |  13 ++--
>  6 files changed, 143 insertions(+), 88 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/6] block: bdrv_load/save_vmstate() cleanups

[Stefan Hajnoczi]

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

On Fri, Jun 10, 2016 at 06:05:16PM +0200, Kevin Wolf wrote:
> This series contains a few cleanups with respect to the vmstate I/O functions.
> Apart from making the interface more consistent (writes were already vectored,
> but not reads), this makes use of the new byte-based .bdrv_co_preadv/pwritev
> callbacks in qcow2 to get rid of a few hacks, including bs->zero_beyond_eof.
> 
> Kevin Wolf (6):
>   block: Introduce bdrv_preadv()
>   block: Make .bdrv_load_vmstate() vectored
>   block: Allow .bdrv_load/save_vmstate() to return 0/-errno
>   block: Make bdrv_load/save_vmstate coroutine_fns
>   qcow2: Let vmstate call qcow2_co_preadv/pwrite directly
>   block: Remove bs->zero_beyond_eof
> 
>  block.c                   |   2 -
>  block/io.c                | 173 +++++++++++++++++++++++++++++++---------------
>  block/qcow2.c             |  28 ++------
>  block/sheepdog.c          |  13 +++-
>  include/block/block.h     |   2 +
>  include/block/block_int.h |  13 ++--
>  6 files changed, 143 insertions(+), 88 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> 

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

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

Re: [Qemu-devel] [PATCH 0/6] block: bdrv_load/save_vmstate() cleanups

[Kevin Wolf]

Am 10.06.2016 um 18:05 hat Kevin Wolf geschrieben:
> This series contains a few cleanups with respect to the vmstate I/O functions.
> Apart from making the interface more consistent (writes were already vectored,
> but not reads), this makes use of the new byte-based .bdrv_co_preadv/pwritev
> callbacks in qcow2 to get rid of a few hacks, including bs->zero_beyond_eof.

Applied to the block branch.

Kevin