* [Qemu-devel] [PATCH 1/3] blkdebug: Add BLKDBG_FLUSH_TO_OS/DISK events
2013-05-29 11:34 [Qemu-devel] [PATCH 0/3] ide: Set BSY bit during FLUSH Kevin Wolf
@ 2013-05-29 11:34 ` Kevin Wolf
2013-05-29 11:34 ` [Qemu-devel] [PATCH 2/3] ide: Set BSY bit during FLUSH Kevin Wolf
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2013-05-29 11:34 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha
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 3f87489..52a2101 100644
--- a/block.c
+++ b/block.c
@@ -3307,13 +3307,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,
@@ -4326,6 +4324,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) {
@@ -4338,6 +4337,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 1251c5c..ce350c7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -449,6 +449,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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/3] ide: Set BSY bit during FLUSH
2013-05-29 11:34 [Qemu-devel] [PATCH 0/3] ide: Set BSY bit during FLUSH Kevin Wolf
2013-05-29 11:34 ` [Qemu-devel] [PATCH 1/3] blkdebug: Add BLKDBG_FLUSH_TO_OS/DISK events Kevin Wolf
@ 2013-05-29 11:34 ` Kevin Wolf
2013-05-29 11:50 ` Andreas Färber
2013-05-29 11:34 ` [Qemu-devel] [PATCH 3/3] ide-test: Add FLUSH CACHE test case Kevin Wolf
2013-05-30 7:25 ` [Qemu-devel] [PATCH 0/3] ide: Set BSY bit during FLUSH Stefan Hajnoczi
3 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2013-05-29 11:34 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha
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 | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index c7a8041..bf1ff18 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -795,6 +795,8 @@ static void ide_flush_cb(void *opaque, int ret)
{
IDEState *s = opaque;
+ s->status &= ~BUSY_STAT;
+
if (ret < 0) {
/* XXX: What sector number to set here? */
if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
@@ -814,6 +816,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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] ide: Set BSY bit during FLUSH
2013-05-29 11:34 ` [Qemu-devel] [PATCH 2/3] ide: Set BSY bit during FLUSH Kevin Wolf
@ 2013-05-29 11:50 ` Andreas Färber
2013-05-29 12:46 ` Kevin Wolf
0 siblings, 1 reply; 8+ messages in thread
From: Andreas Färber @ 2013-05-29 11:50 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha
Am 29.05.2013 13:34, schrieb 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 | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c7a8041..bf1ff18 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -795,6 +795,8 @@ static void ide_flush_cb(void *opaque, int ret)
> {
> IDEState *s = opaque;
>
> + s->status &= ~BUSY_STAT;
> +
> if (ret < 0) {
> /* XXX: What sector number to set here? */
> if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
Didn't you want this hunk to be dropped?
But thanks for picking the patch up already.
Andreas
> @@ -814,6 +816,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);
> }
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] ide: Set BSY bit during FLUSH
2013-05-29 11:50 ` Andreas Färber
@ 2013-05-29 12:46 ` Kevin Wolf
0 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2013-05-29 12:46 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, stefanha
Am 29.05.2013 um 13:50 hat Andreas Färber geschrieben:
> Am 29.05.2013 13:34, schrieb 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 | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index c7a8041..bf1ff18 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -795,6 +795,8 @@ static void ide_flush_cb(void *opaque, int ret)
> > {
> > IDEState *s = opaque;
> >
> > + s->status &= ~BUSY_STAT;
> > +
> > if (ret < 0) {
> > /* XXX: What sector number to set here? */
> > if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
>
> Didn't you want this hunk to be dropped?
>
> But thanks for picking the patch up already.
Ah, right, dropped it now. Thanks.
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 3/3] ide-test: Add FLUSH CACHE test case
2013-05-29 11:34 [Qemu-devel] [PATCH 0/3] ide: Set BSY bit during FLUSH Kevin Wolf
2013-05-29 11:34 ` [Qemu-devel] [PATCH 1/3] blkdebug: Add BLKDBG_FLUSH_TO_OS/DISK events Kevin Wolf
2013-05-29 11:34 ` [Qemu-devel] [PATCH 2/3] ide: Set BSY bit during FLUSH Kevin Wolf
@ 2013-05-29 11:34 ` Kevin Wolf
2013-05-30 7:24 ` Stefan Hajnoczi
2013-05-30 7:25 ` [Qemu-devel] [PATCH 0/3] ide: Set BSY bit during FLUSH Stefan Hajnoczi
3 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2013-05-29 11:34 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha
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 365e995..5744462 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -76,6 +76,7 @@ enum {
enum {
CMD_READ_DMA = 0xc8,
CMD_WRITE_DMA = 0xca,
+ CMD_FLUSH_CACHE = 0xe7,
CMD_IDENTIFY = 0xec,
CMDF_ABORT = 0x100,
@@ -423,6 +424,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':'__org.qemu.debug-qemu-io-command', 'arguments': { "
+ "'device': 'ide0-hd0', 'command': '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':'__org.qemu.debug-qemu-io-command', 'arguments': { "
+ "'device': 'ide0-hd0', 'command': 'resume A'} }");
+
+ /* Check registers */
+ data = inb(IDE_BASE + reg_device);
+ g_assert_cmpint(data & 0x10, ==, 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();
@@ -453,6 +491,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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] ide: Set BSY bit during FLUSH
2013-05-29 11:34 [Qemu-devel] [PATCH 0/3] ide: Set BSY bit during FLUSH Kevin Wolf
` (2 preceding siblings ...)
2013-05-29 11:34 ` [Qemu-devel] [PATCH 3/3] ide-test: Add FLUSH CACHE test case Kevin Wolf
@ 2013-05-30 7:25 ` Stefan Hajnoczi
3 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2013-05-30 7:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, afaerber
On Wed, May 29, 2013 at 01:34:03PM +0200, Kevin Wolf wrote:
> The test case depends on the qemu-io series I sent yesterday.
> ("[PATCH 00/16] Make qemu-io commands available in the monitor")
>
> Andreas Färber (1):
> ide: Set BSY bit during FLUSH
>
> Kevin Wolf (2):
> blkdebug: Add BLKDBG_FLUSH_TO_OS/DISK events
> ide-test: Add FLUSH CACHE test case
>
> block.c | 8 ++++----
> block/blkdebug.c | 3 +++
> hw/ide/core.c | 3 +++
> include/block/block.h | 3 +++
> tests/ide-test.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 53 insertions(+), 4 deletions(-)
Looks good. I really like the test case.
Posted a minor comment on the test case.
Stefan
^ permalink raw reply [flat|nested] 8+ messages in thread