diff mbox

Assertion failure taking external snapshot with virtio drive + iothread

Message ID f8f478b3-70a0-b67b-c4df-3f68ac66308a@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini March 17, 2017, 5:11 p.m. UTC 
On 17/03/2017 17:55, Ed Swierk wrote:
> I'm running into the same problem taking an external snapshot with a
> virtio-blk drive with iothread, so it's not specific to virtio-scsi.
> Run a Linux guest on qemu master
> 
>   qemu-system-x86_64 -nographic -enable-kvm -monitor
> telnet:0.0.0.0:1234,server,nowait -m 1024 -object
> iothread,id=iothread1 -drive file=/x/drive.qcow2,if=none,id=drive0
> -device virtio-blk-pci,iothread=iothread1,drive=drive0
> 
> Then in the monitor
> 
>   snapshot_blkdev drive0 /x/snap1.qcow2
> 
> qemu bombs with
> 
>   qemu-system-x86_64: /x/qemu/include/block/aio.h:457:
> aio_enable_external: Assertion `ctx->external_disable_cnt > 0' failed.
> 
> whereas without the iothread the assertion failure does not occur.

Please try this patch:


This is not a proper fix, the right one would move the calls into
virtio-blk and virtio-scsi, but it might be a start.  I think the
issue is that you have one call to aio_disable_external for drive.qcow2
before the snapshot, and two calls to aio_enable_external (one for
drive.qcow2 and one for snap1.qcow2) after.

Paolo

Comments

Paolo Bonzini March 17, 2017, 5:15 p.m. UTC | #1 
On 17/03/2017 18:11, Paolo Bonzini wrote:
> 
> 
> On 17/03/2017 17:55, Ed Swierk wrote:
>> I'm running into the same problem taking an external snapshot with a
>> virtio-blk drive with iothread, so it's not specific to virtio-scsi.
>> Run a Linux guest on qemu master
>>
>>   qemu-system-x86_64 -nographic -enable-kvm -monitor
>> telnet:0.0.0.0:1234,server,nowait -m 1024 -object
>> iothread,id=iothread1 -drive file=/x/drive.qcow2,if=none,id=drive0
>> -device virtio-blk-pci,iothread=iothread1,drive=drive0
>>
>> Then in the monitor
>>
>>   snapshot_blkdev drive0 /x/snap1.qcow2
>>
>> qemu bombs with
>>
>>   qemu-system-x86_64: /x/qemu/include/block/aio.h:457:
>> aio_enable_external: Assertion `ctx->external_disable_cnt > 0' failed.
>>
>> whereas without the iothread the assertion failure does not occur.
> 
> Please try this patch:

Hmm, no.  I'll post the full fix on top of John Snow's patches.

Paolo

> diff --git a/block/block-backend.c b/block/block-backend.c
> index 5742c09..1d95879 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1876,6 +1876,7 @@ static void blk_root_drained_begin(BdrvChild *child)
>      if (blk->public.io_limits_disabled++ == 0) {
>          throttle_group_restart_blk(blk);
>      }
> +    aio_disable_external(bdrv_get_aio_context(bs));
>  }
>  
>  static void blk_root_drained_end(BdrvChild *child)
> @@ -1883,5 +1884,6 @@ static void blk_root_drained_end(BdrvChild *child)
>      BlockBackend *blk = child->opaque;
>  
>      assert(blk->public.io_limits_disabled);
> +    aio_enable_external(bdrv_get_aio_context(bs));
>      --blk->public.io_limits_disabled;
>  }
> diff --git a/block/io.c b/block/io.c
> index 2709a70..a6dcef5 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -224,7 +224,6 @@ void bdrv_drained_begin(BlockDriverState *bs)
>      }
>  
>      if (!bs->quiesce_counter++) {
> -        aio_disable_external(bdrv_get_aio_context(bs));
>          bdrv_parent_drained_begin(bs);
>      }
>  
> @@ -239,7 +238,6 @@ void bdrv_drained_end(BlockDriverState *bs)
>      }
>  
>      bdrv_parent_drained_end(bs);
> -    aio_enable_external(bdrv_get_aio_context(bs));
>  }
>  
>  /*
> 
> This is not a proper fix, the right one would move the calls into
> virtio-blk and virtio-scsi, but it might be a start.  I think the
> issue is that you have one call to aio_disable_external for drive.qcow2
> before the snapshot, and two calls to aio_enable_external (one for
> drive.qcow2 and one for snap1.qcow2) after.
> 
> Paolo
>
Ed Swierk March 17, 2017, 5:32 p.m. UTC | #2 
On Fri, Mar 17, 2017 at 10:15 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 17/03/2017 18:11, Paolo Bonzini wrote:
>>
>>
>> On 17/03/2017 17:55, Ed Swierk wrote:
>>> I'm running into the same problem taking an external snapshot with a
>>> virtio-blk drive with iothread, so it's not specific to virtio-scsi.
>>> Run a Linux guest on qemu master
>>>
>>>   qemu-system-x86_64 -nographic -enable-kvm -monitor
>>> telnet:0.0.0.0:1234,server,nowait -m 1024 -object
>>> iothread,id=iothread1 -drive file=/x/drive.qcow2,if=none,id=drive0
>>> -device virtio-blk-pci,iothread=iothread1,drive=drive0
>>>
>>> Then in the monitor
>>>
>>>   snapshot_blkdev drive0 /x/snap1.qcow2
>>>
>>> qemu bombs with
>>>
>>>   qemu-system-x86_64: /x/qemu/include/block/aio.h:457:
>>> aio_enable_external: Assertion `ctx->external_disable_cnt > 0' failed.
>>>
>>> whereas without the iothread the assertion failure does not occur.
>>
>> Please try this patch:
>
> Hmm, no.  I'll post the full fix on top of John Snow's patches.

OK. Incidentally, testing with virtio-blk I bisected the assertion
failure to b2c2832c6140cfe3ddc0de2d77eeb0b77dea8fd3 ("block: Add Error
parameter to bdrv_append()").

--Ed
Paolo Bonzini March 17, 2017, 6:10 p.m. UTC | #3 
On 17/03/2017 18:32, Ed Swierk wrote:
> On Fri, Mar 17, 2017 at 10:15 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 17/03/2017 18:11, Paolo Bonzini wrote:
>>>
>>>
>>> On 17/03/2017 17:55, Ed Swierk wrote:
>>>> I'm running into the same problem taking an external snapshot with a
>>>> virtio-blk drive with iothread, so it's not specific to virtio-scsi.
>>>> Run a Linux guest on qemu master
>>>>
>>>>   qemu-system-x86_64 -nographic -enable-kvm -monitor
>>>> telnet:0.0.0.0:1234,server,nowait -m 1024 -object
>>>> iothread,id=iothread1 -drive file=/x/drive.qcow2,if=none,id=drive0
>>>> -device virtio-blk-pci,iothread=iothread1,drive=drive0
>>>>
>>>> Then in the monitor
>>>>
>>>>   snapshot_blkdev drive0 /x/snap1.qcow2
>>>>
>>>> qemu bombs with
>>>>
>>>>   qemu-system-x86_64: /x/qemu/include/block/aio.h:457:
>>>> aio_enable_external: Assertion `ctx->external_disable_cnt > 0' failed.
>>>>
>>>> whereas without the iothread the assertion failure does not occur.
>>>
>>> Please try this patch:
>>
>> Hmm, no.  I'll post the full fix on top of John Snow's patches.
> 
> OK. Incidentally, testing with virtio-blk I bisected the assertion
> failure to b2c2832c6140cfe3ddc0de2d77eeb0b77dea8fd3 ("block: Add Error
> parameter to bdrv_append()").

And in particular to this:

+    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
+    }

     change_parent_backing_link(bs_top, bs_new);
-    /* FIXME Error handling */
-    bdrv_set_backing_hd(bs_new, bs_top, &error_abort);


Paolo
diff mbox

Patch

diff --git a/block/block-backend.c b/block/block-backend.c
index 5742c09..1d95879 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1876,6 +1876,7 @@  static void blk_root_drained_begin(BdrvChild *child)
     if (blk->public.io_limits_disabled++ == 0) {
         throttle_group_restart_blk(blk);
     }
+    aio_disable_external(bdrv_get_aio_context(bs));
 }
 
 static void blk_root_drained_end(BdrvChild *child)
@@ -1883,5 +1884,6 @@  static void blk_root_drained_end(BdrvChild *child)
     BlockBackend *blk = child->opaque;
 
     assert(blk->public.io_limits_disabled);
+    aio_enable_external(bdrv_get_aio_context(bs));
     --blk->public.io_limits_disabled;
 }
diff --git a/block/io.c b/block/io.c
index 2709a70..a6dcef5 100644
--- a/block/io.c
+++ b/block/io.c
@@ -224,7 +224,6 @@  void bdrv_drained_begin(BlockDriverState *bs)
     }
 
     if (!bs->quiesce_counter++) {
-        aio_disable_external(bdrv_get_aio_context(bs));
         bdrv_parent_drained_begin(bs);
     }
 
@@ -239,7 +238,6 @@  void bdrv_drained_end(BlockDriverState *bs)
     }
 
     bdrv_parent_drained_end(bs);
-    aio_enable_external(bdrv_get_aio_context(bs));
 }
 
 /*