[Qemu-devel] [PATCH 00/14] block jobs: Convert I/O to BlockBackend

[Qemu-devel] [PATCH 00/14] block jobs: Convert I/O to BlockBackend

[Kevin Wolf]

This series changes the block jobs so that they have a separate BlockBackend
for every node on which they perform I/O. This doesn't only get us closer to
the goal of only doing I/O through blk_*() from external sources (and block
jobs are considered external), but can also later be used to use BlockBackend
features for jobs: One could imagine replacing the very basic job throttling by
the already existing and more advanced BlockBackend throttling, or using the
BlockBackend error handling for block jobs.

This depends on the following queue of series on top of block-next:
* [PATCH v2 00/13] block: Move I/O throttling to BlockBackend
* [PATCH v2 0/9] block: Remove BlockDriverState.blk
* [PATCH v3 for-2.7 0/8] blockdev: (Nearly) free clean-up work

Alberto Garcia (1):
  block: keep a list of block jobs

John Snow (1):
  backup: Pack Notifier within BackupBlockJob

Kevin Wolf (12):
  block: Cancel jobs first in bdrv_close_all()
  block: Default to enabled write cache in blk_new()
  block: Convert block job core to BlockBackend
  stream: Use BlockBackend for I/O
  mirror: Allow target that already has a BlockBackend
  mirror: Use BlockBackend for I/O
  backup: Don't leak BackupBlockJob in error path
  backup: Remove bs parameter from backup_do_cow()
  block: Add blk_co_readv/writev()
  backup: Use BlockBackend for I/O
  commit: Use BlockBackend for I/O
  blockjob: Remove BlockJob.bs

 block.c                        |  23 +--------
 block/backup.c                 |  67 ++++++++++++++------------
 block/block-backend.c          |  58 ++++++++++++++++++++++-
 block/commit.c                 |  42 +++++++++++------
 block/io.c                     |  18 -------
 block/mirror.c                 | 105 ++++++++++++++++++-----------------------
 block/stream.c                 |  14 +++---
 blockdev.c                     |  12 +----
 blockjob.c                     |  62 +++++++++++++++++-------
 include/block/block.h          |   4 --
 include/block/blockjob.h       |  23 ++++++++-
 include/sysemu/block-backend.h |   8 ++++
 qemu-img.c                     |   2 +-
 tests/qemu-iotests/041         |  27 -----------
 tests/qemu-iotests/041.out     |   4 +-
 trace-events                   |   8 +++-
 16 files changed, 261 insertions(+), 216 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/14] block: keep a list of block jobs

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

The current way to obtain the list of existing block jobs is to
iterate over all root nodes and check which ones own a job.

Since we want to be able to support block jobs in other nodes as well,
this patch keeps a list of jobs that is updated every time one is
created or destroyed.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockjob.c               | 13 +++++++++++++
 include/block/blockjob.h | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 5b840a7..0f1bc77 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -50,6 +50,16 @@ struct BlockJobTxn {
     int refcnt;
 };
 
+static QLIST_HEAD(, BlockJob) block_jobs = QLIST_HEAD_INITIALIZER(block_jobs);
+
+BlockJob *block_job_next(BlockJob *job)
+{
+    if (!job) {
+        return QLIST_FIRST(&block_jobs);
+    }
+    return QLIST_NEXT(job, job_list);
+}
+
 void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp)
@@ -76,6 +86,8 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     job->refcnt        = 1;
     bs->job = job;
 
+    QLIST_INSERT_HEAD(&block_jobs, job, job_list);
+
     /* Only set speed when necessary to avoid NotSupported error */
     if (speed != 0) {
         Error *local_err = NULL;
@@ -103,6 +115,7 @@ void block_job_unref(BlockJob *job)
         bdrv_unref(job->bs);
         error_free(job->blocker);
         g_free(job->id);
+        QLIST_REMOVE(job, job_list);
         g_free(job);
     }
 }
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 073a433..30bb2c6 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -135,6 +135,9 @@ struct BlockJob {
      */
     bool deferred_to_main_loop;
 
+    /** Element of the list of block jobs */
+    QLIST_ENTRY(BlockJob) job_list;
+
     /** Status that is published by the query-block-jobs QMP API */
     BlockDeviceIoStatus iostatus;
 
@@ -173,6 +176,17 @@ struct BlockJob {
 };
 
 /**
+ * block_job_next:
+ * @job: A block job, or %NULL.
+ *
+ * Get the next element from the list of block jobs after @job, or the
+ * first one if @job is %NULL.
+ *
+ * Returns the requested job, or %NULL if there are no more jobs left.
+ */
+BlockJob *block_job_next(BlockJob *job);
+
+/**
  * block_job_create:
  * @job_type: The class object for the newly-created job.
  * @bs: The block
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 01/14] block: keep a list of block jobs

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> From: Alberto Garcia <berto@igalia.com>
> 
> The current way to obtain the list of existing block jobs is to
> iterate over all root nodes and check which ones own a job.
> 
> Since we want to be able to support block jobs in other nodes as well,
> this patch keeps a list of jobs that is updated every time one is
> created or destroyed.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockjob.c               | 13 +++++++++++++
>  include/block/blockjob.h | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)

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


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

[Qemu-devel] [PATCH 02/14] block: Cancel jobs first in bdrv_close_all()

[Kevin Wolf]

So far, bdrv_close_all() first removed all root BlockDriverStates of
BlockBackends and monitor owned BDSes, and then assumed that the
remaining BDSes must be related to jobs and cancelled these jobs.

This order doesn't work that well any more when block jobs use
BlockBackends internally because then they will lose their BDS before
being cancelled.

This patch changes bdrv_close_all() to first cancel all jobs and then
remove all root BDSes from the remaining BBs.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                  | 23 +----------------------
 blockjob.c               | 13 +++++++++++++
 include/block/blockjob.h |  7 +++++++
 3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 37aa0a3..bf6c204 100644
--- a/block.c
+++ b/block.c
@@ -2165,8 +2165,7 @@ static void bdrv_close(BlockDriverState *bs)
 
 void bdrv_close_all(void)
 {
-    BlockDriverState *bs;
-    AioContext *aio_context;
+    block_job_cancel_sync_all();
 
     /* Drop references from requests still in flight, such as canceled block
      * jobs whose AIO context has not been polled yet */
@@ -2174,26 +2173,6 @@ void bdrv_close_all(void)
 
     blk_remove_all_bs();
     blockdev_close_all_bdrv_states();
-
-    /* Cancel all block jobs */
-    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
-        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
-            aio_context = bdrv_get_aio_context(bs);
-
-            aio_context_acquire(aio_context);
-            if (bs->job) {
-                block_job_cancel_sync(bs->job);
-                aio_context_release(aio_context);
-                break;
-            }
-            aio_context_release(aio_context);
-        }
-
-        /* All the remaining BlockDriverStates are referenced directly or
-         * indirectly from block jobs, so there needs to be at least one BDS
-         * directly used by a block job */
-        assert(bs);
-    }
 }
 
 static void change_parent_backing_link(BlockDriverState *from,
diff --git a/blockjob.c b/blockjob.c
index 0f1bc77..e916b41 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -331,6 +331,19 @@ int block_job_cancel_sync(BlockJob *job)
     return block_job_finish_sync(job, &block_job_cancel_err, NULL);
 }
 
+void block_job_cancel_sync_all(void)
+{
+    BlockJob *job;
+    AioContext *aio_context;
+
+    while ((job = QLIST_FIRST(&block_jobs))) {
+        aio_context = bdrv_get_aio_context(job->bs);
+        aio_context_acquire(aio_context);
+        block_job_cancel_sync(job);
+        aio_context_release(aio_context);
+    }
+}
+
 int block_job_complete_sync(BlockJob *job, Error **errp)
 {
     return block_job_finish_sync(job, &block_job_complete, errp);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 30bb2c6..4ac6831 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -371,6 +371,13 @@ bool block_job_is_paused(BlockJob *job);
 int block_job_cancel_sync(BlockJob *job);
 
 /**
+ * block_job_cancel_sync_all:
+ *
+ * Synchronously cancels all jobs using block_job_cancel_sync().
+ */
+void block_job_cancel_sync_all(void);
+
+/**
  * block_job_complete_sync:
  * @job: The job to be completed.
  * @errp: Error object which may be set by block_job_complete(); this is not
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 02/14] block: Cancel jobs first in bdrv_close_all()

[Alberto Garcia]

On Wed 04 May 2016 11:39:13 AM CEST, Kevin Wolf wrote:

>  void bdrv_close_all(void)
>  {
> -    BlockDriverState *bs;
> -    AioContext *aio_context;
> +    block_job_cancel_sync_all();
>  
>      /* Drop references from requests still in flight, such as canceled block
>       * jobs whose AIO context has not been polled yet */
> @@ -2174,26 +2173,6 @@ void bdrv_close_all(void)
>  
>      blk_remove_all_bs();
>      blockdev_close_all_bdrv_states();
> -
> -    /* Cancel all block jobs */
> -    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
> -        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
> -            aio_context = bdrv_get_aio_context(bs);
> -
> -            aio_context_acquire(aio_context);
> -            if (bs->job) {
> -                block_job_cancel_sync(bs->job);
> -                aio_context_release(aio_context);
> -                break;
> -            }
> -            aio_context_release(aio_context);
> -        }
> -
> -        /* All the remaining BlockDriverStates are referenced directly or
> -         * indirectly from block jobs, so there needs to be at least one BDS
> -         * directly used by a block job */
> -        assert(bs);
> -    }
>  }

You could add an assert(QTAILQ_EMPTY(&all_bdrv_states)) as it was
suggested some days ago.

The rest looks good.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 02/14] block: Cancel jobs first in bdrv_close_all()

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> So far, bdrv_close_all() first removed all root BlockDriverStates of
> BlockBackends and monitor owned BDSes, and then assumed that the
> remaining BDSes must be related to jobs and cancelled these jobs.
> 
> This order doesn't work that well any more when block jobs use
> BlockBackends internally because then they will lose their BDS before
> being cancelled.
> 
> This patch changes bdrv_close_all() to first cancel all jobs and then
> remove all root BDSes from the remaining BBs.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                  | 23 +----------------------
>  blockjob.c               | 13 +++++++++++++
>  include/block/blockjob.h |  7 +++++++
>  3 files changed, 21 insertions(+), 22 deletions(-)

I agree with Berto about the assertion because it was me who suggested
it. :-)

And because I didn't give Berto an R-b because of that I'm afraid I
can't give you one either. O:-)

Max


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

[Qemu-devel] [PATCH 03/14] block: Default to enabled write cache in blk_new()

[Kevin Wolf]

The existing users of the function are:

1. blk_new_open(), which already enabled the write cache
2. Some test cases that don't care about the setting
3. blockdev_init() for empty drives, where the cache mode is overridden
   with the value from the options when a medium is inserted

Therefore, this patch doesn't change the current behaviour. It will be
convenient, however, for additional users of blk_new() (like block
jobs) if the most sensible WCE setting is the default.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6732fb4..2932a58 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -125,6 +125,8 @@ BlockBackend *blk_new(Error **errp)
 
     blk = g_new0(BlockBackend, 1);
     blk->refcnt = 1;
+    blk_set_enable_write_cache(blk, true);
+
     qemu_co_queue_init(&blk->public.throttled_reqs[0]);
     qemu_co_queue_init(&blk->public.throttled_reqs[1]);
 
@@ -165,7 +167,6 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
         return NULL;
     }
 
-    blk_set_enable_write_cache(blk, true);
     blk->root = bdrv_root_attach_child(bs, "root", &child_root);
     blk->root->opaque = blk;
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 03/14] block: Default to enabled write cache in blk_new()

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> The existing users of the function are:
> 
> 1. blk_new_open(), which already enabled the write cache
> 2. Some test cases that don't care about the setting
> 3. blockdev_init() for empty drives, where the cache mode is overridden
>    with the value from the options when a medium is inserted
> 
> Therefore, this patch doesn't change the current behaviour. It will be
> convenient, however, for additional users of blk_new() (like block
> jobs) if the most sensible WCE setting is the default.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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


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

[Qemu-devel] [PATCH 04/14] block: Convert block job core to BlockBackend

[Kevin Wolf]

This adds a new BlockBackend field to the BlockJob struct, which
coexists with the BlockDriverState while converting the individual jobs.

When creating a block job, a new BlockBackend is created on top of the
given BlockDriverState, and it is destroyed when the BlockJob ends. The
reference to the BDS is now held by the BlockBackend instead of calling
bdrv_ref/unref manually.

We have to be careful when we use bdrv_replace_in_backing_chain() in
block jobs because this changes the BDS that job->blk points to. At the
moment block jobs are too tightly coupled with their BDS, so that moving
a job to another BDS isn't easily possible; therefore, we need to just
manually undo this change afterwards.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c           |  3 +++
 blockjob.c               | 37 ++++++++++++++++++++-----------------
 include/block/blockjob.h |  3 ++-
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index b9986d8..efca8fc 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -478,6 +478,9 @@ static void mirror_exit(BlockJob *job, void *opaque)
             bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
         }
         bdrv_replace_in_backing_chain(to_replace, s->target);
+        /* We just changed the BDS the job BB refers to */
+        blk_remove_bs(job->blk);
+        blk_insert_bs(job->blk, src);
     }
 
 out:
diff --git a/blockjob.c b/blockjob.c
index e916b41..01b58c7 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -64,13 +64,17 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp)
 {
+    BlockBackend *blk;
     BlockJob *job;
 
     if (bs->job) {
         error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
         return NULL;
     }
-    bdrv_ref(bs);
+
+    blk = blk_new(&error_abort);
+    blk_insert_bs(blk, bs);
+
     job = g_malloc0(driver->instance_size);
     error_setg(&job->blocker, "block device is in use by block job: %s",
                BlockJobType_lookup[driver->job_type]);
@@ -80,6 +84,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     job->driver        = driver;
     job->id            = g_strdup(bdrv_get_device_name(bs));
     job->bs            = bs;
+    job->blk           = blk;
     job->cb            = cb;
     job->opaque        = opaque;
     job->busy          = true;
@@ -110,9 +115,10 @@ void block_job_ref(BlockJob *job)
 void block_job_unref(BlockJob *job)
 {
     if (--job->refcnt == 0) {
-        job->bs->job = NULL;
-        bdrv_op_unblock_all(job->bs, job->blocker);
-        bdrv_unref(job->bs);
+        BlockDriverState *bs = blk_bs(job->blk);
+        bs->job = NULL;
+        bdrv_op_unblock_all(bs, job->blocker);
+        blk_unref(job->blk);
         error_free(job->blocker);
         g_free(job->id);
         QLIST_REMOVE(job, job_list);
@@ -153,7 +159,7 @@ static void block_job_completed_txn_abort(BlockJob *job)
     txn->aborting = true;
     /* We are the first failed job. Cancel other jobs. */
     QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
-        ctx = bdrv_get_aio_context(other_job->bs);
+        ctx = blk_get_aio_context(other_job->blk);
         aio_context_acquire(ctx);
     }
     QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
@@ -170,7 +176,7 @@ static void block_job_completed_txn_abort(BlockJob *job)
         assert(other_job->completed);
     }
     QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
-        ctx = bdrv_get_aio_context(other_job->bs);
+        ctx = blk_get_aio_context(other_job->blk);
         block_job_completed_single(other_job);
         aio_context_release(ctx);
     }
@@ -192,7 +198,7 @@ static void block_job_completed_txn_success(BlockJob *job)
     }
     /* We are the last completed job, commit the transaction. */
     QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
-        ctx = bdrv_get_aio_context(other_job->bs);
+        ctx = blk_get_aio_context(other_job->blk);
         aio_context_acquire(ctx);
         assert(other_job->ret == 0);
         block_job_completed_single(other_job);
@@ -202,9 +208,7 @@ static void block_job_completed_txn_success(BlockJob *job)
 
 void block_job_completed(BlockJob *job, int ret)
 {
-    BlockDriverState *bs = job->bs;
-
-    assert(bs->job == job);
+    assert(blk_bs(job->blk)->job == job);
     assert(!job->completed);
     job->completed = true;
     job->ret = ret;
@@ -295,11 +299,10 @@ static int block_job_finish_sync(BlockJob *job,
                                  void (*finish)(BlockJob *, Error **errp),
                                  Error **errp)
 {
-    BlockDriverState *bs = job->bs;
     Error *local_err = NULL;
     int ret;
 
-    assert(bs->job == job);
+    assert(blk_bs(job->blk)->job == job);
 
     block_job_ref(job);
     finish(job, &local_err);
@@ -310,7 +313,7 @@ static int block_job_finish_sync(BlockJob *job,
     }
     while (!job->completed) {
         aio_poll(job->deferred_to_main_loop ? qemu_get_aio_context() :
-                                              bdrv_get_aio_context(bs),
+                                              blk_get_aio_context(job->blk),
                  true);
     }
     ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
@@ -337,7 +340,7 @@ void block_job_cancel_sync_all(void)
     AioContext *aio_context;
 
     while ((job = QLIST_FIRST(&block_jobs))) {
-        aio_context = bdrv_get_aio_context(job->bs);
+        aio_context = blk_get_aio_context(job->blk);
         aio_context_acquire(aio_context);
         block_job_cancel_sync(job);
         aio_context_release(aio_context);
@@ -362,7 +365,7 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
     if (block_job_is_paused(job)) {
         qemu_coroutine_yield();
     } else {
-        co_aio_sleep_ns(bdrv_get_aio_context(job->bs), type, ns);
+        co_aio_sleep_ns(blk_get_aio_context(job->blk), type, ns);
     }
     job->busy = true;
 }
@@ -491,7 +494,7 @@ static void block_job_defer_to_main_loop_bh(void *opaque)
     aio_context_acquire(data->aio_context);
 
     /* Fetch BDS AioContext again, in case it has changed */
-    aio_context = bdrv_get_aio_context(data->job->bs);
+    aio_context = blk_get_aio_context(data->job->blk);
     aio_context_acquire(aio_context);
 
     data->job->deferred_to_main_loop = false;
@@ -511,7 +514,7 @@ void block_job_defer_to_main_loop(BlockJob *job,
     BlockJobDeferToMainLoopData *data = g_malloc(sizeof(*data));
     data->job = job;
     data->bh = qemu_bh_new(block_job_defer_to_main_loop_bh, data);
-    data->aio_context = bdrv_get_aio_context(job->bs);
+    data->aio_context = blk_get_aio_context(job->blk);
     data->fn = fn;
     data->opaque = opaque;
     job->deferred_to_main_loop = true;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 4ac6831..32012af 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -82,7 +82,8 @@ struct BlockJob {
     const BlockJobDriver *driver;
 
     /** The block device on which the job is operating.  */
-    BlockDriverState *bs;
+    BlockDriverState *bs; /* TODO Remove */
+    BlockBackend *blk;
 
     /**
      * The ID of the block job. Currently the BlockBackend name of the BDS
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 04/14] block: Convert block job core to BlockBackend

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> This adds a new BlockBackend field to the BlockJob struct, which
> coexists with the BlockDriverState while converting the individual jobs.
> 
> When creating a block job, a new BlockBackend is created on top of the
> given BlockDriverState, and it is destroyed when the BlockJob ends. The
> reference to the BDS is now held by the BlockBackend instead of calling
> bdrv_ref/unref manually.
> 
> We have to be careful when we use bdrv_replace_in_backing_chain() in
> block jobs because this changes the BDS that job->blk points to. At the
> moment block jobs are too tightly coupled with their BDS, so that moving
> a job to another BDS isn't easily possible; therefore, we need to just
> manually undo this change afterwards.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c           |  3 +++
>  blockjob.c               | 37 ++++++++++++++++++++-----------------
>  include/block/blockjob.h |  3 ++-
>  3 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index b9986d8..efca8fc 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -478,6 +478,9 @@ static void mirror_exit(BlockJob *job, void *opaque)
>              bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
>          }
>          bdrv_replace_in_backing_chain(to_replace, s->target);
> +        /* We just changed the BDS the job BB refers to */
> +        blk_remove_bs(job->blk);
> +        blk_insert_bs(job->blk, src);
>      }
>  
>  out:
> diff --git a/blockjob.c b/blockjob.c
> index e916b41..01b58c7 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -64,13 +64,17 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>                         int64_t speed, BlockCompletionFunc *cb,
>                         void *opaque, Error **errp)
>  {
> +    BlockBackend *blk;
>      BlockJob *job;
>  
>      if (bs->job) {
>          error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
>          return NULL;
>      }
> -    bdrv_ref(bs);
> +
> +    blk = blk_new(&error_abort);

You're just using &error_abort here because you know that blk_new()
cannot set errp right now. I don't think that's good.

Whenever a function takes an errp parameter, you should pass a valid
Error object to it and handle any potential errors. This is extremely
simple in this case: Just make it blk_new(errp) and return NULL if blk
is NULL.

&error_abort and such should only be used in places where it is
impossible to handle an error, but we can prove that no error can occur.
While we can prove this here, it is trivial to handle an error, so we
should do it.

Alternatively, blk_new()'s errp parameter can be dropped. I kept it
because of my old "But what if we want to reintroduce it later" habit. I
guess I should make a poster of your "Write code for now instead of the
future" mail and get it on my wall...

(See, my intention would break down here. If we reintroduced a way for
blk_new() to fail, this code here would break.)

The rest looks good.

Max

> +    blk_insert_bs(blk, bs);
> +
>      job = g_malloc0(driver->instance_size);
>      error_setg(&job->blocker, "block device is in use by block job: %s",
>                 BlockJobType_lookup[driver->job_type]);


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

[Qemu-devel] [PATCH 05/14] stream: Use BlockBackend for I/O

[Kevin Wolf]

This changes the streaming block job to use the job's BlockBackend for
performing the COR reads. job->bs isn't used by the streaming code any
more afterwards.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 15 +++++++++++++++
 block/io.c                     |  9 ---------
 block/stream.c                 | 14 ++++++++------
 include/block/block.h          |  2 --
 include/sysemu/block-backend.h |  2 ++
 trace-events                   |  4 +++-
 6 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 2932a58..25bb5ca 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -19,6 +19,7 @@
 #include "sysemu/sysemu.h"
 #include "qapi-event.h"
 #include "qemu/id.h"
+#include "trace.h"
 
 /* Number of coroutines to reserve per attached device model */
 #define COROUTINE_POOL_RESERVATION 64
@@ -793,6 +794,20 @@ static int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
     return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
 }
 
+int coroutine_fn blk_co_copy_on_readv(BlockBackend *blk,
+    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
+{
+    trace_blk_co_copy_on_readv(blk, blk_bs(blk), sector_num, nb_sectors);
+
+    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
+        return -EINVAL;
+    }
+
+    return blk_co_preadv(blk, sector_num << BDRV_SECTOR_BITS,
+                         nb_sectors << BDRV_SECTOR_BITS, qiov,
+                         BDRV_REQ_COPY_ON_READ);
+}
+
 typedef struct BlkRwCo {
     BlockBackend *blk;
     int64_t offset;
diff --git a/block/io.c b/block/io.c
index 9aae030..8280e14 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1102,15 +1102,6 @@ int coroutine_fn bdrv_co_readv_no_serialising(BlockDriverState *bs,
                             BDRV_REQ_NO_SERIALISING);
 }
 
-int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
-    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
-{
-    trace_bdrv_co_copy_on_readv(bs, sector_num, nb_sectors);
-
-    return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov,
-                            BDRV_REQ_COPY_ON_READ);
-}
-
 #define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
 
 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
diff --git a/block/stream.c b/block/stream.c
index 40aa322..70ed9be 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -39,7 +39,7 @@ typedef struct StreamBlockJob {
     char *backing_file_str;
 } StreamBlockJob;
 
-static int coroutine_fn stream_populate(BlockDriverState *bs,
+static int coroutine_fn stream_populate(BlockBackend *blk,
                                         int64_t sector_num, int nb_sectors,
                                         void *buf)
 {
@@ -52,7 +52,7 @@ static int coroutine_fn stream_populate(BlockDriverState *bs,
     qemu_iovec_init_external(&qiov, &iov, 1);
 
     /* Copy-on-read the unallocated clusters */
-    return bdrv_co_copy_on_readv(bs, sector_num, nb_sectors, &qiov);
+    return blk_co_copy_on_readv(blk, sector_num, nb_sectors, &qiov);
 }
 
 typedef struct {
@@ -64,6 +64,7 @@ static void stream_complete(BlockJob *job, void *opaque)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common);
     StreamCompleteData *data = opaque;
+    BlockDriverState *bs = blk_bs(job->blk);
     BlockDriverState *base = s->base;
 
     if (!block_job_is_cancelled(&s->common) && data->reached_end &&
@@ -75,8 +76,8 @@ static void stream_complete(BlockJob *job, void *opaque)
                 base_fmt = base->drv->format_name;
             }
         }
-        data->ret = bdrv_change_backing_file(job->bs, base_id, base_fmt);
-        bdrv_set_backing_hd(job->bs, base);
+        data->ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+        bdrv_set_backing_hd(bs, base);
     }
 
     g_free(s->backing_file_str);
@@ -88,7 +89,8 @@ static void coroutine_fn stream_run(void *opaque)
 {
     StreamBlockJob *s = opaque;
     StreamCompleteData *data;
-    BlockDriverState *bs = s->common.bs;
+    BlockBackend *blk = s->common.blk;
+    BlockDriverState *bs = blk_bs(blk);
     BlockDriverState *base = s->base;
     int64_t sector_num = 0;
     int64_t end = -1;
@@ -159,7 +161,7 @@ wait:
                     goto wait;
                 }
             }
-            ret = stream_populate(bs, sector_num, n, buf);
+            ret = stream_populate(blk, sector_num, n, buf);
         }
         if (ret < 0) {
             BlockErrorAction action =
diff --git a/include/block/block.h b/include/block/block.h
index a4eb5be..39b5812 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -243,8 +243,6 @@ 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_copy_on_readv(BlockDriverState *bs,
-    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
 int coroutine_fn bdrv_co_readv_no_serialising(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
 int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 12e4585..eac4d88 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -116,6 +116,8 @@ int blk_read(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
              int nb_sectors);
 int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
                          int nb_sectors);
+int coroutine_fn blk_co_copy_on_readv(BlockBackend *blk,
+    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
 int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
               int nb_sectors);
 int blk_write_zeroes(BlockBackend *blk, int64_t sector_num,
diff --git a/trace-events b/trace-events
index 2f0ec6d..9beb588 100644
--- a/trace-events
+++ b/trace-events
@@ -61,6 +61,9 @@ virtio_console_chr_event(unsigned int port, int event) "port %u, event %d"
 bdrv_open_common(void *bs, const char *filename, int flags, const char *format_name) "bs %p filename \"%s\" flags %#x format_name \"%s\""
 bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
 
+# block/block-backend.c
+blk_co_copy_on_readv(void *blk, void *bs, int64_t sector_num, int nb_sector) "blk %p bs %p sector_num %"PRId64" nb_sectors %d"
+
 # block/io.c
 bdrv_aio_discard(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
 bdrv_aio_flush(void *bs, void *opaque) "bs %p opaque %p"
@@ -68,7 +71,6 @@ bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %
 bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
 bdrv_aio_write_zeroes(void *bs, int64_t sector_num, int nb_sectors, int flags, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x opaque %p"
 bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
-bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
 bdrv_co_readv_no_serialising(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
 bdrv_co_writev(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
 bdrv_co_write_zeroes(void *bs, int64_t sector_num, int nb_sector, int flags) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x"
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 05/14] stream: Use BlockBackend for I/O

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> This changes the streaming block job to use the job's BlockBackend for
> performing the COR reads. job->bs isn't used by the streaming code any
> more afterwards.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 15 +++++++++++++++
>  block/io.c                     |  9 ---------
>  block/stream.c                 | 14 ++++++++------
>  include/block/block.h          |  2 --
>  include/sysemu/block-backend.h |  2 ++
>  trace-events                   |  4 +++-
>  6 files changed, 28 insertions(+), 18 deletions(-)

Hm, I don't quite like adding a new sector-based BB function, but
changing *_co_copy_on_readv()'s interface doesn't seem too reasonable at
this point.

And I guess there are some bdrv_* functions in stream_run() for which a
blk_* equivalent exists, so it might have made sense to change those,
too (e.g. bdrv_getlength()).

Anyway:

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


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

[Qemu-devel] [PATCH 06/14] mirror: Allow target that already has a BlockBackend

[Kevin Wolf]

We had to forbid mirroring to a target BDS that already had a BB
attached because the node swapping at job completion would add a second
BB and we didn't support multiple BBs on a single BDS at the time. Now
we do, so we can lift the restriction.

As we allow additional BlockBackends for the target, we must expect
other users to be sending requests. There may no requests be in flight
during the graph modification, so we have to drain those users now.

The core part of this patch is a revert of commit 40365552.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c             | 33 ++++++---------------------------
 blockdev.c                 |  4 ----
 tests/qemu-iotests/041     | 27 ---------------------------
 tests/qemu-iotests/041.out |  4 ++--
 4 files changed, 8 insertions(+), 60 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index efca8fc..23aa10e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -20,7 +20,6 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
 #include "qemu/bitmap.h"
-#include "qemu/error-report.h"
 
 #define SLICE_TIME    100000000ULL /* ns */
 #define MAX_IN_FLIGHT 16
@@ -466,24 +465,20 @@ static void mirror_exit(BlockJob *job, void *opaque)
             to_replace = s->to_replace;
         }
 
-        /* This was checked in mirror_start_job(), but meanwhile one of the
-         * nodes could have been newly attached to a BlockBackend. */
-        if (bdrv_has_blk(to_replace) && bdrv_has_blk(s->target)) {
-            error_report("block job: Can't create node with two BlockBackends");
-            data->ret = -EINVAL;
-            goto out;
-        }
-
         if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
             bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
         }
+
+        /* The mirror job has no requests in flight any more, but we need to
+         * drain potential other users of the BDS before changing the graph. */
+        bdrv_drained_begin(s->target);
         bdrv_replace_in_backing_chain(to_replace, s->target);
+        bdrv_drained_end(s->target);
+
         /* We just changed the BDS the job BB refers to */
         blk_remove_bs(job->blk);
         blk_insert_bs(job->blk, src);
     }
-
-out:
     if (s->to_replace) {
         bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
         error_free(s->replace_blocker);
@@ -807,7 +802,6 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
                              bool is_none_mode, BlockDriverState *base)
 {
     MirrorBlockJob *s;
-    BlockDriverState *replaced_bs;
 
     if (granularity == 0) {
         granularity = bdrv_get_default_bitmap_granularity(target);
@@ -824,21 +818,6 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         buf_size = DEFAULT_MIRROR_BUF_SIZE;
     }
 
-    /* We can't support this case as long as the block layer can't handle
-     * multiple BlockBackends per BlockDriverState. */
-    if (replaces) {
-        replaced_bs = bdrv_lookup_bs(replaces, replaces, errp);
-        if (replaced_bs == NULL) {
-            return;
-        }
-    } else {
-        replaced_bs = bs;
-    }
-    if (bdrv_has_blk(replaced_bs) && bdrv_has_blk(target)) {
-        error_setg(errp, "Can't create node with two BlockBackends");
-        return;
-    }
-
     s = block_job_create(driver, bs, speed, cb, opaque, errp);
     if (!s) {
         return;
diff --git a/blockdev.c b/blockdev.c
index 516c6cb..da9eeff 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3434,10 +3434,6 @@ static void blockdev_mirror_common(BlockDriverState *bs,
     if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
         return;
     }
-    if (bdrv_has_blk(target)) {
-        error_setg(errp, "Cannot mirror to an attached block device");
-        return;
-    }
 
     if (!bs->backing && sync == MIRROR_SYNC_MODE_TOP) {
         sync = MIRROR_SYNC_MODE_FULL;
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index b1c542f..ed1d9d4 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -207,33 +207,6 @@ class TestSingleBlockdev(TestSingleDrive):
     test_image_not_found = None
     test_small_buffer2 = None
 
-class TestBlockdevAttached(iotests.QMPTestCase):
-    image_len = 1 * 1024 * 1024 # MB
-
-    def setUp(self):
-        iotests.create_image(backing_img, self.image_len)
-        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
-        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, target_img)
-        self.vm = iotests.VM().add_drive(test_img)
-        self.vm.launch()
-
-    def tearDown(self):
-        self.vm.shutdown()
-        os.remove(test_img)
-        os.remove(target_img)
-
-    def test_blockdev_attached(self):
-        self.assert_no_active_block_jobs()
-        args = {'options':
-                    {'driver': iotests.imgfmt,
-                     'id': 'drive1',
-                     'file': { 'filename': target_img, 'driver': 'file' } } }
-        result = self.vm.qmp("blockdev-add", **args)
-        self.assert_qmp(result, 'return', {})
-        result = self.vm.qmp('blockdev-mirror', device='drive0', sync='full',
-                             target='drive1')
-        self.assert_qmp(result, 'error/class', 'GenericError')
-
 class TestSingleDriveZeroLength(TestSingleDrive):
     image_len = 0
     test_small_buffer2 = None
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index b67d050..b0cadc8 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-............................................................................
+...........................................................................
 ----------------------------------------------------------------------
-Ran 76 tests
+Ran 75 tests
 
 OK
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 06/14] mirror: Allow target that already has a BlockBackend

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> We had to forbid mirroring to a target BDS that already had a BB
> attached because the node swapping at job completion would add a second
> BB and we didn't support multiple BBs on a single BDS at the time. Now
> we do, so we can lift the restriction.
> As we allow additional BlockBackends for the target, we must expect
> other users to be sending requests. There may no requests be in flight
> during the graph modification, so we have to drain those users now.
> 
> The core part of this patch is a revert of commit 40365552.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c             | 33 ++++++---------------------------
>  blockdev.c                 |  4 ----
>  tests/qemu-iotests/041     | 27 ---------------------------
>  tests/qemu-iotests/041.out |  4 ++--
>  4 files changed, 8 insertions(+), 60 deletions(-)

I don't really see a use case for this but I guess a ratio of 8+/60- is
enough of an argument.

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


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

[Qemu-devel] [PATCH 07/14] mirror: Use BlockBackend for I/O

[Kevin Wolf]

This changes the mirror block job to use the job's BlockBackend for
performing its I/O. job->bs isn't used by the mirroring code any more
afterwards.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 75 +++++++++++++++++++++++++++++++---------------------------
 blockdev.c     |  4 +---
 2 files changed, 41 insertions(+), 38 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 23aa10e..dc66340 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -35,7 +35,7 @@ typedef struct MirrorBuffer {
 typedef struct MirrorBlockJob {
     BlockJob common;
     RateLimit limit;
-    BlockDriverState *target;
+    BlockBackend *target;
     BlockDriverState *base;
     /* The name of the graph node to replace */
     char *replaces;
@@ -156,8 +156,8 @@ static void mirror_read_complete(void *opaque, int ret)
         mirror_iteration_done(op, ret);
         return;
     }
-    bdrv_aio_writev(s->target, op->sector_num, &op->qiov, op->nb_sectors,
-                    mirror_write_complete, op);
+    blk_aio_writev(s->target, op->sector_num, &op->qiov, op->nb_sectors,
+                   mirror_write_complete, op);
 }
 
 static inline void mirror_clip_sectors(MirrorBlockJob *s,
@@ -185,7 +185,7 @@ static int mirror_cow_align(MirrorBlockJob *s,
     need_cow |= !test_bit((*sector_num + *nb_sectors - 1) / chunk_sectors,
                           s->cow_bitmap);
     if (need_cow) {
-        bdrv_round_to_clusters(s->target, *sector_num, *nb_sectors,
+        bdrv_round_to_clusters(blk_bs(s->target), *sector_num, *nb_sectors,
                                &align_sector_num, &align_nb_sectors);
     }
 
@@ -223,7 +223,7 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
 static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
                           int nb_sectors)
 {
-    BlockDriverState *source = s->common.bs;
+    BlockBackend *source = s->common.blk;
     int sectors_per_chunk, nb_chunks;
     int ret = nb_sectors;
     MirrorOp *op;
@@ -273,8 +273,8 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
     s->sectors_in_flight += nb_sectors;
     trace_mirror_one_iteration(s, sector_num, nb_sectors);
 
-    bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
-                   mirror_read_complete, op);
+    blk_aio_readv(source, sector_num, &op->qiov, nb_sectors,
+                  mirror_read_complete, op);
     return ret;
 }
 
@@ -295,18 +295,18 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
     s->in_flight++;
     s->sectors_in_flight += nb_sectors;
     if (is_discard) {
-        bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
-                         mirror_write_complete, op);
+        blk_aio_discard(s->target, sector_num, op->nb_sectors,
+                        mirror_write_complete, op);
     } else {
-        bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
-                              s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
-                              mirror_write_complete, op);
+        blk_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
+                             s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
+                             mirror_write_complete, op);
     }
 }
 
 static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
 {
-    BlockDriverState *source = s->common.bs;
+    BlockDriverState *source = blk_bs(s->common.blk);
     int64_t sector_num, first_chunk;
     uint64_t delay_ns = 0;
     /* At least the first dirty chunk is mirrored in one iteration. */
@@ -383,7 +383,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         } else if (ret >= 0 && !(ret & BDRV_BLOCK_DATA)) {
             int64_t target_sector_num;
             int target_nb_sectors;
-            bdrv_round_to_clusters(s->target, sector_num, io_sectors,
+            bdrv_round_to_clusters(blk_bs(s->target), sector_num, io_sectors,
                                    &target_sector_num, &target_nb_sectors);
             if (target_sector_num == sector_num &&
                 target_nb_sectors == io_sectors) {
@@ -448,7 +448,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
     MirrorExitData *data = opaque;
     AioContext *replace_aio_context = NULL;
-    BlockDriverState *src = s->common.bs;
+    BlockDriverState *src = blk_bs(s->common.blk);
+    BlockDriverState *target_bs = blk_bs(s->target);
 
     /* Make sure that the source BDS doesn't go away before we called
      * block_job_completed(). */
@@ -460,20 +461,20 @@ static void mirror_exit(BlockJob *job, void *opaque)
     }
 
     if (s->should_complete && data->ret == 0) {
-        BlockDriverState *to_replace = s->common.bs;
+        BlockDriverState *to_replace = src;
         if (s->to_replace) {
             to_replace = s->to_replace;
         }
 
-        if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
-            bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
+        if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
+            bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
         }
 
         /* The mirror job has no requests in flight any more, but we need to
          * drain potential other users of the BDS before changing the graph. */
-        bdrv_drained_begin(s->target);
-        bdrv_replace_in_backing_chain(to_replace, s->target);
-        bdrv_drained_end(s->target);
+        bdrv_drained_begin(target_bs);
+        bdrv_replace_in_backing_chain(to_replace, target_bs);
+        bdrv_drained_end(target_bs);
 
         /* We just changed the BDS the job BB refers to */
         blk_remove_bs(job->blk);
@@ -488,8 +489,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
         aio_context_release(replace_aio_context);
     }
     g_free(s->replaces);
-    bdrv_op_unblock_all(s->target, s->common.blocker);
-    bdrv_unref(s->target);
+    bdrv_op_unblock_all(target_bs, s->common.blocker);
+    blk_unref(s->target);
     block_job_completed(&s->common, data->ret);
     g_free(data);
     bdrv_drained_end(src);
@@ -503,7 +504,8 @@ static void coroutine_fn mirror_run(void *opaque)
 {
     MirrorBlockJob *s = opaque;
     MirrorExitData *data;
-    BlockDriverState *bs = s->common.bs;
+    BlockDriverState *bs = blk_bs(s->common.blk);
+    BlockDriverState *target_bs = blk_bs(s->target);
     int64_t sector_num, end, length;
     uint64_t last_pause_ns;
     BlockDriverInfo bdi;
@@ -539,18 +541,18 @@ static void coroutine_fn mirror_run(void *opaque)
      * the destination do COW.  Instead, we copy sectors around the
      * dirty data if needed.  We need a bitmap to do that.
      */
-    bdrv_get_backing_filename(s->target, backing_filename,
+    bdrv_get_backing_filename(target_bs, backing_filename,
                               sizeof(backing_filename));
-    if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
+    if (!bdrv_get_info(target_bs, &bdi) && bdi.cluster_size) {
         target_cluster_size = bdi.cluster_size;
     }
-    if (backing_filename[0] && !s->target->backing
+    if (backing_filename[0] && !target_bs->backing
         && s->granularity < target_cluster_size) {
         s->buf_size = MAX(s->buf_size, target_cluster_size);
         s->cow_bitmap = bitmap_new(length);
     }
     s->target_cluster_sectors = target_cluster_size >> BDRV_SECTOR_BITS;
-    s->max_iov = MIN(s->common.bs->bl.max_iov, s->target->bl.max_iov);
+    s->max_iov = MIN(bs->bl.max_iov, target_bs->bl.max_iov);
 
     end = s->bdev_length / BDRV_SECTOR_SIZE;
     s->buf = qemu_try_blockalign(bs, s->buf_size);
@@ -565,7 +567,7 @@ static void coroutine_fn mirror_run(void *opaque)
     if (!s->is_none_mode) {
         /* First part, loop on the sectors and initialize the dirty bitmap.  */
         BlockDriverState *base = s->base;
-        bool mark_all_dirty = s->base == NULL && !bdrv_has_zero_init(s->target);
+        bool mark_all_dirty = s->base == NULL && !bdrv_has_zero_init(target_bs);
 
         for (sector_num = 0; sector_num < end; ) {
             /* Just to make sure we are not exceeding int limit. */
@@ -635,7 +637,7 @@ static void coroutine_fn mirror_run(void *opaque)
         should_complete = false;
         if (s->in_flight == 0 && cnt == 0) {
             trace_mirror_before_flush(s);
-            ret = bdrv_flush(s->target);
+            ret = blk_flush(s->target);
             if (ret < 0) {
                 if (mirror_error_action(s, false, -ret) ==
                     BLOCK_ERROR_ACTION_REPORT) {
@@ -713,7 +715,7 @@ immediate_exit:
     data->ret = ret;
     /* Before we switch to target in mirror_exit, make sure data doesn't
      * change. */
-    bdrv_drained_begin(s->common.bs);
+    bdrv_drained_begin(bs);
     if (qemu_get_aio_context() == bdrv_get_aio_context(bs)) {
         /* FIXME: virtio host notifiers run on iohandler_ctx, therefore the
          * above bdrv_drained_end isn't enough to quiesce it. This is ugly, we
@@ -740,7 +742,8 @@ static void mirror_complete(BlockJob *job, Error **errp)
     Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_open_backing_file(s->target, NULL, "backing", &local_err);
+    ret = bdrv_open_backing_file(blk_bs(s->target), NULL, "backing",
+                                 &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         return;
@@ -823,10 +826,12 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    s->target = blk_new(&error_abort);
+    blk_insert_bs(s->target, target);
+
     s->replaces = g_strdup(replaces);
     s->on_source_error = on_source_error;
     s->on_target_error = on_target_error;
-    s->target = target;
     s->is_none_mode = is_none_mode;
     s->base = base;
     s->granularity = granularity;
@@ -836,11 +841,12 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {
         g_free(s->replaces);
+        blk_unref(s->target);
         block_job_unref(&s->common);
         return;
     }
 
-    bdrv_op_block_all(s->target, s->common.blocker);
+    bdrv_op_block_all(target, s->common.blocker);
 
     s->common.co = qemu_coroutine_create(mirror_run);
     trace_mirror_start(bs, s, s->common.co, opaque);
@@ -913,7 +919,6 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
         }
     }
 
-    bdrv_ref(base);
     mirror_start_job(bs, base, NULL, speed, 0, 0,
                      on_error, on_error, false, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base);
diff --git a/blockdev.c b/blockdev.c
index da9eeff..7a0196f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3596,9 +3596,9 @@ void qmp_drive_mirror(const char *device, const char *target,
                            has_on_target_error, on_target_error,
                            has_unmap, unmap,
                            &local_err);
+    bdrv_unref(target_bs);
     if (local_err) {
         error_propagate(errp, local_err);
-        bdrv_unref(target_bs);
     }
 out:
     aio_context_release(aio_context);
@@ -3642,7 +3642,6 @@ void qmp_blockdev_mirror(const char *device, const char *target,
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    bdrv_ref(target_bs);
     bdrv_set_aio_context(target_bs, aio_context);
 
     blockdev_mirror_common(bs, target_bs,
@@ -3656,7 +3655,6 @@ void qmp_blockdev_mirror(const char *device, const char *target,
                            &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        bdrv_unref(target_bs);
     }
 
     aio_context_release(aio_context);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 07/14] mirror: Use BlockBackend for I/O

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> This changes the mirror block job to use the job's BlockBackend for
> performing its I/O. job->bs isn't used by the mirroring code any more
> afterwards.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 75 +++++++++++++++++++++++++++++++---------------------------
>  blockdev.c     |  4 +---
>  2 files changed, 41 insertions(+), 38 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 23aa10e..dc66340 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c

[...]

> @@ -823,10 +826,12 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
>          return;
>      }
>  
> +    s->target = blk_new(&error_abort);

Same as in patch 3.

Rest looks good.

Max

> +    blk_insert_bs(s->target, target);
> +
>      s->replaces = g_strdup(replaces);
>      s->on_source_error = on_source_error;
>      s->on_target_error = on_target_error;
> -    s->target = target;
>      s->is_none_mode = is_none_mode;
>      s->base = base;
>      s->granularity = granularity;



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

Re: [Qemu-devel] [PATCH 07/14] mirror: Use BlockBackend for I/O

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> This changes the mirror block job to use the job's BlockBackend for
> performing its I/O. job->bs isn't used by the mirroring code any more
> afterwards.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 75 +++++++++++++++++++++++++++++++---------------------------
>  blockdev.c     |  4 +---
>  2 files changed, 41 insertions(+), 38 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 23aa10e..dc66340 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -35,7 +35,7 @@ typedef struct MirrorBuffer {
>  typedef struct MirrorBlockJob {
>      BlockJob common;
>      RateLimit limit;
> -    BlockDriverState *target;
> +    BlockBackend *target;
>      BlockDriverState *base;
>      /* The name of the graph node to replace */
>      char *replaces;
> @@ -156,8 +156,8 @@ static void mirror_read_complete(void *opaque, int ret)
>          mirror_iteration_done(op, ret);
>          return;
>      }
> -    bdrv_aio_writev(s->target, op->sector_num, &op->qiov, op->nb_sectors,
> -                    mirror_write_complete, op);
> +    blk_aio_writev(s->target, op->sector_num, &op->qiov, op->nb_sectors,
> +                   mirror_write_complete, op);

Or maybe the rest doesn't look so good after all. We don't have this
function anymore, I'm afraid.

Max

>  }
>  
>  static inline void mirror_clip_sectors(MirrorBlockJob *s,


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

Re: [Qemu-devel] [PATCH 07/14] mirror: Use BlockBackend for I/O

[Eric Blake]

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

On 05/13/2016 08:38 AM, Max Reitz wrote:
> On 04.05.2016 11:39, Kevin Wolf wrote:
>> This changes the mirror block job to use the job's BlockBackend for
>> performing its I/O. job->bs isn't used by the mirroring code any more
>> afterwards.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---

>> @@ -156,8 +156,8 @@ static void mirror_read_complete(void *opaque, int ret)
>>          mirror_iteration_done(op, ret);
>>          return;
>>      }
>> -    bdrv_aio_writev(s->target, op->sector_num, &op->qiov, op->nb_sectors,
>> -                    mirror_write_complete, op);
>> +    blk_aio_writev(s->target, op->sector_num, &op->qiov, op->nb_sectors,
>> +                   mirror_write_complete, op);
> 
> Or maybe the rest doesn't look so good after all. We don't have this
> function anymore, I'm afraid.

Relatively mechanical conversion to
 blk_aio_pwritev(s->target, op->sector_num << BDRV_SECTOR_BITS,
                 &op->qiov, 0, mirror_write_complete, op)

although switching away from sectors to bytes in general is a bigger cleanup

-- 
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 08/14] backup: Don't leak BackupBlockJob in error path

[Kevin Wolf]

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

diff --git a/block/backup.c b/block/backup.c
index fec45e8..a990cf1 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -485,6 +485,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
 {
     int64_t len;
     BlockDriverInfo bdi;
+    BackupBlockJob *job = NULL;
     int ret;
 
     assert(bs);
@@ -542,8 +543,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         goto error;
     }
 
-    BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
-                                           cb, opaque, errp);
+    job = block_job_create(&backup_job_driver, bs, speed, cb, opaque, errp);
     if (!job) {
         goto error;
     }
@@ -584,4 +584,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
     if (sync_bitmap) {
         bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
     }
+    if (job) {
+        block_job_unref(&job->common);
+    }
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 08/14] backup: Don't leak BackupBlockJob in error path

[Alberto Garcia]

On Wed 04 May 2016 11:39:19 AM CEST, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 08/14] backup: Don't leak BackupBlockJob in error path

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/backup.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

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


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

[Qemu-devel] [PATCH 09/14] backup: Pack Notifier within BackupBlockJob

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

Instead of relying on peeking at bs->job, we want to explicitly get
a reference to the job that was involved in this notifier callback.

Pack the Notifier inside of the BackupBlockJob so we can use
container_of to get a reference back to the BackupBlockJob object.

This cuts out one more case where we rely unnecessarily on bs->job.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index a990cf1..a57288f 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -47,6 +47,7 @@ typedef struct BackupBlockJob {
     uint64_t sectors_read;
     unsigned long *done_bitmap;
     int64_t cluster_size;
+    NotifierWithReturn before_write;
     QLIST_HEAD(, CowRequest) inflight_reqs;
 } BackupBlockJob;
 
@@ -94,11 +95,11 @@ static void cow_request_end(CowRequest *req)
 }
 
 static int coroutine_fn backup_do_cow(BlockDriverState *bs,
+                                      BackupBlockJob *job,
                                       int64_t sector_num, int nb_sectors,
                                       bool *error_is_read,
                                       bool is_write_notifier)
 {
-    BackupBlockJob *job = (BackupBlockJob *)bs->job;
     CowRequest cow_request;
     struct iovec iov;
     QEMUIOVector bounce_qiov;
@@ -197,6 +198,7 @@ static int coroutine_fn backup_before_write_notify(
         NotifierWithReturn *notifier,
         void *opaque)
 {
+    BackupBlockJob *job = container_of(notifier, BackupBlockJob, before_write);
     BdrvTrackedRequest *req = opaque;
     int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
     int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
@@ -204,7 +206,8 @@ static int coroutine_fn backup_before_write_notify(
     assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
     assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
 
-    return backup_do_cow(req->bs, sector_num, nb_sectors, NULL, true);
+    return backup_do_cow(req->bs, job, sector_num,
+                         nb_sectors, NULL, true);
 }
 
 static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
@@ -343,7 +346,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
                 if (yield_and_check(job)) {
                     return ret;
                 }
-                ret = backup_do_cow(bs, cluster * sectors_per_cluster,
+                ret = backup_do_cow(bs, job, cluster * sectors_per_cluster,
                                     sectors_per_cluster, &error_is_read,
                                     false);
                 if ((ret < 0) &&
@@ -378,9 +381,6 @@ static void coroutine_fn backup_run(void *opaque)
     BackupCompleteData *data;
     BlockDriverState *bs = job->common.bs;
     BlockDriverState *target = job->target;
-    NotifierWithReturn before_write = {
-        .notify = backup_before_write_notify,
-    };
     int64_t start, end;
     int64_t sectors_per_cluster = cluster_size_sectors(job);
     int ret = 0;
@@ -393,7 +393,8 @@ static void coroutine_fn backup_run(void *opaque)
 
     job->done_bitmap = bitmap_new(end);
 
-    bdrv_add_before_write_notifier(bs, &before_write);
+    job->before_write.notify = backup_before_write_notify;
+    bdrv_add_before_write_notifier(bs, &job->before_write);
 
     if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
         while (!block_job_is_cancelled(&job->common)) {
@@ -445,7 +446,7 @@ static void coroutine_fn backup_run(void *opaque)
                 }
             }
             /* FULL sync mode we copy the whole drive. */
-            ret = backup_do_cow(bs, start * sectors_per_cluster,
+            ret = backup_do_cow(bs, job, start * sectors_per_cluster,
                                 sectors_per_cluster, &error_is_read, false);
             if (ret < 0) {
                 /* Depending on error action, fail now or retry cluster */
@@ -461,7 +462,7 @@ static void coroutine_fn backup_run(void *opaque)
         }
     }
 
-    notifier_with_return_remove(&before_write);
+    notifier_with_return_remove(&job->before_write);
 
     /* wait until pending backup_do_cow() calls have completed */
     qemu_co_rwlock_wrlock(&job->flush_rwlock);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 09/14] backup: Pack Notifier within BackupBlockJob

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> From: John Snow <jsnow@redhat.com>
> 
> Instead of relying on peeking at bs->job, we want to explicitly get
> a reference to the job that was involved in this notifier callback.
> 
> Pack the Notifier inside of the BackupBlockJob so we can use
> container_of to get a reference back to the BackupBlockJob object.
> 
> This cuts out one more case where we rely unnecessarily on bs->job.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/backup.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

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


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

[Qemu-devel] [PATCH 10/14] backup: Remove bs parameter from backup_do_cow()

[Kevin Wolf]

Now that we pass the job to the function, bs is implied by that.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index a57288f..670ba50 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -94,12 +94,12 @@ static void cow_request_end(CowRequest *req)
     qemu_co_queue_restart_all(&req->wait_queue);
 }
 
-static int coroutine_fn backup_do_cow(BlockDriverState *bs,
-                                      BackupBlockJob *job,
+static int coroutine_fn backup_do_cow(BackupBlockJob *job,
                                       int64_t sector_num, int nb_sectors,
                                       bool *error_is_read,
                                       bool is_write_notifier)
 {
+    BlockDriverState *bs = job->common.bs;
     CowRequest cow_request;
     struct iovec iov;
     QEMUIOVector bounce_qiov;
@@ -203,11 +203,11 @@ static int coroutine_fn backup_before_write_notify(
     int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
     int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
 
+    assert(req->bs == job->common.bs);
     assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
     assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
 
-    return backup_do_cow(req->bs, job, sector_num,
-                         nb_sectors, NULL, true);
+    return backup_do_cow(job, sector_num, nb_sectors, NULL, true);
 }
 
 static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
@@ -324,7 +324,6 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
     int64_t end;
     int64_t last_cluster = -1;
     int64_t sectors_per_cluster = cluster_size_sectors(job);
-    BlockDriverState *bs = job->common.bs;
     HBitmapIter hbi;
 
     granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
@@ -346,7 +345,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
                 if (yield_and_check(job)) {
                     return ret;
                 }
-                ret = backup_do_cow(bs, job, cluster * sectors_per_cluster,
+                ret = backup_do_cow(job, cluster * sectors_per_cluster,
                                     sectors_per_cluster, &error_is_read,
                                     false);
                 if ((ret < 0) &&
@@ -446,7 +445,7 @@ static void coroutine_fn backup_run(void *opaque)
                 }
             }
             /* FULL sync mode we copy the whole drive. */
-            ret = backup_do_cow(bs, job, start * sectors_per_cluster,
+            ret = backup_do_cow(job, start * sectors_per_cluster,
                                 sectors_per_cluster, &error_is_read, false);
             if (ret < 0) {
                 /* Depending on error action, fail now or retry cluster */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 10/14] backup: Remove bs parameter from backup_do_cow()

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> Now that we pass the job to the function, bs is implied by that.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/backup.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)

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


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

[Qemu-devel] [PATCH 11/14] block: Add blk_co_readv/writev()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 26 ++++++++++++++++++++++++++
 include/sysemu/block-backend.h |  4 ++++
 trace-events                   |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 25bb5ca..8dc3aa5 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -794,6 +794,19 @@ static int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
     return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
 }
 
+int coroutine_fn blk_co_readv(BlockBackend *blk, int64_t sector_num,
+                              int nb_sectors, QEMUIOVector *qiov)
+{
+    trace_blk_co_readv(blk, blk_bs(blk), sector_num, nb_sectors);
+
+    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
+        return -EINVAL;
+    }
+
+    return blk_co_preadv(blk, sector_num << BDRV_SECTOR_BITS,
+                         nb_sectors << BDRV_SECTOR_BITS, qiov, 0);
+}
+
 int coroutine_fn blk_co_copy_on_readv(BlockBackend *blk,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
@@ -808,6 +821,19 @@ int coroutine_fn blk_co_copy_on_readv(BlockBackend *blk,
                          BDRV_REQ_COPY_ON_READ);
 }
 
+int coroutine_fn blk_co_writev(BlockBackend *blk, int64_t sector_num,
+                               int nb_sectors, QEMUIOVector *qiov)
+{
+    trace_blk_co_writev(blk, blk_bs(blk), sector_num, nb_sectors);
+
+    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
+        return -EINVAL;
+    }
+
+    return blk_co_pwritev(blk, sector_num << BDRV_SECTOR_BITS,
+                          nb_sectors << BDRV_SECTOR_BITS, qiov, 0);
+}
+
 typedef struct BlkRwCo {
     BlockBackend *blk;
     int64_t offset;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index eac4d88..d98552c 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -116,8 +116,12 @@ int blk_read(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
              int nb_sectors);
 int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
                          int nb_sectors);
+int coroutine_fn blk_co_readv(BlockBackend *blk, int64_t sector_num,
+                              int nb_sectors, QEMUIOVector *qiov);
 int coroutine_fn blk_co_copy_on_readv(BlockBackend *blk,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
+int coroutine_fn blk_co_writev(BlockBackend *blk, int64_t sector_num,
+                               int nb_sectors, QEMUIOVector *qiov);
 int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
               int nb_sectors);
 int blk_write_zeroes(BlockBackend *blk, int64_t sector_num,
diff --git a/trace-events b/trace-events
index 9beb588..3b5cce3 100644
--- a/trace-events
+++ b/trace-events
@@ -62,6 +62,8 @@ bdrv_open_common(void *bs, const char *filename, int flags, const char *format_n
 bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
 
 # block/block-backend.c
+blk_co_readv(void *blk, void *bs, int64_t sector_num, int nb_sector) "blk %p bs %p sector_num %"PRId64" nb_sectors %d"
+blk_co_writev(void *blk, void *bs, int64_t sector_num, int nb_sector) "blk %p bs %p sector_num %"PRId64" nb_sectors %d"
 blk_co_copy_on_readv(void *blk, void *bs, int64_t sector_num, int nb_sector) "blk %p bs %p sector_num %"PRId64" nb_sectors %d"
 
 # block/io.c
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 11/14] block: Add blk_co_readv/writev()

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 26 ++++++++++++++++++++++++++
>  include/sysemu/block-backend.h |  4 ++++
>  trace-events                   |  2 ++
>  3 files changed, 32 insertions(+)

Reasonable in principle, but not sure whether we still want to add it now.

Max


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

[Qemu-devel] [PATCH 12/14] backup: Use BlockBackend for I/O

[Kevin Wolf]

This changes the backup block job to use the job's BlockBackend for
performing its I/O. job->bs isn't used by the backup code any more
afterwards.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup.c                 | 42 ++++++++++++++++++++++--------------------
 block/block-backend.c          | 14 ++++++++++++++
 block/io.c                     |  9 ---------
 blockdev.c                     |  4 +---
 include/block/block.h          |  2 --
 include/sysemu/block-backend.h |  2 ++
 trace-events                   |  2 +-
 7 files changed, 40 insertions(+), 35 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 670ba50..ba9eb42 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -36,7 +36,7 @@ typedef struct CowRequest {
 
 typedef struct BackupBlockJob {
     BlockJob common;
-    BlockDriverState *target;
+    BlockBackend *target;
     /* bitmap for sync=incremental */
     BdrvDirtyBitmap *sync_bitmap;
     MirrorSyncMode sync_mode;
@@ -99,7 +99,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
                                       bool *error_is_read,
                                       bool is_write_notifier)
 {
-    BlockDriverState *bs = job->common.bs;
+    BlockBackend *blk = job->common.blk;
     CowRequest cow_request;
     struct iovec iov;
     QEMUIOVector bounce_qiov;
@@ -132,19 +132,18 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
                 start * sectors_per_cluster);
 
         if (!bounce_buffer) {
-            bounce_buffer = qemu_blockalign(bs, job->cluster_size);
+            bounce_buffer = blk_blockalign(blk, job->cluster_size);
         }
         iov.iov_base = bounce_buffer;
         iov.iov_len = n * BDRV_SECTOR_SIZE;
         qemu_iovec_init_external(&bounce_qiov, &iov, 1);
 
         if (is_write_notifier) {
-            ret = bdrv_co_readv_no_serialising(bs,
-                                           start * sectors_per_cluster,
-                                           n, &bounce_qiov);
+            ret = blk_co_readv_no_serialising(blk, start * sectors_per_cluster,
+                                              n, &bounce_qiov);
         } else {
-            ret = bdrv_co_readv(bs, start * sectors_per_cluster, n,
-                                &bounce_qiov);
+            ret = blk_co_readv(blk, start * sectors_per_cluster, n,
+                               &bounce_qiov);
         }
         if (ret < 0) {
             trace_backup_do_cow_read_fail(job, start, ret);
@@ -155,13 +154,13 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
         }
 
         if (buffer_is_zero(iov.iov_base, iov.iov_len)) {
-            ret = bdrv_co_write_zeroes(job->target,
+            ret = blk_co_write_zeroes(job->target,
                                        start * sectors_per_cluster,
                                        n, BDRV_REQ_MAY_UNMAP);
         } else {
-            ret = bdrv_co_writev(job->target,
-                                 start * sectors_per_cluster, n,
-                                 &bounce_qiov);
+            ret = blk_co_writev(job->target,
+                                start * sectors_per_cluster, n,
+                                &bounce_qiov);
         }
         if (ret < 0) {
             trace_backup_do_cow_write_fail(job, start, ret);
@@ -203,7 +202,7 @@ static int coroutine_fn backup_before_write_notify(
     int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
     int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
 
-    assert(req->bs == job->common.bs);
+    assert(req->bs == blk_bs(job->common.blk));
     assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
     assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
 
@@ -224,7 +223,7 @@ static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
 static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
 {
     BdrvDirtyBitmap *bm;
-    BlockDriverState *bs = job->common.bs;
+    BlockDriverState *bs = blk_bs(job->common.blk);
 
     if (ret < 0 || block_job_is_cancelled(&job->common)) {
         /* Merge the successor back into the parent, delete nothing. */
@@ -282,7 +281,7 @@ static void backup_complete(BlockJob *job, void *opaque)
     BackupBlockJob *s = container_of(job, BackupBlockJob, common);
     BackupCompleteData *data = opaque;
 
-    bdrv_unref(s->target);
+    blk_unref(s->target);
 
     block_job_completed(job, data->ret);
     g_free(data);
@@ -378,8 +377,8 @@ static void coroutine_fn backup_run(void *opaque)
 {
     BackupBlockJob *job = opaque;
     BackupCompleteData *data;
-    BlockDriverState *bs = job->common.bs;
-    BlockDriverState *target = job->target;
+    BlockDriverState *bs = blk_bs(job->common.blk);
+    BlockBackend *target = job->target;
     int64_t start, end;
     int64_t sectors_per_cluster = cluster_size_sectors(job);
     int ret = 0;
@@ -468,7 +467,7 @@ static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_unlock(&job->flush_rwlock);
     g_free(job->done_bitmap);
 
-    bdrv_op_unblock_all(target, job->common.blocker);
+    bdrv_op_unblock_all(blk_bs(target), job->common.blocker);
 
     data = g_malloc(sizeof(*data));
     data->ret = ret;
@@ -548,9 +547,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         goto error;
     }
 
+    job->target = blk_new(&error_abort);
+    blk_insert_bs(job->target, target);
+
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
-    job->target = target;
     job->sync_mode = sync_mode;
     job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
                        sync_bitmap : NULL;
@@ -558,7 +559,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
     /* If there is no backing file on the target, we cannot rely on COW if our
      * backup cluster size is smaller than the target cluster size. Even for
      * targets with a backing file, try to avoid COW if possible. */
-    ret = bdrv_get_info(job->target, &bdi);
+    ret = bdrv_get_info(target, &bdi);
     if (ret < 0 && !target->backing) {
         error_setg_errno(errp, -ret,
             "Couldn't determine the cluster size of the target image, "
@@ -585,6 +586,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
     }
     if (job) {
+        blk_unref(job->target);
         block_job_unref(&job->common);
     }
 }
diff --git a/block/block-backend.c b/block/block-backend.c
index 8dc3aa5..2b3c55f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -807,6 +807,20 @@ int coroutine_fn blk_co_readv(BlockBackend *blk, int64_t sector_num,
                          nb_sectors << BDRV_SECTOR_BITS, qiov, 0);
 }
 
+int coroutine_fn blk_co_readv_no_serialising(BlockBackend *blk,
+    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
+{
+    trace_blk_co_readv_no_serialising(blk, blk_bs(blk), sector_num, nb_sectors);
+
+    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
+        return -EINVAL;
+    }
+
+    return blk_co_preadv(blk, sector_num << BDRV_SECTOR_BITS,
+                         nb_sectors << BDRV_SECTOR_BITS, qiov,
+                         BDRV_REQ_NO_SERIALISING);
+}
+
 int coroutine_fn blk_co_copy_on_readv(BlockBackend *blk,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
diff --git a/block/io.c b/block/io.c
index 8280e14..eeb0e4c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1093,15 +1093,6 @@ int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
     return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov, 0);
 }
 
-int coroutine_fn bdrv_co_readv_no_serialising(BlockDriverState *bs,
-    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
-{
-    trace_bdrv_co_readv_no_serialising(bs, sector_num, nb_sectors);
-
-    return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov,
-                            BDRV_REQ_NO_SERIALISING);
-}
-
 #define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
 
 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index 7a0196f..fa381ce 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3268,8 +3268,8 @@ static void do_drive_backup(const char *device, const char *target,
     backup_start(bs, target_bs, speed, sync, bmap,
                  on_source_error, on_target_error,
                  block_job_cb, bs, txn, &local_err);
+    bdrv_unref(target_bs);
     if (local_err != NULL) {
-        bdrv_unref(target_bs);
         error_propagate(errp, local_err);
         goto out;
     }
@@ -3353,12 +3353,10 @@ void do_blockdev_backup(const char *device, const char *target,
     }
     target_bs = blk_bs(target_blk);
 
-    bdrv_ref(target_bs);
     bdrv_set_aio_context(target_bs, aio_context);
     backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
                  on_target_error, block_job_cb, bs, txn, &local_err);
     if (local_err != NULL) {
-        bdrv_unref(target_bs);
         error_propagate(errp, local_err);
     }
 out:
diff --git a/include/block/block.h b/include/block/block.h
index 39b5812..8e7d348 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -243,8 +243,6 @@ 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_no_serialising(BlockDriverState *bs,
-    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);
 /*
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index d98552c..715fd9e 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -118,6 +118,8 @@ int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
                          int nb_sectors);
 int coroutine_fn blk_co_readv(BlockBackend *blk, int64_t sector_num,
                               int nb_sectors, QEMUIOVector *qiov);
+int coroutine_fn blk_co_readv_no_serialising(BlockBackend *blk,
+    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
 int coroutine_fn blk_co_copy_on_readv(BlockBackend *blk,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
 int coroutine_fn blk_co_writev(BlockBackend *blk, int64_t sector_num,
diff --git a/trace-events b/trace-events
index 3b5cce3..2ee3cb7 100644
--- a/trace-events
+++ b/trace-events
@@ -65,6 +65,7 @@ bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
 blk_co_readv(void *blk, void *bs, int64_t sector_num, int nb_sector) "blk %p bs %p sector_num %"PRId64" nb_sectors %d"
 blk_co_writev(void *blk, void *bs, int64_t sector_num, int nb_sector) "blk %p bs %p sector_num %"PRId64" nb_sectors %d"
 blk_co_copy_on_readv(void *blk, void *bs, int64_t sector_num, int nb_sector) "blk %p bs %p sector_num %"PRId64" nb_sectors %d"
+blk_co_readv_no_serialising(void *blk, void *bs, int64_t sector_num, int nb_sector) "blk %p bs %p sector_num %"PRId64" nb_sectors %d"
 
 # block/io.c
 bdrv_aio_discard(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
@@ -73,7 +74,6 @@ bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %
 bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
 bdrv_aio_write_zeroes(void *bs, int64_t sector_num, int nb_sectors, int flags, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x opaque %p"
 bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
-bdrv_co_readv_no_serialising(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
 bdrv_co_writev(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
 bdrv_co_write_zeroes(void *bs, int64_t sector_num, int nb_sector, int flags) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x"
 bdrv_co_do_copy_on_readv(void *bs, int64_t sector_num, int nb_sectors, int64_t cluster_sector_num, int cluster_nb_sectors) "bs %p sector_num %"PRId64" nb_sectors %d cluster_sector_num %"PRId64" cluster_nb_sectors %d"
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 12/14] backup: Use BlockBackend for I/O

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> This changes the backup block job to use the job's BlockBackend for
> performing its I/O. job->bs isn't used by the backup code any more
> afterwards.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/backup.c                 | 42 ++++++++++++++++++++++--------------------
>  block/block-backend.c          | 14 ++++++++++++++
>  block/io.c                     |  9 ---------
>  blockdev.c                     |  4 +---
>  include/block/block.h          |  2 --
>  include/sysemu/block-backend.h |  2 ++
>  trace-events                   |  2 +-
>  7 files changed, 40 insertions(+), 35 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 670ba50..ba9eb42 100644
> --- a/block/backup.c
> +++ b/block/backup.c

[...]

> @@ -548,9 +547,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>          goto error;
>      }
>  
> +    job->target = blk_new(&error_abort);

*cough*

> +    blk_insert_bs(job->target, target);
> +
>      job->on_source_error = on_source_error;
>      job->on_target_error = on_target_error;
> -    job->target = target;
>      job->sync_mode = sync_mode;
>      job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
>                         sync_bitmap : NULL;

[...]

> diff --git a/block/block-backend.c b/block/block-backend.c
> index 8dc3aa5..2b3c55f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -807,6 +807,20 @@ int coroutine_fn blk_co_readv(BlockBackend *blk, int64_t sector_num,
>                           nb_sectors << BDRV_SECTOR_BITS, qiov, 0);
>  }
>  
> +int coroutine_fn blk_co_readv_no_serialising(BlockBackend *blk,
> +    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> +{
> +    trace_blk_co_readv_no_serialising(blk, blk_bs(blk), sector_num, nb_sectors);
> +
> +    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
> +        return -EINVAL;
> +    }
> +
> +    return blk_co_preadv(blk, sector_num << BDRV_SECTOR_BITS,
> +                         nb_sectors << BDRV_SECTOR_BITS, qiov,
> +                         BDRV_REQ_NO_SERIALISING);
> +}
> +

Same concern as in the other places whether we really want this to have
a sector-based interface...

>  int coroutine_fn blk_co_copy_on_readv(BlockBackend *blk,
>      int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
>  {

[...]

> diff --git a/blockdev.c b/blockdev.c
> index 7a0196f..fa381ce 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3268,8 +3268,8 @@ static void do_drive_backup(const char *device, const char *target,
>      backup_start(bs, target_bs, speed, sync, bmap,
>                   on_source_error, on_target_error,
>                   block_job_cb, bs, txn, &local_err);
> +    bdrv_unref(target_bs);

Alternatively you could pull it down under the "out" label and then
remove another bdrv_unref(target_bs) instance in this function.

Max

>      if (local_err != NULL) {
> -        bdrv_unref(target_bs);
>          error_propagate(errp, local_err);
>          goto out;
>      }


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

[Qemu-devel] [PATCH 13/14] commit: Use BlockBackend for I/O

[Kevin Wolf]

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,
+        .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;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 13/14] commit: Use BlockBackend for I/O

[Max Reitz]

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

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



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

[Qemu-devel] [PATCH 14/14] blockjob: Remove BlockJob.bs

[Kevin Wolf]

There is a single remaining user in qemu-img, which can be trivially
converted to using BlockJob.blk instead.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockjob.c               | 1 -
 include/block/blockjob.h | 1 -
 qemu-img.c               | 2 +-
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 01b58c7..c0d8ee9 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -83,7 +83,6 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
 
     job->driver        = driver;
     job->id            = g_strdup(bdrv_get_device_name(bs));
-    job->bs            = bs;
     job->blk           = blk;
     job->cb            = cb;
     job->opaque        = opaque;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 32012af..86d2807 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -82,7 +82,6 @@ struct BlockJob {
     const BlockJobDriver *driver;
 
     /** The block device on which the job is operating.  */
-    BlockDriverState *bs; /* TODO Remove */
     BlockBackend *blk;
 
     /**
diff --git a/qemu-img.c b/qemu-img.c
index 46f2a6d..77148c2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -775,7 +775,7 @@ static void common_block_job_cb(void *opaque, int ret)
 
 static void run_block_job(BlockJob *job, Error **errp)
 {
-    AioContext *aio_context = bdrv_get_aio_context(job->bs);
+    AioContext *aio_context = blk_get_aio_context(job->blk);
 
     do {
         aio_poll(aio_context, true);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 14/14] blockjob: Remove BlockJob.bs

[Max Reitz]

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

On 04.05.2016 11:39, Kevin Wolf wrote:
> There is a single remaining user in qemu-img, which can be trivially
> converted to using BlockJob.blk instead.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockjob.c               | 1 -
>  include/block/blockjob.h | 1 -
>  qemu-img.c               | 2 +-
>  3 files changed, 1 insertion(+), 3 deletions(-)

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


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