Re: [Qemu-block] [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks"

[Paolo Bonzini]

On 30/07/19 11:54, Philippe Mathieu-Daudé wrote:
> Hi John,
> 
> On 7/30/19 12:36 AM, John Snow wrote:
>> This reverts commit 0d910cfeaf2076b116b4517166d5deb0fea76394.
>>
>> It's not correct to just ignore an error code in a callback; we need to
>> handle that error and possible report failure to the guest so that they
>> don't wait indefinitely for an operation that will now never finish.
> 
> Is this 4.1 material? It looks so.

Perhaps could have been last week, but now I suggest Cc qemu-stable and
delaying it to 4.2.

Paolo

>> This ought to help cases reported by Nutanix where iSCSI returns a
>> legitimate -ECANCELED for certain operations which should be propagated
>> normally.
>>
>> Reported-by: Shaju Abraham <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  hw/ide/ahci.c |  3 ---
>>  hw/ide/core.c | 14 --------------
>>  2 files changed, 17 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 00ba422a48..6aaf66534a 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -1023,9 +1023,6 @@ static void ncq_cb(void *opaque, int ret)
>>      IDEState *ide_state = &ncq_tfs->drive->port.ifs[0];
>>  
>>      ncq_tfs->aiocb = NULL;
>> -    if (ret == -ECANCELED) {
>> -        return;
>> -    }
>>  
>>      if (ret < 0) {
>>          bool is_read = ncq_tfs->cmd == READ_FPDMA_QUEUED;
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 6afadf894f..8e1624f7ce 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -722,9 +722,6 @@ static void ide_sector_read_cb(void *opaque, int ret)
>>      s->pio_aiocb = NULL;
>>      s->status &= ~BUSY_STAT;
>>  
>> -    if (ret == -ECANCELED) {
>> -        return;
>> -    }
>>      if (ret != 0) {
>>          if (ide_handle_rw_error(s, -ret, IDE_RETRY_PIO |
>>                                  IDE_RETRY_READ)) {
>> @@ -840,10 +837,6 @@ static void ide_dma_cb(void *opaque, int ret)
>>      uint64_t offset;
>>      bool stay_active = false;
>>  
>> -    if (ret == -ECANCELED) {
>> -        return;
>> -    }
>> -
>>      if (ret == -EINVAL) {
>>          ide_dma_error(s);
>>          return;
>> @@ -975,10 +968,6 @@ static void ide_sector_write_cb(void *opaque, int ret)
>>      IDEState *s = opaque;
>>      int n;
>>  
>> -    if (ret == -ECANCELED) {
>> -        return;
>> -    }
>> -
>>      s->pio_aiocb = NULL;
>>      s->status &= ~BUSY_STAT;
>>  
>> @@ -1058,9 +1047,6 @@ static void ide_flush_cb(void *opaque, int ret)
>>  
>>      s->pio_aiocb = NULL;
>>  
>> -    if (ret == -ECANCELED) {
>> -        return;
>> -    }
>>      if (ret < 0) {
>>          /* XXX: What sector number to set here? */
>>          if (ide_handle_rw_error(s, -ret, IDE_RETRY_FLUSH)) {
>>