[Qemu-devel] [PATCH 0/4] IDE: Do not flush empty drives

[Qemu-devel] [PATCH 0/4] IDE: Do not flush empty drives

[John Snow]

Patches one and two here are a 2.10 bandaid that avoids a crash.
Patches three and four are a more comprehensive fix as written by
Kevin in another discussion and are being posted here for the sake
of a discussion.

Patch three as written causes hangs in iotests 20, 39, 97, 98, 129,
153, 176, and 185. 124 actually segfaults.

For the purposes of 2.10, we'll likely just want patches 1 and 2
for now.

The problem in a nutshell: incrementing the in-flight counter of the
BDS from the BB layer assumes that every BB always has a BDS. That's
not true; and some devices like IDE have not in the past checked to
see if a given blk_ operation WOULD fail.

This culminates in a new regression where issuing a cache flush to a
CDROM (which is, for some reason, specification valid) will crash QEMU
due to a null dereference when attempting to atomically increment that
backend's in-flight counter.

John Snow (1):
  IDE: Do not flush empty CDROM drives

Kevin Wolf (3):
  IDE: test flush on empty CDROM
  block-backend: shift in-flight counter to BB from BDS
  block-backend: test flush op on empty backend

 block.c                    |  2 +-
 block/block-backend.c      | 40 +++++++++++++++++++++++++-----
 hw/ide/core.c              | 11 +++++---
 tests/Makefile.include     |  2 ++
 tests/ide-test.c           | 19 ++++++++++++++
 tests/test-block-backend.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 125 insertions(+), 11 deletions(-)
 create mode 100644 tests/test-block-backend.c

-- 
2.9.4

[Qemu-devel] [PATCH 1/4] IDE: Do not flush empty CDROM drives

[John Snow]

The block backend changed in a way that flushing empty CDROM drives
is now an error. Amend IDE to avoid doing so until the root problem
can be addressed for 2.11.

Reported-by: Kieron Shorrock <kshorrock@paloaltonetworks.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0b48b64..6cbca43 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1053,17 +1053,21 @@ static void ide_flush_cb(void *opaque, int ret)
     ide_set_irq(s->bus);
 }
 
-static void ide_flush_cache(IDEState *s)
+static bool ide_flush_cache(IDEState *s)
 {
     if (s->blk == NULL) {
         ide_flush_cb(s, 0);
-        return;
+        return false;
+    } else if (!blk_bs(s->blk)) {
+        /* Nothing to flush */
+        return true;
     }
 
     s->status |= BUSY_STAT;
     ide_set_retry(s);
     block_acct_start(blk_get_stats(s->blk), &s->acct, 0, BLOCK_ACCT_FLUSH);
     s->pio_aiocb = blk_aio_flush(s->blk, ide_flush_cb, s);
+    return false;
 }
 
 static void ide_cfata_metadata_inquiry(IDEState *s)
@@ -1508,8 +1512,7 @@ static bool cmd_write_dma(IDEState *s, uint8_t cmd)
 
 static bool cmd_flush_cache(IDEState *s, uint8_t cmd)
 {
-    ide_flush_cache(s);
-    return false;
+    return ide_flush_cache(s);
 }
 
 static bool cmd_seek(IDEState *s, uint8_t cmd)
-- 
2.9.4

[Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM

[John Snow]

From: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ide-test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index bfd79dd..ffbfb04 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -689,6 +689,24 @@ static void test_flush_nodev(void)
     ide_test_quit();
 }
 
+static void test_flush_empty_drive(void)
+{
+    QPCIDevice *dev;
+    QPCIBar bmdma_bar, ide_bar;
+
+    ide_test_start("-device ide-cd,bus=ide.0");
+    dev = get_pci_device(&bmdma_bar, &ide_bar);
+
+    /* FLUSH CACHE command on device 0*/
+    qpci_io_writeb(dev, ide_bar, reg_device, 0);
+    qpci_io_writeb(dev, ide_bar, reg_command, CMD_FLUSH_CACHE);
+
+    /* Just testing that qemu doesn't crash... */
+
+    free_pci_device(dev);
+    ide_test_quit();
+}
+
 static void test_pci_retry_flush(void)
 {
     test_retry_flush("pc");
@@ -954,6 +972,7 @@ int main(int argc, char **argv)
 
     qtest_add_func("/ide/flush", test_flush);
     qtest_add_func("/ide/flush/nodev", test_flush_nodev);
+    qtest_add_func("/ide/flush/empty_drive", test_flush_empty_drive);
     qtest_add_func("/ide/flush/retry_pci", test_pci_retry_flush);
     qtest_add_func("/ide/flush/retry_isa", test_isa_retry_flush);
 
-- 
2.9.4

[Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS

[John Snow]

From: Kevin Wolf <kwolf@redhat.com>

This allows us to detect errors in cache flushing (ENOMEDIUM)
without choking on a null dereference because we assume that
blk_bs(bb) is always defined.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 block.c               |  2 +-
 block/block-backend.c | 40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index ce9cce7..834b836 100644
--- a/block.c
+++ b/block.c
@@ -4476,7 +4476,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 968438c..efd7e92 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -68,6 +68,9 @@ struct BlockBackend {
     NotifierList remove_bs_notifiers, insert_bs_notifiers;
 
     int quiesce_counter;
+
+    /* Number of in-flight requests. Accessed with atomic ops. */
+    unsigned int in_flight;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -1109,6 +1112,16 @@ 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;
@@ -1147,7 +1160,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);
     }
@@ -1168,7 +1181,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,
@@ -1405,14 +1418,28 @@ int blk_flush(BlockBackend *blk)
 
 void blk_drain(BlockBackend *blk)
 {
-    if (blk_bs(blk)) {
-        bdrv_drain(blk_bs(blk));
+    AioContext *ctx = blk_get_aio_context(blk);
+
+    while (atomic_read(&blk->in_flight)) {
+        aio_context_acquire(ctx);
+        aio_poll(ctx, false);
+        aio_context_release(ctx);
+
+        if (blk_bs(blk)) {
+            bdrv_drain(blk_bs(blk));
+        }
     }
 }
 
 void blk_drain_all(void)
 {
-    bdrv_drain_all();
+    BlockBackend *blk = NULL;
+
+    bdrv_drain_all_begin();
+    while ((blk = blk_all_next(blk)) != NULL) {
+        blk_drain(blk);
+    }
+    bdrv_drain_all_end();
 }
 
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
@@ -1453,10 +1480,11 @@ static void send_qmp_error_event(BlockBackend *blk,
                                  bool is_read, int error)
 {
     IoOperationType optype;
+    BlockDriverState *bs = blk_bs(blk);
 
     optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
     qapi_event_send_block_io_error(blk_name(blk),
-                                   bdrv_get_node_name(blk_bs(blk)), optype,
+                                   bs ? bdrv_get_node_name(bs) : "", optype,
                                    action, blk_iostatus_is_enabled(blk),
                                    error == ENOSPC, strerror(error),
                                    &error_abort);
-- 
2.9.4

[Qemu-devel] [PATCH 4/4] block-backend: test flush op on empty backend

[John Snow]

From: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/Makefile.include     |  2 ++
 tests/test-block-backend.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 tests/test-block-backend.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index eb4895f..153494b 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -56,6 +56,7 @@ check-unit-y += tests/test-hbitmap$(EXESUF)
 gcov-files-test-hbitmap-y = blockjob.c
 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 =
@@ -556,6 +557,7 @@ tests/test-aio-multithread$(EXESUF): tests/test-aio-multithread.o $(test-block-o
 tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-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 0000000..5348781
--- /dev/null
+++ b/tests/test-block-backend.c
@@ -0,0 +1,62 @@
+/*
+ * 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);
+}
+
+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);
+
+    return g_test_run();
+}
-- 
2.9.4

Re: [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS

[Paolo Bonzini]

----- Original Message -----
> From: "John Snow" <jsnow@redhat.com>
> To: qemu-block@nongnu.org
> Cc: kwolf@redhat.com, qemu-devel@nongnu.org, dgilbert@redhat.com, stefanha@redhat.com, pbonzini@redhat.com,
> pjp@redhat.com, "John Snow" <jsnow@redhat.com>
> Sent: Tuesday, August 8, 2017 7:57:10 PM
> Subject: [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS
> 
> From: Kevin Wolf <kwolf@redhat.com>
> 
> This allows us to detect errors in cache flushing (ENOMEDIUM)
> without choking on a null dereference because we assume that
> blk_bs(bb) is always defined.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>

This is not enough, as discussed in the thread.

Paolo

> ---
>  block.c               |  2 +-
>  block/block-backend.c | 40 ++++++++++++++++++++++++++++++++++------
>  2 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ce9cce7..834b836 100644
> --- a/block.c
> +++ b/block.c
> @@ -4476,7 +4476,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 968438c..efd7e92 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -68,6 +68,9 @@ struct BlockBackend {
>      NotifierList remove_bs_notifiers, insert_bs_notifiers;
>  
>      int quiesce_counter;
> +
> +    /* Number of in-flight requests. Accessed with atomic ops. */
> +    unsigned int in_flight;
>  };
>  
>  typedef struct BlockBackendAIOCB {
> @@ -1109,6 +1112,16 @@ 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;
> @@ -1147,7 +1160,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);
>      }
> @@ -1168,7 +1181,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,
> @@ -1405,14 +1418,28 @@ int blk_flush(BlockBackend *blk)
>  
>  void blk_drain(BlockBackend *blk)
>  {
> -    if (blk_bs(blk)) {
> -        bdrv_drain(blk_bs(blk));
> +    AioContext *ctx = blk_get_aio_context(blk);
> +
> +    while (atomic_read(&blk->in_flight)) {
> +        aio_context_acquire(ctx);
> +        aio_poll(ctx, false);
> +        aio_context_release(ctx);
> +
> +        if (blk_bs(blk)) {
> +            bdrv_drain(blk_bs(blk));
> +        }
>      }
>  }
>  
>  void blk_drain_all(void)
>  {
> -    bdrv_drain_all();
> +    BlockBackend *blk = NULL;
> +
> +    bdrv_drain_all_begin();
> +    while ((blk = blk_all_next(blk)) != NULL) {
> +        blk_drain(blk);
> +    }
> +    bdrv_drain_all_end();
>  }
>  
>  void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
> @@ -1453,10 +1480,11 @@ static void send_qmp_error_event(BlockBackend *blk,
>                                   bool is_read, int error)
>  {
>      IoOperationType optype;
> +    BlockDriverState *bs = blk_bs(blk);
>  
>      optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
>      qapi_event_send_block_io_error(blk_name(blk),
> -                                   bdrv_get_node_name(blk_bs(blk)), optype,
> +                                   bs ? bdrv_get_node_name(bs) : "", optype,
>                                     action, blk_iostatus_is_enabled(blk),
>                                     error == ENOSPC, strerror(error),
>                                     &error_abort);
> --
> 2.9.4
> 
> 

Re: [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS

[John Snow]

On 08/08/2017 02:34 PM, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
>> From: "John Snow" <jsnow@redhat.com>
>> To: qemu-block@nongnu.org
>> Cc: kwolf@redhat.com, qemu-devel@nongnu.org, dgilbert@redhat.com, stefanha@redhat.com, pbonzini@redhat.com,
>> pjp@redhat.com, "John Snow" <jsnow@redhat.com>
>> Sent: Tuesday, August 8, 2017 7:57:10 PM
>> Subject: [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS
>>
>> From: Kevin Wolf <kwolf@redhat.com>
>>
>> This allows us to detect errors in cache flushing (ENOMEDIUM)
>> without choking on a null dereference because we assume that
>> blk_bs(bb) is always defined.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> This is not enough, as discussed in the thread.
> 
> Paolo
> 

Sure, I amended Kevin's later fix and rolled it into one patch and split
the tests out. The cover letter states that this is busted, but I wanted
it on the list instead of buried in a now-unrelated thread.

So now it's here as a patch, can we keep discussion here instead of on
the other thread?

--John

Re: [Qemu-devel] [PATCH 1/4] IDE: Do not flush empty CDROM drives

[Eric Blake]

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

On 08/08/2017 12:57 PM, John Snow wrote:
> The block backend changed in a way that flushing empty CDROM drives
> is now an error. Amend IDE to avoid doing so until the root problem
> can be addressed for 2.11.
> 
> Reported-by: Kieron Shorrock <kshorrock@paloaltonetworks.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/core.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 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


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

Re: [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM

[Eric Blake]

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

On 08/08/2017 12:57 PM, John Snow wrote:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/ide-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 

> +static void test_flush_empty_drive(void)
> +{
> +    QPCIDevice *dev;
> +    QPCIBar bmdma_bar, ide_bar;
> +
> +    ide_test_start("-device ide-cd,bus=ide.0");
> +    dev = get_pci_device(&bmdma_bar, &ide_bar);
> +
> +    /* FLUSH CACHE command on device 0*/

Space before */

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

I agree with your assessment of 1 and 2 being 2.10 material.

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


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

Re: [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM

[John Snow]

On 08/08/2017 03:20 PM, Eric Blake wrote:
> On 08/08/2017 12:57 PM, John Snow wrote:
>> From: Kevin Wolf <kwolf@redhat.com>
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/ide-test.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
> 
>> +static void test_flush_empty_drive(void)
>> +{
>> +    QPCIDevice *dev;
>> +    QPCIBar bmdma_bar, ide_bar;
>> +
>> +    ide_test_start("-device ide-cd,bus=ide.0");
>> +    dev = get_pci_device(&bmdma_bar, &ide_bar);
>> +
>> +    /* FLUSH CACHE command on device 0*/
> 
> Space before */
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I agree with your assessment of 1 and 2 being 2.10 material.
> 

Yep, thanks. I just wanted to include Kevin's attempt at fixing the root
problem to make it clear that:

(A) The root problem is known and being worked on, but
(B) Is evidently not ready for prime time.

I'll stage 1 & 2 with your minor typo edit here, thank you.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 1/4] IDE: Do not flush empty CDROM drives

[Stefan Hajnoczi]

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

On Tue, Aug 08, 2017 at 01:57:08PM -0400, John Snow wrote:
> The block backend changed in a way that flushing empty CDROM drives
> is now an error. Amend IDE to avoid doing so until the root problem
> can be addressed for 2.11.
> 
> Reported-by: Kieron Shorrock <kshorrock@paloaltonetworks.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/core.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 0b48b64..6cbca43 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1053,17 +1053,21 @@ static void ide_flush_cb(void *opaque, int ret)
>      ide_set_irq(s->bus);
>  }
>  
> -static void ide_flush_cache(IDEState *s)
> +static bool ide_flush_cache(IDEState *s)

Previously this function invoked ide_flush_cb() to complete the request
and raise an IRQ.  Now it may return true instead of invoking
ide_flush_cb().  It's no longer a helper function, it's more like an IDE
command handler.

cmd_set_features() must be updated:

  case 0x82: /* write cache disable */
      blk_set_enable_write_cache(s->blk, false);
      identify_data = (uint16_t *)s->identify_data;
      put_le16(identify_data + 85, (1 << 14) | 1);
      ide_flush_cache(s);
      return false;  <--- should be "return ide_flush_cache(s)"

To make the code clearer I suggest deleting ide_flush_cache() and
calling cmd_flush_cache() directly instead.  That way it's obvious that
this is an IDE command handler and it may return true to indicate the
the command completed immediately.

>  {
>      if (s->blk == NULL) {
>          ide_flush_cb(s, 0);
> -        return;
> +        return false;
> +    } else if (!blk_bs(s->blk)) {
> +        /* Nothing to flush */

Please move information from the commit description into this comment if
you respin.  Someone looking at the code might think this is a nop that
is safe to remove.  Once blk_*() is fixed this code path can be removed
again.

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

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] IDE: test flush on empty CDROM

[Stefan Hajnoczi]

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

On Tue, Aug 08, 2017 at 01:57:09PM -0400, John Snow wrote:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/ide-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/4] IDE: Do not flush empty drives

[Stefan Hajnoczi]

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

On Tue, Aug 08, 2017 at 01:57:07PM -0400, John Snow wrote:
> Patches one and two here are a 2.10 bandaid that avoids a crash.
> Patches three and four are a more comprehensive fix as written by
> Kevin in another discussion and are being posted here for the sake
> of a discussion.
> 
> Patch three as written causes hangs in iotests 20, 39, 97, 98, 129,
> 153, 176, and 185. 124 actually segfaults.
> 
> For the purposes of 2.10, we'll likely just want patches 1 and 2
> for now.
> 
> The problem in a nutshell: incrementing the in-flight counter of the
> BDS from the BB layer assumes that every BB always has a BDS. That's
> not true; and some devices like IDE have not in the past checked to
> see if a given blk_ operation WOULD fail.
> 
> This culminates in a new regression where issuing a cache flush to a
> CDROM (which is, for some reason, specification valid) will crash QEMU
> due to a null dereference when attempting to atomically increment that
> backend's in-flight counter.
> 
> John Snow (1):
>   IDE: Do not flush empty CDROM drives
> 
> Kevin Wolf (3):
>   IDE: test flush on empty CDROM
>   block-backend: shift in-flight counter to BB from BDS
>   block-backend: test flush op on empty backend
> 
>  block.c                    |  2 +-
>  block/block-backend.c      | 40 +++++++++++++++++++++++++-----
>  hw/ide/core.c              | 11 +++++---
>  tests/Makefile.include     |  2 ++
>  tests/ide-test.c           | 19 ++++++++++++++
>  tests/test-block-backend.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 125 insertions(+), 11 deletions(-)
>  create mode 100644 tests/test-block-backend.c

John will be offline until Monday.  I'm sending a new patch series for
2.10 with updated versions of Patch 1 & 2.

Stefan

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

Re: [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS

[Kevin Wolf]

Am 08.08.2017 um 19:57 hat John Snow geschrieben:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> This allows us to detect errors in cache flushing (ENOMEDIUM)
> without choking on a null dereference because we assume that
> blk_bs(bb) is always defined.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block.c               |  2 +-
>  block/block-backend.c | 40 ++++++++++++++++++++++++++++++++++------
>  2 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ce9cce7..834b836 100644
> --- a/block.c
> +++ b/block.c
> @@ -4476,7 +4476,7 @@ out:
>  
>  AioContext *bdrv_get_aio_context(BlockDriverState *bs)
>  {
> -    return bs->aio_context;
> +    return bs ? bs->aio_context : qemu_get_aio_context();
>  }

This should probably be a separate patch; it's not really related to
moving the in-flight counter, but fixes another NULL dereference in
blk_aio_prwv().

>  void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co)
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 968438c..efd7e92 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -68,6 +68,9 @@ struct BlockBackend {
>      NotifierList remove_bs_notifiers, insert_bs_notifiers;
>  
>      int quiesce_counter;
> +
> +    /* Number of in-flight requests. Accessed with atomic ops. */
> +    unsigned int in_flight;
>  };
>  
>  typedef struct BlockBackendAIOCB {
> @@ -1109,6 +1112,16 @@ 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;
> @@ -1147,7 +1160,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);
>      }
> @@ -1168,7 +1181,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,
> @@ -1405,14 +1418,28 @@ int blk_flush(BlockBackend *blk)
>  
>  void blk_drain(BlockBackend *blk)
>  {
> -    if (blk_bs(blk)) {
> -        bdrv_drain(blk_bs(blk));
> +    AioContext *ctx = blk_get_aio_context(blk);
> +
> +    while (atomic_read(&blk->in_flight)) {
> +        aio_context_acquire(ctx);
> +        aio_poll(ctx, false);
> +        aio_context_release(ctx);
> +
> +        if (blk_bs(blk)) {
> +            bdrv_drain(blk_bs(blk));
> +        }
>      }
>  }
>  
>  void blk_drain_all(void)
>  {
> -    bdrv_drain_all();
> +    BlockBackend *blk = NULL;
> +
> +    bdrv_drain_all_begin();
> +    while ((blk = blk_all_next(blk)) != NULL) {
> +        blk_drain(blk);
> +    }
> +    bdrv_drain_all_end();
>  }

We still need to check that everyone who should call blk_drain_all()
rather than bdrv_drain_all() actually does so.

>  void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
> @@ -1453,10 +1480,11 @@ static void send_qmp_error_event(BlockBackend *blk,
>                                   bool is_read, int error)
>  {
>      IoOperationType optype;
> +    BlockDriverState *bs = blk_bs(blk);
>  
>      optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
>      qapi_event_send_block_io_error(blk_name(blk),
> -                                   bdrv_get_node_name(blk_bs(blk)), optype,
> +                                   bs ? bdrv_get_node_name(bs) : "", optype,
>                                     action, blk_iostatus_is_enabled(blk),
>                                     error == ENOSPC, strerror(error),
>                                     &error_abort);

And this is another independent NULL dereference fix.

Kevin

Re: [Qemu-devel] [PATCH 4/4] block-backend: test flush op on empty backend

[Kevin Wolf]

Am 08.08.2017 um 19:57 hat John Snow geschrieben:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/Makefile.include     |  2 ++
>  tests/test-block-backend.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
>  create mode 100644 tests/test-block-backend.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index eb4895f..153494b 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -56,6 +56,7 @@ check-unit-y += tests/test-hbitmap$(EXESUF)
>  gcov-files-test-hbitmap-y = blockjob.c
>  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 =
> @@ -556,6 +557,7 @@ tests/test-aio-multithread$(EXESUF): tests/test-aio-multithread.o $(test-block-o
>  tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-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 0000000..5348781
> --- /dev/null
> +++ b/tests/test-block-backend.c
> @@ -0,0 +1,62 @@
> +/*
> + * 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);
> +}

Locally, I added a second test for blk_drain_all():

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);
> +
> +    return g_test_run();
> +}

Kevin