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

[Qemu-devel] [PATCH v2 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 my current block branch (because of the drain fixes).

v2:
- Rebase conflict resolution, in particular:
  * bdrv_new() without parameter
  * blk_*write_zeroes() is sector based now
- Assert that all BDSes are gone after bdrv_close_all [Max, Berto]
- Don't add new sector based blk_* interfaces. Instead, blk_co_preadv/pwritev
  are made public. [Max]
- commit: Removed unnecessary cast [Max]
- commit: Use blk_* functions in commit_run() [Max]

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
  block: Make blk_co_preadv/pwritev() public
  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()
  backup: Use BlockBackend for I/O
  commit: Use BlockBackend for I/O
  blockjob: Remove BlockJob.bs

 block.c                        |  23 +--------
 block/backup.c                 |  71 ++++++++++++++--------------
 block/block-backend.c          |  24 ++++++----
 block/commit.c                 |  53 +++++++++++++--------
 block/io.c                     |  18 -------
 block/mirror.c                 | 104 +++++++++++++++++++----------------------
 block/stream.c                 |  15 +++---
 blockdev.c                     |  12 +----
 blockjob.c                     |  62 +++++++++++++++++-------
 include/block/block.h          |   4 --
 include/block/blockjob.h       |  23 ++++++++-
 include/sysemu/block-backend.h |   6 +++
 qemu-img.c                     |   2 +-
 tests/qemu-iotests/041         |  27 -----------
 tests/qemu-iotests/041.out     |   4 +-
 trace-events                   |   6 ++-
 16 files changed, 224 insertions(+), 230 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Max Reitz <mreitz@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

[Qemu-devel] [PATCH v2 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, 22 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index 45cddd6..736432f 100644
--- a/block.c
+++ b/block.c
@@ -2209,8 +2209,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 */
@@ -2219,25 +2218,7 @@ 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);
-    }
+    assert(QTAILQ_EMPTY(&all_bdrv_states));
 }
 
 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 v2 02/14] block: Cancel jobs first in bdrv_close_all()

[Eric Blake]

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

On 05/24/2016 07:47 AM, 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, 22 insertions(+), 21 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Alberto Garcia]

On Tue 24 May 2016 03:47:22 PM CEST, 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>
Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Max Reitz <mreitz@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 14e528e..7257d7b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -125,6 +125,8 @@ BlockBackend *blk_new(void)
 
     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]);
 
@@ -160,7 +162,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);
 
     return blk;
-- 
1.8.3.1

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

[Alberto Garcia]

On Tue 24 May 2016 03:47:23 PM CEST, 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH v2 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..2097e1d 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();
+    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 v2 04/14] block: Convert block job core to BlockBackend

[Eric Blake]

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

On 05/24/2016 07:47 AM, 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(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Changlong Xie]

On 05/24/2016 09:47 PM, Kevin Wolf wrote:
> +    blk = blk_new();
blk_new(errp);

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

[Alberto Garcia]

On Tue 24 May 2016 03:47:24 PM CEST, 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>

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

Berto

[Qemu-devel] [PATCH v2 05/14] block: Make blk_co_preadv/pwritev() public

[Kevin Wolf]

Also add trace points now that the function can be directly called.

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

diff --git a/block/block-backend.c b/block/block-backend.c
index 7257d7b..47e22ed 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
@@ -741,11 +742,15 @@ static int blk_check_request(BlockBackend *blk, int64_t sector_num,
                                   nb_sectors * BDRV_SECTOR_SIZE);
 }
 
-static int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
-                                      unsigned int bytes, QEMUIOVector *qiov,
-                                      BdrvRequestFlags flags)
+int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
+                               unsigned int bytes, QEMUIOVector *qiov,
+                               BdrvRequestFlags flags)
 {
-    int ret = blk_check_byte_request(blk, offset, bytes);
+    int ret;
+
+    trace_blk_co_preadv(blk, blk_bs(blk), offset, bytes, flags);
+
+    ret = blk_check_byte_request(blk, offset, bytes);
     if (ret < 0) {
         return ret;
     }
@@ -758,12 +763,14 @@ static int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
     return bdrv_co_preadv(blk_bs(blk), offset, bytes, qiov, flags);
 }
 
-static int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
-                                      unsigned int bytes, QEMUIOVector *qiov,
-                                      BdrvRequestFlags flags)
+int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
+                                unsigned int bytes, QEMUIOVector *qiov,
+                                BdrvRequestFlags flags)
 {
     int ret;
 
+    trace_blk_co_pwritev(blk, blk_bs(blk), offset, bytes, flags);
+
     ret = blk_check_byte_request(blk, offset, bytes);
     if (ret < 0) {
         return ret;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9d6615c..f2de8ac 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -113,6 +113,12 @@ void *blk_get_attached_dev(BlockBackend *blk);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
 int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
                           int count);
+int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
+                               unsigned int bytes, QEMUIOVector *qiov,
+                               BdrvRequestFlags flags);
+int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
+                               unsigned int bytes, QEMUIOVector *qiov,
+                               BdrvRequestFlags flags);
 int blk_write_zeroes(BlockBackend *blk, int64_t offset,
                      int count, BdrvRequestFlags flags);
 BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t offset,
diff --git a/trace-events b/trace-events
index b53c354..5d431bd 100644
--- a/trace-events
+++ b/trace-events
@@ -61,6 +61,10 @@ 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_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags %x"
+blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags %x"
+
 # 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"
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 05/14] block: Make blk_co_preadv/pwritev() public

[Eric Blake]

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

On 05/24/2016 07:47 AM, Kevin Wolf wrote:
> Also add trace points now that the function can be directly called.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 21 ++++++++++++++-------
>  include/sysemu/block-backend.h |  6 ++++++
>  trace-events                   |  4 ++++
>  3 files changed, 24 insertions(+), 7 deletions(-)
> 

> @@ -741,11 +742,15 @@ static int blk_check_request(BlockBackend *blk, int64_t sector_num,
>                                    nb_sectors * BDRV_SECTOR_SIZE);
>  }
>  
> -static int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
> -                                      unsigned int bytes, QEMUIOVector *qiov,
> -                                      BdrvRequestFlags flags)
> +int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
> +                               unsigned int bytes, QEMUIOVector *qiov,

Isn't bytes redundant with qiov->size? Or can qiov be NULL?  Should we
assert(!qiov || qiov->size == bytes)?

> +int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
> +                                unsigned int bytes, QEMUIOVector *qiov,
> +                                BdrvRequestFlags flags)
>  {

Ditto. When doing write_zeroes, do we want qiov == NULL, must we always
have qiov but just leave qiov->iov[0].base as NULL?  Probably worth
documenting as part of making it public.


> +++ b/include/sysemu/block-backend.h
> @@ -113,6 +113,12 @@ void *blk_get_attached_dev(BlockBackend *blk);
>  void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
>  int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
>                            int count);
> +int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
> +                               unsigned int bytes, QEMUIOVector *qiov,
> +                               BdrvRequestFlags flags);
> +int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
> +                               unsigned int bytes, QEMUIOVector *qiov,
> +                               BdrvRequestFlags flags);

Note that my earlier addition of blk_aio_pwritev intentionally omitted
the bytes argument, relying solely on qiov->size.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v2 05/14] block: Make blk_co_preadv/pwritev() public

[Kevin Wolf]

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

Am 25.05.2016 um 02:24 hat Eric Blake geschrieben:
> On 05/24/2016 07:47 AM, Kevin Wolf wrote:
> > Also add trace points now that the function can be directly called.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/block-backend.c          | 21 ++++++++++++++-------
> >  include/sysemu/block-backend.h |  6 ++++++
> >  trace-events                   |  4 ++++
> >  3 files changed, 24 insertions(+), 7 deletions(-)
> > 
> 
> > @@ -741,11 +742,15 @@ static int blk_check_request(BlockBackend *blk, int64_t sector_num,
> >                                    nb_sectors * BDRV_SECTOR_SIZE);
> >  }
> >  
> > -static int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
> > -                                      unsigned int bytes, QEMUIOVector *qiov,
> > -                                      BdrvRequestFlags flags)
> > +int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
> > +                               unsigned int bytes, QEMUIOVector *qiov,
> 
> Isn't bytes redundant with qiov->size? Or can qiov be NULL?  Should we
> assert(!qiov || qiov->size == bytes)?

It's redundant for reads, as far as I know. I'm not completely sure yet
if the redundancy is completely useless; it might help to catch bugs
with the qiov initialisation in devices.

In any case, if we want to remove bytes from the interface, I think
that's a change that should be done consistently across all functions in
a series of its own and not hidden in something that is only meant to
touch block jobs.

> > +int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
> > +                                unsigned int bytes, QEMUIOVector *qiov,
> > +                                BdrvRequestFlags flags)
> >  {
> 
> Ditto. When doing write_zeroes, do we want qiov == NULL, must we always
> have qiov but just leave qiov->iov[0].base as NULL?  Probably worth
> documenting as part of making it public.

Here it's currently not redundant because we use qiov == NULL in some
callers. Other callers do the .iov_base = NULL, .iov_len = bytes thing.
Perhaps worth cleaning up, but again, not in this series.

Kevin

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

[Qemu-devel] [PATCH v2 06/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/io.c            |  9 ---------
 block/stream.c        | 15 +++++++++------
 include/block/block.h |  2 --
 trace-events          |  1 -
 4 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/block/io.c b/block/io.c
index 9bc1d45..bc2eda2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1117,15 +1117,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..c0efbda 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,8 @@ 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_preadv(blk, sector_num * BDRV_SECTOR_SIZE, qiov.size, &qiov,
+                         BDRV_REQ_COPY_ON_READ);
 }
 
 typedef struct {
@@ -64,6 +65,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 +77,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 +90,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 +162,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 e899f7d..0a4b973 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -242,8 +242,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/trace-events b/trace-events
index 5d431bd..1bd595b 100644
--- a/trace-events
+++ b/trace-events
@@ -72,7 +72,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 v2 06/14] stream: Use BlockBackend for I/O

[Eric Blake]

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

On 05/24/2016 07:47 AM, 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/io.c            |  9 ---------
>  block/stream.c        | 15 +++++++++------
>  include/block/block.h |  2 --
>  trace-events          |  1 -
>  4 files changed, 9 insertions(+), 18 deletions(-)
> 

> +++ b/trace-events
> @@ -72,7 +72,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"

Yay - one less sector-based trace point - we're slowly getting to an
all-bytes destination.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Alberto Garcia]

On Tue 24 May 2016 03:47:26 PM CEST, Kevin Wolf <kwolf@redhat.com> 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>

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

Berto

[Qemu-devel] [PATCH v2 07/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>
Reviewed-by: Max Reitz <mreitz@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 af0b022..3c06746 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3459,10 +3459,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

[Qemu-devel] [PATCH v2 08/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 | 74 ++++++++++++++++++++++++++++++++--------------------------
 blockdev.c     |  4 +---
 2 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 23aa10e..d596660 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,7 +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,
+    blk_aio_pwritev(s->target, op->sector_num * BDRV_SECTOR_SIZE, &op->qiov,
+                    op->nb_sectors * BDRV_SECTOR_SIZE,
                     mirror_write_complete, op);
 }
 
@@ -185,7 +186,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 +224,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,7 +274,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,
+    blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov,
+                   nb_sectors * BDRV_SECTOR_SIZE,
                    mirror_read_complete, op);
     return ret;
 }
@@ -295,18 +297,19 @@ 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 * BDRV_SECTOR_SIZE,
+                             op->nb_sectors * BDRV_SECTOR_SIZE,
+                             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 +386,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 +451,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 +464,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 +492,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 +507,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 +544,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 +570,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 +640,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 +718,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 +745,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 +829,12 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    s->target = blk_new();
+    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 +844,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 +922,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 3c06746..66477ff 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3621,9 +3621,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);
@@ -3667,7 +3667,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,
@@ -3681,7 +3680,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 v2 08/14] mirror: Use BlockBackend for I/O

[Eric Blake]

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

On 05/24/2016 07:47 AM, 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 | 74 ++++++++++++++++++++++++++++++++--------------------------
>  blockdev.c     |  4 +---
>  2 files changed, 42 insertions(+), 36 deletions(-)
> 

> @@ -295,18 +297,19 @@ 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);

Eventually, discard should be converted to bytes. (Did I just get
volunteered?)

>      } 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 * BDRV_SECTOR_SIZE,
> +                             op->nb_sectors * BDRV_SECTOR_SIZE,
> +                             s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> +                             mirror_write_complete, op);

Conflicts with the write_zeroes stuff I just posted, let me know if I
need to rebase mine.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Changlong Xie]

On 05/24/2016 09:47 PM, Kevin Wolf wrote:
> +    s->target = blk_new();
blk_new(errp);

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

[Eric Blake]

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

On 05/24/2016 09:51 PM, Changlong Xie wrote:
> On 05/24/2016 09:47 PM, Kevin Wolf wrote:
>> +    s->target = blk_new();
> blk_new(errp);

Depends on Kevin's block/next branch, which currently includes:

commit 5d7dd50566a4f9786b95f49448f48fead0bb34d8
Author: Max Reitz <mreitz@redhat.com>
Date:   Tue May 17 16:41:34 2016 +0200

    block: Drop errp parameter from blk_new()

    blk_new() cannot fail so its Error ** parameter has become superfluous.

    Signed-off-by: Max Reitz <mreitz@redhat.com>
    Signed-off-by: Kevin Wolf <kwolf@redhat.com>

So the patch builds as written when applied to the correct branch.
http://repo.or.cz/qemu/kevin.git

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Changlong Xie]

On 05/25/2016 12:01 PM, Eric Blake wrote:
> On 05/24/2016 09:51 PM, Changlong Xie wrote:
>> On 05/24/2016 09:47 PM, Kevin Wolf wrote:
>>> +    s->target = blk_new();
>> blk_new(errp);
>
> Depends on Kevin's block/next branch, which currently includes:
>
> commit 5d7dd50566a4f9786b95f49448f48fead0bb34d8
> Author: Max Reitz <mreitz@redhat.com>
> Date:   Tue May 17 16:41:34 2016 +0200
>
>      block: Drop errp parameter from blk_new()
>
>      blk_new() cannot fail so its Error ** parameter has become superfluous.
>
>      Signed-off-by: Max Reitz <mreitz@redhat.com>
>      Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>
> So the patch builds as written when applied to the correct branch.
> http://repo.or.cz/qemu/kevin.git
>

Thanks for you reminder.

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

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.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

[Qemu-devel] [PATCH v2 10/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>
Reviewed-by: Max Reitz <mreitz@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

[Qemu-devel] [PATCH v2 11/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>
Reviewed-by: Max Reitz <mreitz@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 v2 11/14] backup: Remove bs parameter from backup_do_cow()

[Alberto Garcia]

On Tue 24 May 2016 03:47:31 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> Now that we pass the job to the function, bs is implied by that.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

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

Berto

[Qemu-devel] [PATCH v2 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        | 46 +++++++++++++++++++++-------------------------
 block/io.c            |  9 ---------
 blockdev.c            |  4 +---
 include/block/block.h |  2 --
 trace-events          |  1 -
 5 files changed, 22 insertions(+), 40 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 670ba50..be18f91 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,20 +132,15 @@ 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);
-        } else {
-            ret = bdrv_co_readv(bs, start * sectors_per_cluster, n,
-                                &bounce_qiov);
-        }
+        ret = blk_co_preadv(blk, start * job->cluster_size,
+                            bounce_qiov.size, &bounce_qiov,
+                            is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
         if (ret < 0) {
             trace_backup_do_cow_read_fail(job, start, ret);
             if (error_is_read) {
@@ -155,13 +150,11 @@ 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,
-                                       start * sectors_per_cluster,
-                                       n, BDRV_REQ_MAY_UNMAP);
+            ret = blk_co_write_zeroes(job->target, start * job->cluster_size,
+                                      n * BDRV_SECTOR_SIZE, BDRV_REQ_MAY_UNMAP);
         } else {
-            ret = bdrv_co_writev(job->target,
-                                 start * sectors_per_cluster, n,
-                                 &bounce_qiov);
+            ret = blk_co_pwritev(job->target, start * job->cluster_size,
+                                 bounce_qiov.size, &bounce_qiov, 0);
         }
         if (ret < 0) {
             trace_backup_do_cow_write_fail(job, start, ret);
@@ -203,7 +196,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 +217,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 +275,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 +371,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 +461,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 +541,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         goto error;
     }
 
+    job->target = blk_new();
+    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 +553,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 +580,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/io.c b/block/io.c
index bc2eda2..2d832aa 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1108,15 +1108,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 66477ff..717785e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3293,8 +3293,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;
     }
@@ -3378,12 +3378,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 0a4b973..70ea299 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -242,8 +242,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/trace-events b/trace-events
index 1bd595b..a7aca8d 100644
--- a/trace-events
+++ b/trace-events
@@ -72,7 +72,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 v2 12/14] backup: Use BlockBackend for I/O

[Eric Blake]

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

On 05/24/2016 07:47 AM, 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        | 46 +++++++++++++++++++++-------------------------
>  block/io.c            |  9 ---------
>  blockdev.c            |  4 +---
>  include/block/block.h |  2 --
>  trace-events          |  1 -
>  5 files changed, 22 insertions(+), 40 deletions(-)
> 

>  
> -        if (is_write_notifier) {
> -            ret = bdrv_co_readv_no_serialising(bs,
> -                                           start * sectors_per_cluster,
> -                                           n, &bounce_qiov);
> -        } else {
> -            ret = bdrv_co_readv(bs, start * sectors_per_cluster, n,
> -                                &bounce_qiov);
> -        }
> +        ret = blk_co_preadv(blk, start * job->cluster_size,
> +                            bounce_qiov.size, &bounce_qiov,
> +                            is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);

See my question earlier in the series about whether we want a redundant
size parameter in blk_co_preadv() now that it is public.

>          if (ret < 0) {
>              trace_backup_do_cow_read_fail(job, start, ret);
>              if (error_is_read) {
> @@ -155,13 +150,11 @@ 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,
> -                                       start * sectors_per_cluster,
> -                                       n, BDRV_REQ_MAY_UNMAP);
> +            ret = blk_co_write_zeroes(job->target, start * job->cluster_size,
> +                                      n * BDRV_SECTOR_SIZE, BDRV_REQ_MAY_UNMAP);

More rebasing fun.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Changlong Xie]

On 05/24/2016 09:47 PM, Kevin Wolf wrote:
> +    job->target = blk_new();
blk_new(errp);

[Qemu-devel] [PATCH v2 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 | 53 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index f308c8c..8a00e11 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -36,28 +36,36 @@ 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 = 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_preadv(bs, sector_num * BDRV_SECTOR_SIZE,
+                        qiov.size, &qiov, 0);
+    if (ret < 0) {
         return ret;
     }
 
-    ret = bdrv_write(base, sector_num, buf, nb_sectors);
-    if (ret) {
+    ret = blk_co_pwritev(base, sector_num * BDRV_SECTOR_SIZE,
+                         qiov.size, &qiov, 0);
+    if (ret < 0) {
         return ret;
     }
 
@@ -73,8 +81,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 +102,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 +112,6 @@ static void coroutine_fn commit_run(void *opaque)
 {
     CommitBlockJob *s = opaque;
     CommitCompleteData *data;
-    BlockDriverState *top = s->top;
-    BlockDriverState *base = s->base;
     int64_t sector_num, end;
     int ret = 0;
     int n = 0;
@@ -111,27 +119,27 @@ static void coroutine_fn commit_run(void *opaque)
     int bytes_written = 0;
     int64_t base_len;
 
-    ret = s->common.len = bdrv_getlength(top);
+    ret = s->common.len = blk_getlength(s->top);
 
 
     if (s->common.len < 0) {
         goto out;
     }
 
-    ret = base_len = bdrv_getlength(base);
+    ret = base_len = blk_getlength(s->base);
     if (base_len < 0) {
         goto out;
     }
 
     if (base_len < s->common.len) {
-        ret = bdrv_truncate(base, s->common.len);
+        ret = blk_truncate(s->base, s->common.len);
         if (ret) {
             goto out;
         }
     }
 
     end = s->common.len >> BDRV_SECTOR_BITS;
-    buf = qemu_blockalign(top, COMMIT_BUFFER_SIZE);
+    buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
 
     for (sector_num = 0; sector_num < end; sector_num += n) {
         uint64_t delay_ns = 0;
@@ -146,7 +154,8 @@ wait:
             break;
         }
         /* Copy if allocated above the base */
-        ret = bdrv_is_allocated_above(top, base, sector_num,
+        ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base),
+                                      sector_num,
                                       COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
                                       &n);
         copy = (ret == 1);
@@ -158,7 +167,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 +262,12 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
-    s->base   = base;
-    s->top    = top;
+    s->base = blk_new();
+    blk_insert_bs(s->base, base);
+
+    s->top = blk_new();
+    blk_insert_bs(s->top, top);
+
     s->active = bs;
 
     s->base_flags          = orig_base_flags;
-- 
1.8.3.1

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

[Eric Blake]

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

On 05/24/2016 07:47 AM, 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 | 53 +++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 20 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Changlong Xie]

On 05/24/2016 09:47 PM, Kevin Wolf wrote:
> +    s->base = blk_new();
blk_new(errp);
> +    blk_insert_bs(s->base, base);
> +
> +    s->top = blk_new();
blk_new(errp);

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

[Eric Blake]

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

On 05/24/2016 09:51 PM, Changlong Xie wrote:
> On 05/24/2016 09:47 PM, Kevin Wolf wrote:
>> +    s->base = blk_new();
> blk_new(errp);
>> +    blk_insert_bs(s->base, base);
>> +
>> +    s->top = blk_new();
> blk_new(errp);

Wrong. Even if it weren't for basing it on top of Kevin's branch which
removes the Error parameter, you can't safely pass errp to two functions
in a row (if both functions fail, the second will cause an assertion
error for trying to set an already-set error); the correct usage when
calling multiple functions that can set errors requires the use of a
local Error *err = NULL; up front, then error_propagate(errp, err) after
each place where it can be set.

-- 
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 v2 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>
Reviewed-by: Max Reitz <mreitz@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 2097e1d..c095cc5 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 7ed8ef2..dd2ba0a 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 v2 14/14] blockjob: Remove BlockJob.bs

[Changlong Xie]

On 05/24/2016 09:47 PM, 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>
> Reviewed-by: Max Reitz <mreitz@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 2097e1d..c095cc5 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 7ed8ef2..dd2ba0a 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);
>

Compiled with your block-jobs-bb branch:

changlox ~/w/qemu/qemu% make check-unit -j8 
 
                                     [ 1 ]  :-(
   CC    tests/test-blockjob-txn.o
GTESTER tests/test-x86-cpuid
GTESTER tests/test-cutils
GTESTER tests/test-int128
GTESTER tests/check-qdict
GTESTER tests/check-qfloat
GTESTER tests/check-qint
GTESTER tests/check-qstring
GTESTER tests/check-qlist
GTESTER tests/check-qjson
GTESTER tests/check-qnull
GTESTER tests/test-qmp-output-visitor
GTESTER tests/test-qmp-input-strict
GTESTER tests/test-qmp-input-visitor
GTESTER tests/test-qmp-commands
GTESTER tests/test-string-input-visitor
GTESTER tests/test-string-output-visitor
GTESTER tests/test-qmp-event
GTESTER tests/test-opts-visitor
GTESTER tests/test-coroutine
GTESTER tests/test-visitor-serialization
GTESTER tests/test-iov
GTESTER tests/test-aio
GTESTER tests/test-rfifolock
tests/test-blockjob-txn.c: In function ‘test_block_job_complete’:
tests/test-blockjob-txn.c:33:31: error: ‘BlockJob’ has no member named ‘bs’
      BlockDriverState *bs = job->bs;
                                ^
GTESTER tests/test-throttle
GTESTER tests/test-thread-pool
make: *** [tests/test-blockjob-txn.o] Error 1
make: *** Waiting for unfinished jobs....

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

[Kevin Wolf]

Am 25.05.2016 um 08:29 hat Changlong Xie geschrieben:
> On 05/24/2016 09:47 PM, 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>
> >Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> Compiled with your block-jobs-bb branch:
> 
> changlox ~/w/qemu/qemu% make check-unit -j8
> [...]
> tests/test-blockjob-txn.c: In function ‘test_block_job_complete’:
> tests/test-blockjob-txn.c:33:31: error: ‘BlockJob’ has no member named ‘bs’
>      BlockDriverState *bs = job->bs;

Thanks, will fix.

Kevin

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

[Alberto Garcia]

On Tue 24 May 2016 03:47:34 PM CEST, Kevin Wolf <kwolf@redhat.com> 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

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

Berto