[Qemu-devel] [REPOST] [PATCH 0/2] IDE error checking

[Qemu-devel] [REPOST] [PATCH 0/2] IDE error checking

[Ian Jackson]

In these two patches I make the IDE controller emulation pay attention
to errors returned by the bdrv functions.

The first of these patches was first posted in February 2008 and has
been discussed extensively.

There was some discussion as to whether it would be better, in cases
where the error was ENOSPC, to stop the guest than to pass it an IDE
error.  However it must be clear that regardless of whether that would
be better in some circumstances, giving the guest an IDE error is
better than silently ignoring the error, throwing away the guest's
data.

I would therefore submit that my patch should be applied immediately.
If anyone wants to implement automatic stopping of guests on ENOSPC
errors this should then be done as an optional enhancement.

In this patch pair I don't fix the call to bdrv_flush in FLUSH CACHE.
This is because that's entangled with another dropped series of
patches to implement use of aio for cache flushing and guest control
of the host write cache.

Ian.

[Qemu-devel] [PATCH 1/2] Actually check read/write errors when doing IDE DMA and IDE PIO writes

[Ian Jackson]

This patch makes the ide emulation actually take notice of
error returns from bdrv_write and bdrv_aio_{read,write}.

(Cherry picked from qemu-xen e0e7a0afe0e324a1f7d64c240f567b15dbe454cf,
 first posted to qemu-devel Wed, 20 Feb 2008 15:26:41 +0000)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/ide.c |   34 ++++++++++++++++++++++++++++++++--
 1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c
index 1e60591..cddc0d5 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -817,6 +817,11 @@ static void ide_set_sector(IDEState *s, int64_t sector_num)
     }
 }
 
+static void ide_rw_error(IDEState *s) {
+    ide_abort_command(s);
+    ide_set_irq(s);
+}
+
 static void ide_sector_read(IDEState *s)
 {
     int64_t sector_num;
@@ -836,6 +841,10 @@ static void ide_sector_read(IDEState *s)
         if (n > s->req_nb_sectors)
             n = s->req_nb_sectors;
         ret = bdrv_read(s->bs, sector_num, s->io_buffer, n);
+        if (ret != 0) {
+            ide_rw_error(s);
+            return;
+        }
         ide_transfer_start(s, s->io_buffer, 512 * n, ide_sector_read);
         ide_set_irq(s);
         ide_set_sector(s, sector_num + n);
@@ -843,6 +852,14 @@ static void ide_sector_read(IDEState *s)
     }
 }
 
+static void ide_dma_error(IDEState *s)
+{
+    ide_transfer_stop(s);
+    s->error = ABRT_ERR;
+    s->status = READY_STAT | ERR_STAT;
+    ide_set_irq(s);
+}
+
 /* return 0 if buffer completed */
 static int dma_buf_rw(BMDMAState *bm, int is_write)
 {
@@ -891,7 +908,6 @@ static int dma_buf_rw(BMDMAState *bm, int is_write)
     return 1;
 }
 
-/* XXX: handle errors */
 static void ide_read_dma_cb(void *opaque, int ret)
 {
     BMDMAState *bm = opaque;
@@ -899,6 +915,11 @@ static void ide_read_dma_cb(void *opaque, int ret)
     int n;
     int64_t sector_num;
 
+    if (ret < 0) {
+	ide_dma_error(s);
+	return;
+    }
+
     n = s->io_buffer_size >> 9;
     sector_num = ide_get_sector(s);
     if (n > 0) {
@@ -963,6 +984,11 @@ static void ide_sector_write(IDEState *s)
     if (n > s->req_nb_sectors)
         n = s->req_nb_sectors;
     ret = bdrv_write(s->bs, sector_num, s->io_buffer, n);
+    if (ret != 0) {
+	ide_rw_error(s);
+	return;
+    }
+
     s->nsector -= n;
     if (s->nsector == 0) {
         /* no more sectors to write */
@@ -992,7 +1018,6 @@ static void ide_sector_write(IDEState *s)
     }
 }
 
-/* XXX: handle errors */
 static void ide_write_dma_cb(void *opaque, int ret)
 {
     BMDMAState *bm = opaque;
@@ -1000,6 +1025,11 @@ static void ide_write_dma_cb(void *opaque, int ret)
     int n;
     int64_t sector_num;
 
+    if (ret < 0) {
+	ide_dma_error(s);
+	return;
+    }
+
     n = s->io_buffer_size >> 9;
     sector_num = ide_get_sector(s);
     if (n > 0) {
-- 
1.4.4.4

[Qemu-devel] [PATCH 2/2] [ide] Check that asynchronous (DMA) submission succeeds

[Ian Jackson]

If it does not, abort the command immediately rather than dropping
it on the floor.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/ide.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c
index cddc0d5..fd3a055 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -737,6 +737,13 @@ static inline void ide_abort_command(IDEState *s)
     s->status = READY_STAT | ERR_STAT;
     s->error = ABRT_ERR;
 }
+static inline void ide_dma_submit_check(IDEState *s,
+          BlockDriverCompletionFunc *dma_cb, BMDMAState *bm)
+{
+    if (bm->aiocb)
+	return;
+    dma_cb(bm, -1);
+}
 
 static inline void ide_set_irq(IDEState *s)
 {
@@ -954,6 +961,7 @@ static void ide_read_dma_cb(void *opaque, int ret)
 #endif
     bm->aiocb = bdrv_aio_read(s->bs, sector_num, s->io_buffer, n,
                               ide_read_dma_cb, bm);
+    ide_dma_submit_check(s, ide_read_dma_cb, bm);
 }
 
 static void ide_sector_read_dma(IDEState *s)
@@ -1065,6 +1073,7 @@ static void ide_write_dma_cb(void *opaque, int ret)
 #endif
     bm->aiocb = bdrv_aio_write(s->bs, sector_num, s->io_buffer, n,
                                ide_write_dma_cb, bm);
+    ide_dma_submit_check(s, ide_write_dma_cb, bm);
 }
 
 static void ide_sector_write_dma(IDEState *s)
-- 
1.4.4.4

Re: [Qemu-devel] [REPOST] [PATCH 0/2] IDE error checking

[Anthony Liguori]

Ian Jackson wrote:
> In these two patches I make the IDE controller emulation pay attention
> to errors returned by the bdrv functions.
>
> The first of these patches was first posted in February 2008 and has
> been discussed extensively.
>
> There was some discussion as to whether it would be better, in cases
> where the error was ENOSPC, to stop the guest than to pass it an IDE
> error.  However it must be clear that regardless of whether that would
> be better in some circumstances, giving the guest an IDE error is
> better than silently ignoring the error, throwing away the guest's
> data.
>   

The proper solution is not significantly more difficult.  Just add a 
switch(errno) { case ENOSPC: vm_stop(0); term_printf("Ran out of disk 
space\n"); break; }.  That's all I'm asking for.

Regards,

Anthony Liguori

> I would therefore submit that my patch should be applied immediately.
> If anyone wants to implement automatic stopping of guests on ENOSPC
> errors this should then be done as an optional enhancement.
>
> In this patch pair I don't fix the call to bdrv_flush in FLUSH CACHE.
> This is because that's entangled with another dropped series of
> patches to implement use of aio for cache flushing and guest control
> of the host write cache.
>
> Ian.
>
>
>   

Re: [Qemu-devel] [REPOST] [PATCH 0/2] IDE error checking

[andrzej zaborowski]

2008/9/7 Anthony Liguori <anthony@codemonkey.ws>:
> Ian Jackson wrote:
>>
>> In these two patches I make the IDE controller emulation pay attention
>> to errors returned by the bdrv functions.
>>
>> The first of these patches was first posted in February 2008 and has
>> been discussed extensively.
>>
>> There was some discussion as to whether it would be better, in cases
>> where the error was ENOSPC, to stop the guest than to pass it an IDE
>> error.  However it must be clear that regardless of whether that would
>> be better in some circumstances, giving the guest an IDE error is
>> better than silently ignoring the error, throwing away the guest's
>> data.
>>
>
> The proper solution is not significantly more difficult.  Just add a
> switch(errno) { case ENOSPC: vm_stop(0); term_printf("Ran out of disk
> space\n"); break; }.  That's all I'm asking for.

I will commit these two patches if there are no further comments /
iterations to them.  There's been enough discussion about the issue
and this is definitely an improvement, there can be perhaps further
improvements later.

I will not commit a patch that does the vm_stop(0) thing because I
don't see a reason hard discs should be treated differently than other
peripherals.  Any peripheral being physically distroyed is a critical
situation to the OS and the OS should be left to decide what to do
with it.  Furthermore a ENOSPC doesn't destroy all the capability of
the emulated drive, it's just a disc that lost the capability to write
changes.  It should report write errors.  It should continue to read.
Note that the disc isn't any more critical than other peripherals to
the system, there are diskless systems, with possibly hotpluggable
storage, there are VMs using many images in different formats some of
which can't return an ENOSPC.

I submitted a patch to report IDE errors in early 2006 when I lost
data due to lack of write error reporting and there's no change till
today.

Cheers