[Qemu-devel] AIO and savevm

[Qemu-devel] AIO and savevm

[Blue Swirl]

Hi,

I don't think savevm works reliably together with current AIO. At least some 
kind of AIO flushing or completion waiting should happen and then this 
should be done before any devices have started saving their state. Is this 
correct?

_________________________________________________________________
Don't just search. Find. Check out the new MSN Search! 
http://search.msn.com/

Re: [Qemu-devel] AIO and savevm

[Paul Brook]

> I don't think savevm works reliably together with current AIO. At least
> some kind of AIO flushing or completion waiting should happen and then this
> should be done before any devices have started saving their state. Is this
> correct?

Correct.

Implementing it this way does mean savevm can effect the guest VM state (it 
causes all pending IO to complete immediately). However this should be safe, 
ie. it could occur be chance anyway, and qemu isn't deterministic to start 
with.

Trying to make async AIO restartable is IMHO not worth the effort at this 
point.

Paul

Re: [Qemu-devel] AIO and savevm

[Blue Swirl]

>Implementing it this way does mean savevm can effect the guest VM state (it
>causes all pending IO to complete immediately). However this should be 
>safe,
>ie. it could occur be chance anyway, and qemu isn't deterministic to start
>with.

What would be the right place for the AIO flush, how about bdrv_flush? Would 
it be OK to add bdrv_flush just before qemu_fopen_bdrv in do_savevm?

_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE! 
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/

Re: [Qemu-devel] AIO and savevm

[Paul Brook]

On Sunday 03 September 2006 09:51, Blue Swirl wrote:
> >Implementing it this way does mean savevm can effect the guest VM state
> > (it causes all pending IO to complete immediately). However this should
> > be safe, ie. it could occur by chance anyway, and qemu isn't deterministic
> > to start with. 
>
> What would be the right place for the AIO flush, how about bdrv_flush?
> Would it be OK to add bdrv_flush just before qemu_fopen_bdrv in do_savevm?

No bdrv_flush if completely different, and has no observable effect[1].

When savevm is issued, you need to halt the guest CPUs, and wait for all IO to 
complete. While doing this you need to allow device AIO completion routines 
to run, which may trigger additional IO. Once all IO has been completed you 
should then be able to safely save state.

Paul

[1] ie. should be safe to call bdrv_flush at any time, and bdrv_flush is never 
necessary for correct operations. It's purpose is to help ensure data 
consistency if the host machine dies unexpectedly.

Re: [Qemu-devel] AIO and savevm

[Blue Swirl]

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

>When savevm is issued, you need to halt the guest CPUs, and wait for all IO 
>to
>complete. While doing this you need to allow device AIO completion routines
>to run, which may trigger additional IO. Once all IO has been completed you
>should then be able to safely save state.

How's this patch? Qemu_aio_poll waits for all AIOs and calls the device 
completion routines when each AIO finishes.

_________________________________________________________________
Don't just search. Find. Check out the new MSN Search! 
http://search.msn.com/

[-- Attachment #2: aio_flush.diff --]
[-- Type: text/plain, Size: 426 bytes --]

Improve savevm consistency by flushing pending AIO before saving.

Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c	2006-09-03 10:31:48.000000000 +0000
+++ qemu/vl.c	2006-09-03 10:33:51.000000000 +0000
@@ -4551,6 +4551,9 @@
         return;
     }

+    /* Flush pending AIO before saving */
+    qemu_aio_poll();
+
     saved_vm_running = vm_running;
     vm_stop(0);

Re: [Qemu-devel] AIO and savevm

[Paul Brook]

On Sunday 03 September 2006 11:42, Blue Swirl wrote:
> >When savevm is issued, you need to halt the guest CPUs, and wait for all
> > IO to
> >complete. While doing this you need to allow device AIO completion
> > routines to run, which may trigger additional IO. Once all IO has been
> > completed you should then be able to safely save state.
>
> How's this patch? Qemu_aio_poll waits for all AIOs and calls the device
> completion routines when each AIO finishes.

qemu_aio_poll doesn't wait. It returns immediately if IO has not completed 
yet.

Paul

Re: [Qemu-devel] AIO and savevm

[Blue Swirl]

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

>qemu_aio_poll doesn't wait. It returns immediately if IO has not completed
>yet.

You're right, sorry. How's this version then? Though there is a race 
condition where the AIO signal is received between checking for AIO and 
waiting for it.

_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE! 
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/

[-- Attachment #2: aio_flush.diff --]
[-- Type: text/plain, Size: 1827 bytes --]

Improve savevm consistency by flushing pending AIO before saving.

Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c	2006-09-03 10:45:28.000000000 +0000
+++ qemu/vl.c	2006-09-03 11:06:13.000000000 +0000
@@ -4551,6 +4551,17 @@
         return;
     }

+    /* Flush pending AIO before saving */
+    ret = qemu_aio_poll();
+    if (ret) {
+        qemu_aio_wait_start();
+        do {
+            qemu_aio_wait();
+            ret = qemu_aio_poll();
+        } while (ret);
+        qemu_aio_wait_end();
+    }
+
     saved_vm_running = vm_running;
     vm_stop(0);

Index: qemu/block-raw.c
===================================================================
--- qemu.orig/block-raw.c	2006-09-03 11:00:39.000000000 +0000
+++ qemu/block-raw.c	2006-09-03 11:03:40.000000000 +0000
@@ -206,10 +206,12 @@
#endif
}

-void qemu_aio_poll(void)
+int qemu_aio_poll(void)
{
     RawAIOCB *acb, **pacb;
-    int ret;
+    int ret, aios_active;
+
+    aios_active = 0;

     for(;;) {
         pacb = &first_aio;
@@ -240,11 +242,14 @@
                 qemu_aio_release(acb);
                 break;
             } else {
+                /* aio still active */
                 pacb = &acb->next;
+                aios_active = 1;
             }
         }
     }
- the_end: ;
+ the_end:
+    return aios_active;
}

/* wait until at least one AIO was handled */
Index: qemu/vl.h
===================================================================
--- qemu.orig/vl.h	2006-09-03 11:00:43.000000000 +0000
+++ qemu/vl.h	2006-09-03 11:04:10.000000000 +0000
@@ -579,7 +579,7 @@
void bdrv_aio_cancel(BlockDriverAIOCB *acb);

void qemu_aio_init(void);
-void qemu_aio_poll(void);
+int qemu_aio_poll(void);
void qemu_aio_wait_start(void);
void qemu_aio_wait(void);
void qemu_aio_wait_end(void);

Re: [Qemu-devel] AIO and savevm

[Paul Brook]

On Sunday 03 September 2006 12:14, Blue Swirl wrote:
> >qemu_aio_poll doesn't wait. It returns immediately if IO has not completed
> >yet.
>
> You're right, sorry. How's this version then? Though there is a race
> condition where the AIO signal is received between checking for AIO and
> waiting for it.

I moved it into a separate routine, and fixed the race condition.
We also need to flush AIO requests before doing loadvm.

Paul