Re: [Qemu-devel] [4367] Align file accesses with cache=off (Kevin Wolf, Laurent Vivier)

[Kevin Wolf <kwolf@suse.de>]

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

Fabrice Bellard schrieb:
> A note: in order to avoid uncontrolled recursions, it is better to call
> the read/write AIO callback outside the aio_read/write (see
> bdrv_aio_read_em).

Something along the lines of the attached patch?

> Personally I would not trust the OS to correctly handle the mix of
> O_DIRECT and buffered operations, especially if the corresponding file
> regions intersect !

We might have to go back to the pwrite implementation of my first patch 
then which emulates the accesses by using a temporary aligned buffer.

Btw, it is quite interesting to see that a serious discussion of a patch 
happens only if it is already committed. This could have been discussed 
a week ago when we agreed to go in the apparently wrong direction. And 
the patch has been on the list much longer than this one week.

Kevin

[-- Attachment #2: avoid-recursions.patch --]
[-- Type: text/x-patch, Size: 2757 bytes --]

Index: qemu-svn/block-raw-posix.c
===================================================================
--- qemu-svn.orig/block-raw-posix.c
+++ qemu-svn/block-raw-posix.c
@@ -313,6 +313,7 @@ typedef struct RawAIOCB {
     BlockDriverAIOCB common;
     struct aiocb aiocb;
     struct RawAIOCB *next;
+    int ret;
 } RawAIOCB;
 
 static int aio_sig_num = SIGUSR2;
@@ -473,26 +474,37 @@ static RawAIOCB *raw_aio_setup(BlockDriv
     return acb;
 }
 
+#ifndef QEMU_IMG
+static void raw_aio_em_cb(void* opaque)
+{
+    RawAIOCB *acb = opaque;
+    acb->common.cb(acb->common.opaque, acb->ret);
+    qemu_aio_release(acb);
+}
+#endif
+
 static BlockDriverAIOCB *raw_aio_read(BlockDriverState *bs,
         int64_t sector_num, uint8_t *buf, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
     RawAIOCB *acb;
-    BDRVRawState *s = bs->opaque;
 
-    /* 
+    /*
      * If O_DIRECT is used and the buffer is not aligned fall back
      * to synchronous IO.
      */
-    if (unlikely((s->open_flags & O_DIRECT) && ((uintptr_t) buf % 512))) {
-        int ret;
+#ifndef QEMU_IMG
+    BDRVRawState *s = bs->opaque;
 
+    if (unlikely((s->open_flags & O_DIRECT) && ((uintptr_t) buf % 512))) {
+        QEMUBH *bh;
         acb = qemu_aio_get(bs, cb, opaque);
-        ret = raw_pread(bs, 512 * sector_num, buf, 512 * nb_sectors);
-        acb->common.cb(acb->common.opaque, ret);
-        qemu_aio_release(acb);
+        acb->ret = raw_pread(bs, 512 * sector_num, buf, 512 * nb_sectors);
+        bh = qemu_bh_new(raw_aio_em_cb, acb);
+        qemu_bh_schedule(bh);
         return &acb->common;
     }
+#endif
 
     acb = raw_aio_setup(bs, sector_num, buf, nb_sectors, cb, opaque);
     if (!acb)
@@ -510,21 +522,23 @@ static BlockDriverAIOCB *raw_aio_write(B
         BlockDriverCompletionFunc *cb, void *opaque)
 {
     RawAIOCB *acb;
-    BDRVRawState *s = bs->opaque;
 
-    /* 
+    /*
      * If O_DIRECT is used and the buffer is not aligned fall back
      * to synchronous IO.
      */
-    if (unlikely((s->open_flags & O_DIRECT) && ((uintptr_t) buf % 512))) {
-        int ret;
+#ifndef QEMU_IMG
+    BDRVRawState *s = bs->opaque;
 
+    if (unlikely((s->open_flags & O_DIRECT) && ((uintptr_t) buf % 512))) {
+        QEMUBH *bh;
         acb = qemu_aio_get(bs, cb, opaque);
-        ret = raw_pwrite(bs, 512 * sector_num, buf, 512 * nb_sectors);
-        acb->common.cb(acb->common.opaque, ret);
-        qemu_aio_release(acb);
+        acb->ret = raw_pwrite(bs, 512 * sector_num, buf, 512 * nb_sectors);
+        bh = qemu_bh_new(raw_aio_em_cb, acb);
+        qemu_bh_schedule(bh);
         return &acb->common;
     }
+#endif
 
     acb = raw_aio_setup(bs, sector_num, (uint8_t*)buf, nb_sectors, cb, opaque);
     if (!acb)