[Qemu-devel] [PATCH 0/4] Misc block job patches

[Qemu-devel] [PATCH 0/4] Misc block job patches

[Alberto Garcia]

Hi,

here's a few patches from my block-stream series that I think are
ready to be merged now.

I don't think there's anything controversial here:

  - Patches 1 and 2 simply use block_job_next() in places where
    bdrv_next() was being used to look for block jobs.

  - Patch 3 is a fix that was already proposed in the list.

  - Patch 4 is quite simple and it deals with the failure of
    block_job_create() in commit_start().

Regards,

Berto

Alberto Garcia (4):
  block: use the block job list in bdrv_drain_all()
  block: use the block job list in qmp_query_block_jobs()
  block: Prevent sleeping jobs from resuming if they have been paused
  block: Create the commit block job before reopening any image

 block/commit.c | 11 ++++++-----
 block/io.c     | 24 ++++++++++++++++++------
 blockdev.c     | 20 ++++++++------------
 blockjob.c     |  6 ++++--
 4 files changed, 36 insertions(+), 25 deletions(-)

-- 
2.8.1

[Qemu-devel] [PATCH 1/4] block: use the block job list in bdrv_drain_all()

[Alberto Garcia]

bdrv_drain_all() pauses all block jobs by using bdrv_next() to iterate
over all top-level BlockDriverStates. Therefore the code is unable to
find block jobs in other nodes.

This patch uses block_job_next() to iterate over all block jobs.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/io.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/block/io.c b/block/io.c
index 2d832aa..c848b18 100644
--- a/block/io.c
+++ b/block/io.c
@@ -289,15 +289,21 @@ void bdrv_drain_all(void)
     bool busy = true;
     BlockDriverState *bs;
     BdrvNextIterator it;
+    BlockJob *job = NULL;
     GSList *aio_ctxs = NULL, *ctx;
 
+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = blk_get_aio_context(job->blk);
+
+        aio_context_acquire(aio_context);
+        block_job_pause(job);
+        aio_context_release(aio_context);
+    }
+
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
         aio_context_acquire(aio_context);
-        if (bs->job) {
-            block_job_pause(bs->job);
-        }
         bdrv_parent_drained_begin(bs);
         bdrv_io_unplugged_begin(bs);
         bdrv_drain_recurse(bs);
@@ -340,12 +346,18 @@ void bdrv_drain_all(void)
         aio_context_acquire(aio_context);
         bdrv_io_unplugged_end(bs);
         bdrv_parent_drained_end(bs);
-        if (bs->job) {
-            block_job_resume(bs->job);
-        }
         aio_context_release(aio_context);
     }
     g_slist_free(aio_ctxs);
+
+    job = NULL;
+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = blk_get_aio_context(job->blk);
+
+        aio_context_acquire(aio_context);
+        block_job_resume(job);
+        aio_context_release(aio_context);
+    }
 }
 
 /**
-- 
2.8.1

Re: [Qemu-devel] [PATCH 1/4] block: use the block job list in bdrv_drain_all()

[Max Reitz]

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

On 27.05.2016 12:53, Alberto Garcia wrote:
> bdrv_drain_all() pauses all block jobs by using bdrv_next() to iterate
> over all top-level BlockDriverStates. Therefore the code is unable to
> find block jobs in other nodes.
> 
> This patch uses block_job_next() to iterate over all block jobs.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/io.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)

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


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

[Qemu-devel] [PATCH 2/4] block: use the block job list in qmp_query_block_jobs()

[Alberto Garcia]

qmp_query_block_jobs() uses bdrv_next() to look for block jobs, but
this function can only find those in top-level BlockDriverStates.

This patch uses block_job_next() instead.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 717785e..ca7fa82 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4141,22 +4141,18 @@ void qmp_x_blockdev_change(const char *parent, bool has_child,
 BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 {
     BlockJobInfoList *head = NULL, **p_next = &head;
-    BlockDriverState *bs;
-    BdrvNextIterator it;
+    BlockJob *job;
 
-    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
+    for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+        BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
+        AioContext *aio_context = blk_get_aio_context(job->blk);
 
         aio_context_acquire(aio_context);
-
-        if (bs->job) {
-            BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
-            elem->value = block_job_query(bs->job);
-            *p_next = elem;
-            p_next = &elem->next;
-        }
-
+        elem->value = block_job_query(job);
         aio_context_release(aio_context);
+
+        *p_next = elem;
+        p_next = &elem->next;
     }
 
     return head;
-- 
2.8.1

Re: [Qemu-devel] [PATCH 2/4] block: use the block job list in qmp_query_block_jobs()

[Max Reitz]

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

On 27.05.2016 12:53, Alberto Garcia wrote:
> qmp_query_block_jobs() uses bdrv_next() to look for block jobs, but
> this function can only find those in top-level BlockDriverStates.
> 
> This patch uses block_job_next() instead.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)

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


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

[Qemu-devel] [PATCH 3/4] block: Prevent sleeping jobs from resuming if they have been paused

[Alberto Garcia]

If we pause a block job and drain its BlockDriverState we want that
the job remains inactive until we call block_job_resume() again.

However if we pause the job while it is sleeping then it will resume
when the sleep timer fires.

This patch prevents that from happening by checking if the job has
been paused after it comes back from sleeping.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Suggested-by: Kevin Wolf <kwolf@redhat.com>
---
 blockjob.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index c095cc5..01b896b 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -361,10 +361,12 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
     }
 
     job->busy = false;
+    if (!block_job_is_paused(job)) {
+        co_aio_sleep_ns(blk_get_aio_context(job->blk), type, ns);
+    }
+    /* The job can be paused while sleeping, so check this again */
     if (block_job_is_paused(job)) {
         qemu_coroutine_yield();
-    } else {
-        co_aio_sleep_ns(blk_get_aio_context(job->blk), type, ns);
     }
     job->busy = true;
 }
-- 
2.8.1

Re: [Qemu-devel] [PATCH 3/4] block: Prevent sleeping jobs from resuming if they have been paused

[Max Reitz]

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

On 27.05.2016 12:53, Alberto Garcia wrote:
> If we pause a block job and drain its BlockDriverState we want that
> the job remains inactive until we call block_job_resume() again.
> 
> However if we pause the job while it is sleeping then it will resume
> when the sleep timer fires.
> 
> This patch prevents that from happening by checking if the job has
> been paused after it comes back from sleeping.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockjob.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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


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

[Qemu-devel] [PATCH 4/4] block: Create the commit block job before reopening any image

[Alberto Garcia]

If the base or overlay images need to be reopened in read-write mode
but the block_job_create() call fails then no one will put those
images back in read-only mode.

We can solve this problem easily by calling block_job_create() first.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/commit.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 8a00e11..444333b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -236,6 +236,11 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
+    s = block_job_create(&commit_job_driver, bs, speed, cb, opaque, errp);
+    if (!s) {
+        return;
+    }
+
     orig_base_flags    = bdrv_get_flags(base);
     orig_overlay_flags = bdrv_get_flags(overlay_bs);
 
@@ -252,16 +257,12 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
         bdrv_reopen_multiple(reopen_queue, &local_err);
         if (local_err != NULL) {
             error_propagate(errp, local_err);
+            block_job_unref(&s->common);
             return;
         }
     }
 
 
-    s = block_job_create(&commit_job_driver, bs, speed, cb, opaque, errp);
-    if (!s) {
-        return;
-    }
-
     s->base = blk_new();
     blk_insert_bs(s->base, base);
 
-- 
2.8.1

Re: [Qemu-devel] [PATCH 4/4] block: Create the commit block job before reopening any image

[Max Reitz]

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

On 27.05.2016 12:53, Alberto Garcia wrote:
> If the base or overlay images need to be reopened in read-write mode
> but the block_job_create() call fails then no one will put those
> images back in read-only mode.
> 
> We can solve this problem easily by calling block_job_create() first.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/commit.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

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


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

Re: [Qemu-devel] [PATCH 0/4] Misc block job patches

[Max Reitz]

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

On 27.05.2016 12:53, Alberto Garcia wrote:
> Hi,
> 
> here's a few patches from my block-stream series that I think are
> ready to be merged now.
> 
> I don't think there's anything controversial here:
> 
>   - Patches 1 and 2 simply use block_job_next() in places where
>     bdrv_next() was being used to look for block jobs.
> 
>   - Patch 3 is a fix that was already proposed in the list.
> 
>   - Patch 4 is quite simple and it deals with the failure of
>     block_job_create() in commit_start().
> 
> Regards,
> 
> Berto
> 
> Alberto Garcia (4):
>   block: use the block job list in bdrv_drain_all()
>   block: use the block job list in qmp_query_block_jobs()
>   block: Prevent sleeping jobs from resuming if they have been paused
>   block: Create the commit block job before reopening any image
> 
>  block/commit.c | 11 ++++++-----
>  block/io.c     | 24 ++++++++++++++++++------
>  blockdev.c     | 20 ++++++++------------
>  blockjob.c     |  6 ++++--
>  4 files changed, 36 insertions(+), 25 deletions(-)

Thanks Berto, I've applied the series to my block branch:

https://github.com/XanClic/qemu/commits/block

Max


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