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

[Qemu-devel] [PATCH v3] 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..cda6398 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -457,6 +457,8 @@ 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_DMA_RETRY  0x08
+#define BM_STATUS_PIO_RETRY  0x10
 
 #define BM_CMD_START     0x01
 #define BM_CMD_READ      0x08
@@ -488,6 +490,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 +502,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)
@@ -991,8 +996,13 @@ static void ide_sector_write(IDEState *s)
         n = s->req_nb_sectors;
     ret = bdrv_write(s->bs, sector_num, s->io_buffer, n);
     if (ret != 0) {
-	ide_rw_error(s);
-	return;
+        if (ret == -ENOSPC) {
+            s->bmdma->ide_if = s;
+            s->bmdma->status |= BM_STATUS_PIO_RETRY;
+            vm_stop(0);
+        } else
+            ide_rw_error(s);
+        return;
     }
 
     s->nsector -= n;
@@ -1024,6 +1034,20 @@ 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_DMA_RETRY) {
+        bm->status &= ~BM_STATUS_DMA_RETRY;
+        ide_dma_restart(bm->ide_if);
+    } else if (bm->status & BM_STATUS_PIO_RETRY) {
+        bm->status &= ~BM_STATUS_PIO_RETRY;
+        ide_sector_write(bm->ide_if);
+    }
+}
+
 static void ide_write_dma_cb(void *opaque, int ret)
 {
     BMDMAState *bm = opaque;
@@ -1032,8 +1056,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_DMA_RETRY;
+            vm_stop(0);
+        } else
+            ide_dma_error(s);
+        return;
     }
 
     n = s->io_buffer_size >> 9;
@@ -2849,11 +2877,25 @@ 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;
+    bm->dma_cb = ide_write_dma_cb;
+    ide_dma_start(s, bm->dma_cb);
+}
+
 static void ide_dma_cancel(BMDMAState *bm)
 {
     if (bm->status & BM_STATUS_DMAING) {
@@ -3043,6 +3085,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);
 
@@ -3068,9 +3111,14 @@ static void pci_ide_save(QEMUFile* f, void *opaque)
 
     for(i = 0; i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
+        uint8_t ifidx;
         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);
+        ifidx = bm->ide_if ? bm->ide_if - d->ide_if : 0;
+        qemu_put_8s(f, &ifidx);
         /* XXX: if a transfer is pending, we do not save it yet */
     }
 
@@ -3094,7 +3142,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)
     PCIIDEState *d = opaque;
     int ret, i;
 
-    if (version_id != 1)
+    if (version_id != 2)
         return -EINVAL;
     ret = pci_device_load(&d->dev, f);
     if (ret < 0)
@@ -3102,9 +3150,14 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)
 
     for(i = 0; i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
+        uint8_t ifidx;
         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);
+        qemu_get_8s(f, &ifidx);
+        bm->ide_if = &d->ide_if[ifidx];
         /* XXX: if a transfer is pending, we do not save it yet */
     }
 
@@ -3212,7 +3265,7 @@ void pci_cmd646_ide_init(PCIBus *bus, BlockDriverState **hd_table,
     ide_init2(&d->ide_if[0], hd_table[0], hd_table[1], irq[0]);
     ide_init2(&d->ide_if[2], hd_table[2], hd_table[3], irq[1]);
 
-    register_savevm("ide", 0, 1, pci_ide_save, pci_ide_load, d);
+    register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d);
     qemu_register_reset(cmd646_reset, d);
     cmd646_reset(d);
 }
@@ -3269,7 +3322,7 @@ void pci_piix3_ide_init(PCIBus *bus, BlockDriverState **hd_table, int devfn,
     ide_init_ioport(&d->ide_if[0], 0x1f0, 0x3f6);
     ide_init_ioport(&d->ide_if[2], 0x170, 0x376);
 
-    register_savevm("ide", 0, 1, pci_ide_save, pci_ide_load, d);
+    register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d);
 }
 
 /* hd_table must contain 4 block drivers */
@@ -3308,7 +3361,7 @@ void pci_piix4_ide_init(PCIBus *bus, BlockDriverState **hd_table, int devfn,
     ide_init_ioport(&d->ide_if[0], 0x1f0, 0x3f6);
     ide_init_ioport(&d->ide_if[2], 0x170, 0x376);
 
-    register_savevm("ide", 0, 1, pci_ide_save, pci_ide_load, d);
+    register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d);
 }
 
 /***********************************************************/
--
			Gleb.

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

[Ian Jackson]

Gleb Natapov writes ("[Qemu-devel] [PATCH v3] Stop VM on ENOSPC error."):
> And repeat last IDE command after VM restart.

Once again, this feature should be optional.

Personally I would think it should be off by default.  While that's a
holy war, I think it's clear that it needs to be configurable,
probably with a command-line option.

Ian.

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

[Gleb Natapov]

On Mon, Jan 19, 2009 at 11:56:24AM +0000, Ian Jackson wrote:
> Gleb Natapov writes ("[Qemu-devel] [PATCH v3] Stop VM on ENOSPC error."):
> > And repeat last IDE command after VM restart.
> 
> Once again, this feature should be optional.
> 
> Personally I would think it should be off by default.  While that's a
> holy war, I think it's clear that it needs to be configurable,
> probably with a command-line option.
> 
I don't want to participate in this particular holy war. But after the war
is over can somebody (preferably maintainer) update be what the default
should be?

--
			Gleb.

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

[Anthony Liguori]

Ian Jackson wrote:
> Gleb Natapov writes ("[Qemu-devel] [PATCH v3] Stop VM on ENOSPC error."):
>   
>> And repeat last IDE command after VM restart.
>>     
>
> Once again, this feature should be optional.
>   

Why?

I'm not looking to have a long discussion on the merits one way or 
another, but I'd at least like to know why you would want to disable 
this in Xen.

Assuming this comes along with some way to detect when the VM has been 
stopped by this condition programmatically, Xend could simply cont if it 
cared to ignore the error?  I guess it would then get stuck in an 
infinite loop.  But why would you ignore the error in the first place?

Regards,

Anthony Liguori

> Personally I would think it should be off by default.  While that's a
> holy war, I think it's clear that it needs to be configurable,
> probably with a command-line option.
>
> Ian.
>
>
>   

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

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] [PATCH v3] Stop VM on ENOSPC error."):
> Ian Jackson wrote:
> > Once again, this feature should be optional.
> 
> Why?

Well, three reasons, one general and theoretical, and two practical
and rather Xen-specific.

The theoretical reason is that a guest is in a better postion to deal
with the situation because it knows its access patterns.  Often the
response to a failing write in a mission-critical system will be some
kind a fallback behaviour, which is likely to work.  Stopping the VM
unconditionally is not something that the guest can cope with.

The practical reasons are that we would want to retain existing
behaviour unless it was clearly broken (which we don't think it is),
and that we don't currently have any useful mechanism for reporting
and dealing with the problem.

Fundamentally I think we're seeing this different because of the way
that Xen uses qemu is contextually quite different to the
`traditional' qemu.  Traditionally qemu is used as a subprogram of
other tasks, as an interactive debugging or GUI tool, or whatever.

But in the Xen context, a Xen VM is not a `task' in the same way.
(Xen users make much less use of the built-in cow formats for this
reason, often preferring LVM snapshots or even deeper storage magic.)
We expect the VM to be up and stay up and if it can't continue it
needs to fail or crash.

Ian.

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

[Avi Kivity]

Ian Jackson wrote:
> Anthony Liguori writes ("Re: [Qemu-devel] [PATCH v3] Stop VM on ENOSPC error."):
>   
>> Ian Jackson wrote:
>>     
>>> Once again, this feature should be optional.
>>>       
>> Why?
>>     
>
> Well, three reasons, one general and theoretical, and two practical
> and rather Xen-specific.
>   

This has been tried before, but...

> The theoretical reason is that a guest is in a better postion to deal
> with the situation because it knows its access patterns.  Often the
> response to a failing write in a mission-critical system will be some
> kind a fallback behaviour, which is likely to work.  

A situation where many writes fail and many writes succeed is unlikely 
to have been tested and is therefore unlikely to work. Particularly as 
some time afterwards all writes start to succeed again as if nothing has 
happened.

A single disk guest will thrash its disk, eventually remounting it 
read-only (in the case of Linux) and then failing left and right.

A multiple disk guest in a RAID 5 configuration will enter degraded 
mode, and then corrupt data. RAID 5 wasn't designed for multiple disk 
failures. By induction RAID 6 fails as well.

> Stopping the VM
> unconditionally is not something that the guest can cope with.
>   

The guest doesn't need to cope with it; the management system does.

> The practical reasons are that we would want to retain existing
> behaviour unless it was clearly broken (which we don't think it is),
> and that we don't currently have any useful mechanism for reporting
> and dealing with the problem.
>
> Fundamentally I think we're seeing this different because of the way
> that Xen uses qemu is contextually quite different to the
> `traditional' qemu.  Traditionally qemu is used as a subprogram of
> other tasks, as an interactive debugging or GUI tool, or whatever.
>
> But in the Xen context, a Xen VM is not a `task' in the same way.
> (Xen users make much less use of the built-in cow formats for this
> reason, often preferring LVM snapshots or even deeper storage magic.)
> We expect the VM to be up and stay up and if it can't continue it
> needs to fail or crash

You can resume the guest over the monitor (or xenstore if you insist) 
once more storage is allocated, same as everyone else. I don't see how 
qemu's role in Xen makes a difference.

The only alternative I see to stopping the VM is to offline the disk for 
both reads and writes. This at least protects data, and is similar to 
controller or cable failure which guests may have been tested with. An 
advantage is that if an unimportant disk fails, the guest can continue 
to work.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Gleb Natapov]

On Mon, Jan 19, 2009 at 05:22:04PM +0000, Ian Jackson wrote:
> But in the Xen context, a Xen VM is not a `task' in the same way.
> (Xen users make much less use of the built-in cow formats for this
> reason, often preferring LVM snapshots or even deeper storage magic.)
> We expect the VM to be up and stay up and if it can't continue it
> needs to fail or crash.
> 
Even if you can add storage space dynamically with LVM you prefer VM to
crash on ENOSPC?

--
			Gleb.

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

[Gerd Hoffmann]

Ian Jackson wrote:
> Anthony Liguori writes ("Re: [Qemu-devel] [PATCH v3] Stop VM on ENOSPC error."):
>> Ian Jackson wrote:
>>> Once again, this feature should be optional.
>> Why?
> 
> Well, three reasons, one general and theoretical, and two practical
> and rather Xen-specific.
> 
> The theoretical reason is that a guest is in a better postion to deal
> with the situation because it knows its access patterns.  Often the
> response to a failing write in a mission-critical system will be some
> kind a fallback behaviour, which is likely to work.  Stopping the VM
> unconditionally is not something that the guest can cope with.

The fundamental issue is that you can't signal ENOSPC to the guest via
IDE protocol because that is an error condition which simply can't
happen on real hardware.  You can only signal EIO, which is something
very different, and the OS likely goes into "Oops, disk broken" mode.
Which probably isn't what you want here ...

It might make sense to signal ENOSPC to the guest with a paravirtual
disk.  Then the guest knows what really happened has at least the chance
to do something more reasonable in response, although I suspect most of
the guests out there today wouldn't.

> But in the Xen context, a Xen VM is not a `task' in the same way.
> (Xen users make much less use of the built-in cow formats for this
> reason, often preferring LVM snapshots or even deeper storage magic.)

With LVM volumes as storage you shouldn't run into ENOSPC error
conditions in the first place, and the patch should be a no-op for you.

cheers,
  Gerd

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

[Gleb Natapov]

On Tue, Jan 20, 2009 at 10:25:04AM +0100, Gerd Hoffmann wrote:
> Ian Jackson wrote:
> > Anthony Liguori writes ("Re: [Qemu-devel] [PATCH v3] Stop VM on ENOSPC error."):
> >> Ian Jackson wrote:
> >>> Once again, this feature should be optional.
> >> Why?
> > 
> > Well, three reasons, one general and theoretical, and two practical
> > and rather Xen-specific.
> > 
> > The theoretical reason is that a guest is in a better postion to deal
> > with the situation because it knows its access patterns.  Often the
> > response to a failing write in a mission-critical system will be some
> > kind a fallback behaviour, which is likely to work.  Stopping the VM
> > unconditionally is not something that the guest can cope with.
> 
> The fundamental issue is that you can't signal ENOSPC to the guest via
> IDE protocol because that is an error condition which simply can't
> happen on real hardware.  You can only signal EIO, which is something
> very different, and the OS likely goes into "Oops, disk broken" mode.
> Which probably isn't what you want here ...
> 
Windows using IDE retries DMA 3 times and then moves to PIO mode. And it
stays in PIO mode even after reboot. The only way to return to DMA mode
again is to reinstall the driver.

--
			Gleb.

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

[Carl-Daniel Hailfinger]

On 20.01.2009 10:35, Gleb Natapov wrote:
> On Tue, Jan 20, 2009 at 10:25:04AM +0100, Gerd Hoffmann wrote:
>   
>> Ian Jackson wrote:
>>     
>>> Anthony Liguori writes ("Re: [Qemu-devel] [PATCH v3] Stop VM on ENOSPC error."):
>>>       
>>>> Ian Jackson wrote:
>>>>         
>>>>> Once again, this feature should be optional.
>>>>>           
>>>> Why?
>>>>         
>>> Well, three reasons, one general and theoretical, and two practical
>>> and rather Xen-specific.
>>>
>>> The theoretical reason is that a guest is in a better postion to deal
>>> with the situation because it knows its access patterns.  Often the
>>> response to a failing write in a mission-critical system will be some
>>> kind a fallback behaviour, which is likely to work.  Stopping the VM
>>> unconditionally is not something that the guest can cope with.
>>>       
>> The fundamental issue is that you can't signal ENOSPC to the guest via
>> IDE protocol because that is an error condition which simply can't
>> happen on real hardware.  You can only signal EIO, which is something
>> very different, and the OS likely goes into "Oops, disk broken" mode.
>> Which probably isn't what you want here ...
>>
>>     
> Windows using IDE retries DMA 3 times and then moves to PIO mode. And it
> stays in PIO mode even after reboot. The only way to return to DMA mode
> again is to reinstall the driver.
>   

You can fix this in the registry without any reinstallation by setting
the bitmask of allowed IDE modes back to 0xffffffff. It's a single value
that needs to be changed. http://winhlp.com/node/10 has more info and a
script to reset the bitmask.
There's also an alternative for automatically resetting the mode after a
successful access: http://support.microsoft.com/kb/817472

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

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

[Gleb Natapov]

On Tue, Jan 20, 2009 at 11:44:57AM +0100, Carl-Daniel Hailfinger wrote:
> On 20.01.2009 10:35, Gleb Natapov wrote:
> > On Tue, Jan 20, 2009 at 10:25:04AM +0100, Gerd Hoffmann wrote:
> >   
> >> Ian Jackson wrote:
> >>     
> >>> Anthony Liguori writes ("Re: [Qemu-devel] [PATCH v3] Stop VM on ENOSPC error."):
> >>>       
> >>>> Ian Jackson wrote:
> >>>>         
> >>>>> Once again, this feature should be optional.
> >>>>>           
> >>>> Why?
> >>>>         
> >>> Well, three reasons, one general and theoretical, and two practical
> >>> and rather Xen-specific.
> >>>
> >>> The theoretical reason is that a guest is in a better postion to deal
> >>> with the situation because it knows its access patterns.  Often the
> >>> response to a failing write in a mission-critical system will be some
> >>> kind a fallback behaviour, which is likely to work.  Stopping the VM
> >>> unconditionally is not something that the guest can cope with.
> >>>       
> >> The fundamental issue is that you can't signal ENOSPC to the guest via
> >> IDE protocol because that is an error condition which simply can't
> >> happen on real hardware.  You can only signal EIO, which is something
> >> very different, and the OS likely goes into "Oops, disk broken" mode.
> >> Which probably isn't what you want here ...
> >>
> >>     
> > Windows using IDE retries DMA 3 times and then moves to PIO mode. And it
> > stays in PIO mode even after reboot. The only way to return to DMA mode
> > again is to reinstall the driver.
> >   
> 
> You can fix this in the registry without any reinstallation by setting
> the bitmask of allowed IDE modes back to 0xffffffff. It's a single value
> that needs to be changed. http://winhlp.com/node/10 has more info and a
> script to reset the bitmask.
Doesn't really matter how you re-enable DMA mode again, the fact is it
needs admin attention. Reinstalling the driver also not so hard. One
mouse click.

> There's also an alternative for automatically resetting the mode after a
> successful access: http://support.microsoft.com/kb/817472
> 
After 3 _consecutive_ errors IDE will still move to PIO mode and stay
there till admin intervention. And with ENOSPC you can be sure you'll
get 3 consecutive errors.

--
			Gleb.

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

[Vincent Hanquez]

On Mon, Jan 19, 2009 at 05:22:04PM +0000, Ian Jackson wrote:
> The theoretical reason is that a guest is in a better postion to deal
> with the situation because it knows its access patterns.  Often the
> response to a failing write in a mission-critical system will be some
> kind a fallback behaviour, which is likely to work.  Stopping the VM
> unconditionally is not something that the guest can cope with.

The guest isn't in a better position to do something here.
In a first place, the guest see a "hardware" disk of size X, that the
host is providing. the host need to do what is necessary to provide it
(or at least try to). the guest has absolutely no idea about the fact
this harddisk isn't a true harddisk or that it should be treated
specially (expecting ENOSPC at any point).

> But in the Xen context, a Xen VM is not a `task' in the same way.
> (Xen users make much less use of the built-in cow formats for this
> reason, often preferring LVM snapshots or even deeper storage magic.)
> We expect the VM to be up and stay up and if it can't continue it
> needs to fail or crash.

That's *your* opinion. as long as there's a mechanism to notify the
management part, so that it can try to do something, pausing the VM
seems sensible to me.

-- 
Vincent