diff mbox

[2/3] ide: ignore retry_unit check for non-retry operations

Message ID 1466780802-30424-3-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev June 24, 2016, 3:06 p.m. UTC 
From: Evgeny Yakovlev <eyakovlev@virtuozzo.com>

The following sequence of tests discovered a problem in IDE emulation:
1. Send DMA write to IDE device 0
2. Send CMD_FLUSH_CACHE to same IDE device which will be failed by block
layer using blkdebug script in tests/ide-test:test_retry_flush

When doing DMA request ide/core.c will set s->retry_unit to s->unit in
ide_start_dma. When dma completes ide_set_inactive sets retry_unit to -1.
After that ide_flush_cache runs and fails thanks to blkdebug.
ide_flush_cb calls ide_handle_rw_error which asserts that s->retry_unit
== s->unit. But s->retry_unit is still -1 after previous DMA completion
and flush does not use anything related to retry.

This patch restricts retry unit assertion only to ops that actually use
retry logic.

Signed-off-by: Evgeny Yakovlev <eyakovlev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Fam Zheng <famz@redhat.com>
CC: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini June 28, 2016, 8:56 p.m. UTC | #1 
On 24/06/2016 17:06, Denis V. Lunev wrote:
> When doing DMA request ide/core.c will set s->retry_unit to s->unit in
> ide_start_dma. When dma completes ide_set_inactive sets retry_unit to -1.
> After that ide_flush_cache runs and fails thanks to blkdebug.
> ide_flush_cb calls ide_handle_rw_error which asserts that s->retry_unit
> == s->unit. But s->retry_unit is still -1 after previous DMA completion
> and flush does not use anything related to retry.

Wouldn't the assertion fail for a PIO read/write too?  Perhaps
retry_unit should be set to s->unit in ide_transfer_start too.

Paolo
Evgeny Yakovlev June 29, 2016, 8:35 a.m. UTC | #2 
On 28.06.2016 23:56, Paolo Bonzini wrote:
>
> On 24/06/2016 17:06, Denis V. Lunev wrote:
>> When doing DMA request ide/core.c will set s->retry_unit to s->unit in
>> ide_start_dma. When dma completes ide_set_inactive sets retry_unit to -1.
>> After that ide_flush_cache runs and fails thanks to blkdebug.
>> ide_flush_cb calls ide_handle_rw_error which asserts that s->retry_unit
>> == s->unit. But s->retry_unit is still -1 after previous DMA completion
>> and flush does not use anything related to retry.
> Wouldn't the assertion fail for a PIO read/write too?  Perhaps
> retry_unit should be set to s->unit in ide_transfer_start too.

If PIO follows DMA and fails then yes, it looks like it will trigger an 
assert. I am not sure about setting retry_unit in ide_transfer_start. It 
looks like currently only DMA I/O entries touch retry_unit at all. Does 
that mean that PIO, flush, etc do not support retries by design and we 
need to add more exceptions to assert check or is it a real bug in how 
retries are initialized?

>
> Paolo
Paolo Bonzini June 29, 2016, 10:41 a.m. UTC | #3 
On 29/06/2016 10:35, Evgeny Yakovlev wrote:
>>>
>> Wouldn't the assertion fail for a PIO read/write too?  Perhaps
>> retry_unit should be set to s->unit in ide_transfer_start too.
> 
> If PIO follows DMA and fails then yes, it looks like it will trigger an
> assert. I am not sure about setting retry_unit in ide_transfer_start. It
> looks like currently only DMA I/O entries touch retry_unit at all. Does
> that mean that PIO, flush, etc do not support retries by design and we
> need to add more exceptions to assert check or is it a real bug in how
> retries are initialized?

Both PIO and flush do support retries, so I think it is a real bug.

Paolo
diff mbox

Patch

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 029f6b9..17f884b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -779,7 +779,8 @@  int ide_handle_rw_error(IDEState *s, int error, int op)
     BlockErrorAction action = blk_get_error_action(s->blk, is_read, error);
 
     if (action == BLOCK_ERROR_ACTION_STOP) {
-        assert(s->bus->retry_unit == s->unit);
+        assert(!(IS_IDE_RETRY_DMA(op) || IS_IDE_RETRY_PIO(op))
+                || s->bus->retry_unit == s->unit);
         s->bus->error_status = op;
     } else if (action == BLOCK_ERROR_ACTION_REPORT) {
         block_acct_failed(blk_get_stats(s->blk), &s->acct);