[Qemu-devel] [PATCH] ide: cleanup warnings

[Qemu-devel] [PATCH] ide: cleanup warnings

[Andrea Arcangeli]

Hello,

This is just a minor cleanup adding \n.

On a side note, it was good idea to keep it under DEBUG_IDE because if
iscsi server reboots or goes down, this would generate a flood of
errors if it's enabled (a flood of these warnings would have been
shown with DEBUG_IDE on in such a condition).

So I've also been wondering if it's safe to ignore these warnings when
the iscsi server is down. The error is delivered up to ide which
simulates a I/O error for the guest (something like
BM_STATUS_PIO_RETRY), so then it should be up to the guest OS to retry
long enough and deal with it without corrupting the guest filesystem
image. Often local filesystems in the guest (ext4/brfs/xfs etc...)
won't be heavily tested for I/O errors. So it would be somewhat safer
to retry on the qemu side without passing errors up to the guest, OTOH
the guest ide driver might eventually notice a DMA timeout if the I/O
doesn't complete in a timely fashion, so retrying indefinitely in qemu
aio layer wouldn't be a definitive solution to avoid triggering guest
os bugs... So in the end this is probably the best way we can handle
the iscsi server disconnect with ide.

Maybe we can deal with iscsi server going down in a safer way by
retrying in the host (transparently to the guest and unnoticed to the
local fs in the guest) with virtio-blk as we have full control the
guest driver (we don't control the guest ide driver instead)? OTOH
that may lead to indefinite guest I/O hangs which may not be nice for
the guest either if iscsi never returns up (though it would diminish
the chance of triggering guest fs bugs in not well tested I/O error
paths).

===
Subject: ide: cleanup warnings

From: Andrea Arcangeli <aarcange@redhat.com>

Add \n.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 hw/ide/pci.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -298,9 +298,9 @@ void bmdma_cmd_writeb(void *opaque, uint
                 qemu_aio_flush();
 #ifdef DEBUG_IDE
                 if (bm->bus->dma->aiocb)
-                    printf("ide_dma_cancel: aiocb still pending");
+                    printf("ide_dma_cancel: aiocb still pending\n");
                 if (bm->status & BM_STATUS_DMAING)
-                    printf("ide_dma_cancel: BM_STATUS_DMAING still pending");
+                    printf("ide_dma_cancel: BM_STATUS_DMAING still pending\n");
 #endif
             }
         } else {

Re: [Qemu-devel] [PATCH] ide: cleanup warnings

[Kevin Wolf]

Am 03.05.2011 22:03, schrieb Andrea Arcangeli:
> Hello,
> 
> This is just a minor cleanup adding \n.

Thanks, applied to the block branch.

> On a side note, it was good idea to keep it under DEBUG_IDE because if
> iscsi server reboots or goes down, this would generate a flood of
> errors if it's enabled (a flood of these warnings would have been
> shown with DEBUG_IDE on in such a condition).

Isn't it a bug that qemu_aio_flush() doesn't clear aiocb/status? Should
we move the ide_set_inactive() call from ide_dma_error to ide_dma_cb?

> So I've also been wondering if it's safe to ignore these warnings when
> the iscsi server is down. The error is delivered up to ide which
> simulates a I/O error for the guest (something like
> BM_STATUS_PIO_RETRY), so then it should be up to the guest OS to retry
> long enough and deal with it without corrupting the guest filesystem
> image. Often local filesystems in the guest (ext4/brfs/xfs etc...)
> won't be heavily tested for I/O errors. So it would be somewhat safer
> to retry on the qemu side without passing errors up to the guest, OTOH
> the guest ide driver might eventually notice a DMA timeout if the I/O
> doesn't complete in a timely fashion, so retrying indefinitely in qemu
> aio layer wouldn't be a definitive solution to avoid triggering guest
> os bugs... So in the end this is probably the best way we can handle
> the iscsi server disconnect with ide.

This is actually something controlled by werror. With werror=stop, the
VM is stopped and the guest never sees the error, but qemu retries. With
werror=report, the error is reported to the guest which can retry.

Kevin

Re: [Qemu-devel] [PATCH] ide: cleanup warnings

[Andrea Arcangeli]

On Wed, May 04, 2011 at 10:08:12AM +0200, Kevin Wolf wrote:
> Isn't it a bug that qemu_aio_flush() doesn't clear aiocb/status? Should
> we move the ide_set_inactive() call from ide_dma_error to ide_dma_cb?

How would that make a difference, it's still running in aio context,
running it a bit earlier won't move the needle? I think it's more
likely an error path currently not covered by ide_set_inactive that
may have to be covered. It doesn't seem fatal but I tend to agree if
we can make that warning go away without putting it under #ifdef like
usptream, we should do that too.

Maybe something like this will make it go away?

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 90f553b..b81f1d7 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -377,6 +377,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num)
 
 static void ide_rw_error(IDEState *s) {
     ide_abort_command(s);
+    ide_set_inactive(s);
     ide_set_irq(s->bus);
 }
 

Re: [Qemu-devel] [PATCH] ide: cleanup warnings

[Kevin Wolf]

Am 04.05.2011 15:41, schrieb Andrea Arcangeli:
> On Wed, May 04, 2011 at 10:08:12AM +0200, Kevin Wolf wrote:
>> Isn't it a bug that qemu_aio_flush() doesn't clear aiocb/status? Should
>> we move the ide_set_inactive() call from ide_dma_error to ide_dma_cb?
> 
> How would that make a difference, it's still running in aio context,
> running it a bit earlier won't move the needle?

Yes, sorry, you're right. I was thinking of the werror=stop case, but
this isn't your case and ide_set_inactive would even be wrong there.

> I think it's more
> likely an error path currently not covered by ide_set_inactive that
> may have to be covered. It doesn't seem fatal but I tend to agree if
> we can make that warning go away without putting it under #ifdef like
> usptream, we should do that too.
> 
> Maybe something like this will make it go away?
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 90f553b..b81f1d7 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -377,6 +377,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num)
>  
>  static void ide_rw_error(IDEState *s) {
>      ide_abort_command(s);
> +    ide_set_inactive(s);
>      ide_set_irq(s->bus);
>  }

No, this looks wrong. ide_rw_error is only used for PIO, and
ide_set_inactive() resets the DMA status.

I can't see how you could leave ide_dma_cb without either scheduling
another AIO request or setting aiocb = NULL in ide_set_inactive. I guess
I need to reproduce this and do some debugging...

Kevin

Re: [Qemu-devel] [PATCH] ide: cleanup warnings

[Andrea Arcangeli]

On Wed, May 04, 2011 at 03:57:28PM +0200, Kevin Wolf wrote:
> I can't see how you could leave ide_dma_cb without either scheduling
> another AIO request or setting aiocb = NULL in ide_set_inactive. I guess
> I need to reproduce this and do some debugging...

That would be nice to be sure. This has been triggering on a older
codebase, it's uncertain if this would happen with current code
too. The \n so far was the only obvious improvement I could make to
the upstream code, until we understand better what's going on...

The good thing is it doesn't seem to cause problems, other than the
printf. The leftover in the dma->aiocb likely gets overwritten when
guest retries without failure.

Re: [Qemu-devel] [PATCH] ide: cleanup warnings

[Kevin Wolf]

Am 04.05.2011 16:04, schrieb Andrea Arcangeli:
> On Wed, May 04, 2011 at 03:57:28PM +0200, Kevin Wolf wrote:
>> I can't see how you could leave ide_dma_cb without either scheduling
>> another AIO request or setting aiocb = NULL in ide_set_inactive. I guess
>> I need to reproduce this and do some debugging...
> 
> That would be nice to be sure. This has been triggering on a older
> codebase, it's uncertain if this would happen with current code
> too. The \n so far was the only obvious improvement I could make to
> the upstream code, until we understand better what's going on...
> 
> The good thing is it doesn't seem to cause problems, other than the
> printf. The leftover in the dma->aiocb likely gets overwritten when
> guest retries without failure.

Right. I'm not so much concerned about aiocb (though it would certainly
be cleaner if we cleared it; I think it's checked in more places), but
more about bm->status which is a guest-visible register.

On the other hand we store things in bm->status that are definitely not
meant to be guest visible, so the emulation of this register is broken
anyway. Fixing all of this might turn out to be hard if you don't want
to break migration.

Kevin