diff mbox

ide: cleanup warnings

Message ID 20110503200339.GA7838@random.random
State New
Headers show

Commit Message

Andrea Arcangeli May 3, 2011, 8:03 p.m. UTC 
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).

Comments

Kevin Wolf May 4, 2011, 8:08 a.m. UTC | #1 
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
diff mbox

Patch

===
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 {