RFC/PATCH: hdparm tunable IDE write cache for HVM

RFC/PATCH: hdparm tunable IDE write cache for HVM

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

qemu 0.8.2 has a flush callback to the storage backends, so now it is
possible to implement hdparm tunable IDE write cache enable/disable for
guest domains, allowing people to pick speed or data consistency on a
case by case basis.

As an added benefit, really large LBA48 IOs will now no longer be broken
up into smaller IOs on the host side.

What do you think?

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

[-- Attachment #2: ioemu-writecache --]
[-- Type: text/plain, Size: 4803 bytes --]

--- tools/ioemu/hw/ide.c.hdparm	2006-08-11 02:16:43.000000000 -0400
+++ tools/ioemu/hw/ide.c	2006-08-11 02:23:33.000000000 -0400
@@ -305,6 +305,7 @@ typedef struct IDEState {
     PCIDevice *pci_dev;
     struct BMDMAState *bmdma;
     int drive_serial;
+    int write_cache;
     /* ide regs */
     uint8_t feature;
     uint8_t error;
@@ -788,6 +789,9 @@ static void ide_sector_write(IDEState *s
         ide_transfer_start(s, s->io_buffer, 512 * n1, ide_sector_write);
     }
     ide_set_sector(s, sector_num + n);
+
+    if (!s->write_cache)
+        bdrv_flush(s->bs);
     
 #ifdef TARGET_I386
     if (win2k_install_hack && ((++s->irq_count % 16) == 0)) {
@@ -863,6 +867,10 @@ static int ide_write_dma_cb(IDEState *s,
         transfer_size -= len;
         phys_addr += len;
     }
+    /* Ensure the data hit disk before telling the guest OS so. */
+    if (!s->write_cache)
+        bdrv_flush(s->bs);
+
     return transfer_size1 - transfer_size;
 }
 
@@ -1672,7 +1680,15 @@ static void ide_ioport_write(void *opaqu
             /* XXX: valid for CDROM ? */
             switch(s->feature) {
             case 0x02: /* write cache enable */
+                s->write_cache = 1;
+                s->status = READY_STAT | SEEK_STAT;
+                ide_set_irq(s);
+                break;
             case 0x82: /* write cache disable */
+                s->write_cache = 0;
+                s->status = READY_STAT | SEEK_STAT;
+                ide_set_irq(s);
+                break;
             case 0xaa: /* read look-ahead enable */
             case 0x55: /* read look-ahead disable */
                 s->status = READY_STAT | SEEK_STAT;
@@ -2090,6 +2106,7 @@ static void ide_init2(IDEState *ide_stat
         s->irq = irq;
         s->sector_write_timer = qemu_new_timer(vm_clock, 
                                                ide_sector_write_timer_cb, s);
+        s->write_cache = 0;
         ide_reset(s);
     }
 }
--- tools/ioemu/block-bochs.c.hdparm	2006-08-11 02:24:04.000000000 -0400
+++ tools/ioemu/block-bochs.c	2006-08-11 02:24:27.000000000 -0400
@@ -91,7 +91,7 @@ static int bochs_open(BlockDriverState *
     int fd, i;
     struct bochs_header bochs;
 
-    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE | O_SYNC);
+    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
     if (fd < 0) {
         fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
         if (fd < 0)
--- tools/ioemu/block.c.hdparm	2006-08-11 02:24:04.000000000 -0400
+++ tools/ioemu/block.c	2006-08-11 02:24:33.000000000 -0400
@@ -685,7 +685,7 @@ static int raw_open(BlockDriverState *bs
     int rv;
 #endif
 
-    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE | O_SYNC);
+    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
     if (fd < 0) {
         fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
         if (fd < 0)
--- tools/ioemu/block-cloop.c.hdparm	2006-08-11 02:24:04.000000000 -0400
+++ tools/ioemu/block-cloop.c	2006-08-11 02:24:39.000000000 -0400
@@ -55,7 +55,7 @@ static int cloop_open(BlockDriverState *
     BDRVCloopState *s = bs->opaque;
     uint32_t offsets_size,max_compressed_block_size=1,i;
 
-    s->fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE | O_SYNC);
+    s->fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
     if (s->fd < 0)
         return -1;
     bs->read_only = 1;
--- tools/ioemu/block-cow.c.hdparm	2006-08-11 02:24:04.000000000 -0400
+++ tools/ioemu/block-cow.c	2006-08-11 02:24:44.000000000 -0400
@@ -69,7 +69,7 @@ static int cow_open(BlockDriverState *bs
     struct cow_header_v2 cow_header;
     int64_t size;
 
-    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE | O_SYNC);
+    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
     if (fd < 0) {
         fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
         if (fd < 0)
--- tools/ioemu/block-qcow.c.hdparm	2006-08-11 02:24:04.000000000 -0400
+++ tools/ioemu/block-qcow.c	2006-08-11 02:24:49.000000000 -0400
@@ -95,7 +95,7 @@ static int qcow_open(BlockDriverState *b
     int fd, len, i, shift;
     QCowHeader header;
     
-    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE | O_SYNC);
+    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
     if (fd < 0) {
         fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
         if (fd < 0)
--- tools/ioemu/block-vmdk.c.hdparm	2006-08-11 02:24:05.000000000 -0400
+++ tools/ioemu/block-vmdk.c	2006-08-11 02:24:54.000000000 -0400
@@ -96,7 +96,7 @@ static int vmdk_open(BlockDriverState *b
     uint32_t magic;
     int l1_size;
 
-    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE | O_SYNC);
+    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
     if (fd < 0) {
         fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
         if (fd < 0)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: RFC/PATCH: hdparm tunable IDE write cache for HVM

[Christian Limpach]

On 8/11/06, Rik van Riel <riel@redhat.com> wrote:
> qemu 0.8.2 has a flush callback to the storage backends, so now it is
> possible to implement hdparm tunable IDE write cache enable/disable for
> guest domains, allowing people to pick speed or data consistency on a
> case by case basis.
>
> As an added benefit, really large LBA48 IOs will now no longer be broken
> up into smaller IOs on the host side.
>
> What do you think?

Looks good, are you confident that you have identified all the places
where a flush is needed?  Doesn't qemu do flushes in the first place
and wouldn't it then have the same issues?

    christian

Re: RFC/PATCH: hdparm tunable IDE write cache for HVM

[Rik van Riel]

Christian Limpach wrote:
> On 8/11/06, Rik van Riel <riel@redhat.com> wrote:
>> qemu 0.8.2 has a flush callback to the storage backends, so now it is
>> possible to implement hdparm tunable IDE write cache enable/disable for
>> guest domains, allowing people to pick speed or data consistency on a
>> case by case basis.
>>
>> As an added benefit, really large LBA48 IOs will now no longer be broken
>> up into smaller IOs on the host side.
>>
>> What do you think?
> 
> Looks good, are you confident that you have identified all the places
> where a flush is needed? 

Yes, bdrv_write() is only called from 2 places in ide.c, and
both of them have a bdrv_flush() call now.

> Doesn't qemu do flushes in the first place
> and wouldn't it then have the same issues?

Nope, qemu does not seem to do any flushes by itself...

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: RFC/PATCH: hdparm tunable IDE write cache for HVM

[Christian Limpach]

On 8/11/06, Rik van Riel <riel@redhat.com> wrote:
> qemu 0.8.2 has a flush callback to the storage backends, so now it is
> possible to implement hdparm tunable IDE write cache enable/disable for
> guest domains, allowing people to pick speed or data consistency on a
> case by case basis.
>
> As an added benefit, really large LBA48 IOs will now no longer be broken
> up into smaller IOs on the host side.
>
> What do you think?

Checked in.  Please add a Signed-off-by line in the future.

    christian