[Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Gleb Natapov]

And repeat last IDE command after VM restart.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/hw/ide.c b/hw/ide.c
index 7dd41f7..36be9f0 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -457,6 +457,7 @@ static inline int media_is_cd(IDEState *s)
 #define BM_STATUS_DMAING 0x01
 #define BM_STATUS_ERROR  0x02
 #define BM_STATUS_INT    0x04
+#define BM_STATUS_RETRY  0x08
 
 #define BM_CMD_START     0x01
 #define BM_CMD_READ      0x08
@@ -488,6 +489,8 @@ typedef struct BMDMAState {
     IDEState *ide_if;
     BlockDriverCompletionFunc *dma_cb;
     BlockDriverAIOCB *aiocb;
+    int64_t sector_num;
+    uint32_t nsector;
 } BMDMAState;
 
 typedef struct PCIIDEState {
@@ -498,6 +501,7 @@ typedef struct PCIIDEState {
 } PCIIDEState;
 
 static void ide_dma_start(IDEState *s, BlockDriverCompletionFunc *dma_cb);
+static void ide_dma_restart(IDEState *s);
 static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret);
 
 static void padstr(char *str, const char *src, int len)
@@ -1024,6 +1028,17 @@ static void ide_sector_write(IDEState *s)
     }
 }
 
+static void ide_dma_restart_cb(void *opaque, int running)
+{
+    BMDMAState *bm = opaque;
+    if (!running)
+        return;
+    if (bm->status & BM_STATUS_RETRY) {
+        bm->status &= ~BM_STATUS_RETRY;
+        ide_dma_restart(bm->ide_if);
+    }
+}
+
 static void ide_write_dma_cb(void *opaque, int ret)
 {
     BMDMAState *bm = opaque;
@@ -1032,8 +1047,12 @@ static void ide_write_dma_cb(void *opaque, int ret)
     int64_t sector_num;
 
     if (ret < 0) {
-	ide_dma_error(s);
-	return;
+        if (ret == -ENOSPC) {
+            bm->status |= BM_STATUS_RETRY;
+            vm_stop(0);
+        } else
+            ide_dma_error(s);
+        return;
     }
 
     n = s->io_buffer_size >> 9;
@@ -2849,11 +2868,24 @@ static void ide_dma_start(IDEState *s, BlockDriverCompletionFunc *dma_cb)
     bm->cur_prd_last = 0;
     bm->cur_prd_addr = 0;
     bm->cur_prd_len = 0;
+    bm->sector_num = ide_get_sector(s);
+    bm->nsector = s->nsector;
     if (bm->status & BM_STATUS_DMAING) {
         bm->dma_cb(bm, 0);
     }
 }
 
+static void ide_dma_restart(IDEState *s)
+{
+    BMDMAState *bm = s->bmdma;
+    ide_set_sector(s, bm->sector_num);
+    s->io_buffer_index = 0;
+    s->io_buffer_size = 0;
+    s->nsector = bm->nsector;
+    bm->cur_addr = bm->addr;
+    ide_dma_start(s, bm->dma_cb);
+}
+
 static void ide_dma_cancel(BMDMAState *bm)
 {
     if (bm->status & BM_STATUS_DMAING) {
@@ -3043,6 +3075,7 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
         d->ide_if[2 * i].bmdma = bm;
         d->ide_if[2 * i + 1].bmdma = bm;
         bm->pci_dev = (PCIIDEState *)pci_dev;
+        qemu_add_vm_change_state_handler(ide_dma_restart_cb, bm);
 
         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);
 
@@ -3071,6 +3104,8 @@ static void pci_ide_save(QEMUFile* f, void *opaque)
         qemu_put_8s(f, &bm->cmd);
         qemu_put_8s(f, &bm->status);
         qemu_put_be32s(f, &bm->addr);
+        qemu_put_sbe64s(f, &bm->sector_num);
+        qemu_put_be32s(f, &bm->nsector);
         /* XXX: if a transfer is pending, we do not save it yet */
     }
 
@@ -3105,6 +3140,8 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)
         qemu_get_8s(f, &bm->cmd);
         qemu_get_8s(f, &bm->status);
         qemu_get_be32s(f, &bm->addr);
+        qemu_get_sbe64s(f, &bm->sector_num);
+        qemu_get_be32s(f, &bm->nsector);
         /* XXX: if a transfer is pending, we do not save it yet */
     }
 
--
			Gleb.

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Daniel P. Berrange]

On Wed, Jan 14, 2009 at 02:03:58PM +0200, Gleb Natapov wrote:
> And repeat last IDE command after VM restart.

How is a management application to determine when this occurs ?
It is not desirable to poll for whether the VM is stopped all
the time, and we need a reliable way to detect this, otherwise
the user and/or mgmt app will just think their VM has hung. 

Thus I'd suggest we need an async notification of this event,
and only enable this behaviour if the app controlling QEMU has
explicitly enabled this notification / feature.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Dor Laor]

Daniel P. Berrange wrote:
> On Wed, Jan 14, 2009 at 02:03:58PM +0200, Gleb Natapov wrote:
>   
>> And repeat last IDE command after VM restart.
>>     
>
> How is a management application to determine when this occurs ?
> It is not desirable to poll for whether the VM is stopped all
> the time, and we need a reliable way to detect this, otherwise
> the user and/or mgmt app will just think their VM has hung. 
>
> Thus I'd suggest we need an async notification of this event,
> and only enable this behaviour if the app controlling QEMU has
> explicitly enabled this notification / feature.
>
>   
Correct. There is async message for it (==printf...).
Once the libmonitor is introduced it will be real async event
> Daniel
>   

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Gleb Natapov]

On Wed, Jan 14, 2009 at 02:20:20PM +0200, Dor Laor wrote:
> Daniel P. Berrange wrote:
>> On Wed, Jan 14, 2009 at 02:03:58PM +0200, Gleb Natapov wrote:
>>   
>>> And repeat last IDE command after VM restart.
>>>     
>>
>> How is a management application to determine when this occurs ?
>> It is not desirable to poll for whether the VM is stopped all
>> the time, and we need a reliable way to detect this, otherwise
>> the user and/or mgmt app will just think their VM has hung. 
>>
>> Thus I'd suggest we need an async notification of this event,
>> and only enable this behaviour if the app controlling QEMU has
>> explicitly enabled this notification / feature.
>>
>>   
> Correct. There is async message for it (==printf...).
> Once the libmonitor is introduced it will be real async event
There is not printf in this version of the patch, but I can add it if
needed.

--
			Gleb.

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Kevin Wolf]

Gleb Natapov schrieb:
> On Wed, Jan 14, 2009 at 02:20:20PM +0200, Dor Laor wrote:
>> Daniel P. Berrange wrote:
>>> On Wed, Jan 14, 2009 at 02:03:58PM +0200, Gleb Natapov wrote:
>>>   
>>>> And repeat last IDE command after VM restart.
>>>>     
>>> How is a management application to determine when this occurs ?
>>> It is not desirable to poll for whether the VM is stopped all
>>> the time, and we need a reliable way to detect this, otherwise
>>> the user and/or mgmt app will just think their VM has hung. 
>>>
>>> Thus I'd suggest we need an async notification of this event,
>>> and only enable this behaviour if the app controlling QEMU has
>>> explicitly enabled this notification / feature.
>>>
>>>   
>> Correct. There is async message for it (==printf...).
>> Once the libmonitor is introduced it will be real async event
> There is not printf in this version of the patch, but I can add it if
> needed.

I think you should add it. Not only because of management applications
but also for the user. I for one would be at least a bit confused if my
VM stopped automatically without saying anything about the reason.

Kevin

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Jamie Lokier]

Daniel P. Berrange wrote:
> Thus I'd suggest we need an async notification of this event,
> and only enable this behaviour if the app controlling QEMU has
> explicitly enabled this notification / feature.

I think the behaviour should always be enabled (unless explicitly
disabled, but I'm not sure why you'd want to do that).

A corrupt VM with data loss sounds much worse than a stopped VM to me.

If the VM is in some critical server role, you probably have a
watchdog or something pinging it anyway, same as real hardware.

-- Jamie

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Jamie Lokier]

Gleb Natapov wrote:
> And repeat last IDE command after VM restart.

If there are multiple AIOs in flight when you get ENOSPC, you may need
to repeat all of them.

-- Jamie

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Gleb Natapov]

On Wed, Jan 14, 2009 at 04:47:17PM +0000, Jamie Lokier wrote:
> Gleb Natapov wrote:
> > And repeat last IDE command after VM restart.
> 
> If there are multiple AIOs in flight when you get ENOSPC, you may need
> to repeat all of them.
> 
IDE does not support multiple outstanding commands. For SCSI that will have
to be done though. What my patch is missing is PIO mode support, but this
should be easy to add.

--
			Gleb.

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Daniel P. Berrange]

On Wed, Jan 14, 2009 at 04:46:17PM +0000, Jamie Lokier wrote:
> Daniel P. Berrange wrote:
> > Thus I'd suggest we need an async notification of this event,
> > and only enable this behaviour if the app controlling QEMU has
> > explicitly enabled this notification / feature.
> 
> I think the behaviour should always be enabled (unless explicitly
> disabled, but I'm not sure why you'd want to do that).
> 
> A corrupt VM with data loss sounds much worse than a stopped VM to me.

You're not corrupting data in current code - you're just unable to finish
new writes, because an IO failure is propagated back to the guest. If the
guest is properly checking for & handling I/O failures, it should be pretty
much OK once the host space problem is resolved - perhaps a reboot + journal
recovery. 

Older QEMU certainly had catastrophic data loss on ENOSPC due to not sending
any I/O errors back to the guest, so it thought its write had succeeded when
in fact it had been thrown away. Current QEMU is more careful about error
propagation now.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Wed, Jan 14, 2009 at 04:46:17PM +0000, Jamie Lokier wrote:
>   
>> Daniel P. Berrange wrote:
>>     
>>> Thus I'd suggest we need an async notification of this event,
>>> and only enable this behaviour if the app controlling QEMU has
>>> explicitly enabled this notification / feature.
>>>       
>> I think the behaviour should always be enabled (unless explicitly
>> disabled, but I'm not sure why you'd want to do that).
>>
>> A corrupt VM with data loss sounds much worse than a stopped VM to me.
>>     
>
> You're not corrupting data in current code - you're just unable to finish
> new writes, because an IO failure is propagated back to the guest. If the
> guest is properly checking for & handling I/O failures, it should be pretty
> much OK once the host space problem is resolved - perhaps a reboot + journal
> recovery. 
>   

Not at all.  When the guest gets an IO error, it's going to try and mark 
the sector bad and move on.  If it does do a journal recover on reboot, 
you're even more screwed because writes will randomly fail.  Writes to 
pre-allocated storage will succeed but unallocated storage will fail.

The guest has no awareness into this error scenario so there's nothing 
that it can reasonably do to recover.

> Older QEMU certainly had catastrophic data loss on ENOSPC due to not sending
> any I/O errors back to the guest, so it thought its write had succeeded when
> in fact it had been thrown away. Current QEMU is more careful about error
> propagation now.
>   

But the error propagation in the event of ENOSPC is totally wrong.  Try 
it out and your guest will corrupt itself.  It's even more catastrophic 
with qcow2 but that shouldn't be surprising at this point.

Regards,

Anthony Liguori

> Daniel
>   

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Jamie Lokier]

Daniel P. Berrange wrote:
> > A corrupt VM with data loss sounds much worse than a stopped VM to me.
> 
> You're not corrupting data in current code - you're just unable to finish
> new writes, because an IO failure is propagated back to the guest. If the
> guest is properly checking for & handling I/O failures, it should be pretty
> much OK once the host space problem is resolved - perhaps a reboot + journal
> recovery. 

Think about journalling filesystem writes.  If one returns I/O error,
some following queued requests must not proceed, otherwise it puts the
filesystem in an inconsistent state.  That's what I mean by corruption.

What that in mind, please name any OS which properly checks for and
handles write I/O errors, aside from reporting EIO to apps.  I'm
pretty sure Linux is not among them.

(Heck, even ENOSPC isn't handled well in apps.  Why, I remember the
time Firefox corrupted by Bookmarks by storing a zero-length file due
to ENOSPC...  I remember a few times when after Make I had zero-length
.o object files, and we're not talking about my personal crappy
Makefiles but good quality ones, etc.  This seems to be quite common.)

-- Jamie

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Jamie Lokier]

Gleb Natapov wrote:
> On Wed, Jan 14, 2009 at 04:47:17PM +0000, Jamie Lokier wrote:
> > Gleb Natapov wrote:
> > > And repeat last IDE command after VM restart.
> > 
> > If there are multiple AIOs in flight when you get ENOSPC, you may need
> > to repeat all of them.
> > 
> IDE does not support multiple outstanding commands. For SCSI that will have
> to be done though. What my patch is missing is PIO mode support, but this
> should be easy to add.

For some reason I imagined IDE with TCQ(sp?), or SATA with NCQ :-)

What about multiple IDE controllers.  If the VM is stopped (for any
reason, not just ENOSPC) and resumed later, do _other_ controllers'
AIOs in flight need to be replayed?  Are they saved in snapshot state?

-- Jamie

Re: [Qemu-devel] [PATCH] Stop VM on ENOSPC error

[Gleb Natapov]

On Wed, Jan 14, 2009 at 06:39:55PM +0000, Jamie Lokier wrote:
> Gleb Natapov wrote:
> > On Wed, Jan 14, 2009 at 04:47:17PM +0000, Jamie Lokier wrote:
> > > Gleb Natapov wrote:
> > > > And repeat last IDE command after VM restart.
> > > 
> > > If there are multiple AIOs in flight when you get ENOSPC, you may need
> > > to repeat all of them.
> > > 
> > IDE does not support multiple outstanding commands. For SCSI that will have
> > to be done though. What my patch is missing is PIO mode support, but this
> > should be easy to add.
> 
> For some reason I imagined IDE with TCQ(sp?), or SATA with NCQ :-)
> 
But we don't support it yet.

> What about multiple IDE controllers.  If the VM is stopped (for any
> reason, not just ENOSPC) and resumed later, do _other_ controllers'
> AIOs in flight need to be replayed?  Are they saved in snapshot state?
> 
I think (not sure) vmstop stops only CPU execution, but all outstanding IOs
will complete.

--
			Gleb.