Open disk images with O_DIRECT

PATCH Superseded Laurent Vivier 4 participants 12 messages Nov 28, 2007 – Nov 29, 2007

lore

Share & tools

lore

Laurent Vivier

author

2007-11-28 14:03 UTC #0 lore raw

cover Open disk images with O_DIRECT

These patches allow to open file using O_DIRECT and bypass the host I/O cache.

[PATCH 1/2] Add "directio" parameter to "-drive"

Using "directio=on" with "-drive" will open the disk image file using "O_DIRECT".

[PATCH 2/2] Direct IDE I/O

This patch enhances the "-drive ,directio=on" mode with IDE drive emulation by removing the buffer used in the IDE emulation.

Laurent

Laurent Vivier

author

2007-11-28 14:03 UTC #1 lore raw

PATCH 1/2 Add "directio" parameter to "-drive"

This patch add a new parameter to "-drive"

Using "directio=on" with "-drive" will open the disk image file using "O_DIRECT".

By default, "directio" is set to "off".

example:

"-drive file=my_disk.qcow2,directio=on"

---
 block-raw.c    |    4 ++++
 block.c        |    2 +-
 block.h        |    1 +
 hw/fdc.c       |    6 +++++-
 hw/ide.c       |    8 ++++++--
 hw/scsi-disk.c |    3 ++-
 hw/sd.c        |   10 +++++++++-
 vl.c           |   26 +++++++++++++++++++++++---
 8 files changed, 51 insertions(+), 9 deletions(-)

block-raw.c +4 -0

Index: qemu/block-raw.c
===================================================================
@@ -107,6 +107,8 @@ static int raw_open(BlockDriverState *bs
     }
     if (flags & BDRV_O_CREAT)
         open_flags |= O_CREAT | O_TRUNC;
+    if (flags & BDRV_O_DIRECT)
+        open_flags |= O_DIRECT;
 
     s->type = FTYPE_FILE;

@@ -660,6 +662,8 @@ static int hdev_open(BlockDriverState *b
         open_flags |= O_RDONLY;
         bs->read_only = 1;
     }
+    if (flags & BDRV_O_DIRECT)
+        open_flags |= O_DIRECT;
 
     s->type = FTYPE_FILE;
 #if defined(__linux__)

block.h +1 -0

Index: qemu/block.h
===================================================================
@@ -44,6 +44,7 @@ typedef struct QEMUSnapshotInfo {
                                      use a disk image format on top of
                                      it (default for
                                      bdrv_file_open()) */
+#define BDRV_O_DIRECT      0x0020
 
 #ifndef QEMU_IMG
 void bdrv_info(void);

vl.c +23 -3

Index: qemu/vl.c
===================================================================
@@ -4847,8 +4847,11 @@ static int drive_init(const char *str, i
     BlockDriverState *bdrv;
     int max_devs;
     int index;
+    int directio;
+    int bdrv_flags;
     char *params[] = { "bus", "unit", "if", "index", "cyls", "heads",
-                       "secs", "trans", "media", "snapshot", "file", NULL };
+                       "secs", "trans", "media", "snapshot", "file",
+                       "directio", NULL };
 
     if (check_params(buf, sizeof(buf), params, str) < 0) {
          fprintf(stderr, "qemu: unknowm parameter '%s' in '%s'\n",

@@ -4862,6 +4865,7 @@ static int drive_init(const char *str, i
     unit_id = -1;
     translation = BIOS_ATA_TRANSLATION_AUTO;
     index = -1;
+    directio = 0;
 
     if (!strcmp(machine->name, "realview") ||
         !strcmp(machine->name, "SS-5") ||

@@ -5001,6 +5005,17 @@ static int drive_init(const char *str, i
 	}
     }
 
+    if (get_param_value(buf, sizeof(buf), "directio", str)) {
+        if (!strcmp(buf, "off"))
+            directio = 0;
+        else if (!strcmp(buf, "on"))
+            directio = 1;
+        else {
+	    fprintf(stderr, "qemu: invalid directio option\n");
+	    return -1;
+        }
+    }
+
     get_param_value(file, sizeof(file), "file", str);
 
     /* compute bus and unit according index */

@@ -5088,8 +5103,12 @@ static int drive_init(const char *str, i
     }
     if (!file[0])
         return 0;
-    if (bdrv_open(bdrv, file, snapshot ? BDRV_O_SNAPSHOT : 0) < 0 ||
-        qemu_key_check(bdrv, file)) {
+    bdrv_flags = 0;
+    if (snapshot)
+        bdrv_flags |= BDRV_O_SNAPSHOT;
+    if (directio)
+        bdrv_flags |= BDRV_O_DIRECT;
+    if (bdrv_open(bdrv, file, bdrv_flags) < 0 || qemu_key_check(bdrv, file)) {
         fprintf(stderr, "qemu: could not open disk image %s\n",
                         file);
         return -1;

@@ -7434,6 +7453,7 @@ static void help(int exitcode)
            "-cdrom file     use 'file' as IDE cdrom image (cdrom is ide1 master)\n"
 	   "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][index=i]\n"
            "       [,cyls=c,heads=h,secs=s[,trans=t]][snapshot=on|off]\n"
+           "       [directio=on|off]\n"
 	   "                use 'file' as a drive image\n"
            "-mtdblock file  use 'file' as on-board Flash memory image\n"
            "-sd file        use 'file' as SecureDigital card image\n"

hw/ide.c +6 -2

Index: qemu/hw/ide.c
===================================================================
@@ -365,7 +365,7 @@ typedef struct IDEState {
     EndTransferFunc *end_transfer_func;
     uint8_t *data_ptr;
     uint8_t *data_end;
-    uint8_t io_buffer[MAX_MULT_SECTORS*512 + 4];
+    uint8_t *io_buffer;
     QEMUTimer *sector_write_timer; /* only used for win2k install hack */
     uint32_t irq_count; /* counts IRQs when using win2k install hack */
     /* CF-ATA extended error */

@@ -2372,11 +2372,14 @@ struct partition {
 static int guess_disk_lchs(IDEState *s,
                            int *pcylinders, int *pheads, int *psectors)
 {
-    uint8_t buf[512];
+    uint8_t *buf;
     int ret, i, heads, sectors, cylinders;
     struct partition *p;
     uint32_t nr_sects;
 
+    ret = posix_memalign((void**)&buf, 0x200, 512);
+    if (ret < 0)
+        return -1;
     ret = bdrv_read(s->bs, 0, buf, 1);
     if (ret < 0)
         return -1;

@@ -2420,6 +2423,7 @@ static void ide_init2(IDEState *ide_stat
 
     for(i = 0; i < 2; i++) {
         s = ide_state + i;
+        posix_memalign((void**)&s->io_buffer, 0x200, MAX_MULT_SECTORS*512 + 4);
         if (i == 0)
             s->bs = hd0;
         else

block.c +1 -1

Index: qemu/block.c
===================================================================
@@ -380,7 +380,7 @@ int bdrv_open2(BlockDriverState *bs, con
     /* Note: for compatibility, we open disk image files as RDWR, and
        RDONLY as fallback */
     if (!(flags & BDRV_O_FILE))
-        open_flags = BDRV_O_RDWR;
+        open_flags = BDRV_O_RDWR | (flags & BDRV_O_DIRECT);
     else
         open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
     ret = drv->bdrv_open(bs, filename, open_flags);

hw/scsi-disk.c +2 -1

Index: qemu/hw/scsi-disk.c
===================================================================
@@ -46,7 +46,7 @@ typedef struct SCSIRequest {
     int sector_count;
     /* The amounnt of data in the buffer.  */
     int buf_len;
-    uint8_t dma_buf[SCSI_DMA_BUF_SIZE];
+    uint8_t *dma_buf;
     BlockDriverAIOCB *aiocb;
     struct SCSIRequest *next;
 } SCSIRequest;

@@ -78,6 +78,7 @@ static SCSIRequest *scsi_new_request(SCS
         free_requests = r->next;
     } else {
         r = qemu_malloc(sizeof(SCSIRequest));
+        posix_memalign(&r->dma_buf, 0x200, SCSI_DMA_BUF_SIZE);
     }
     r->dev = s;
     r->tag = tag;

hw/sd.c +9 -1

Index: qemu/hw/sd.c
===================================================================
@@ -1284,10 +1284,16 @@ int sd_do_command(SDState *sd, struct sd
 static void sd_blk_read(BlockDriverState *bdrv,
                 void *data, uint32_t addr, uint32_t len)
 {
-    uint8_t buf[512];
+    uint8_t *buf;
     uint32_t end = addr + len;
 
+    if (posix_memalign(&buf, 512, 512) < 0) {
+        printf("sd_blk_read: cannot allocate memory\n");
+        return;
+    }
+
     if (!bdrv || bdrv_read(bdrv, addr >> 9, buf, 1) == -1) {
+        free(buf);
         printf("sd_blk_read: read error on host side\n");
         return;
     }

@@ -1297,11 +1303,13 @@ static void sd_blk_read(BlockDriverState
 
         if (bdrv_read(bdrv, end >> 9, buf, 1) == -1) {
             printf("sd_blk_read: read error on host side\n");
+            free(buf);
             return;
         }
         memcpy(data + 512 - (addr & 511), buf, end & 511);
     } else
         memcpy(data, buf + (addr & 511), len);
+    free(buf);
 }
 
 static void sd_blk_write(BlockDriverState *bdrv,

hw/fdc.c +5 -1

Index: qemu/hw/fdc.c
===================================================================
@@ -382,7 +382,7 @@ struct fdctrl_t {
     uint8_t cur_drv;
     uint8_t bootsel;
     /* Command FIFO */
-    uint8_t fifo[FD_SECTOR_LEN];
+    uint8_t *fifo;
     uint32_t data_pos;
     uint32_t data_len;
     uint8_t data_state;

@@ -598,6 +598,10 @@ fdctrl_t *fdctrl_init (qemu_irq irq, int
     fdctrl = qemu_mallocz(sizeof(fdctrl_t));
     if (!fdctrl)
         return NULL;
+    if (posix_memalign((void**)&fdctrl->fifo, 512, FD_SECTOR_LEN) <0) {
+        free(fdctrl);
+        return NULL;
+    }
     fdctrl->result_timer = qemu_new_timer(vm_clock,
                                           fdctrl_result_timer, fdctrl);

Laurent Vivier

author

2007-11-28 14:03 UTC #2 lore raw

PATCH 2/2 Direct IDE I/O

This patch enhances the "-drive ,directio=on" mode with IDE drive emulation by removing the buffer used in the IDE emulation.

---
 block.c     |   10 +++
 block.h     |    2 
 block_int.h |    1 
 cpu-all.h   |    1 
 exec.c      |   19 ++++++
 hw/ide.c    |  176 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 vl.c        |    1 
 7 files changed, 204 insertions(+), 6 deletions(-)

block.c +10 -0

Index: qemu/block.c
===================================================================
@@ -752,6 +752,11 @@ void bdrv_set_translation_hint(BlockDriv
     bs->translation = translation;
 }
 
+void bdrv_set_directio_hint(BlockDriverState *bs, int directio)
+{
+    bs->directio = directio;
+}
+
 void bdrv_get_geometry_hint(BlockDriverState *bs,
                             int *pcyls, int *pheads, int *psecs)
 {

@@ -780,6 +785,11 @@ int bdrv_is_read_only(BlockDriverState *
     return bs->read_only;
 }
 
+int bdrv_is_directio(BlockDriverState *bs)
+{
+    return bs->directio;
+}
+
 /* XXX: no longer used */
 void bdrv_set_change_cb(BlockDriverState *bs,
                         void (*change_cb)(void *opaque), void *opaque)

block.h +2 -0

Index: qemu/block.h
===================================================================
@@ -112,6 +112,7 @@ void bdrv_set_geometry_hint(BlockDriverS
                             int cyls, int heads, int secs);
 void bdrv_set_type_hint(BlockDriverState *bs, int type);
 void bdrv_set_translation_hint(BlockDriverState *bs, int translation);
+void bdrv_set_directio_hint(BlockDriverState *bs, int directio);
 void bdrv_get_geometry_hint(BlockDriverState *bs,
                             int *pcyls, int *pheads, int *psecs);
 int bdrv_get_type_hint(BlockDriverState *bs);

@@ -119,6 +120,7 @@ int bdrv_get_translation_hint(BlockDrive
 int bdrv_is_removable(BlockDriverState *bs);
 int bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_is_inserted(BlockDriverState *bs);
+int bdrv_is_directio(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
 int bdrv_is_locked(BlockDriverState *bs);
 void bdrv_set_locked(BlockDriverState *bs, int locked);

block_int.h +1 -0

Index: qemu/block_int.h
===================================================================
@@ -118,6 +118,7 @@ struct BlockDriverState {
        drivers. They are not used by the block driver */
     int cyls, heads, secs, translation;
     int type;
+    int directio;
     char device_name[32];
     BlockDriverState *next;
 };

vl.c +1 -0

Index: qemu/vl.c
===================================================================
@@ -5108,6 +5108,7 @@ static int drive_init(const char *str, i
         bdrv_flags |= BDRV_O_SNAPSHOT;
     if (directio)
         bdrv_flags |= BDRV_O_DIRECT;
+    bdrv_set_directio_hint(bdrv, directio);
     if (bdrv_open(bdrv, file, bdrv_flags) < 0 || qemu_key_check(bdrv, file)) {
         fprintf(stderr, "qemu: could not open disk image %s\n",
                         file);

hw/ide.c +170 -6

212 lines not on this page · raw message

exec.c +19 -0

Index: qemu/exec.c
===================================================================
@@ -2054,6 +2054,25 @@ void cpu_register_physical_memory(target
     }
 }
 
+uint8_t * cpu_physical_page_addr(target_phys_addr_t addr)
+{
+    target_phys_addr_t page;
+    unsigned long pd;
+    PhysPageDesc *p;
+    unsigned long addr1;
+
+    page = addr & TARGET_PAGE_MASK;
+    p = phys_page_find(page >> TARGET_PAGE_BITS);
+    if (!p)
+       return (uint8_t*)-1;
+
+    pd = p->phys_offset;
+
+    addr1 = (pd & TARGET_PAGE_MASK) + (addr & ~TARGET_PAGE_MASK);
+
+    return phys_ram_base + addr1;
+}
+
 /* XXX: temporary until new memory mapping API */
 uint32_t cpu_get_physical_page_desc(target_phys_addr_t addr)
 {

cpu-all.h +1 -0

Index: qemu/cpu-all.h
===================================================================
@@ -837,6 +837,7 @@ int cpu_register_io_memory(int io_index,
 CPUWriteMemoryFunc **cpu_get_io_memory_write(int io_index);
 CPUReadMemoryFunc **cpu_get_io_memory_read(int io_index);
 
+extern uint8_t * cpu_physical_page_addr(target_phys_addr_t addr);
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
                             int len, int is_write);
 static inline void cpu_physical_memory_read(target_phys_addr_t addr,

Samuel Thibault

2007-11-28 14:24 UTC #3 lore raw

Re: [Qemu-devel] [PATCH 1/2] Add "directio" parameter to "-drive"

In reply to Laurent Vivier

Hi,

Quoted 1 line · Laurent Vivier, le Wed 28 Nov 2007 15:02:50 +0100, a écrit :

+ ret = posix_memalign((void**)&buf, 0x200, 512);

For making this more easily portable, maybe it should be a new qemu_memalign() function? Also, the alignment may probably be better as a global macro, since the alignment requirements depend on the OS.

Samuel

Daniel P. Berrange

2007-11-28 14:28 UTC #7 lore raw

In reply to Laurent Vivier

Quoted 6 lines · Laurent Vivier

These patches allow to open file using O_DIRECT and bypass the host I/O cache.
[PATCH 1/2] Add "directio" parameter to "-drive"
Using "directio=on" with "-drive" will open the disk image file using "O_DIRECT".

I don't see the point in adding a config param for this. If it provides a useful performance improvement (or other benefit) we should enable it by default all the time.

Dan.

Signature · 4 lines

|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|

Samuel Thibault

2007-11-28 14:34 UTC #8 lore raw

Quoted 11 lines · Daniel P. Berrange, le Wed 28 Nov 2007 14:27:39 +0000, a écrit :

Quoted 6 lines · Laurent Vivier
These patches allow to open file using O_DIRECT and bypass the host I/O cache.
[PATCH 1/2] Add "directio" parameter to "-drive"
Using "directio=on" with "-drive" will open the disk image file using "O_DIRECT".
I don't see the point in adding a config param for this. If it provides a useful performance improvement (or other benefit) we should enable it by default all the time.

That depends on the mileage of the user. Sometimes it is useful since it avoids the duplication of page or buffer cache between the guest and the host, sometimes it is not because the guest is not so i/o friendly and hence using the host page/buffer cache is useful.

Samuel

Anthony Liguori

2007-11-28 14:59 UTC #9 lore raw

Quoted 11 lines · Samuel Thibault

Quoted 9 lines · Daniel P. Berrange, le Wed 28 Nov 2007 14:27:39 +0000, a écrit :
Quoted 7 lines · Laurent Vivier
These patches allow to open file using O_DIRECT and bypass the host I/O cache.
[PATCH 1/2] Add "directio" parameter to "-drive"
Using "directio=on" with "-drive" will open the disk image file using "O_DIRECT".

Quoted 9 lines

Quoted 3 lines
I don't see the point in adding a config param for this. If it provides a useful performance improvement (or other benefit) we should enable it by default all the time.
That depends on the mileage of the user. Sometimes it is useful since it avoids the duplication of page or buffer cache between the guest and the host, sometimes it is not because the guest is not so i/o friendly and hence using the host page/buffer cache is useful.

In the very least, I think it should be named something more along the lines of "caching=off" or "buffering=off" since that's closer to what the hardware concept is without exposing implementation details to the user.

Regards,

Anthony Liguori

Quoted 5 lines

Samuel

Laurent Vivier

author

2007-11-28 15:02 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 1/2] Add "directio" parameter to "-drive"

In reply to Samuel Thibault

Quoted 8 lines · Samuel Thibault

Hi,
Quoted 1 line · Laurent Vivier, le Wed 28 Nov 2007 15:02:50 +0100, a écrit :
+ ret = posix_memalign((void**)&buf, 0x200, 512);
For making this more easily portable, maybe it should be a new qemu_memalign() function? Also, the alignment may probably be better as a global macro, since the alignment requirements depend on the OS.

I think O_DIRECT is linux specific whereas posix_memalign() is part of POSIX specification. And the memory alignement should only needed by the implemetation of O_DIRECT on linux.

But I have nothing against to write a qemu_memalign().

Laurent

Signature · 3 lines

------------- [email protected]  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke

Samuel Thibault

2007-11-28 15:08 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 1/2] Add "directio" parameter to "-drive"

Quoted 9 lines · Laurent Vivier, le Wed 28 Nov 2007 16:00:17 +0100, a écrit :

Quoted 6 lines · Samuel Thibault
Quoted 1 line · Laurent Vivier, le Wed 28 Nov 2007 15:02:50 +0100, a écrit :
+ ret = posix_memalign((void**)&buf, 0x200, 512);
For making this more easily portable, maybe it should be a new qemu_memalign() function? Also, the alignment may probably be better as a global macro, since the alignment requirements depend on the OS.
I think O_DIRECT is linux specific

Yes.

Quoted 1 line

whereas posix_memalign() is part of POSIX specification.

Yes, but not so many systems have it.

Quoted 2 lines

And the memory alignement should only needed by the implemetation of O_DIRECT on linux.

IIRC it is also required e.g. by the equivalent raw devices of Solaris. It happens that I also need it for the Xen stubdomains support :)

Samuel

Daniel P. Berrange

2007-11-28 15:09 UTC #10 lore raw

In reply to Samuel Thibault

Quoted 9 lines · Samuel Thibault

Quoted 8 lines · Daniel P. Berrange, le Wed 28 Nov 2007 14:27:39 +0000, a écrit :
Quoted 6 lines · Laurent Vivier
These patches allow to open file using O_DIRECT and bypass the host I/O cache.
[PATCH 1/2] Add "directio" parameter to "-drive"
Using "directio=on" with "-drive" will open the disk image file using "O_DIRECT".

Quoted 8 lines

Quoted 3 lines
I don't see the point in adding a config param for this. If it provides a useful performance improvement (or other benefit) we should enable it by default all the time.
That depends on the mileage of the user. Sometimes it is useful since it avoids the duplication of page or buffer cache between the guest and the host, sometimes it is not because the guest is not so i/o friendly and hence using the host page/buffer cache is useful.

I don't buy that - all OS already do I/O caching because its useful even on baremetal. As an end-user how do you decide whether to turn on the directoio=on/off option or not ? Most people won't notice it & will just run with the default setting - those who do notice are just subjected to trial-and-error to figure out whether the setting is any use. IMHO, QEMU should just pick the best setting.

Dan.

Signature · 4 lines

|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|

Samuel Thibault

2007-11-28 15:17 UTC #11 lore raw

Quoted 5 lines · Daniel P. Berrange, le Wed 28 Nov 2007 15:08:20 +0000, a écrit :

Quoted 2 lines
sometimes it is not because the guest is not so i/o friendly and hence using the host page/buffer cache is useful.
I don't buy that - all OS already do I/O caching because its useful even on baremetal.

Well, I can see the difference when installing windows for instance.

Samuel

Laurent Vivier

author

2007-11-29 09:40 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 1/2] Add "directio" parameter to "-drive"

In reply to Samuel Thibault

Quoted 8 lines · Samuel Thibault

Hi,
Quoted 1 line · Laurent Vivier, le Wed 28 Nov 2007 15:02:50 +0100, a écrit :
+ ret = posix_memalign((void**)&buf, 0x200, 512);
For making this more easily portable, maybe it should be a new qemu_memalign() function? Also, the alignment may probably be better as a global macro, since the alignment requirements depend on the OS.

In fact, I think I should use qemu_vmalloc().
I modify the patch for that.

Laurent

Signature · 3 lines

------------- [email protected]  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke