[Qemu-devel] [PATCH v2 0/4] ide: Set BSY bit during FLUSH

[Qemu-devel] [PATCH v2 0/4] ide: Set BSY bit during FLUSH

[Kevin Wolf]

Still depends on the qemu-io series ("[PATCH v2 00/16] Make qemu-io commands
available in the monitor")

v2:
- Dropped unnecessary hunk in the actual fix
- Avoid magic number 0x10 and change the preexisting case

Andreas Färber (1):
  ide: Set BSY bit during FLUSH

Kevin Wolf (3):
  blkdebug: Add BLKDBG_FLUSH_TO_OS/DISK events
  ide-test: Add enum value for DEV
  ide-test: Add FLUSH CACHE test case

 block.c               |  8 ++++----
 block/blkdebug.c      |  3 +++
 hw/ide/core.c         |  1 +
 include/block/block.h |  3 +++
 tests/ide-test.c      | 43 ++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 53 insertions(+), 5 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 1/4] blkdebug: Add BLKDBG_FLUSH_TO_OS/DISK events

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 8 ++++----
 block/blkdebug.c      | 3 +++
 include/block/block.h | 3 +++
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 3f616de..79ad33d 100644
--- a/block.c
+++ b/block.c
@@ -3186,13 +3186,11 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
 
 void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
 {
-    BlockDriver *drv = bs->drv;
-
-    if (!drv || !drv->bdrv_debug_event) {
+    if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
         return;
     }
 
-    drv->bdrv_debug_event(bs, event);
+    bs->drv->bdrv_debug_event(bs, event);
 }
 
 int bdrv_debug_breakpoint(BlockDriverState *bs, const char *event,
@@ -4024,6 +4022,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
     }
 
     /* Write back cached data to the OS even with cache=unsafe */
+    BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_OS);
     if (bs->drv->bdrv_co_flush_to_os) {
         ret = bs->drv->bdrv_co_flush_to_os(bs);
         if (ret < 0) {
@@ -4036,6 +4035,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
         goto flush_parent;
     }
 
+    BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_DISK);
     if (bs->drv->bdrv_co_flush_to_disk) {
         ret = bs->drv->bdrv_co_flush_to_disk(bs);
     } else if (bs->drv->bdrv_aio_flush) {
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 71f99e4..ccb627a 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -182,6 +182,9 @@ static const char *event_names[BLKDBG_EVENT_MAX] = {
     [BLKDBG_CLUSTER_ALLOC]                  = "cluster_alloc",
     [BLKDBG_CLUSTER_ALLOC_BYTES]            = "cluster_alloc_bytes",
     [BLKDBG_CLUSTER_FREE]                   = "cluster_free",
+
+    [BLKDBG_FLUSH_TO_OS]                    = "flush_to_os",
+    [BLKDBG_FLUSH_TO_DISK]                  = "flush_to_disk",
 };
 
 static int get_event_by_name(const char *name, BlkDebugEvent *event)
diff --git a/include/block/block.h b/include/block/block.h
index dc5b388..2307f67 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -424,6 +424,9 @@ typedef enum {
     BLKDBG_CLUSTER_ALLOC_BYTES,
     BLKDBG_CLUSTER_FREE,
 
+    BLKDBG_FLUSH_TO_OS,
+    BLKDBG_FLUSH_TO_DISK,
+
     BLKDBG_EVENT_MAX,
 } BlkDebugEvent;
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 2/4] ide-test: Add enum value for DEV

[Kevin Wolf]

Get rid of the magic number.

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

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 365e995..1c31a2e 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -64,6 +64,7 @@ enum {
 };
 
 enum {
+    DEV     = 0x10,
     LBA     = 0x40,
 };
 
@@ -394,7 +395,7 @@ static void test_identify(void)
 
     /* Read in the IDENTIFY buffer and check registers */
     data = inb(IDE_BASE + reg_device);
-    g_assert_cmpint(data & 0x10, ==, 0);
+    g_assert_cmpint(data & DEV, ==, 0);
 
     for (i = 0; i < 256; i++) {
         data = inb(IDE_BASE + reg_status);
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 3/4] ide: Set BSY bit during FLUSH

[Kevin Wolf]

From: Andreas Färber <afaerber@suse.de>

The implementation of the ATA FLUSH command invokes a flush at the block
layer, which may on raw files on POSIX entail a synchronous fdatasync().
This may in some cases take so long that the SLES 11 SP1 guest driver
reports I/O errors and filesystems get corrupted or remounted read-only.

Avoid this by setting BUSY_STAT, so that the guest is made aware we are
in the middle of an operation and no ATA commands are attempted to be
processed concurrently.

Addresses BNC#637297.

Suggested-by: Gonglei (Arei) <arei.gonglei@huawei.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index c7a8041..9926d92 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -814,6 +814,7 @@ void ide_flush_cache(IDEState *s)
         return;
     }
 
+    s->status |= BUSY_STAT;
     bdrv_acct_start(s->bs, &s->acct, 0, BDRV_ACCT_FLUSH);
     bdrv_aio_flush(s->bs, ide_flush_cb, s);
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 4/4] ide-test: Add FLUSH CACHE test case

[Kevin Wolf]

This checks in particular that BSY is set while the flush request is in
flight.

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

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 1c31a2e..828e71a 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -77,6 +77,7 @@ enum {
 enum {
     CMD_READ_DMA    = 0xc8,
     CMD_WRITE_DMA   = 0xca,
+    CMD_FLUSH_CACHE = 0xe7,
     CMD_IDENTIFY    = 0xec,
 
     CMDF_ABORT      = 0x100,
@@ -424,6 +425,43 @@ static void test_identify(void)
     ide_test_quit();
 }
 
+static void test_flush(void)
+{
+    uint8_t data;
+
+    ide_test_start(
+        "-vnc none "
+        "-drive file=blkdebug::%s,if=ide,cache=writeback",
+        tmp_path);
+
+    /* Delay the completion of the flush request until we explicitly do it */
+    qmp("{'execute':'human-monitor-command', 'arguments': { "
+        "'command-line': 'qemu-io ide0-hd0 \"break flush_to_os A\"'} }");
+
+    /* FLUSH CACHE command on device 0*/
+    outb(IDE_BASE + reg_device, 0);
+    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
+
+    /* Check status while request is in flight*/
+    data = inb(IDE_BASE + reg_status);
+    assert_bit_set(data, BSY | DRDY);
+    assert_bit_clear(data, DF | ERR | DRQ);
+
+    /* Complete the command */
+    qmp("{'execute':'human-monitor-command', 'arguments': { "
+        "'command-line': 'qemu-io ide0-hd0 \"resume A\"'} }");
+
+    /* Check registers */
+    data = inb(IDE_BASE + reg_device);
+    g_assert_cmpint(data & DEV, ==, 0);
+
+    data = inb(IDE_BASE + reg_status);
+    assert_bit_set(data, DRDY);
+    assert_bit_clear(data, BSY | DF | ERR | DRQ);
+
+    ide_test_quit();
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -454,6 +492,8 @@ int main(int argc, char **argv)
     qtest_add_func("/ide/bmdma/long_prdt", test_bmdma_long_prdt);
     qtest_add_func("/ide/bmdma/teardown", test_bmdma_teardown);
 
+    qtest_add_func("/ide/flush", test_flush);
+
     ret = g_test_run();
 
     /* Cleanup */
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 0/4] ide: Set BSY bit during FLUSH

[Stefan Hajnoczi]

On Wed, Jun 05, 2013 at 03:17:54PM +0200, Kevin Wolf wrote:
> Still depends on the qemu-io series ("[PATCH v2 00/16] Make qemu-io commands
> available in the monitor")
> 
> v2:
> - Dropped unnecessary hunk in the actual fix
> - Avoid magic number 0x10 and change the preexisting case
> 
> Andreas Färber (1):
>   ide: Set BSY bit during FLUSH
> 
> Kevin Wolf (3):
>   blkdebug: Add BLKDBG_FLUSH_TO_OS/DISK events
>   ide-test: Add enum value for DEV
>   ide-test: Add FLUSH CACHE test case
> 
>  block.c               |  8 ++++----
>  block/blkdebug.c      |  3 +++
>  hw/ide/core.c         |  1 +
>  include/block/block.h |  3 +++
>  tests/ide-test.c      | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 53 insertions(+), 5 deletions(-)

I'm getting non-deterministic make check failures:

ERROR:tests/ide-test.c:447:test_flush: assertion failed ((data) & (BSY | DRDY) == (BSY | DRDY)): (0x00000040 == 0x000000c0)
GTester: last random seed: R02S5fdc203e989638061808bd6049a31a77

Re: [Qemu-devel] [PATCH v2 0/4] ide: Set BSY bit during FLUSH

[Stefan Hajnoczi]

On Wed, Jun 05, 2013 at 03:17:54PM +0200, Kevin Wolf wrote:
> Still depends on the qemu-io series ("[PATCH v2 00/16] Make qemu-io commands
> available in the monitor")
> 
> v2:
> - Dropped unnecessary hunk in the actual fix
> - Avoid magic number 0x10 and change the preexisting case
> 
> Andreas Färber (1):
>   ide: Set BSY bit during FLUSH
> 
> Kevin Wolf (3):
>   blkdebug: Add BLKDBG_FLUSH_TO_OS/DISK events
>   ide-test: Add enum value for DEV
>   ide-test: Add FLUSH CACHE test case
> 
>  block.c               |  8 ++++----
>  block/blkdebug.c      |  3 +++
>  hw/ide/core.c         |  1 +
>  include/block/block.h |  3 +++
>  tests/ide-test.c      | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 53 insertions(+), 5 deletions(-)
> 
> -- 
> 1.8.1.4

The make check error is explained by the fact that I forgot to apply the
qemu-io series first :-).

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [PATCH v2 3/4] ide: Set BSY bit during FLUSH

[Alex Williamson]

On Wed, 2013-06-05 at 15:17 +0200, Kevin Wolf wrote:
> From: Andreas Färber <afaerber@suse.de>
> 
> The implementation of the ATA FLUSH command invokes a flush at the block
> layer, which may on raw files on POSIX entail a synchronous fdatasync().
> This may in some cases take so long that the SLES 11 SP1 guest driver
> reports I/O errors and filesystems get corrupted or remounted read-only.
> 
> Avoid this by setting BUSY_STAT, so that the guest is made aware we are
> in the middle of an operation and no ATA commands are attempted to be
> processed concurrently.
> 
> Addresses BNC#637297.
> 
> Suggested-by: Gonglei (Arei) <arei.gonglei@huawei.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/ide/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c7a8041..9926d92 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -814,6 +814,7 @@ void ide_flush_cache(IDEState *s)
>          return;
>      }
>  
> +    s->status |= BUSY_STAT;
>      bdrv_acct_start(s->bs, &s->acct, 0, BDRV_ACCT_FLUSH);
>      bdrv_aio_flush(s->bs, ide_flush_cb, s);
>  }


I can no longer boot win7 x64 on q35 with IDE using a qcow2 image.  git
bisect determined this patch is the culprit.

-M q35 -nodefconfig -readconfig docs/q35-chipset.cfg -drive
file=image.qcow2,if=none,id=mydisk -device
ide-drive,drive=mydisk,bus=ide.0

Thanks,
Alex

Re: [Qemu-devel] [PATCH v2 3/4] ide: Set BSY bit during FLUSH

[Kevin Wolf]

Am 03.07.2013 um 22:02 hat Alex Williamson geschrieben:
> On Wed, 2013-06-05 at 15:17 +0200, Kevin Wolf wrote:
> > From: Andreas Färber <afaerber@suse.de>
> > 
> > The implementation of the ATA FLUSH command invokes a flush at the block
> > layer, which may on raw files on POSIX entail a synchronous fdatasync().
> > This may in some cases take so long that the SLES 11 SP1 guest driver
> > reports I/O errors and filesystems get corrupted or remounted read-only.
> > 
> > Avoid this by setting BUSY_STAT, so that the guest is made aware we are
> > in the middle of an operation and no ATA commands are attempted to be
> > processed concurrently.
> > 
> > Addresses BNC#637297.
> > 
> > Suggested-by: Gonglei (Arei) <arei.gonglei@huawei.com>
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/ide/core.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index c7a8041..9926d92 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -814,6 +814,7 @@ void ide_flush_cache(IDEState *s)
> >          return;
> >      }
> >  
> > +    s->status |= BUSY_STAT;
> >      bdrv_acct_start(s->bs, &s->acct, 0, BDRV_ACCT_FLUSH);
> >      bdrv_aio_flush(s->bs, ide_flush_cb, s);
> >  }
> 
> 
> I can no longer boot win7 x64 on q35 with IDE using a qcow2 image.  git
> bisect determined this patch is the culprit.
> 
> -M q35 -nodefconfig -readconfig docs/q35-chipset.cfg -drive
> file=image.qcow2,if=none,id=mydisk -device
> ide-drive,drive=mydisk,bus=ide.0

This means you're using AHCI, right?

handle_cmd() in ahci.c checks the flags and does indeed behave
differently now:

    if (s->dev[port].port.ifs[0].status & (BUSY_STAT|DRQ_STAT)) {
        /* async command, complete later */
        s->dev[port].busy_slot = slot;
        return -1;
    }

    /* done handling the command */
    return 0;

The caller of this code updates pr->cmd_issue to clear the bit for the
respective command slot. This is missed now, and the later completion
mentioned in the comment doesn't happen for flushes, the IDE core never
calls back into the AHCI core for the completion.

The correct fix might be to call ide_set_inactive() in the flush
callback, though I haven't checked in detail yet whether there's
anything specific to DMA read/write in ide_set_inactive().

Kevin

Re: [Qemu-devel] [PATCH v2 3/4] ide: Set BSY bit during FLUSH

[Michael S. Tsirkin]

On Thu, Jul 04, 2013 at 09:55:42AM +0200, Kevin Wolf wrote:
> Am 03.07.2013 um 22:02 hat Alex Williamson geschrieben:
> > On Wed, 2013-06-05 at 15:17 +0200, Kevin Wolf wrote:
> > > From: Andreas Färber <afaerber@suse.de>
> > > 
> > > The implementation of the ATA FLUSH command invokes a flush at the block
> > > layer, which may on raw files on POSIX entail a synchronous fdatasync().
> > > This may in some cases take so long that the SLES 11 SP1 guest driver
> > > reports I/O errors and filesystems get corrupted or remounted read-only.
> > > 
> > > Avoid this by setting BUSY_STAT, so that the guest is made aware we are
> > > in the middle of an operation and no ATA commands are attempted to be
> > > processed concurrently.
> > > 
> > > Addresses BNC#637297.
> > > 
> > > Suggested-by: Gonglei (Arei) <arei.gonglei@huawei.com>
> > > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  hw/ide/core.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > > index c7a8041..9926d92 100644
> > > --- a/hw/ide/core.c
> > > +++ b/hw/ide/core.c
> > > @@ -814,6 +814,7 @@ void ide_flush_cache(IDEState *s)
> > >          return;
> > >      }
> > >  
> > > +    s->status |= BUSY_STAT;
> > >      bdrv_acct_start(s->bs, &s->acct, 0, BDRV_ACCT_FLUSH);
> > >      bdrv_aio_flush(s->bs, ide_flush_cb, s);
> > >  }
> > 
> > 
> > I can no longer boot win7 x64 on q35 with IDE using a qcow2 image.  git
> > bisect determined this patch is the culprit.
> > 
> > -M q35 -nodefconfig -readconfig docs/q35-chipset.cfg -drive
> > file=image.qcow2,if=none,id=mydisk -device
> > ide-drive,drive=mydisk,bus=ide.0
> 
> This means you're using AHCI, right?
> 
> handle_cmd() in ahci.c checks the flags and does indeed behave
> differently now:
> 
>     if (s->dev[port].port.ifs[0].status & (BUSY_STAT|DRQ_STAT)) {
>         /* async command, complete later */
>         s->dev[port].busy_slot = slot;
>         return -1;
>     }
> 
>     /* done handling the command */
>     return 0;
> 
> The caller of this code updates pr->cmd_issue to clear the bit for the
> respective command slot. This is missed now, and the later completion
> mentioned in the comment doesn't happen for flushes, the IDE core never
> calls back into the AHCI core for the completion.
> 
> The correct fix might be to call ide_set_inactive() in the flush
> callback, though I haven't checked in detail yet whether there's
> anything specific to DMA read/write in ide_set_inactive().
> 
> Kevin

Any resolution yet? This blocks testing for me.