* [Qemu-devel] [PATCH 1/4] IDE: Do not flush empty CDROM drives
2017-08-08 17:57 [Qemu-devel] [PATCH 0/4] IDE: Do not flush empty drives John Snow
@ 2017-08-08 17:57 ` John Snow
2017-08-08 19:19 ` Eric Blake
2017-08-09 9:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2017-08-08 17:57 ` [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM John Snow
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: John Snow @ 2017-08-08 17:57 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] IDE: Do not flush empty CDROM drives
2017-08-08 17:57 ` [Qemu-devel] [PATCH 1/4] IDE: Do not flush empty CDROM drives John Snow
@ 2017-08-08 19:19 ` Eric Blake
2017-08-09 9:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
1 sibling, 0 replies; 15+ messages in thread
From: Eric Blake @ 2017-08-08 19:19 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 1/4] IDE: Do not flush empty CDROM drives
2017-08-08 17:57 ` [Qemu-devel] [PATCH 1/4] IDE: Do not flush empty CDROM drives John Snow
2017-08-08 19:19 ` Eric Blake
@ 2017-08-09 9:34 ` Stefan Hajnoczi
1 sibling, 0 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2017-08-09 9:34 UTC (permalink / raw)
To: John Snow
Cc: qemu-block, kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM
2017-08-08 17:57 [Qemu-devel] [PATCH 0/4] IDE: Do not flush empty drives John Snow
2017-08-08 17:57 ` [Qemu-devel] [PATCH 1/4] IDE: Do not flush empty CDROM drives John Snow
@ 2017-08-08 17:57 ` John Snow
2017-08-08 19:20 ` Eric Blake
2017-08-09 9:35 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2017-08-08 17:57 ` [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS John Snow
` (2 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: John Snow @ 2017-08-08 17:57 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM
2017-08-08 17:57 ` [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM John Snow
@ 2017-08-08 19:20 ` Eric Blake
2017-08-08 19:32 ` John Snow
2017-08-09 9:35 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
1 sibling, 1 reply; 15+ messages in thread
From: Eric Blake @ 2017-08-08 19:20 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM
2017-08-08 19:20 ` Eric Blake
@ 2017-08-08 19:32 ` John Snow
0 siblings, 0 replies; 15+ messages in thread
From: John Snow @ 2017-08-08 19:32 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] IDE: test flush on empty CDROM
2017-08-08 17:57 ` [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM John Snow
2017-08-08 19:20 ` Eric Blake
@ 2017-08-09 9:35 ` Stefan Hajnoczi
1 sibling, 0 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2017-08-09 9:35 UTC (permalink / raw)
To: John Snow
Cc: qemu-block, kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS
2017-08-08 17:57 [Qemu-devel] [PATCH 0/4] IDE: Do not flush empty drives John Snow
2017-08-08 17:57 ` [Qemu-devel] [PATCH 1/4] IDE: Do not flush empty CDROM drives John Snow
2017-08-08 17:57 ` [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM John Snow
@ 2017-08-08 17:57 ` John Snow
2017-08-08 18:34 ` Paolo Bonzini
2017-08-09 16:01 ` Kevin Wolf
2017-08-08 17:57 ` [Qemu-devel] [PATCH 4/4] block-backend: test flush op on empty backend John Snow
2017-08-09 15:53 ` [Qemu-devel] [Qemu-block] [PATCH 0/4] IDE: Do not flush empty drives Stefan Hajnoczi
4 siblings, 2 replies; 15+ messages in thread
From: John Snow @ 2017-08-08 17:57 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS
2017-08-08 17:57 ` [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS John Snow
@ 2017-08-08 18:34 ` Paolo Bonzini
2017-08-08 18:48 ` John Snow
2017-08-09 16:01 ` Kevin Wolf
1 sibling, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2017-08-08 18:34 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, qemu-devel, dgilbert, stefanha, pjp
----- 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
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS
2017-08-08 18:34 ` Paolo Bonzini
@ 2017-08-08 18:48 ` John Snow
0 siblings, 0 replies; 15+ messages in thread
From: John Snow @ 2017-08-08 18:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-block, qemu-devel, dgilbert, stefanha, pjp
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS
2017-08-08 17:57 ` [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS John Snow
2017-08-08 18:34 ` Paolo Bonzini
@ 2017-08-09 16:01 ` Kevin Wolf
1 sibling, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2017-08-09 16:01 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, qemu-devel, dgilbert, stefanha, pbonzini, pjp
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 4/4] block-backend: test flush op on empty backend
2017-08-08 17:57 [Qemu-devel] [PATCH 0/4] IDE: Do not flush empty drives John Snow
` (2 preceding siblings ...)
2017-08-08 17:57 ` [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS John Snow
@ 2017-08-08 17:57 ` John Snow
2017-08-09 16:02 ` Kevin Wolf
2017-08-09 15:53 ` [Qemu-devel] [Qemu-block] [PATCH 0/4] IDE: Do not flush empty drives Stefan Hajnoczi
4 siblings, 1 reply; 15+ messages in thread
From: John Snow @ 2017-08-08 17:57 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] block-backend: test flush op on empty backend
2017-08-08 17:57 ` [Qemu-devel] [PATCH 4/4] block-backend: test flush op on empty backend John Snow
@ 2017-08-09 16:02 ` Kevin Wolf
0 siblings, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2017-08-09 16:02 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, qemu-devel, dgilbert, stefanha, pbonzini, pjp
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 0/4] IDE: Do not flush empty drives
2017-08-08 17:57 [Qemu-devel] [PATCH 0/4] IDE: Do not flush empty drives John Snow
` (3 preceding siblings ...)
2017-08-08 17:57 ` [Qemu-devel] [PATCH 4/4] block-backend: test flush op on empty backend John Snow
@ 2017-08-09 15:53 ` Stefan Hajnoczi
4 siblings, 0 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2017-08-09 15:53 UTC (permalink / raw)
To: John Snow
Cc: qemu-block, kwolf, qemu-devel, dgilbert, stefanha, pbonzini, pjp
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread