ide: cleanup warnings

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 <[email protected]>

Add \n.

---
 hw/ide/pci.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

Am 03.05.2011 22:03, schrieb Andrea Arcangeli:

Thanks, applied to the block branch.

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?

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

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?

index 90f553b..b81f1d7 100644
@@ -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);
 }
 

Am 04.05.2011 15:41, schrieb Andrea Arcangeli:

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.

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.

Am 04.05.2011 16:04, schrieb Andrea Arcangeli:

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