QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks"
@ 2019-07-29 22:36 John Snow
  2019-07-30  9:54 ` Philippe Mathieu-Daudé
  2019-08-13 22:48 ` John Snow
  0 siblings, 2 replies; 6+ messages in thread
From: John Snow @ 2019-07-29 22:36 UTC (permalink / raw)
  To: qemu-block, qemu-devel
  Cc: Paolo Bonzini, Shaju Abraham, John Snow, qemu-stable

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.

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 <shaju.abraham@nutanix.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 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)) {
-- 
2.21.0



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks"
  2019-07-29 22:36 [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks" John Snow
@ 2019-07-30  9:54 ` Philippe Mathieu-Daudé
  2019-07-30 11:37   ` Paolo Bonzini
  2019-07-30 12:50   ` John Snow
  2019-08-13 22:48 ` John Snow
  1 sibling, 2 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-30  9:54 UTC (permalink / raw)
  To: John Snow, qemu-block, qemu-devel, Stefan Hajnoczi
  Cc: Fam Zheng, Paolo Bonzini, Shaju Abraham, qemu-stable

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.

> 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 <shaju.abraham@nutanix.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  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)) {
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks"
  2019-07-30  9:54 ` Philippe Mathieu-Daudé
@ 2019-07-30 11:37   ` Paolo Bonzini
  2019-07-30 12:47     ` Philippe Mathieu-Daudé
  2019-07-30 12:50   ` John Snow
  1 sibling, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2019-07-30 11:37 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé,
	John Snow, qemu-block, qemu-devel, Stefan Hajnoczi
  Cc: Fam Zheng, Shaju Abraham, qemu-stable

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 <shaju.abraham@nutanix.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  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)) {
>>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks"
  2019-07-30 11:37   ` Paolo Bonzini
@ 2019-07-30 12:47     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-30 12:47 UTC (permalink / raw)
  To: Paolo Bonzini, John Snow, qemu-block, qemu-devel, Stefan Hajnoczi
  Cc: Fam Zheng, Shaju Abraham, qemu-stable

On 7/30/19 1:37 PM, Paolo Bonzini wrote:
> 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.

I was wondering because Kevin was preparing a block@ pull request.

Anyway this seems fair because the reverted commit is dated from 2014.

Thanks,

Phil.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks"
  2019-07-30  9:54 ` Philippe Mathieu-Daudé
  2019-07-30 11:37   ` Paolo Bonzini
@ 2019-07-30 12:50   ` John Snow
  1 sibling, 0 replies; 6+ messages in thread
From: John Snow @ 2019-07-30 12:50 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-block, qemu-devel, Stefan Hajnoczi
  Cc: Fam Zheng, Paolo Bonzini, Shaju Abraham, qemu-stable



On 7/30/19 5:54 AM, 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.
> 

Nah. This reverts a commit from 2014. I have no valid argument for why
this is critical :)

Thanks for checking!

--js


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks"
  2019-07-29 22:36 [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks" John Snow
  2019-07-30  9:54 ` Philippe Mathieu-Daudé
@ 2019-08-13 22:48 ` John Snow
  1 sibling, 0 replies; 6+ messages in thread
From: John Snow @ 2019-08-13 22:48 UTC (permalink / raw)
  To: qemu-block, qemu-devel; +Cc: Paolo Bonzini, Shaju Abraham, qemu-stable



On 7/29/19 6:36 PM, 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.
> 
> 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 <shaju.abraham@nutanix.com>
> Signed-off-by: John Snow <jsnow@redhat.com>

Nobody's yelling, so this is getting staged on my IDE branch, alongside
Paolo's dma-helpers fix.

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-29 22:36 [Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks" John Snow
2019-07-30  9:54 ` Philippe Mathieu-Daudé
2019-07-30 11:37   ` Paolo Bonzini
2019-07-30 12:47     ` Philippe Mathieu-Daudé
2019-07-30 12:50   ` John Snow
2019-08-13 22:48 ` John Snow

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox