[Qemu-devel] [PATCH 0/2] qed: Periodically flush and clear need check bit

[Qemu-devel] [PATCH 0/2] qed: Periodically flush and clear need check bit

[Stefan Hajnoczi]

This patch marks QED images as clean periodically when it is safe to do so.
This reduces the chance of having to perform a consistency check at startup.
Previously we left the image dirty even when it was consistent, therefore
risking an unnecessary consistency check after crash.

The first patch just stubs out qemu-timer.c functions for qemu-tool.c, which is
needed since block drivers get built into qemu-img and friends.  I don't know
the timers code well so please suggest alternatives if simple stubbing is
wrong.

[Qemu-devel] [PATCH 1/2] qemu-tool: Stub out qemu-timer functions

[Stefan Hajnoczi]

Block drivers may need timers for flushing data to disk or reconnecting
to a network drive.  Stub out the following functions in qemu-tool.c:

QEMUTimer *qemu_new_timer(QEMUClock *clock, QEMUTimerCB *cb, void *opaque)
void qemu_free_timer(QEMUTimer *ts)
void qemu_del_timer(QEMUTimer *ts)
void qemu_mod_timer(QEMUTimer *ts, int64_t expire_time)
int64_t qemu_get_clock(QEMUClock *clock)

They will result in timers never firing when linked against qemu-tool.o.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 qemu-tool.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/qemu-tool.c b/qemu-tool.c
index 392e1c9..2e2f2a8 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -20,6 +20,7 @@
 #include <sys/time.h>
 
 QEMUClock *rt_clock;
+QEMUClock *vm_clock;
 
 FILE *logfile;
 
@@ -111,3 +112,26 @@ int qemu_set_fd_handler2(int fd,
 {
     return 0;
 }
+
+QEMUTimer *qemu_new_timer(QEMUClock *clock, QEMUTimerCB *cb, void *opaque)
+{
+    return qemu_malloc(1);
+}
+
+void qemu_free_timer(QEMUTimer *ts)
+{
+    qemu_free(ts);
+}
+
+void qemu_del_timer(QEMUTimer *ts)
+{
+}
+
+void qemu_mod_timer(QEMUTimer *ts, int64_t expire_time)
+{
+}
+
+int64_t qemu_get_clock(QEMUClock *clock)
+{
+    return 0;
+}
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/2] qemu-tool: Stub out qemu-timer functions

[Michael Roth]

On 02/17/2011 08:59 AM, Stefan Hajnoczi wrote:
> Block drivers may need timers for flushing data to disk or reconnecting
> to a network drive.  Stub out the following functions in qemu-tool.c:
>
> QEMUTimer *qemu_new_timer(QEMUClock *clock, QEMUTimerCB *cb, void *opaque)
> void qemu_free_timer(QEMUTimer *ts)
> void qemu_del_timer(QEMUTimer *ts)
> void qemu_mod_timer(QEMUTimer *ts, int64_t expire_time)
> int64_t qemu_get_clock(QEMUClock *clock)
>
> They will result in timers never firing when linked against qemu-tool.o.
>
> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
> ---
>   qemu-tool.c |   24 ++++++++++++++++++++++++
>   1 files changed, 24 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-tool.c b/qemu-tool.c
> index 392e1c9..2e2f2a8 100644
> --- a/qemu-tool.c
> +++ b/qemu-tool.c
> @@ -20,6 +20,7 @@
>   #include<sys/time.h>
>
>   QEMUClock *rt_clock;
> +QEMUClock *vm_clock;
>
>   FILE *logfile;
>
> @@ -111,3 +112,26 @@ int qemu_set_fd_handler2(int fd,
>   {
>       return 0;
>   }
> +
> +QEMUTimer *qemu_new_timer(QEMUClock *clock, QEMUTimerCB *cb, void *opaque)
> +{
> +    return qemu_malloc(1);
> +}
> +
> +void qemu_free_timer(QEMUTimer *ts)
> +{
> +    qemu_free(ts);
> +}
> +
> +void qemu_del_timer(QEMUTimer *ts)
> +{
> +}
> +
> +void qemu_mod_timer(QEMUTimer *ts, int64_t expire_time)
> +{
> +}
> +
> +int64_t qemu_get_clock(QEMUClock *clock)
> +{
> +    return 0;
> +}

As an alternative to stubbing, can we consider the following patches 
which make timers functional for qemu-tools (if you add code to actually 
drive the timers...otherwise they'll just never fire as is the case with 
your patches)? These also make qemu_set_fd_handler() available for 
qemu-tools (again, if you actually drive the select() loop), but you 
could pull the timer-specific stuff out if that's a bit too out-of-scope 
for your changes. I could also post these as a separate patchset... I 
think these may be useful for driving some of the block testing utilities:

___
Move code related to fd handlers into utility functions

This allows us to implement an i/o loop outside of vl.c that can
interact with objects that use qemu_set_fd_handler()

http://repo.or.cz/w/qemu/mdroth.git/commitdiff/2becd511df5f064e32f84e93dc5018933dcb5351

___
Add qemu_set_fd_handler() wrappers to qemu-tools.c

This adds state information for managing fd handlers to qemu-tools.c so
that tools that build against it can implement an I/O loop for
interacting with objects that use qemu_set_fd_handler()

http://repo.or.cz/w/qemu/mdroth.git/commitdiff/fd1e5887e4aaa9dc8b6a25950a3f31b20e4b6390

___
Make qemu timers available for tools

To be able to use qemu_mod_timer() and friends to register timeout
events for virtagent's qemu-va tool, we need to do the following:

Move several blocks of code out of cpus.c that handle initialization
of qemu's io_thread_fd and working with it via
qemu_notify_event()/qemu_event_read()/etc, and make them accessible
as backend functions to both the emulator code and qemu-tool.c via
wrapper functions within cpus.c and qemu-tool.c, respectively. These
have been added to qemu-ioh.c, where similar treatment was given to
qemu_set_fd_handler() and friends.

Some of these wrapper functions lack declarations when being
built into tools, so we add those via qemu-tool.h, which can be included
by a tool to access them. With these changes we can drive timers in a
tool linking it against qemu-timer.o and then implementing something
similar to the main i/o loop in vl.c:

init_clocks();
configure_alarms("dynticks");
if (init_timer_alarm() < 0) {
     errx(EXIT_FAILURE, "could not initialize alarm timer");
}

while (running) {
     //do work
     qemu_run_all_timers();
}

http://repo.or.cz/w/qemu/mdroth.git/commitdiff/65dd692242334806f26e38d6f5cf9bc20c22ec2e

[Qemu-devel] [PATCH 2/2] qed: Periodically flush and clear need check bit

[Stefan Hajnoczi]

One strategy to limit the startup delay of consistency check when
opening image files is to ensure that the file is marked dirty for as
little time as possible.

QED currently marks the image dirty when the first allocating write
request is issued and clears the dirty bit again when the image is
cleanly closed.  In practice that means the image is marked dirty for
most of a guest's lifetime and prone to being in a dirty state upon
crash or power failure.

It is safe to clear the dirty bit after all allocating write requests
have completed and a flush has been performed.  This patch adds a timer
after the last allocating write request completes.  When the timer fires
it will flush and then clear the dirty bit.  The timer is set to 5
seconds and is cancelled upon arrival of a new allocating write request.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/qed.c  |  103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 block/qed.h  |    7 ++++
 trace-events |    3 ++
 3 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 75ae244..138a05b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include "qemu-timer.h"
 #include "trace.h"
 #include "qed.h"
 #include "qerror.h"
@@ -291,6 +292,88 @@ static CachedL2Table *qed_new_l2_table(BDRVQEDState *s)
 
 static void qed_aio_next_io(void *opaque, int ret);
 
+static void qed_plug_allocating_write_reqs(BDRVQEDState *s)
+{
+    assert(!s->allocating_write_reqs_plugged);
+
+    s->allocating_write_reqs_plugged = true;
+}
+
+static void qed_unplug_allocating_write_reqs(BDRVQEDState *s)
+{
+    QEDAIOCB *acb;
+
+    assert(s->allocating_write_reqs_plugged);
+
+    s->allocating_write_reqs_plugged = false;
+
+    acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs);
+    if (acb) {
+        qed_aio_next_io(acb, 0);
+    }
+}
+
+static void qed_finish_clear_need_check(void *opaque, int ret)
+{
+    /* Do nothing */
+}
+
+static void qed_flush_after_clear_need_check(void *opaque, int ret)
+{
+    BDRVQEDState *s = opaque;
+
+    bdrv_aio_flush(s->bs, qed_finish_clear_need_check, s);
+
+    /* No need to wait until flush completes */
+    qed_unplug_allocating_write_reqs(s);
+}
+
+static void qed_clear_need_check(void *opaque, int ret)
+{
+    BDRVQEDState *s = opaque;
+
+    if (ret) {
+        qed_unplug_allocating_write_reqs(s);
+        return;
+    }
+
+    s->header.features &= ~QED_F_NEED_CHECK;
+    qed_write_header(s, qed_flush_after_clear_need_check, s);
+}
+
+static void qed_need_check_timer_cb(void *opaque)
+{
+    BDRVQEDState *s = opaque;
+
+    /* The timer should only fire when allocating writes have drained */
+    assert(!QSIMPLEQ_FIRST(&s->allocating_write_reqs));
+
+    trace_qed_need_check_timer_cb(s);
+
+    qed_plug_allocating_write_reqs(s);
+
+    /* Ensure writes are on disk before clearing flag */
+    bdrv_aio_flush(s->bs, qed_clear_need_check, s);
+}
+
+static void qed_start_need_check_timer(BDRVQEDState *s)
+{
+    trace_qed_start_need_check_timer(s);
+
+    /* Use vm_clock so we don't alter the image file while suspended for
+     * migration.
+     */
+    qemu_mod_timer(s->need_check_timer, qemu_get_clock(vm_clock) +
+                   get_ticks_per_sec() * QED_NEED_CHECK_TIMEOUT);
+}
+
+/* It's okay to call this multiple times or when no timer is started */
+static void qed_cancel_need_check_timer(BDRVQEDState *s)
+{
+    trace_qed_cancel_need_check_timer(s);
+    qemu_del_timer(s->need_check_timer);
+}
+
 static int bdrv_qed_open(BlockDriverState *bs, int flags)
 {
     BDRVQEDState *s = bs->opaque;
@@ -406,7 +489,10 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
             BdrvCheckResult result = {0};
 
             ret = qed_check(s, &result, true);
-            if (!ret && !result.corruptions && !result.check_errors) {
+            if (ret) {
+                goto out;
+            }
+            if (!result.corruptions && !result.check_errors) {
                 /* Ensure fixes reach storage before clearing check bit */
                 bdrv_flush(s->bs);
 
@@ -416,6 +502,8 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
         }
     }
 
+    s->need_check_timer = qemu_new_timer(vm_clock, qed_need_check_timer_cb, s);
+
 out:
     if (ret) {
         qed_free_l2_cache(&s->l2_cache);
@@ -428,6 +516,9 @@ static void bdrv_qed_close(BlockDriverState *bs)
 {
     BDRVQEDState *s = bs->opaque;
 
+    qed_cancel_need_check_timer(s);
+    qemu_free_timer(s->need_check_timer);
+
     /* Ensure writes reach stable storage */
     bdrv_flush(bs->file);
 
@@ -803,6 +894,8 @@ static void qed_aio_complete(QEDAIOCB *acb, int ret)
         acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs);
         if (acb) {
             qed_aio_next_io(acb, 0);
+        } else if (s->header.features & QED_F_NEED_CHECK) {
+            qed_start_need_check_timer(s);
         }
     }
 }
@@ -1008,11 +1101,17 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
 {
     BDRVQEDState *s = acb_to_s(acb);
 
+    /* Cancel timer when the first allocating request comes in */
+    if (QSIMPLEQ_EMPTY(&s->allocating_write_reqs)) {
+        qed_cancel_need_check_timer(s);
+    }
+
     /* Freeze this request if another allocating write is in progress */
     if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs)) {
         QSIMPLEQ_INSERT_TAIL(&s->allocating_write_reqs, acb, next);
     }
-    if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs)) {
+    if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs) ||
+        s->allocating_write_reqs_plugged) {
         return; /* wait for existing request to finish */
     }
 
diff --git a/block/qed.h b/block/qed.h
index 2925e37..717f7db 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -78,6 +78,9 @@ enum {
     QED_MIN_TABLE_SIZE = 1,        /* in clusters */
     QED_MAX_TABLE_SIZE = 16,
     QED_DEFAULT_TABLE_SIZE = 4,
+
+    /* Delay to flush and clean image after last allocating write completes */
+    QED_NEED_CHECK_TIMEOUT = 5,    /* in seconds */
 };
 
 typedef struct {
@@ -157,6 +160,10 @@ typedef struct {
 
     /* Allocating write request queue */
     QSIMPLEQ_HEAD(, QEDAIOCB) allocating_write_reqs;
+    bool allocating_write_reqs_plugged;
+
+    /* Periodic flush and clear need check flag */
+    QEMUTimer *need_check_timer;
 } BDRVQEDState;
 
 enum {
diff --git a/trace-events b/trace-events
index e6138ea..51c8f86 100644
--- a/trace-events
+++ b/trace-events
@@ -216,6 +216,9 @@ disable qed_write_table(void *s, uint64_t offset, void *table, unsigned int inde
 disable qed_write_table_cb(void *s, void *table, int flush, int ret) "s %p table %p flush %d ret %d"
 
 # block/qed.c
+disable qed_need_check_timer_cb(void *s) "s %p"
+disable qed_start_need_check_timer(void *s) "s %p"
+disable qed_cancel_need_check_timer(void *s) "s %p"
 disable qed_aio_complete(void *s, void *acb, int ret) "s %p acb %p ret %d"
 disable qed_aio_setup(void *s, void *acb, int64_t sector_num, int nb_sectors, void *opaque, int is_write) "s %p acb %p sector_num %"PRId64" nb_sectors %d opaque %p is_write %d"
 disable qed_aio_next_io(void *s, void *acb, int ret, uint64_t cur_pos) "s %p acb %p ret %d cur_pos %"PRIu64""
-- 
1.7.2.3