[Qemu-devel] [PATCH 0/3] block: fix blk_aio_*() segfault when blk->root == NULL

[Qemu-devel] [PATCH 0/3] block: fix blk_aio_*() segfault when blk->root == NULL

[Stefan Hajnoczi]

Using bdrv_inc_in_flight(blk_bs(blk)) doesn't work since BlockBackend->root may
be NULL.

This patch series solves the issue by adding an BlockBackend->in_flight counter
so requests can be tracked even when there is no BlockDriverState.

This should fix the IDE and virtio-blk segfaults that have been encountered
when there is no BlockDriverState.

The patch is based on work by Kevin Wolf.

Kevin Wolf (1):
  block: test blk_aio_flush() with blk->root == NULL

Stefan Hajnoczi (2):
  block: add BlockBackend->in_flight counter
  Revert "IDE: Do not flush empty CDROM drives"

 tests/Makefile.include     |  2 ++
 block.c                    |  2 +-
 block/block-backend.c      | 59 +++++++++++++++++++++++++++++----
 hw/ide/core.c              | 10 +-----
 tests/test-block-backend.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 138 insertions(+), 17 deletions(-)
 create mode 100644 tests/test-block-backend.c

-- 
2.14.3

[Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter

[Stefan Hajnoczi]

BlockBackend currently relies on BlockDriverState->in_flight to track
requests for blk_drain().  There is a corner case where
BlockDriverState->in_flight cannot be used though: blk->root can be NULL
when there is no medium.  This results in a segfault when the NULL
pointer is dereferenced.

Introduce a BlockBackend->in_flight counter for aio requests so it works
even when blk->root == NULL.

Based on a patch by Kevin Wolf <kwolf@redhat.com>.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block.c               |  2 +-
 block/block-backend.c | 59 +++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index a8da4f2b25..140f7919f8 100644
--- a/block.c
+++ b/block.c
@@ -4716,7 +4716,7 @@ out:
 
 AioContext *bdrv_get_aio_context(BlockDriverState *bs)
 {
-    return bs->aio_context;
+    return bs ? bs->aio_context : qemu_get_aio_context();
 }
 
 void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co)
diff --git a/block/block-backend.c b/block/block-backend.c
index baef8e7abc..4811d8bc55 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -71,6 +71,13 @@ struct BlockBackend {
     int quiesce_counter;
     VMChangeStateEntry *vmsh;
     bool force_allow_inactivate;
+
+    /* Number of in-flight aio requests.  BlockDriverState also counts
+     * in-flight requests but aio requests can exist even when blk->root is
+     * NULL, so we cannot rely on its counter for that case.
+     * Accessed with atomic ops.
+     */
+    unsigned int in_flight;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -1223,11 +1230,21 @@ int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags)
     return bdrv_make_zero(blk->root, flags);
 }
 
+static void blk_inc_in_flight(BlockBackend *blk)
+{
+    atomic_inc(&blk->in_flight);
+}
+
+static void blk_dec_in_flight(BlockBackend *blk)
+{
+    atomic_dec(&blk->in_flight);
+}
+
 static void error_callback_bh(void *opaque)
 {
     struct BlockBackendAIOCB *acb = opaque;
 
-    bdrv_dec_in_flight(acb->common.bs);
+    blk_dec_in_flight(acb->blk);
     acb->common.cb(acb->common.opaque, acb->ret);
     qemu_aio_unref(acb);
 }
@@ -1238,7 +1255,7 @@ BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
 {
     struct BlockBackendAIOCB *acb;
 
-    bdrv_inc_in_flight(blk_bs(blk));
+    blk_inc_in_flight(blk);
     acb = blk_aio_get(&block_backend_aiocb_info, blk, cb, opaque);
     acb->blk = blk;
     acb->ret = ret;
@@ -1261,7 +1278,7 @@ static const AIOCBInfo blk_aio_em_aiocb_info = {
 static void blk_aio_complete(BlkAioEmAIOCB *acb)
 {
     if (acb->has_returned) {
-        bdrv_dec_in_flight(acb->common.bs);
+        blk_dec_in_flight(acb->rwco.blk);
         acb->common.cb(acb->common.opaque, acb->rwco.ret);
         qemu_aio_unref(acb);
     }
@@ -1282,7 +1299,7 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes,
     BlkAioEmAIOCB *acb;
     Coroutine *co;
 
-    bdrv_inc_in_flight(blk_bs(blk));
+    blk_inc_in_flight(blk);
     acb = blk_aio_get(&blk_aio_em_aiocb_info, blk, cb, opaque);
     acb->rwco = (BlkRwCo) {
         .blk    = blk,
@@ -1519,14 +1536,42 @@ int blk_flush(BlockBackend *blk)
 
 void blk_drain(BlockBackend *blk)
 {
-    if (blk_bs(blk)) {
-        bdrv_drain(blk_bs(blk));
+    BlockDriverState *bs = blk_bs(blk);
+
+    if (bs) {
+        bdrv_drained_begin(bs);
+    }
+
+    /* We may have aio requests like -ENOMEDIUM in flight */
+    while (atomic_mb_read(&blk->in_flight) > 0) {
+        aio_poll(blk_get_aio_context(blk), true);
+    }
+
+    if (bs) {
+        bdrv_drained_end(bs);
     }
 }
 
 void blk_drain_all(void)
 {
-    bdrv_drain_all();
+    BlockBackend *blk = NULL;
+
+    bdrv_drain_all_begin();
+
+    while ((blk = blk_all_next(blk)) != NULL) {
+        AioContext *ctx = blk_get_aio_context(blk);
+
+        aio_context_acquire(ctx);
+
+        /* We may have aio requests like -ENOMEDIUM in flight */
+        while (atomic_mb_read(&blk->in_flight) > 0) {
+            aio_poll(blk_get_aio_context(blk), true);
+        }
+
+        aio_context_release(ctx);
+    }
+
+    bdrv_drain_all_end();
 }
 
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter

[Eric Blake]

On 02/08/2018 11:18 AM, Stefan Hajnoczi wrote:
> BlockBackend currently relies on BlockDriverState->in_flight to track
> requests for blk_drain().  There is a corner case where
> BlockDriverState->in_flight cannot be used though: blk->root can be NULL
> when there is no medium.  This results in a segfault when the NULL
> pointer is dereferenced.
> 
> Introduce a BlockBackend->in_flight counter for aio requests so it works
> even when blk->root == NULL.
> 
> Based on a patch by Kevin Wolf <kwolf@redhat.com>.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block.c               |  2 +-
>   block/block-backend.c | 59 +++++++++++++++++++++++++++++++++++++++++++++------
>   2 files changed, 53 insertions(+), 8 deletions(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter

[Paolo Bonzini]

On 08/02/2018 18:18, Stefan Hajnoczi wrote:
> +    BlockDriverState *bs = blk_bs(blk);
> +
> +    if (bs) {
> +        bdrv_drained_begin(bs);
> +    }
> +
> +    /* We may have aio requests like -ENOMEDIUM in flight */
> +    while (atomic_mb_read(&blk->in_flight) > 0) {
> +        aio_poll(blk_get_aio_context(blk), true);
> +    }
> +
> +    if (bs) {
> +        bdrv_drained_end(bs);

This only works if you are in the default AioContext, otherwise you can
run handlers for one AioContexts in two threads.

bdrv_dec_in_flight uses bdrv_wakeup and BDRV_POLL_WHILE to ensure that
this doesn't happen, so there would be more code that you have to copy
into block-backend.c.

Paolo

Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter

[Kevin Wolf]

Am 09.02.2018 um 17:28 hat Paolo Bonzini geschrieben:
> On 08/02/2018 18:18, Stefan Hajnoczi wrote:
> > +    BlockDriverState *bs = blk_bs(blk);
> > +
> > +    if (bs) {
> > +        bdrv_drained_begin(bs);
> > +    }
> > +
> > +    /* We may have aio requests like -ENOMEDIUM in flight */
> > +    while (atomic_mb_read(&blk->in_flight) > 0) {
> > +        aio_poll(blk_get_aio_context(blk), true);
> > +    }
> > +
> > +    if (bs) {
> > +        bdrv_drained_end(bs);
> 
> This only works if you are in the default AioContext, otherwise you can
> run handlers for one AioContexts in two threads.

Should aio_poll() assert that it's called in the right thread to make
sure we obey the rules?

> bdrv_dec_in_flight uses bdrv_wakeup and BDRV_POLL_WHILE to ensure that
> this doesn't happen, so there would be more code that you have to copy
> into block-backend.c.

Instead of copying, can't we generalise it into a POLL_WHILE(ctx,
wakeup, cond) and make BDRV_POLL_WHILE() a wrapper for that?

Kevin

Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter

[Paolo Bonzini]

On 09/02/2018 18:23, Kevin Wolf wrote:
> Am 09.02.2018 um 17:28 hat Paolo Bonzini geschrieben:
>> On 08/02/2018 18:18, Stefan Hajnoczi wrote:
>>> +    BlockDriverState *bs = blk_bs(blk);
>>> +
>>> +    if (bs) {
>>> +        bdrv_drained_begin(bs);
>>> +    }
>>> +
>>> +    /* We may have aio requests like -ENOMEDIUM in flight */
>>> +    while (atomic_mb_read(&blk->in_flight) > 0) {
>>> +        aio_poll(blk_get_aio_context(blk), true);
>>> +    }
>>> +
>>> +    if (bs) {
>>> +        bdrv_drained_end(bs);
>>
>> This only works if you are in the default AioContext, otherwise you can
>> run handlers for one AioContexts in two threads.
> 
> Should aio_poll() assert that it's called in the right thread to make
> sure we obey the rules?
> 
>> bdrv_dec_in_flight uses bdrv_wakeup and BDRV_POLL_WHILE to ensure that
>> this doesn't happen, so there would be more code that you have to copy
>> into block-backend.c.
> 
> Instead of copying, can't we generalise it into a POLL_WHILE(ctx,
> wakeup, cond) and make BDRV_POLL_WHILE() a wrapper for that?

Yes, or even move bdrv_wakeup to AioContext would do.  We already have
block layer-specific fields such as the linux-aio state(*).

	(*) though now that linux-aio has been improved to do something
	    like epoll, there may be a better reason to place linux-aio
	    state in AioContext.

Paolo

Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter

[Stefan Hajnoczi]

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

On Fri, Feb 09, 2018 at 06:26:41PM +0100, Paolo Bonzini wrote:
> On 09/02/2018 18:23, Kevin Wolf wrote:
> > Am 09.02.2018 um 17:28 hat Paolo Bonzini geschrieben:
> >> On 08/02/2018 18:18, Stefan Hajnoczi wrote:
> >>> +    BlockDriverState *bs = blk_bs(blk);
> >>> +
> >>> +    if (bs) {
> >>> +        bdrv_drained_begin(bs);
> >>> +    }
> >>> +
> >>> +    /* We may have aio requests like -ENOMEDIUM in flight */
> >>> +    while (atomic_mb_read(&blk->in_flight) > 0) {
> >>> +        aio_poll(blk_get_aio_context(blk), true);
> >>> +    }
> >>> +
> >>> +    if (bs) {
> >>> +        bdrv_drained_end(bs);
> >>
> >> This only works if you are in the default AioContext, otherwise you can
> >> run handlers for one AioContexts in two threads.
> > 
> > Should aio_poll() assert that it's called in the right thread to make
> > sure we obey the rules?
> > 
> >> bdrv_dec_in_flight uses bdrv_wakeup and BDRV_POLL_WHILE to ensure that
> >> this doesn't happen, so there would be more code that you have to copy
> >> into block-backend.c.
> > 
> > Instead of copying, can't we generalise it into a POLL_WHILE(ctx,
> > wakeup, cond) and make BDRV_POLL_WHILE() a wrapper for that?
> 
> Yes, or even move bdrv_wakeup to AioContext would do.  We already have
> block layer-specific fields such as the linux-aio state(*).
> 
> 	(*) though now that linux-aio has been improved to do something
> 	    like epoll, there may be a better reason to place linux-aio
> 	    state in AioContext.

Thanks for the ideas, will fix in v2.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 2/3] block: test blk_aio_flush() with blk->root == NULL

[Stefan Hajnoczi]

From: Kevin Wolf <kwolf@redhat.com>

This patch adds test cases for the scenario where blk_aio_flush() is
called on a BlockBackend with no root.  Calling drain afterwards should
complete the requests with -ENOMEDIUM.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/Makefile.include     |  2 ++
 tests/test-block-backend.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)
 create mode 100644 tests/test-block-backend.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index f41da235ae..1a457c29bc 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -83,6 +83,7 @@ gcov-files-test-hbitmap-y = blockjob.c
 check-unit-y += tests/test-bdrv-drain$(EXESUF)
 check-unit-y += tests/test-blockjob$(EXESUF)
 check-unit-y += tests/test-blockjob-txn$(EXESUF)
+check-unit-y += tests/test-block-backend$(EXESUF)
 check-unit-y += tests/test-x86-cpuid$(EXESUF)
 # all code tested by test-x86-cpuid is inside topology.h
 gcov-files-test-x86-cpuid-y =
@@ -609,6 +610,7 @@ tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-obj-y)
 tests/test-bdrv-drain$(EXESUF): tests/test-bdrv-drain.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
+tests/test-block-backend$(EXESUF): tests/test-block-backend.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
 tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) $(test-crypto-obj-y)
diff --git a/tests/test-block-backend.c b/tests/test-block-backend.c
new file mode 100644
index 0000000000..fd59f02bd0
--- /dev/null
+++ b/tests/test-block-backend.c
@@ -0,0 +1,82 @@
+/*
+ * BlockBackend tests
+ *
+ * Copyright (c) 2017 Kevin Wolf <kwolf@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block.h"
+#include "sysemu/block-backend.h"
+#include "qapi/error.h"
+
+static void test_drain_aio_error_flush_cb(void *opaque, int ret)
+{
+    bool *completed = opaque;
+
+    g_assert(ret == -ENOMEDIUM);
+    *completed = true;
+}
+
+static void test_drain_aio_error(void)
+{
+    BlockBackend *blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+    BlockAIOCB *acb;
+    bool completed = false;
+
+    acb = blk_aio_flush(blk, test_drain_aio_error_flush_cb, &completed);
+    g_assert(acb != NULL);
+    g_assert(completed == false);
+
+    blk_drain(blk);
+    g_assert(completed == true);
+
+    blk_unref(blk);
+}
+
+static void test_drain_all_aio_error(void)
+{
+    BlockBackend *blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+    BlockAIOCB *acb;
+    bool completed = false;
+
+    acb = blk_aio_flush(blk, test_drain_aio_error_flush_cb, &completed);
+    g_assert(acb != NULL);
+    g_assert(completed == false);
+
+    blk_drain_all();
+    g_assert(completed == true);
+
+    blk_unref(blk);
+}
+
+int main(int argc, char **argv)
+{
+    bdrv_init();
+    qemu_init_main_loop(&error_abort);
+
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/block-backend/drain_aio_error", test_drain_aio_error);
+    g_test_add_func("/block-backend/drain_all_aio_error",
+                    test_drain_all_aio_error);
+
+    return g_test_run();
+}
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/3] block: test blk_aio_flush() with blk->root == NULL

[Eric Blake]

On 02/08/2018 11:18 AM, Stefan Hajnoczi wrote:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> This patch adds test cases for the scenario where blk_aio_flush() is
> called on a BlockBackend with no root.  Calling drain afterwards should
> complete the requests with -ENOMEDIUM.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   tests/Makefile.include     |  2 ++
>   tests/test-block-backend.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 84 insertions(+)
>   create mode 100644 tests/test-block-backend.c
> 

> +++ b/tests/test-block-backend.c
> @@ -0,0 +1,82 @@
> +/*
> + * BlockBackend tests
> + *
> + * Copyright (c) 2017 Kevin Wolf <kwolf@redhat.com>

Want to include 2018?  Using a redhat.com address but a personal 
copyright?  (Then again, Red Hat has some pretty cool policies on what 
it permits for code written on an employee's own time).

As long as Kevin is happy with the final copyright line that goes into 
the commit:

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/3] block: test blk_aio_flush() with blk->root == NULL

[Stefan Hajnoczi]

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

On Fri, Feb 09, 2018 at 09:23:53AM -0600, Eric Blake wrote:
> On 02/08/2018 11:18 AM, Stefan Hajnoczi wrote:
> > From: Kevin Wolf <kwolf@redhat.com>
> > 
> > This patch adds test cases for the scenario where blk_aio_flush() is
> > called on a BlockBackend with no root.  Calling drain afterwards should
> > complete the requests with -ENOMEDIUM.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   tests/Makefile.include     |  2 ++
> >   tests/test-block-backend.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 84 insertions(+)
> >   create mode 100644 tests/test-block-backend.c
> > 
> 
> > +++ b/tests/test-block-backend.c
> > @@ -0,0 +1,82 @@
> > +/*
> > + * BlockBackend tests
> > + *
> > + * Copyright (c) 2017 Kevin Wolf <kwolf@redhat.com>
> 
> Want to include 2018?  Using a redhat.com address but a personal copyright?
> (Then again, Red Hat has some pretty cool policies on what it permits for
> code written on an employee's own time).
> 
> As long as Kevin is happy with the final copyright line that goes into the
> commit:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

This is how I got the test case from Kevin, I didn't change the
copyright line.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 3/3] Revert "IDE: Do not flush empty CDROM drives"

[Stefan Hajnoczi]

This reverts commit 4da97120d51a4383aa96d741a2b837f8c4bbcd0b.

blk_aio_flush() now handles the blk->root == NULL case, so we no longer
need this workaround.

Cc: John Snow <jsnow@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/ide/core.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5be72d41dc..6154bc6ad7 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1085,15 +1085,7 @@ static void ide_flush_cache(IDEState *s)
     s->status |= BUSY_STAT;
     ide_set_retry(s);
     block_acct_start(blk_get_stats(s->blk), &s->acct, 0, BLOCK_ACCT_FLUSH);
-
-    if (blk_bs(s->blk)) {
-        s->pio_aiocb = blk_aio_flush(s->blk, ide_flush_cb, s);
-    } else {
-        /* XXX blk_aio_flush() crashes when blk_bs(blk) is NULL, remove this
-         * temporary workaround when blk_aio_*() functions handle NULL blk_bs.
-         */
-        ide_flush_cb(s, 0);
-    }
+    s->pio_aiocb = blk_aio_flush(s->blk, ide_flush_cb, s);
 }
 
 static void ide_cfata_metadata_inquiry(IDEState *s)
-- 
2.14.3

Re: [Qemu-devel] [PATCH 3/3] Revert "IDE: Do not flush empty CDROM drives"

[Eric Blake]

On 02/08/2018 11:18 AM, Stefan Hajnoczi wrote:
> This reverts commit 4da97120d51a4383aa96d741a2b837f8c4bbcd0b.
> 
> blk_aio_flush() now handles the blk->root == NULL case, so we no longer
> need this workaround.
> 
> Cc: John Snow <jsnow@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   hw/ide/core.c | 10 +---------
>   1 file changed, 1 insertion(+), 9 deletions(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 0/3] block: fix blk_aio_*() segfault when blk->root == NULL

[Eric Blake]

On 02/08/2018 11:18 AM, Stefan Hajnoczi wrote:
> Using bdrv_inc_in_flight(blk_bs(blk)) doesn't work since BlockBackend->root may
> be NULL.
> 
> This patch series solves the issue by adding an BlockBackend->in_flight counter
> so requests can be tracked even when there is no BlockDriverState.
> 
> This should fix the IDE and virtio-blk segfaults that have been encountered
> when there is no BlockDriverState.
> 
> The patch is based on work by Kevin Wolf.
> 
> Kevin Wolf (1):
>    block: test blk_aio_flush() with blk->root == NULL
> 
> Stefan Hajnoczi (2):
>    block: add BlockBackend->in_flight counter
>    Revert "IDE: Do not flush empty CDROM drives"

Tested by applying the series out of order (2, 3, 1) - the new test 
fails after 2 (so even our workaround was not robust), fails after 3 
(expected there, because we revert the workaround), then finally passes 
after 1 (the correct fix with no workaround needed).  So for the series, 
you can add:
Tested-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org