diff mbox

[13/14] commit: Use BlockBackend for I/O

Message ID 1462354765-14037-14-git-send-email-kwolf@redhat.com
State New, archived
Headers show

Commit Message

Kevin Wolf May 4, 2016, 9:39 a.m. UTC
This changes the commit block job to use the job's BlockBackend for
performing its I/O. job->bs isn't used by the commit code any more
afterwards.

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

Comments

Max Reitz May 13, 2016, 3:36 p.m. UTC | #1
On 04.05.2016 11:39, Kevin Wolf wrote:
> This changes the commit block job to use the job's BlockBackend for
> performing its I/O. job->bs isn't used by the commit code any more
> afterwards.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/commit.c | 42 +++++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index f308c8c..863e059 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -36,28 +36,34 @@ typedef struct CommitBlockJob {
>      BlockJob common;
>      RateLimit limit;
>      BlockDriverState *active;
> -    BlockDriverState *top;
> -    BlockDriverState *base;
> +    BlockBackend *top;
> +    BlockBackend *base;
>      BlockdevOnError on_error;
>      int base_flags;
>      int orig_overlay_flags;
>      char *backing_file_str;
>  } CommitBlockJob;
>  
> -static int coroutine_fn commit_populate(BlockDriverState *bs,
> -                                        BlockDriverState *base,
> +static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
>                                          int64_t sector_num, int nb_sectors,
>                                          void *buf)
>  {
>      int ret = 0;
> +    QEMUIOVector qiov;
> +    struct iovec iov = {
> +        .iov_base = (void *)buf,

Why the cast?

> +        .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
> +    };
>  
> -    ret = bdrv_read(bs, sector_num, buf, nb_sectors);
> -    if (ret) {
> +    qemu_iovec_init_external(&qiov, &iov, 1);
> +
> +    ret = blk_co_readv(bs, sector_num, nb_sectors, &qiov);
> +    if (ret < 0) {
>          return ret;
>      }
>  
> -    ret = bdrv_write(base, sector_num, buf, nb_sectors);
> -    if (ret) {
> +    ret = blk_co_writev(base, sector_num, nb_sectors, &qiov);
> +    if (ret < 0) {
>          return ret;
>      }
>  
> @@ -73,8 +79,8 @@ static void commit_complete(BlockJob *job, void *opaque)
>      CommitBlockJob *s = container_of(job, CommitBlockJob, common);
>      CommitCompleteData *data = opaque;
>      BlockDriverState *active = s->active;
> -    BlockDriverState *top = s->top;
> -    BlockDriverState *base = s->base;
> +    BlockDriverState *top = blk_bs(s->top);
> +    BlockDriverState *base = blk_bs(s->base);
>      BlockDriverState *overlay_bs;
>      int ret = data->ret;
>  
> @@ -94,6 +100,8 @@ static void commit_complete(BlockJob *job, void *opaque)
>          bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
>      }
>      g_free(s->backing_file_str);
> +    blk_unref(s->top);
> +    blk_unref(s->base);
>      block_job_completed(&s->common, ret);
>      g_free(data);
>  }
> @@ -102,8 +110,8 @@ static void coroutine_fn commit_run(void *opaque)
>  {
>      CommitBlockJob *s = opaque;
>      CommitCompleteData *data;
> -    BlockDriverState *top = s->top;
> -    BlockDriverState *base = s->base;
> +    BlockDriverState *top = blk_bs(s->top);
> +    BlockDriverState *base = blk_bs(s->base);

I think for this function it would definitely be nicer to drop these
variables (or make them BlockBackend pointers) and use them with the
blk_* functions. The only function where this is not possible is
bdrv_is_allocated_above(), and I think it won't be too ugly to use
blk_bs() in its parameter list there.

>      int64_t sector_num, end;
>      int ret = 0;
>      int n = 0;
> @@ -158,7 +166,7 @@ wait:
>                      goto wait;
>                  }
>              }
> -            ret = commit_populate(top, base, sector_num, n, buf);
> +            ret = commit_populate(s->top, s->base, sector_num, n, buf);
>              bytes_written += n * BDRV_SECTOR_SIZE;
>          }
>          if (ret < 0) {
> @@ -253,8 +261,12 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
>          return;
>      }
>  
> -    s->base   = base;
> -    s->top    = top;
> +    s->base = blk_new(&error_abort);
> +    blk_insert_bs(s->base, base);
> +
> +    s->top = blk_new(&error_abort);
> +    blk_insert_bs(s->top, top);
> +

But again, this is why I'm hesitant to give an R-b for this patch.

Max

>      s->active = bs;
>  
>      s->base_flags          = orig_base_flags;
>
diff mbox

Patch

diff --git a/block/commit.c b/block/commit.c
index f308c8c..863e059 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -36,28 +36,34 @@  typedef struct CommitBlockJob {
     BlockJob common;
     RateLimit limit;
     BlockDriverState *active;
-    BlockDriverState *top;
-    BlockDriverState *base;
+    BlockBackend *top;
+    BlockBackend *base;
     BlockdevOnError on_error;
     int base_flags;
     int orig_overlay_flags;
     char *backing_file_str;
 } CommitBlockJob;
 
-static int coroutine_fn commit_populate(BlockDriverState *bs,
-                                        BlockDriverState *base,
+static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
                                         int64_t sector_num, int nb_sectors,
                                         void *buf)
 {
     int ret = 0;
+    QEMUIOVector qiov;
+    struct iovec iov = {
+        .iov_base = (void *)buf,
+        .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
+    };
 
-    ret = bdrv_read(bs, sector_num, buf, nb_sectors);
-    if (ret) {
+    qemu_iovec_init_external(&qiov, &iov, 1);
+
+    ret = blk_co_readv(bs, sector_num, nb_sectors, &qiov);
+    if (ret < 0) {
         return ret;
     }
 
-    ret = bdrv_write(base, sector_num, buf, nb_sectors);
-    if (ret) {
+    ret = blk_co_writev(base, sector_num, nb_sectors, &qiov);
+    if (ret < 0) {
         return ret;
     }
 
@@ -73,8 +79,8 @@  static void commit_complete(BlockJob *job, void *opaque)
     CommitBlockJob *s = container_of(job, CommitBlockJob, common);
     CommitCompleteData *data = opaque;
     BlockDriverState *active = s->active;
-    BlockDriverState *top = s->top;
-    BlockDriverState *base = s->base;
+    BlockDriverState *top = blk_bs(s->top);
+    BlockDriverState *base = blk_bs(s->base);
     BlockDriverState *overlay_bs;
     int ret = data->ret;
 
@@ -94,6 +100,8 @@  static void commit_complete(BlockJob *job, void *opaque)
         bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
     }
     g_free(s->backing_file_str);
+    blk_unref(s->top);
+    blk_unref(s->base);
     block_job_completed(&s->common, ret);
     g_free(data);
 }
@@ -102,8 +110,8 @@  static void coroutine_fn commit_run(void *opaque)
 {
     CommitBlockJob *s = opaque;
     CommitCompleteData *data;
-    BlockDriverState *top = s->top;
-    BlockDriverState *base = s->base;
+    BlockDriverState *top = blk_bs(s->top);
+    BlockDriverState *base = blk_bs(s->base);
     int64_t sector_num, end;
     int ret = 0;
     int n = 0;
@@ -158,7 +166,7 @@  wait:
                     goto wait;
                 }
             }
-            ret = commit_populate(top, base, sector_num, n, buf);
+            ret = commit_populate(s->top, s->base, sector_num, n, buf);
             bytes_written += n * BDRV_SECTOR_SIZE;
         }
         if (ret < 0) {
@@ -253,8 +261,12 @@  void commit_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
-    s->base   = base;
-    s->top    = top;
+    s->base = blk_new(&error_abort);
+    blk_insert_bs(s->base, base);
+
+    s->top = blk_new(&error_abort);
+    blk_insert_bs(s->top, top);
+
     s->active = bs;
 
     s->base_flags          = orig_base_flags;