[Qemu-devel] [PATCH 0/2] Minor fixes for the mirror block job

[Qemu-devel] [PATCH 0/2] Minor fixes for the mirror block job

[Alberto Garcia]

Hi,

a couple of minor fixes for the mirror block job. I don't think these
bugs can be reproduced at the moment so this shouldn't be 3.1
material.

Berto

Alberto Garcia (2):
  mirror: Release the dirty bitmap if mirror_start_job() fails
  mirror: Block the source BlockDriverState in mirror_start_job()

 block/mirror.c             | 11 +++++++++++
 tests/qemu-iotests/141.out |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/2] mirror: Release the dirty bitmap if mirror_start_job() fails

[Alberto Garcia]

At the moment I don't see how to make this function fail after the
dirty bitmap has been created, but if that was possible then we would
hit the assert(QLIST_EMPTY(&bs->dirty_bitmaps)) in bdrv_close().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/mirror.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 56d9ef7474..664c452f71 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1649,6 +1649,9 @@ fail:
         g_free(s->replaces);
         blk_unref(s->target);
         bs_opaque->job = NULL;
+        if (s->dirty_bitmap) {
+            bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
+        }
         job_early_fail(&s->common.job);
     }
 
-- 
2.11.0

[Qemu-devel] [PATCH 2/2] mirror: Block the source BlockDriverState in mirror_start_job()

[Alberto Garcia]

The mirror_start_job() function used for the commit-active job blocks
the source, target and all intermediate nodes for the duration of the
job.

   target <- intermediate <- source

Since 4ef85a9c2339 this function creates a dummy mirror_top_bs that
goes on top of the source node, and it is this dummy node that gets
blocked instead. The source node is never blocked or added to the
job's list of nodes.

   target <- intermediate <- source <- mirror_top

At the moment I don't think it is possible to exploit this problem
because any additional job on 'source' would either be forbidden for
other reasons or it would need to involve an additional node that is
blocked, causing an error.

This can be seen in the error messages, however, because they never
refer to the source node being blocked:

  $ qemu-img create -f qcow2 hd0.qcow2 1M
  $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2
  $ qemu-io -c 'write 0 1M' hd0.qcow2
  $ $QEMU -drive if=none,file=hd1.qcow2,node-name=hd1
  { "execute": "qmp_capabilities" }
  { "execute": "block-commit", "arguments": {"device": "hd1", "speed": 256}}
  { "execute": "block-stream", "arguments": {"device": "hd1"}}
  { "error": {"class": "GenericError",
    "desc": "Node 'hd0' is busy: block device is in use by block job: commit"}}

After this patch the error message refers to 'hd1', as it should.

The expected output of iotest 141 also needs to be updated for the
same reason.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/mirror.c             | 8 ++++++++
 tests/qemu-iotests/141.out | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 664c452f71..467eec9b89 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1612,6 +1612,14 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
         goto fail;
     }
 
+    ret = block_job_add_bdrv(&s->common, "source", bs, 0,
+                             BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE |
+                             BLK_PERM_CONSISTENT_READ,
+                             errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
     /* Required permissions are already taken with blk_new() */
     block_job_add_bdrv(&s->common, "target", target, 0, BLK_PERM_ALL,
                        &error_abort);
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index f252c86875..41c7291258 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -28,7 +28,7 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: node is used as backing hd of 'NODE_NAME'"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: mirror"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "job0"}}
@@ -45,7 +45,7 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: node is used as backing hd of 'NODE_NAME'"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "job0"}}
-- 
2.11.0

Re: [Qemu-devel] [PATCH 0/2] Minor fixes for the mirror block job

[Alberto Garcia]

ping

On Thu 22 Nov 2018 04:00:25 PM CET, Alberto Garcia wrote:
> Hi,
>
> a couple of minor fixes for the mirror block job. I don't think these
> bugs can be reproduced at the moment so this shouldn't be 3.1
> material.
>
> Berto
>
> Alberto Garcia (2):
>   mirror: Release the dirty bitmap if mirror_start_job() fails
>   mirror: Block the source BlockDriverState in mirror_start_job()
>
>  block/mirror.c             | 11 +++++++++++
>  tests/qemu-iotests/141.out |  4 ++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> -- 
> 2.11.0

Re: [Qemu-devel] [PATCH 0/2] Minor fixes for the mirror block job

[Alberto Garcia]

ping 2

On Thu 22 Nov 2018 04:00:25 PM CET, Alberto Garcia wrote:
> Hi,
>
> a couple of minor fixes for the mirror block job. I don't think these
> bugs can be reproduced at the moment so this shouldn't be 3.1
> material.
>
> Berto
>
> Alberto Garcia (2):
>   mirror: Release the dirty bitmap if mirror_start_job() fails
>   mirror: Block the source BlockDriverState in mirror_start_job()
>
>  block/mirror.c             | 11 +++++++++++
>  tests/qemu-iotests/141.out |  4 ++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> -- 
> 2.11.0

Re: [Qemu-devel] [PATCH 0/2] Minor fixes for the mirror block job

[Kevin Wolf]

Am 22.11.2018 um 16:00 hat Alberto Garcia geschrieben:
> Hi,
> 
> a couple of minor fixes for the mirror block job. I don't think these
> bugs can be reproduced at the moment so this shouldn't be 3.1
> material.

Thanks, applied to the block branch.

Kevin