QEMU: fsync AIO writes on flush request

PATCH In review Marcelo Tosatti 4 participants 17 messages Mar 28, 2008 – Mar 29, 2008 KVM

lore

Share & tools

lore

Marcelo Tosatti

author

2008-03-28 15:02 UTC #0 lore raw

PATCH 1/1 QEMU: fsync AIO writes on flush request

Its necessary to guarantee that pending AIO writes have reached stable storage when the flush request returns.

Also change fsync() to fdatasync(), since the modification time is not critical data.

Signed-off-by Marcelo Tosatti <[email protected]>

qemu/block-raw-posix.c +31 -1

Index: kvm-userspace.io/qemu/block-raw-posix.c
===================================================================
@@ -557,10 +557,40 @@ static int raw_create(const char *filena
     return 0;
 }
 
+static void raw_aio_flush_complete(void *opaque, int ret)
+{
+    if (ret)
+        printf("WARNING: aio_fsync failed (completion)\n");
+}
+
+static void raw_aio_flush(BlockDriverState *bs)
+{
+    RawAIOCB *acb;
+
+    acb = raw_aio_setup(bs, 0, NULL, 0, raw_aio_flush_complete, NULL);
+    if (!acb)
+        return;
+
+    if (aio_fsync(O_DSYNC, &acb->aiocb) < 0) {
+        qemu_aio_release(acb);
+        perror("aio_fsync");
+        printf("WARNING: aio_fsync failed\n");
+        return;
+    }
+}
+
 static void raw_flush(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
-    fsync(s->fd);
+    raw_aio_flush(bs);
+    fdatasync(s->fd);
+
+    /* We rely on the fact that no other AIO will be submitted
+     * in parallel, but this should be fixed by per-device
+     * AIO queues when allowing multiple CPU's to process IO
+     * in QEMU.
+     */
+    qemu_aio_flush();
 }
 
 BlockDriver bdrv_raw = {

Jamie Lokier

2008-03-28 15:07 UTC #1 lore raw

Quoted 8 lines · Marcelo Tosatti

Its necessary to guarantee that pending AIO writes have reached stable storage when the flush request returns.
Also change fsync() to fdatasync(), since the modification time is not critical data.
qemu/block-raw-posix.c +1 -0
+    if (aio_fsync(O_DSYNC, &acb->aiocb) < 0) {
 
     BDRVRawState *s = bs->opaque;

qemu/block-raw-posix.c static int raw_create(const char *filena · line 593

Quoted 10 lines

qemu/block-raw-posix.c +9 -1

-    fsync(s->fd);
+    raw_aio_flush(bs);
+    fdatasync(s->fd);
+
+    /* We rely on the fact that no other AIO will be submitted
+     * in parallel, but this should be fixed by per-device
+     * AIO queues when allowing multiple CPU's to process IO
+     * in QEMU.
+     */
+    qemu_aio_flush();

I'm a bit confused by this. Why do you need aio_fsync(O_DSYNC) _and_ synchronous fdatasync() calls? Aren't they equivalent?

-- Jamie

Marcelo Tosatti

author

2008-03-28 16:28 UTC #2 lore raw

Quoted 20 lines · Jamie Lokier

Quoted 8 lines · Marcelo Tosatti
Its necessary to guarantee that pending AIO writes have reached stable storage when the flush request returns.
Also change fsync() to fdatasync(), since the modification time is not critical data.
qemu/block-raw-posix.c +1 -0
+    if (aio_fsync(O_DSYNC, &acb->aiocb) < 0) {
 
     BDRVRawState *s = bs->opaque;
qemu/block-raw-posix.c static int raw_create(const char *filena · line 593
Quoted 10 lines
qemu/block-raw-posix.c +9 -1
-    fsync(s->fd);
+    raw_aio_flush(bs);
+    fdatasync(s->fd);
+
+    /* We rely on the fact that no other AIO will be submitted
+     * in parallel, but this should be fixed by per-device
+     * AIO queues when allowing multiple CPU's to process IO
+     * in QEMU.
+     */
+    qemu_aio_flush();

Quoted 2 lines

I'm a bit confused by this. Why do you need aio_fsync(O_DSYNC) _and_ synchronous fdatasync() calls? Aren't they equivalent?

fdatasync() will write and wait for completion of dirty file data present in memory.

aio_write() only queues data for submission:

The "asynchronous" means that this call returns as soon as the request has been enqueued; the write may or may not have completed when the call returns. One tests for completion using aio_error(3).

So fdatasync() is not enough because data written via AIO may not have been reflected as "dirty file data" through write() by the time raw_flush() is called.

The SCSI and IDE drivers use flush() in response to a "flush cache" request, which is used by the guest OS to implement barriers, for example.

Paul Brook

2008-03-28 16:40 UTC #3 lore raw

Quoted 24 lines · Marcelo Tosatti

Quoted 20 lines · Jamie Lokier
Quoted 8 lines · Marcelo Tosatti
Its necessary to guarantee that pending AIO writes have reached stable storage when the flush request returns.
Also change fsync() to fdatasync(), since the modification time is not critical data.
qemu/block-raw-posix.c +1 -0
+    if (aio_fsync(O_DSYNC, &acb->aiocb) < 0) {
 
     BDRVRawState *s = bs->opaque;
qemu/block-raw-posix.c static int raw_create(const char *filena · line 593
Quoted 10 lines
qemu/block-raw-posix.c +9 -1
-    fsync(s->fd);
+    raw_aio_flush(bs);
+    fdatasync(s->fd);
+
+    /* We rely on the fact that no other AIO will be submitted
+     * in parallel, but this should be fixed by per-device
+     * AIO queues when allowing multiple CPU's to process IO
+     * in QEMU.
+     */
+    qemu_aio_flush();
Quoted 2 lines
I'm a bit confused by this. Why do you need aio_fsync(O_DSYNC) _and_ synchronous fdatasync() calls? Aren't they equivalent?

Quoted 8 lines

fdatasync() will write and wait for completion of dirty file data present in memory.
aio_write() only queues data for submission:
The "asynchronous" means that this call returns as soon as the
request has been enqueued; the write may or may not have completed when the call returns. One tests for completion using aio_error(3).

Surely you should be using the normal aio notification to wait for the aio_fsync to complete before reporting success to the device.

Paul

Marcelo Tosatti

author

2008-03-28 16:56 UTC #4 lore raw

Quoted 28 lines · Paul Brook

Quoted 27 lines · Marcelo Tosatti
Quoted 20 lines · Jamie Lokier
Quoted 8 lines · Marcelo Tosatti
Its necessary to guarantee that pending AIO writes have reached stable storage when the flush request returns.
Also change fsync() to fdatasync(), since the modification time is not critical data.
qemu/block-raw-posix.c +1 -0
+    if (aio_fsync(O_DSYNC, &acb->aiocb) < 0) {
 
     BDRVRawState *s = bs->opaque;
qemu/block-raw-posix.c static int raw_create(const char *filena · line 593
Quoted 10 lines
qemu/block-raw-posix.c +9 -1
-    fsync(s->fd);
+    raw_aio_flush(bs);
+    fdatasync(s->fd);
+
+    /* We rely on the fact that no other AIO will be submitted
+     * in parallel, but this should be fixed by per-device
+     * AIO queues when allowing multiple CPU's to process IO
+     * in QEMU.
+     */
+    qemu_aio_flush();
Quoted 2 lines
I'm a bit confused by this. Why do you need aio_fsync(O_DSYNC) _and_ synchronous fdatasync() calls? Aren't they equivalent?
fdatasync() will write and wait for completion of dirty file data present in memory.

Quoted 8 lines

Quoted 5 lines
aio_write() only queues data for submission:
The "asynchronous" means that this call returns as soon as the
request has been enqueued; the write may or may not have completed when the call returns. One tests for completion using aio_error(3).
Surely you should be using the normal aio notification to wait for the aio_fsync to complete before reporting success to the device.

qemu_aio_flush() will wait for all pending AIO requests (including aio_fsync) to complete.

Or do you mean something else?

Paul Brook

2008-03-28 17:00 UTC #5 lore raw

Quoted 5 lines

Quoted 2 lines
Surely you should be using the normal aio notification to wait for the aio_fsync to complete before reporting success to the device.
qemu_aio_flush() will wait for all pending AIO requests (including aio_fsync) to complete.

Then why do you need to separate fdatasync?

Paul

Marcelo Tosatti

author

2008-03-28 18:10 UTC #6 lore raw

Quoted 7 lines · Paul Brook

Quoted 5 lines
Quoted 2 lines
Surely you should be using the normal aio notification to wait for the aio_fsync to complete before reporting success to the device.
qemu_aio_flush() will wait for all pending AIO requests (including aio_fsync) to complete.
Then why do you need to separate fdatasync?

Oh, I see what Jamie means now: fdatasync() is redundant with aio_fsync(O_DSYNC).

How's this?

qemu/block-raw-posix.c +30 -1

Index: kvm-userspace.io/qemu/block-raw-posix.c
===================================================================
@@ -557,10 +557,39 @@ static int raw_create(const char *filena
     return 0;
 }
 
+static void raw_aio_flush_complete(void *opaque, int ret)
+{
+    if (ret)
+        printf("WARNING: aio_fsync failed (completion)\n");
+}
+
+static void raw_aio_flush(BlockDriverState *bs)
+{
+    RawAIOCB *acb;
+
+    acb = raw_aio_setup(bs, 0, NULL, 0, raw_aio_flush_complete, NULL);
+    if (!acb)
+        return;
+
+    if (aio_fsync(O_DSYNC, &acb->aiocb) < 0) {
+        qemu_aio_release(acb);
+        perror("aio_fsync");
+        printf("WARNING: aio_fsync failed\n");
+        return;
+    }
+}
+
 static void raw_flush(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
-    fsync(s->fd);
+    raw_aio_flush(bs);
+
+    /* We rely on the fact that no other AIO will be submitted
+     * in parallel, but this should be fixed by per-device
+     * AIO queues when allowing multiple CPU's to process IO
+     * in QEMU.
+     */
+    qemu_aio_flush();
 }
 
 BlockDriver bdrv_raw = {

Jamie Lokier

2008-03-29 01:17 UTC #7 lore raw

qemu/block-raw-posix.c static int raw_create(const char *filena · line 593

Quoted 13 lines · Marcelo Tosatti

static void raw_flush(BlockDriverState *bs)

 {
     BDRVRawState *s = bs->opaque;

qemu/block-raw-posix.c +8 -1

-    fsync(s->fd);
+    raw_aio_flush(bs);
+
+    /* We rely on the fact that no other AIO will be submitted
+     * in parallel, but this should be fixed by per-device
+     * AIO queues when allowing multiple CPU's to process IO
+     * in QEMU.
+     */
+    qemu_aio_flush();
 }

It depends what raw_flush() is used for.

If you want to be sure this flushes AIO writes in-flight at the time of the call, I still reckon you need an extra qemu_aio_flush() before raw_aio_flush() - on at least some POSIX AIO implementations. (I'd be very interested to find out otherwise, if you know better).

But if, as Ian Jackson suggests, raw_flush() is _only_ used when the guest driver issues a CACHE FLUSH command _and_ the guest driver either cannot overlap operations, or cannot depend on overlapping operations occuring in order, then you don't need it.

That'll depend on what kind of device is emulated. Does the SCSI emulation handle multiple in-flight commands with any guarantee on order? To be on the safe side, I'd include the extra qemu_aio_flush, as I expect it's very unlikely to harm performance and might save someone's data.

-- Jamie

Paul Brook

2008-03-29 02:02 UTC #8 lore raw

Quoted 3 lines

That'll depend on what kind of device is emulated. Does the SCSI emulation handle multiple in-flight commands with any guarantee on order?

SCSI definitely allows (and we emulate) multiple in flight commands. I can't find any requirement that writes must complete before a subsequent SYNCHRONISE_CACHE. However I don't claim to know the spec that well, and it's probably not a bad idea have them complete anyway. Preferably this would be a completely asynchronous operation. i.e. the sync command returns immediately, but only completes when all preceding writes have completed and been flushed to disk.

Paul

Jamie Lokier

2008-03-29 02:11 UTC #9 lore raw

Quoted 8 lines · Paul Brook

Quoted 3 lines
That'll depend on what kind of device is emulated. Does the SCSI emulation handle multiple in-flight commands with any guarantee on order?
SCSI definitely allows (and we emulate) multiple in flight commands. I can't find any requirement that writes must complete before a subsequent SYNCHRONISE_CACHE. However I don't claim to know the spec that well,

Aren't there SCSI tagged barrier commands or something like that, which allow a host to request ordering between commands?

Quoted 4 lines

it's probably not a bad idea have them complete anyway. Preferably this would be a completely asynchronous operation. i.e. the sync command returns immediately, but only completes when all preceding writes have completed and been flushed to disk.

I agree, that seems the optimal implementation.

-- Jamie

Paul Brook

2008-03-29 02:43 UTC #10 lore raw

Quoted 5 lines · Jamie Lokier

Quoted 4 lines · Paul Brook
Quoted 3 lines
That'll depend on what kind of device is emulated. Does the SCSI emulation handle multiple in-flight commands with any guarantee on order?

Quoted 7 lines

Quoted 4 lines
SCSI definitely allows (and we emulate) multiple in flight commands. I can't find any requirement that writes must complete before a subsequent SYNCHRONISE_CACHE. However I don't claim to know the spec that well,
Aren't there SCSI tagged barrier commands or something like that, which allow a host to request ordering between commands?

Ah, yes. Ordering is defined as part of the task management model, not part of the commands themselves. For parallel scsi barrier tasks are created using the ORDERED message (The implementation for this in the LSI HBA is currently absent).

So in theory we don't care about this for SCSI because ordering will already have been enforced by upper levels.

Paul

Jamie Lokier

2008-03-28 18:03 UTC #11 lore raw

In reply to Marcelo Tosatti

Quoted 29 lines · Marcelo Tosatti

Quoted 20 lines · Jamie Lokier
Quoted 8 lines · Marcelo Tosatti
Its necessary to guarantee that pending AIO writes have reached stable storage when the flush request returns.
Also change fsync() to fdatasync(), since the modification time is not critical data.
qemu/block-raw-posix.c +1 -0
+    if (aio_fsync(O_DSYNC, &acb->aiocb) < 0) {
 
     BDRVRawState *s = bs->opaque;
qemu/block-raw-posix.c static int raw_create(const char *filena · line 593
Quoted 10 lines
qemu/block-raw-posix.c +9 -1
-    fsync(s->fd);
+    raw_aio_flush(bs);
+    fdatasync(s->fd);
+
+    /* We rely on the fact that no other AIO will be submitted
+     * in parallel, but this should be fixed by per-device
+     * AIO queues when allowing multiple CPU's to process IO
+     * in QEMU.
+     */
+    qemu_aio_flush();
Quoted 2 lines
I'm a bit confused by this. Why do you need aio_fsync(O_DSYNC) _and_ synchronous fdatasync() calls? Aren't they equivalent?
fdatasync() will write and wait for completion of dirty file data present in memory.
aio_write() only queues data for submission:

Quoted 8 lines

The "asynchronous" means that this call returns as soon as the request has been enqueued; the write may or may not have completed when the call returns. One tests for completion using aio_error(3).
So fdatasync() is not enough because data written via AIO may not have been reflected as "dirty file data" through write() by the time raw_flush() is called.

Sure. But why isn't the aio_fsync(O_DSYNC) enough by itself?

It seems to me you should have something like this:

    /* Flush pending aio_writes until they are dirty data,
       and wait before the aio_fsync. */
    qemu_aio_flush();

    /* Call aio_fsync(O_DSYNC). */
    raw_aio_flush(bs);

    /* Wait for the aio_fsync to complete. */
    qemu_aio_flush();

What am I missing?

-- Jamie

Marcelo Tosatti

author

2008-03-28 18:33 UTC #12 lore raw

Quoted 35 lines · Jamie Lokier

Quoted 33 lines · Marcelo Tosatti
Quoted 20 lines · Jamie Lokier
Quoted 8 lines · Marcelo Tosatti
Its necessary to guarantee that pending AIO writes have reached stable storage when the flush request returns.
Also change fsync() to fdatasync(), since the modification time is not critical data.
qemu/block-raw-posix.c +1 -0
+    if (aio_fsync(O_DSYNC, &acb->aiocb) < 0) {
 
     BDRVRawState *s = bs->opaque;
qemu/block-raw-posix.c static int raw_create(const char *filena · line 593
Quoted 10 lines
qemu/block-raw-posix.c +9 -1
-    fsync(s->fd);
+    raw_aio_flush(bs);
+    fdatasync(s->fd);
+
+    /* We rely on the fact that no other AIO will be submitted
+     * in parallel, but this should be fixed by per-device
+     * AIO queues when allowing multiple CPU's to process IO
+     * in QEMU.
+     */
+    qemu_aio_flush();
Quoted 2 lines
I'm a bit confused by this. Why do you need aio_fsync(O_DSYNC) _and_ synchronous fdatasync() calls? Aren't they equivalent?
fdatasync() will write and wait for completion of dirty file data present in memory.
aio_write() only queues data for submission:
The "asynchronous" means that this call returns as soon as the request has been enqueued; the write may or may not have completed when the call returns. One tests for completion using aio_error(3).

Quoted 5 lines

Quoted 3 lines
So fdatasync() is not enough because data written via AIO may not have been reflected as "dirty file data" through write() by the time raw_flush() is called.
Sure. But why isn't the aio_fsync(O_DSYNC) enough by itself?

It is enough, fdatasync() becomes redundant.

Quoted 12 lines

It seems to me you should have something like this:

    /* Flush pending aio_writes until they are dirty data,
       and wait before the aio_fsync. */
    qemu_aio_flush();

    /* Call aio_fsync(O_DSYNC). */
    raw_aio_flush(bs);

    /* Wait for the aio_fsync to complete. */
    qemu_aio_flush();

Quoted 1 line

What am I missing?

I don't think the first qemu_aio_flush() is necessary because the fsync request will be enqueued after pending ones:

aio_fsync() function does a sync on all outstanding asynchronous I/O operations associated with aiocbp->aio_fildes.

More precisely, if op is O_SYNC, then all currently queued I/O opera- tions shall be completed as if by a call of fsync(2), and if op is
O_DSYNC, this call is the asynchronous analog of fdatasync(2). Note
that this is a request only — this call does not wait for I/O comple- tion.

glibc sets the priority for fsync as 0, which is the same priority AIO reads and writes are submitted by QEMU.

Jamie Lokier

2008-03-29 01:09 UTC #13 lore raw

Quoted 7 lines · Marcelo Tosatti

I don't think the first qemu_aio_flush() is necessary because the fsync request will be enqueued after pending ones:
aio_fsync() function does a sync on all outstanding asynchronous I/O operations associated with aiocbp->aio_fildes.

Quoted 8 lines

More precisely, if op is O_SYNC, then all currently queued I/O operations shall be completed as if by a call of fsync(2), and if op is O_DSYNC, this call is the asynchronous analog of fdatasync(2). Note that this is a request only — this call does not wait for I/O completion.
glibc sets the priority for fsync as 0, which is the same priority AIO reads and writes are submitted by QEMU.

Do AIO operations always get executed in the order they are submitted?

I was under the impression this is not guaranteed, as relaxed ordering permits better I/O scheduling (e.g. to reduce disk seeks) - which is one of the most useful points of AIO. (Otherwise you might as well just have one worker thread doing synchronous IO in order).

And because of that, I was under the impression the only way to implement a write barrier+flush in AIO was (1) wait for pending writes to complete, then (2) aio_fsync, then (3) wait for the aio_fsync.

I could be wrong, but I haven't seen any documentation which says otherwise, and it's what I'd expect of an implementation. I.e. it's just an asynchronous version of fsync().

The quoted man page doesn't convince me. It says "all currently queued I/O operations shall be completed" which _could_ mean that aio_fsync is an AIO barrier too.

But then "if by a call of fsync(2)" implies that aio_fsync+aio_suspend could just be replaced by fsync() with no change of semantics. So "queued I/O operations" means what fsync() handles: dirty file data, not in-flight AIO writes.

And you already noticed that fsync() is _not_ guaranteed to flush in-flight AIO writes. Being the asynchronous analog, aio_fsync() would not either.

-- Jamie

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Marcelo Tosatti

author

2008-03-29 06:46 UTC #14 lore raw

Quoted 14 lines · Jamie Lokier

Quoted 13 lines · Marcelo Tosatti
I don't think the first qemu_aio_flush() is necessary because the fsync request will be enqueued after pending ones:
aio_fsync() function does a sync on all outstanding asynchronous I/O operations associated with aiocbp->aio_fildes.
More precisely, if op is O_SYNC, then all currently queued I/O operations shall be completed as if by a call of fsync(2), and if op is O_DSYNC, this call is the asynchronous analog of fdatasync(2). Note that this is a request only — this call does not wait for I/O completion.

Quoted 4 lines

Quoted 2 lines
glibc sets the priority for fsync as 0, which is the same priority AIO reads and writes are submitted by QEMU.
Do AIO operations always get executed in the order they are submitted?

With glibc AIO on the same file descriptor, yes. See sysdeps/pthreads/aio_misc.c.

The kernel AIO implementation (used for DIRECT IO) does not implement aio_fsync.

Quoted 5 lines

I was under the impression this is not guaranteed, as relaxed ordering permits better I/O scheduling (e.g. to reduce disk seeks) - which is one of the most useful points of AIO. (Otherwise you might as well just have one worker thread doing synchronous IO in order).

Quoted 7 lines

And because of that, I was under the impression the only way to implement a write barrier+flush in AIO was (1) wait for pending writes to complete, then (2) aio_fsync, then (3) wait for the aio_fsync.
I could be wrong, but I haven't seen any documentation which says otherwise, and it's what I'd expect of an implementation. I.e. it's just an asynchronous version of fsync().

All documentation (and source code) I can find indicates that aio_fsync() will wait for pending AIO requests to finish before doing the fsync/fdatasync system call.

I find it hard to see much purpose in such an interface otherwise.

http://www.opengroup.org/onlinepubs/009695399/functions/aio_fsync.html

http://docs.hp.com/en/B9106-90012/aio.5.html "Synchronizing Permanent Storage"

http://www.gnu.org/software/libtool/manual/libc/Synchronizing-AIO-Operations.html#Synchronizing-AIO-Operations

"When dealing with asynchronous operations it is sometimes necessary to get into a consistent state. This would mean for AIO that one wants to know whether a certain request or a group of request were processed. This could be done by waiting for the notification sent by the system after the operation terminated, but this sometimes would mean wasting resources (mainly computation time). Instead POSIX.1b defines two functions which will help with most kinds of consistency."

http://www.governmentsecurity.org/articles/articles2/B2355-90693.pdf_fl/B2355-90693-29.html

This is the HPUX man page, much better than the Linux one.

Quoted 4 lines

The quoted man page doesn't convince me. It says "all currently queued I/O operations shall be completed" which _could_ mean that aio_fsync is an AIO barrier too.

Quoted 8 lines

But then "if by a call of fsync(2)" implies that aio_fsync+aio_suspend could just be replaced by fsync() with no change of semantics. So "queued I/O operations" means what fsync() handles: dirty file data, not in-flight AIO writes.
And you already noticed that fsync() is _not_ guaranteed to flush in-flight AIO writes. Being the asynchronous analog, aio_fsync() would not either.

That seems to be the whole point of aio_fsync().

Ian Jackson

2008-03-28 17:25 UTC #15 lore raw

In reply to Marcelo Tosatti

Marcelo Tosatti writes ("[Qemu-devel] [PATCH] QEMU: fsync AIO writes on flush request "):

Quoted 2 lines

Its necessary to guarantee that pending AIO writes have reached stable storage when the flush request returns.

Surely it isn't necessary to call qemu_aio_flush ? Because those pending AIO writes have not yet been returned to the guest as complete, the guest is not entitled to assume that a FLUSH CACHE command (issued before those writes have completed) completing successfully means that those interleaved writes have reached stable storage.

Also, this patch does a synchronous flush (which is bad because it stalls the guest while the flush takes place) and it ignores any error return (which is quite bad - see my other messages about bdrv_flush, caches, etc.)

So I think it would be better to apply
- my bdrv_flush patch from February which I've reposted today
and then
- the asynchronous FLUSH CACHE patch which I've posted today

I think we concluded last time that the change of fsync to fdatasync is correct but I think we should wait for the dust to settle before introducing another change on top of all this ...

Ian.

Marcelo Tosatti

author

2008-03-28 19:08 UTC #16 lore raw

Quoted 10 lines · Ian Jackson

Marcelo Tosatti writes ("[Qemu-devel] [PATCH] QEMU: fsync AIO writes on flush request "):
Quoted 2 lines
Its necessary to guarantee that pending AIO writes have reached stable storage when the flush request returns.
Surely it isn't necessary to call qemu_aio_flush ? Because those pending AIO writes have not yet been returned to the guest as complete, the guest is not entitled to assume that a FLUSH CACHE command (issued before those writes have completed) completing successfully means that those interleaved writes have reached stable storage.

By the time qemu_aio_flush() finishes, all pending requests submitted before aio_fsync() will have reached stable storage (aio_fsync will be queued in the last position of the priority 0 AIO queue).

Unless the scheduling priority of the QEMU process is changed between AIO write's and the fsync().

Quoted 5 lines

Also, this patch does a synchronous flush (which is bad because it stalls the guest while the flush takes place) and it ignores any error return (which is quite bad - see my other messages about bdrv_flush, caches, etc.)

Quoted 8 lines

So I think it would be better to apply
- my bdrv_flush patch from February which I've reposted today
and then
- the asynchronous FLUSH CACHE patch which I've posted today
I think we concluded last time that the change of fsync to fdatasync is correct but I think we should wait for the dust to settle before introducing another change on top of all this ...

Alright, please also fix the SCSI driver to perform asynchronous flush with bdrv_aio_flush() and complete the SYNC_CACHE command on the callback.