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

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

[Blue Swirl]

Revision: 4367
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4367
Author:   blueswir1
Date:     2008-05-06 17:26:59 +0000 (Tue, 06 May 2008)

Log Message:
-----------
Align file accesses with cache=off (Kevin Wolf, Laurent Vivier)

Modified Paths:
--------------
    trunk/block-raw-posix.c

Modified: trunk/block-raw-posix.c
===================================================================
--- trunk/block-raw-posix.c	2008-05-06 16:33:45 UTC (rev 4366)
+++ trunk/block-raw-posix.c	2008-05-06 17:26:59 UTC (rev 4367)
@@ -77,10 +77,10 @@
 typedef struct BDRVRawState {
     int fd;
     int type;
+    int open_flags;
     unsigned int lseek_err_cnt;
 #if defined(__linux__)
     /* linux floppy specific */
-    int fd_open_flags;
     int64_t fd_open_time;
     int64_t fd_error_time;
     int fd_got_error;
@@ -111,6 +111,7 @@
         open_flags |= O_DIRECT;
 #endif
 
+    s->open_flags = open_flags;
     s->type = FTYPE_FILE;
 
     fd = open(filename, open_flags, 0644);
@@ -141,7 +142,14 @@
 #endif
 */
 
-static int raw_pread(BlockDriverState *bs, int64_t offset,
+/* 
+ * offset and count are in bytes, but must be multiples of 512 for files 
+ * opened with O_DIRECT. buf must be aligned to 512 bytes then.
+ *
+ * This function may be called without alignment if the caller ensures
+ * that O_DIRECT is not in effect.
+ */
+static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
                      uint8_t *buf, int count)
 {
     BDRVRawState *s = bs->opaque;
@@ -194,7 +202,14 @@
     return ret;
 }
 
-static int raw_pwrite(BlockDriverState *bs, int64_t offset,
+/* 
+ * offset and count are in bytes, but must be multiples of 512 for files 
+ * opened with O_DIRECT. buf must be aligned to 512 bytes then.
+ *
+ * This function may be called without alignment if the caller ensures
+ * that O_DIRECT is not in effect.
+ */
+static int raw_pwrite_aligned(BlockDriverState *bs, int64_t offset,
                       const uint8_t *buf, int count)
 {
     BDRVRawState *s = bs->opaque;
@@ -230,6 +245,67 @@
     return ret;
 }
 
+
+#ifdef O_DIRECT
+/* 
+ * offset and count are in bytes and possibly not aligned. For files opened 
+ * with O_DIRECT, necessary alignments are ensured before calling 
+ * raw_pread_aligned to do the actual read.
+ */
+static int raw_pread(BlockDriverState *bs, int64_t offset,
+                     uint8_t *buf, int count)
+{
+    BDRVRawState *s = bs->opaque;
+
+    if (unlikely((s->open_flags & O_DIRECT) &&
+            (offset % 512 || count % 512 || (uintptr_t) buf % 512))) {
+
+        int ret;
+
+        // Temporarily disable O_DIRECT for unaligned access
+        fcntl(s->fd, F_SETFL, s->open_flags & ~O_DIRECT);
+        ret = raw_pread_aligned(bs, offset, buf, count);
+        fcntl(s->fd, F_SETFL, s->open_flags);
+
+        return ret;
+
+    } else {
+        return raw_pread_aligned(bs, offset, buf, count);
+    }
+}
+
+/* 
+ * offset and count are in bytes and possibly not aligned. For files opened 
+ * with O_DIRECT, necessary alignments are ensured before calling 
+ * raw_pwrite_aligned to do the actual write.
+ */
+static int raw_pwrite(BlockDriverState *bs, int64_t offset,
+                      const uint8_t *buf, int count)
+{
+    BDRVRawState *s = bs->opaque;
+
+    if (unlikely((s->open_flags & O_DIRECT) &&
+            (offset % 512 || count % 512 || (uintptr_t) buf % 512))) {
+
+        int ret;
+
+        // Temporarily disable O_DIRECT for unaligned access
+        fcntl(s->fd, F_SETFL, s->open_flags & ~O_DIRECT);
+        ret = raw_pwrite_aligned(bs, offset, buf, count);
+        fcntl(s->fd, F_SETFL, s->open_flags);
+
+        return ret;
+    } else {
+        return raw_pwrite_aligned(bs, offset, buf, count);
+    }
+}
+
+#else
+#define raw_pread raw_pread_aligned
+#define raw_pwrite raw_pwrite_aligned
+#endif
+
+
 /***********************************************************/
 /* Unix AIO using POSIX AIO */
 
@@ -402,10 +478,26 @@
         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;
+
+        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);
+        return &acb->common;
+    }
+
     acb = raw_aio_setup(bs, sector_num, buf, nb_sectors, cb, opaque);
     if (!acb)
         return NULL;
+
     if (aio_read(&acb->aiocb) < 0) {
         qemu_aio_release(acb);
         return NULL;
@@ -418,7 +510,22 @@
         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;
+
+        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);
+        return &acb->common;
+    }
+
     acb = raw_aio_setup(bs, sector_num, (uint8_t*)buf, nb_sectors, cb, opaque);
     if (!acb)
         return NULL;
@@ -679,7 +786,7 @@
         s->type = FTYPE_CD;
     } else if (strstart(filename, "/dev/fd", NULL)) {
         s->type = FTYPE_FD;
-        s->fd_open_flags = open_flags;
+        s->open_flags = open_flags;
         /* open will not fail even if no floppy is inserted */
         open_flags |= O_NONBLOCK;
     } else if (strstart(filename, "/dev/sg", NULL)) {
@@ -734,7 +841,7 @@
 #endif
             return -EIO;
         }
-        s->fd = open(bs->filename, s->fd_open_flags);
+        s->fd = open(bs->filename, s->open_flags);
         if (s->fd < 0) {
             s->fd_error_time = qemu_get_clock(rt_clock);
             s->fd_got_error = 1;
@@ -831,7 +938,7 @@
                 close(s->fd);
                 s->fd = -1;
             }
-            fd = open(bs->filename, s->fd_open_flags | O_NONBLOCK);
+            fd = open(bs->filename, s->open_flags | O_NONBLOCK);
             if (fd >= 0) {
                 if (ioctl(fd, FDEJECT, 0) < 0)
                     perror("FDEJECT");

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

[Fabrice Bellard]

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).

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 !

Fabrice.

Blue Swirl wrote:
> Revision: 4367
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4367
> Author:   blueswir1
> Date:     2008-05-06 17:26:59 +0000 (Tue, 06 May 2008)
> 
> Log Message:
> -----------
> Align file accesses with cache=off (Kevin Wolf, Laurent Vivier)
> 
> Modified Paths:
> --------------
>     trunk/block-raw-posix.c
> [...]

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

[Anthony Liguori]

Fabrice Bellard wrote:
> 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).
>
> 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 !
>   

Indeed, why not just allocate a temporary buffer and realign the access 
such that it's correct?  If that involves doing a read first then a 
write, so be it.

Messing with file flags is very dangerous as there may be a request 
in-flight already in a different thread.  I don't think this patch is 
safe at all.

Regards,

Anthony Liguori

> Fabrice.
>
> Blue Swirl wrote:
>   
>> Revision: 4367
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4367
>> Author:   blueswir1
>> Date:     2008-05-06 17:26:59 +0000 (Tue, 06 May 2008)
>>
>> Log Message:
>> -----------
>> Align file accesses with cache=off (Kevin Wolf, Laurent Vivier)
>>
>> Modified Paths:
>> --------------
>>     trunk/block-raw-posix.c
>> [...]
>>     
>
>
>   

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

[Jamie Lokier]

Fabrice Bellard wrote:
> 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).

Definitely, I've been bitten by equivalent recursions and deadlocks in
my own code recently.

> 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 !

I agree.  Several OSes document that _any_ open buffered descriptors
suppress O_DIRECT, causing all descriptors to do buffered I/O, and
(explicitly or implied) opening a descriptor with O_DIRECT, and no
buffered descriptors at the time, flushes any buffers for that file so
that switching between them is coherent.  (Though I picked up hints
that some OSes don't even manage that coherence.)

Because it's not merely a flag change, it requires some cache flushing
for coherence, I would not trust fcntl(fd, F_SETFL, x) where x changes
between O_DIRECT and non-O_DIRECT to do the right thing on some OSes -
even for _synchronous_ I/O on separate regions.

-- Jamie

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

[Kevin Wolf]

[-- 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)

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

[Jamie Lokier]

Kevin Wolf wrote:
> 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.

IIRC, the same issues (recursive callback vs. queued callback,
problems with direct and non-direct I/O in flight at the same time)
were raised before it was committed, but it was committed anyway.

-- Jamie

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

[Kevin Wolf]

Jamie Lokier schrieb:
> Kevin Wolf wrote:
>> 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.
> 
> IIRC, the same issues (recursive callback vs. queued callback,
> problems with direct and non-direct I/O in flight at the same time)
> were raised before it was committed, but it was committed anyway.

No, nobody mentioned the recursion problem. And you were talking about 
problem with two different file descriptors for one file, not about the 
fcntl solution. Ok, might also be that the hints were just not explicit 
enough for me. ;-)

But even if so, this is more of a general feeling about how patches are 
handled and not only related to this patch.

Kevin

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

[Blue Swirl]

On 5/7/08, Kevin Wolf <kwolf@suse.de> wrote:
> Jamie Lokier schrieb:
>
>
> > Kevin Wolf wrote:
> >
> > > 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.
> > >
> >
> > IIRC, the same issues (recursive callback vs. queued callback,
> > problems with direct and non-direct I/O in flight at the same time)
> > were raised before it was committed, but it was committed anyway.
> >
>
>  No, nobody mentioned the recursion problem. And you were talking about
> problem with two different file descriptors for one file, not about the
> fcntl solution. Ok, might also be that the hints were just not explicit
> enough for me. ;-)
>
>  But even if so, this is more of a general feeling about how patches are
> handled and not only related to this patch.

Good point. For most patches sent to the list I don't have strong
opinions, for example I don't know the necessary technical background
or the patch could affect areas that I'm not interested in. Maybe if I
comment on the patches it gives a wrong signal about the future
prospects for the patch getting committed. I still think it is a waste
of resources if the effort made for the patches gets lost or don't get
much review like what is happening here very often.

Usually I run a set of tests before committing. From the amount of
breakage fixes and reverts I get the impression that some of the other
developers test less. Though testing will not catch all deeper
problems like in this case.

Maybe more developers with strong focus on x86/x86_64 host and target
would help.

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

[Jamie Lokier]

Kevin Wolf wrote:
> No, nobody mentioned the recursion problem.

Hmm.  I concede you're right in the sense that it was mentioned, but
on a different thread about QEMU AIO recently :-)

Message-ID: <20080403113128.GA17900@shareable.org>
Avi Kivity wrote:
> Long term we want to replace the recursion by queuing.
Ah yes.  Reminds me of a bug in a program of mine with asynchronous
sockets.
    <explanation of bug>
I learned a few lessons about async callbacks:
    <general advice>

> And you were talking about 
> problem with two different file descriptors for one file, not about the 
> fcntl solution. Ok, might also be that the hints were just not explicit 
> enough for me. ;-)

Following the paragraph about two file descriptors, there was:

>> I'm not sure if that works, though.  On some OSes, if a file has any
>> non-O_DIRECT open descriptor, all I/O is buffered ignoring the
>> O_DIRECT flag.  If both are allowed simultaneously, I'm not sure what
>> happens with cache-coherency between direct I/Os and buffered I/Os.

Not sure if that is quite the same thing :-)

I did miss that switching O_DIRECT on/off while AIOs are in flight on
that descriptor might be dodgy (implementation dependent), and that it
might not do the right things w.r.t. cohrency.

> But even if so, this is more of a general feeling about how patches are 
> handled and not only related to this patch.

I agree and have a similar feeling, though it's not a bad thing
provided the issues are actually noticed, which they do seem to be.  I
have the impression there are many people working on different
specific features and subsystems, but not so much on overall
architecture in a coordinated and "visionary" way.

Most of my issues with QEMU are the epic list of difficulties with
Microsoft guests (espcially when people send me images for a different
VM), the peculiar divergence between KVM and QEMU features, and the
awkwardness of the monitor/command line interface.  Since I'm not an
active code contributor, and those are to a great extent feature
requests or only debuggable by users, I keep those thoughts largely to
myself :-)

-- Jamie

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

[Kevin Wolf]

Blue Swirl schrieb:
> I still think it is a waste
> of resources if the effort made for the patches gets lost or don't get
> much review like what is happening here very often.

Thank you, there's nothing more to add. This is exactly what I meant.

> Usually I run a set of tests before committing. From the amount of
> breakage fixes and reverts I get the impression that some of the other
> developers test less. Though testing will not catch all deeper
> problems like in this case.

My impression is that most of the commiters only commit their own 
patches (maybe with the exception of Aurelien), but don't want to bother 
with patches from ordinary mailing list subscribers. In comparison to 
other projects, it's quite difficult to get something into qemu and I'd 
not be surprised if this is discouraging people from doing qemu 
development at all.

Apropos revert... ;-) The patch didn't break anything previously 
working, it's just that previously completely broken functionality might 
still be slightly broken. And I've already posted a patch for the 
recursion problem, so I would have preferred committing that one on top 
over reverting the whole thing.

Kevin

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

[Kevin Wolf]

Jamie Lokier schrieb:
> Kevin Wolf wrote:
>> No, nobody mentioned the recursion problem.
> 
> Hmm.  I concede you're right in the sense that it was mentioned, but
> on a different thread about QEMU AIO recently :-)

Hm, okay. Will read that thread if I can find it. But I hope the 
recursion thing is fixed now anyway.

> Following the paragraph about two file descriptors, there was:
> 
>>> I'm not sure if that works, though.  On some OSes, if a file has any
>>> non-O_DIRECT open descriptor, all I/O is buffered ignoring the
>>> O_DIRECT flag.  If both are allowed simultaneously, I'm not sure what
>>> happens with cache-coherency between direct I/Os and buffered I/Os.
> 
> Not sure if that is quite the same thing :-)

Am I completely mistaken or is this still about two (or more) different 
file descriptors where one of them is non-O_DIRECT?

> I did miss that switching O_DIRECT on/off while AIOs are in flight on
> that descriptor might be dodgy (implementation dependent), and that it
> might not do the right things w.r.t. cohrency.

Me too. I just was too confident that it actually works when Laurent 
says it was better. You know, for every problem there is an answer that 
is clear, simple and wrong. ;-)

>> But even if so, this is more of a general feeling about how patches are 
>> handled and not only related to this patch.
> 
> I agree and have a similar feeling, though it's not a bad thing
> provided the issues are actually noticed, which they do seem to be.  

Actually, I do think it's a bad thing. Obviously, issues are noticed 
when the patch goes in and can be fixed then. Right. But what about the 
other 90% of the patches which don't get no attention at all? Nobody 
comments on them, they aren't committed, and after all they are wasted 
efforts.

It's a bad thing because it slows down qemu development by (passively) 
rejecting patches which are fine or could be fixed easily.

Kevin