ide: Set BSY bit during FLUSH

PATCH Applied Kevin Wolf 5 participants 10 messages Jun 5, 2013 – Jul 10, 2013 Block layer

lore

Share & tools

lore

Kevin Wolf

author

2013-06-05 13:18 UTC #0 lore raw

v2 cover ide: Set BSY bit during FLUSH

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(-)

Signature · 1 line

1.8.1.4

Kevin Wolf

author

2013-06-05 13:18 UTC #1 lore raw

PATCH v2 1/4 blkdebug: Add BLKDBG_FLUSH_TO_OS/DISK events

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c               | 8 ++++----
 block/blkdebug.c      | 3 +++
 include/block/block.h | 3 +++
 3 files changed, 10 insertions(+), 4 deletions(-)

block.c +4 -4

index 3f616de..79ad33d 100644
@@ -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) {

block/blkdebug.c +3 -0

index 71f99e4..ccb627a 100644
@@ -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)

include/block/block.h +3 -0

index dc5b388..2307f67 100644
@@ -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;

Signature · 1 line

1.8.1.4

Kevin Wolf

author

2013-06-05 13:18 UTC #2 lore raw

PATCH v2 2/4 ide-test: Add enum value for DEV

In reply to Kevin Wolf

Get rid of the magic number.

Signed-off-by Kevin Wolf <[email protected]>

---
 tests/ide-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/ide-test.c +2 -1

index 365e995..1c31a2e 100644
@@ -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);

Signature · 1 line

1.8.1.4

Kevin Wolf

author

2013-06-05 13:18 UTC #3 lore raw

PATCH v2 3/4 ide: Set BSY bit during FLUSH

In reply to Kevin Wolf

From: Andreas Färber <[email protected]>

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.

Signed-off-by Andreas Färber <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)

hw/ide/core.c +1 -0

index c7a8041..9926d92 100644
@@ -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);
 }

Signature · 1 line

1.8.1.4

Alex Williamson

2013-07-03 20:02 UTC #4 lore raw

Quoted 25 lines · Kevin Wolf

From: Andreas Färber <[email protected]>
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) <[email protected]>
Signed-off-by Andreas Färber <[email protected]>
Signed-off-by Kevin Wolf <[email protected]>
---
 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)
hw/ide/core.c +0 -0
index c7a8041..9926d92 100644

hw/ide/core.c void ide_flush_cache(IDEState *s) · line 820

Quoted 7 lines

hw/ide/core.c +1 -0

@@ -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

Kevin Wolf

author

2013-07-04 07:55 UTC #5 lore raw

Am 03.07.2013 um 22:02 hat Alex Williamson geschrieben:

Quoted 38 lines · Kevin Wolf

Quoted 25 lines
From: Andreas Färber <[email protected]>
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) <[email protected]>
Signed-off-by Andreas Färber <[email protected]>
Signed-off-by Kevin Wolf <[email protected]>
---
 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)
hw/ide/core.c +0 -0
index c7a8041..9926d92 100644
hw/ide/core.c void ide_flush_cache(IDEState *s) · line 820
Quoted 7 lines
hw/ide/core.c +1 -0
@@ -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.

Quoted 3 lines

-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

Michael S. Tsirkin

2013-07-10 06:49 UTC #6 lore raw

Quoted 62 lines · Kevin Wolf

Am 03.07.2013 um 22:02 hat Alex Williamson geschrieben:
Quoted 38 lines · Kevin Wolf
Quoted 25 lines
From: Andreas Färber <[email protected]>
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) <[email protected]>
Signed-off-by Andreas Färber <[email protected]>
Signed-off-by Kevin Wolf <[email protected]>
---
 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)
hw/ide/core.c +0 -0
index c7a8041..9926d92 100644
hw/ide/core.c void ide_flush_cache(IDEState *s) · line 820
Quoted 7 lines
hw/ide/core.c +1 -0
@@ -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.
Quoted 3 lines
-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.

Quoted 5 lines

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.

Kevin Wolf

author

2013-06-05 13:18 UTC #7 lore raw

PATCH v2 4/4 ide-test: Add FLUSH CACHE test case

In reply to Kevin Wolf

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

Signed-off-by Kevin Wolf <[email protected]>

---
 tests/ide-test.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

tests/ide-test.c +40 -0

index 1c31a2e..828e71a 100644
@@ -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 */

Signature · 1 line

1.8.1.4

Stefan Hajnoczi

2013-06-06 08:57 UTC #8 lore raw

In reply to Kevin Wolf

Quoted 15 lines · 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

Quoted 6 lines

 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

Stefan Hajnoczi

Applied

2013-06-06 09:44 UTC #9 lore raw

In reply to Kevin Wolf

Quoted 15 lines · 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

Quoted 9 lines

 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(-)

Signature · 1 line

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