[Qemu-devel] [PATCH 0/6] block: do request processing in a coroutine

[Qemu-devel] [PATCH 0/6] block: do request processing in a coroutine

[Stefan Hajnoczi]

Block layer features like dirty block tracing, I/O throttling, and live block
copy are forced to duplicate code due to the three different interfaces:
synchronous, asynchronous, and coroutines.

Since there are bdrv_read(), bdrv_aio_readv(), and bdrv_co_readv() interfaces
for read (and similar for write), per-request processing needs to be duplicated
for each of these execution contexts.  For example, dirty block tracking code
is duplicated across these three interfaces.

This patch series unifies request processing so that there is only one code
path.  I see this as a prerequisite to the live block copy (image streaming)
code I am working on, so I'm pushing it now.

The short-term win from this series is that it becomes easy to add live block
copy and other features.  We now have a single code path where the perf-request
processing is done.

The longer-term win will be dropping the BlockDriver .bdrv_read(),
.bdrv_write(), .bdrv_aio_readv(), and .bdrv_aio_writev() interfaces.  By doing
that we can bring all BlockDrivers onto a common interface, namely
.bdrv_co_readv() and .bdrv_co_writev().  It will also allow us to drop most of
the sync and aio emulation code.

A consequence of this patch series is that every I/O request goes through at
least one coroutine.  There is no longer a direct .bdrv_read(), .bdrv_write(),
.bdrv_aio_readv(), or .bdrv_aio_writev() call - we're trying to phase out those
interfaces.  I have not noticed performance degradation in correctness tests
but we need to confirm that there has not been a performance regression.

Stefan Hajnoczi (6):
  block: directly invoke .bdrv_aio_*() in bdrv_co_io_em()
  block: split out bdrv_co_do_readv() and bdrv_co_do_writev()
  block: switch bdrv_read()/bdrv_write() to coroutines
  block: switch bdrv_aio_readv() to coroutines
  block: mark blocks dirty on coroutine write completion
  block: switch bdrv_aio_writev() to coroutines

 block.c |  273 +++++++++++++++++++++++++++++++--------------------------------
 1 files changed, 134 insertions(+), 139 deletions(-)

-- 
1.7.6.3

[Qemu-devel] [PATCH 1/6] block: directly invoke .bdrv_aio_*() in bdrv_co_io_em()

[Stefan Hajnoczi]

We will unify block layer request processing across sync, aio, and
coroutines and this means a .bdrv_co_*() emulation function should not
call back into the public interface.  There's no need here, just call
.bdrv_aio_*() directly.

The gory details: bdrv_co_io_em() cannot call back into the public
bdrv_aio_*() interface since that will be handled using coroutines,
which causes us to call into bdrv_co_io_em() again in an infinite loop
:).

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index e3fe97f..dc36b2b 100644
--- a/block.c
+++ b/block.c
@@ -2990,11 +2990,11 @@ static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
     BlockDriverAIOCB *acb;
 
     if (is_write) {
-        acb = bdrv_aio_writev(bs, sector_num, iov, nb_sectors,
-                              bdrv_co_io_em_complete, &co);
+        acb = bs->drv->bdrv_aio_writev(bs, sector_num, iov, nb_sectors,
+                                       bdrv_co_io_em_complete, &co);
     } else {
-        acb = bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
-                             bdrv_co_io_em_complete, &co);
+        acb = bs->drv->bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
+                                      bdrv_co_io_em_complete, &co);
     }
 
     trace_bdrv_co_io(is_write, acb);
-- 
1.7.6.3

[Qemu-devel] [PATCH 2/6] block: split out bdrv_co_do_readv() and bdrv_co_do_writev()

[Stefan Hajnoczi]

The public interface for I/O in coroutine context is bdrv_co_readv() and
bdrv_co_writev().  Split out the request processing code into
bdrv_co_do_readv() and bdrv_co_writev() so that it can be called
internally when we refactor all request processing to use coroutines.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |   34 +++++++++++++++++++++++++++-------
 1 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index dc36b2b..d15784e 100644
--- a/block.c
+++ b/block.c
@@ -72,6 +72,8 @@ static int coroutine_fn bdrv_co_writev_em(BlockDriverState *bs,
                                          int64_t sector_num, int nb_sectors,
                                          QEMUIOVector *iov);
 static int coroutine_fn bdrv_co_flush_em(BlockDriverState *bs);
+static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
+    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
 
 static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
     QTAILQ_HEAD_INITIALIZER(bdrv_states);
@@ -1249,13 +1251,14 @@ int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
     return 0;
 }
 
-int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
-    int nb_sectors, QEMUIOVector *qiov)
+/*
+ * Handle a read request in coroutine context
+ */
+static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
+    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
     BlockDriver *drv = bs->drv;
 
-    trace_bdrv_co_readv(bs, sector_num, nb_sectors);
-
     if (!drv) {
         return -ENOMEDIUM;
     }
@@ -1266,12 +1269,21 @@ int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
     return drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
 }
 
-int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
+int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
     int nb_sectors, QEMUIOVector *qiov)
 {
-    BlockDriver *drv = bs->drv;
+    trace_bdrv_co_readv(bs, sector_num, nb_sectors);
 
-    trace_bdrv_co_writev(bs, sector_num, nb_sectors);
+    return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov);
+}
+
+/*
+ * Handle a write request in coroutine context
+ */
+static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
+    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
+{
+    BlockDriver *drv = bs->drv;
 
     if (!bs->drv) {
         return -ENOMEDIUM;
@@ -1294,6 +1306,14 @@ int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
     return drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
 }
 
+int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
+    int nb_sectors, QEMUIOVector *qiov)
+{
+    trace_bdrv_co_writev(bs, sector_num, nb_sectors);
+
+    return bdrv_co_do_writev(bs, sector_num, nb_sectors, qiov);
+}
+
 /**
  * Truncate file to 'offset' bytes (needed only for file protocols)
  */
-- 
1.7.6.3

[Qemu-devel] [PATCH 3/6] block: switch bdrv_read()/bdrv_write() to coroutines

[Stefan Hajnoczi]

The bdrv_read()/bdrv_write() functions call .bdrv_read()/.bdrv_write().
They should go through bdrv_co_do_readv() and bdrv_co_do_writev()
instead in order to unify request processing code across sync, aio, and
coroutine interfaces.  This is also an important step towards removing
BlockDriverState .bdrv_read()/.bdrv_write() in the future.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |  112 +++++++++++++++++++++++++++++++++++----------------------------
 1 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/block.c b/block.c
index d15784e..90c29db 100644
--- a/block.c
+++ b/block.c
@@ -44,6 +44,8 @@
 #include <windows.h>
 #endif
 
+#define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
+
 static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load);
 static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
@@ -74,6 +76,8 @@ static int coroutine_fn bdrv_co_writev_em(BlockDriverState *bs,
 static int coroutine_fn bdrv_co_flush_em(BlockDriverState *bs);
 static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
+static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
+    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
 
 static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
     QTAILQ_HEAD_INITIALIZER(bdrv_states);
@@ -1038,30 +1042,69 @@ static inline bool bdrv_has_async_flush(BlockDriver *drv)
     return drv->bdrv_aio_flush != bdrv_aio_flush_em;
 }
 
-/* return < 0 if error. See bdrv_write() for the return codes */
-int bdrv_read(BlockDriverState *bs, int64_t sector_num,
-              uint8_t *buf, int nb_sectors)
+typedef struct RwCo {
+    BlockDriverState *bs;
+    int64_t sector_num;
+    int nb_sectors;
+    QEMUIOVector *qiov;
+    bool is_write;
+    int ret;
+} RwCo;
+
+static void coroutine_fn bdrv_rw_co_entry(void *opaque)
 {
-    BlockDriver *drv = bs->drv;
+    RwCo *rwco = opaque;
 
-    if (!drv)
-        return -ENOMEDIUM;
+    if (!rwco->is_write) {
+        rwco->ret = bdrv_co_do_readv(rwco->bs, rwco->sector_num,
+                                     rwco->nb_sectors, rwco->qiov);
+    } else {
+        rwco->ret = bdrv_co_do_writev(rwco->bs, rwco->sector_num,
+                                      rwco->nb_sectors, rwco->qiov);
+    }
+}
 
-    if (bdrv_has_async_rw(drv) && qemu_in_coroutine()) {
-        QEMUIOVector qiov;
-        struct iovec iov = {
-            .iov_base = (void *)buf,
-            .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
-        };
+/*
+ * Process a synchronous request using coroutines
+ */
+static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
+                      int nb_sectors, bool is_write)
+{
+    QEMUIOVector qiov;
+    struct iovec iov = {
+        .iov_base = (void *)buf,
+        .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
+    };
+    Coroutine *co;
+    RwCo rwco = {
+        .bs = bs,
+        .sector_num = sector_num,
+        .nb_sectors = nb_sectors,
+        .qiov = &qiov,
+        .is_write = is_write,
+        .ret = NOT_DONE,
+    };
 
-        qemu_iovec_init_external(&qiov, &iov, 1);
-        return bdrv_co_readv(bs, sector_num, nb_sectors, &qiov);
-    }
+    qemu_iovec_init_external(&qiov, &iov, 1);
 
-    if (bdrv_check_request(bs, sector_num, nb_sectors))
-        return -EIO;
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_rw_co_entry(&rwco);
+    } else {
+        co = qemu_coroutine_create(bdrv_rw_co_entry);
+        qemu_coroutine_enter(co, &rwco);
+        while (rwco.ret == NOT_DONE) {
+            qemu_aio_wait();
+        }
+    }
+    return rwco.ret;
+}
 
-    return drv->bdrv_read(bs, sector_num, buf, nb_sectors);
+/* return < 0 if error. See bdrv_write() for the return codes */
+int bdrv_read(BlockDriverState *bs, int64_t sector_num,
+              uint8_t *buf, int nb_sectors)
+{
+    return bdrv_rw_co(bs, sector_num, buf, nb_sectors, false);
 }
 
 static void set_dirty_bitmap(BlockDriverState *bs, int64_t sector_num,
@@ -1101,36 +1144,7 @@ static void set_dirty_bitmap(BlockDriverState *bs, int64_t sector_num,
 int bdrv_write(BlockDriverState *bs, int64_t sector_num,
                const uint8_t *buf, int nb_sectors)
 {
-    BlockDriver *drv = bs->drv;
-
-    if (!bs->drv)
-        return -ENOMEDIUM;
-
-    if (bdrv_has_async_rw(drv) && qemu_in_coroutine()) {
-        QEMUIOVector qiov;
-        struct iovec iov = {
-            .iov_base = (void *)buf,
-            .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
-        };
-
-        qemu_iovec_init_external(&qiov, &iov, 1);
-        return bdrv_co_writev(bs, sector_num, nb_sectors, &qiov);
-    }
-
-    if (bs->read_only)
-        return -EACCES;
-    if (bdrv_check_request(bs, sector_num, nb_sectors))
-        return -EIO;
-
-    if (bs->dirty_bitmap) {
-        set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
-    }
-
-    if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
-        bs->wr_highest_sector = sector_num + nb_sectors - 1;
-    }
-
-    return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
+    return bdrv_rw_co(bs, sector_num, (uint8_t *)buf, nb_sectors, true);
 }
 
 int bdrv_pread(BlockDriverState *bs, int64_t offset,
@@ -2891,8 +2905,6 @@ static void bdrv_rw_em_cb(void *opaque, int ret)
     *(int *)opaque = ret;
 }
 
-#define NOT_DONE 0x7fffffff
-
 static int bdrv_read_em(BlockDriverState *bs, int64_t sector_num,
                         uint8_t *buf, int nb_sectors)
 {
-- 
1.7.6.3

Re: [Qemu-devel] [PATCH 3/6] block: switch bdrv_read()/bdrv_write() to coroutines

[Zhi Yong Wu]

On Thu, Oct 6, 2011 at 12:17 AM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> The bdrv_read()/bdrv_write() functions call .bdrv_read()/.bdrv_write().
> They should go through bdrv_co_do_readv() and bdrv_co_do_writev()
> instead in order to unify request processing code across sync, aio, and
> coroutine interfaces.  This is also an important step towards removing
> BlockDriverState .bdrv_read()/.bdrv_write() in the future.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c |  112 +++++++++++++++++++++++++++++++++++----------------------------
>  1 files changed, 62 insertions(+), 50 deletions(-)
>
> diff --git a/block.c b/block.c
> index d15784e..90c29db 100644
> --- a/block.c
> +++ b/block.c
> @@ -44,6 +44,8 @@
>  #include <windows.h>
>  #endif
>
> +#define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
> +
>  static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load);
>  static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
>         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> @@ -74,6 +76,8 @@ static int coroutine_fn bdrv_co_writev_em(BlockDriverState *bs,
>  static int coroutine_fn bdrv_co_flush_em(BlockDriverState *bs);
>  static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
>     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
> +static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> +    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
>
>  static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
>     QTAILQ_HEAD_INITIALIZER(bdrv_states);
> @@ -1038,30 +1042,69 @@ static inline bool bdrv_has_async_flush(BlockDriver *drv)
>     return drv->bdrv_aio_flush != bdrv_aio_flush_em;
>  }
>
> -/* return < 0 if error. See bdrv_write() for the return codes */
> -int bdrv_read(BlockDriverState *bs, int64_t sector_num,
> -              uint8_t *buf, int nb_sectors)
> +typedef struct RwCo {
> +    BlockDriverState *bs;
> +    int64_t sector_num;
> +    int nb_sectors;
> +    QEMUIOVector *qiov;
> +    bool is_write;
> +    int ret;
> +} RwCo;
> +
> +static void coroutine_fn bdrv_rw_co_entry(void *opaque)
>  {
> -    BlockDriver *drv = bs->drv;
> +    RwCo *rwco = opaque;
>
> -    if (!drv)
> -        return -ENOMEDIUM;
> +    if (!rwco->is_write) {
> +        rwco->ret = bdrv_co_do_readv(rwco->bs, rwco->sector_num,
> +                                     rwco->nb_sectors, rwco->qiov);
> +    } else {
> +        rwco->ret = bdrv_co_do_writev(rwco->bs, rwco->sector_num,
> +                                      rwco->nb_sectors, rwco->qiov);
> +    }
> +}
>
> -    if (bdrv_has_async_rw(drv) && qemu_in_coroutine()) {
> -        QEMUIOVector qiov;
> -        struct iovec iov = {
> -            .iov_base = (void *)buf,
> -            .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
> -        };
> +/*
> + * Process a synchronous request using coroutines
> + */
> +static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
> +                      int nb_sectors, bool is_write)
> +{
> +    QEMUIOVector qiov;
> +    struct iovec iov = {
> +        .iov_base = (void *)buf,
> +        .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
> +    };
> +    Coroutine *co;
> +    RwCo rwco = {
> +        .bs = bs,
> +        .sector_num = sector_num,
> +        .nb_sectors = nb_sectors,
> +        .qiov = &qiov,
> +        .is_write = is_write,
> +        .ret = NOT_DONE,
> +    };
>
> -        qemu_iovec_init_external(&qiov, &iov, 1);
> -        return bdrv_co_readv(bs, sector_num, nb_sectors, &qiov);
> -    }
> +    qemu_iovec_init_external(&qiov, &iov, 1);
>
> -    if (bdrv_check_request(bs, sector_num, nb_sectors))
> -        return -EIO;
> +    if (qemu_in_coroutine()) {
> +        /* Fast-path if already in coroutine context */
> +        bdrv_rw_co_entry(&rwco);
> +    } else {
> +        co = qemu_coroutine_create(bdrv_rw_co_entry);
> +        qemu_coroutine_enter(co, &rwco);
> +        while (rwco.ret == NOT_DONE) {
> +            qemu_aio_wait();
> +        }
> +    }
> +    return rwco.ret;
> +}
>
> -    return drv->bdrv_read(bs, sector_num, buf, nb_sectors);
> +/* return < 0 if error. See bdrv_write() for the return codes */
> +int bdrv_read(BlockDriverState *bs, int64_t sector_num,
> +              uint8_t *buf, int nb_sectors)
> +{
> +    return bdrv_rw_co(bs, sector_num, buf, nb_sectors, false);
>  }
>
>  static void set_dirty_bitmap(BlockDriverState *bs, int64_t sector_num,
> @@ -1101,36 +1144,7 @@ static void set_dirty_bitmap(BlockDriverState *bs, int64_t sector_num,
>  int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>                const uint8_t *buf, int nb_sectors)
>  {
> -    BlockDriver *drv = bs->drv;
> -
> -    if (!bs->drv)
> -        return -ENOMEDIUM;
> -
> -    if (bdrv_has_async_rw(drv) && qemu_in_coroutine()) {
> -        QEMUIOVector qiov;
> -        struct iovec iov = {
> -            .iov_base = (void *)buf,
> -            .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
> -        };
> -
> -        qemu_iovec_init_external(&qiov, &iov, 1);
> -        return bdrv_co_writev(bs, sector_num, nb_sectors, &qiov);
> -    }
> -
> -    if (bs->read_only)
> -        return -EACCES;
> -    if (bdrv_check_request(bs, sector_num, nb_sectors))
> -        return -EIO;
> -
> -    if (bs->dirty_bitmap) {
> -        set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
> -    }
> -
> -    if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
> -        bs->wr_highest_sector = sector_num + nb_sectors - 1;
> -    }
The above codes are removed, will it be safe?
> -
> -    return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
> +    return bdrv_rw_co(bs, sector_num, (uint8_t *)buf, nb_sectors, true);
>  }
>
>  int bdrv_pread(BlockDriverState *bs, int64_t offset,
> @@ -2891,8 +2905,6 @@ static void bdrv_rw_em_cb(void *opaque, int ret)
>     *(int *)opaque = ret;
>  }
>
> -#define NOT_DONE 0x7fffffff
> -
>  static int bdrv_read_em(BlockDriverState *bs, int64_t sector_num,
>                         uint8_t *buf, int nb_sectors)
>  {
> --
> 1.7.6.3
>
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 3/6] block: switch bdrv_read()/bdrv_write() to coroutines

[Stefan Hajnoczi]

On Tue, Oct 11, 2011 at 7:44 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
> On Thu, Oct 6, 2011 at 12:17 AM, Stefan Hajnoczi
> <stefanha@linux.vnet.ibm.com> wrote:
>> @@ -1101,36 +1144,7 @@ static void set_dirty_bitmap(BlockDriverState *bs, int64_t sector_num,
>>  int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>>                const uint8_t *buf, int nb_sectors)
>>  {
>> -    BlockDriver *drv = bs->drv;
>> -
>> -    if (!bs->drv)
>> -        return -ENOMEDIUM;
>> -
>> -    if (bdrv_has_async_rw(drv) && qemu_in_coroutine()) {
>> -        QEMUIOVector qiov;
>> -        struct iovec iov = {
>> -            .iov_base = (void *)buf,
>> -            .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
>> -        };
>> -
>> -        qemu_iovec_init_external(&qiov, &iov, 1);
>> -        return bdrv_co_writev(bs, sector_num, nb_sectors, &qiov);
>> -    }
>> -
>> -    if (bs->read_only)
>> -        return -EACCES;
>> -    if (bdrv_check_request(bs, sector_num, nb_sectors))
>> -        return -EIO;
>> -
>> -    if (bs->dirty_bitmap) {
>> -        set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
>> -    }
>> -
>> -    if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
>> -        bs->wr_highest_sector = sector_num + nb_sectors - 1;
>> -    }
> The above codes are removed, will it be safe?

If you are checking that removing bs->wr_highest_sector code is okay,
then yes, it is safe because bdrv_co_do_writev() does the dirty bitmap
and wr_highest_sector updates.  We haven't lost any code by unifying
request processing - bdrv_co_do_writev() must do everything that
bdrv_aio_writev() and bdrv_write() did.

Stefan

Re: [Qemu-devel] [PATCH 3/6] block: switch bdrv_read()/bdrv_write() to coroutines

[Zhi Yong Wu]

On Wed, Oct 12, 2011 at 5:03 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Oct 11, 2011 at 7:44 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>> On Thu, Oct 6, 2011 at 12:17 AM, Stefan Hajnoczi
>> <stefanha@linux.vnet.ibm.com> wrote:
>>> @@ -1101,36 +1144,7 @@ static void set_dirty_bitmap(BlockDriverState *bs, int64_t sector_num,
>>>  int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>>>                const uint8_t *buf, int nb_sectors)
>>>  {
>>> -    BlockDriver *drv = bs->drv;
>>> -
>>> -    if (!bs->drv)
>>> -        return -ENOMEDIUM;
>>> -
>>> -    if (bdrv_has_async_rw(drv) && qemu_in_coroutine()) {
>>> -        QEMUIOVector qiov;
>>> -        struct iovec iov = {
>>> -            .iov_base = (void *)buf,
>>> -            .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
>>> -        };
>>> -
>>> -        qemu_iovec_init_external(&qiov, &iov, 1);
>>> -        return bdrv_co_writev(bs, sector_num, nb_sectors, &qiov);
>>> -    }
>>> -
>>> -    if (bs->read_only)
>>> -        return -EACCES;
>>> -    if (bdrv_check_request(bs, sector_num, nb_sectors))
>>> -        return -EIO;
How about the above four lines of codes?
>>> -
>>> -    if (bs->dirty_bitmap) {
>>> -        set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
>>> -    }
>>> -
>>> -    if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
>>> -        bs->wr_highest_sector = sector_num + nb_sectors - 1;
>>> -    }
>> The above codes are removed, will it be safe?
>
> If you are checking that removing bs->wr_highest_sector code is okay,
> then yes, it is safe because bdrv_co_do_writev() does the dirty bitmap
> and wr_highest_sector updates.  We haven't lost any code by unifying
OK. got it. thanks.
> request processing - bdrv_co_do_writev() must do everything that
> bdrv_aio_writev() and bdrv_write() did.
>
> Stefan
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 3/6] block: switch bdrv_read()/bdrv_write() to coroutines

[Kevin Wolf]

Am 05.10.2011 18:17, schrieb Stefan Hajnoczi:
> The bdrv_read()/bdrv_write() functions call .bdrv_read()/.bdrv_write().
> They should go through bdrv_co_do_readv() and bdrv_co_do_writev()
> instead in order to unify request processing code across sync, aio, and
> coroutine interfaces.  This is also an important step towards removing
> BlockDriverState .bdrv_read()/.bdrv_write() in the future.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

This breaks drivers that only provide synchronous .bdrv_read/write.
Attempts to read or write anything results in endless recursion.

Kevin

[Qemu-devel] [PATCH 4/6] block: switch bdrv_aio_readv() to coroutines

[Stefan Hajnoczi]

More sync, aio, and coroutine unification.  Make bdrv_aio_readv() go
through coroutine request processing.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |   48 +++++++++++++++++++++++++++++++++++-------------
 1 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 90c29db..b83e911 100644
--- a/block.c
+++ b/block.c
@@ -78,6 +78,15 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
 static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
+static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
+                                               int64_t sector_num,
+                                               QEMUIOVector *qiov,
+                                               int nb_sectors,
+                                               BlockDriverCompletionFunc *cb,
+                                               void *opaque,
+                                               bool is_write,
+                                               CoroutineEntry *entry);
+static void coroutine_fn bdrv_co_do_rw(void *opaque);
 
 static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
     QTAILQ_HEAD_INITIALIZER(bdrv_states);
@@ -2346,17 +2355,10 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
                                  QEMUIOVector *qiov, int nb_sectors,
                                  BlockDriverCompletionFunc *cb, void *opaque)
 {
-    BlockDriver *drv = bs->drv;
-
     trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
 
-    if (!drv)
-        return NULL;
-    if (bdrv_check_request(bs, sector_num, nb_sectors))
-        return NULL;
-
-    return drv->bdrv_aio_readv(bs, sector_num, qiov, nb_sectors,
-                               cb, opaque);
+    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors,
+                                 cb, opaque, false, bdrv_co_do_rw);
 }
 
 typedef struct BlockCompleteData {
@@ -2803,6 +2805,7 @@ static void bdrv_co_rw_bh(void *opaque)
     qemu_aio_release(acb);
 }
 
+/* Invoke .bdrv_co_readv/.bdrv_co_writev */
 static void coroutine_fn bdrv_co_rw(void *opaque)
 {
     BlockDriverAIOCBCoroutine *acb = opaque;
@@ -2820,13 +2823,32 @@ static void coroutine_fn bdrv_co_rw(void *opaque)
     qemu_bh_schedule(acb->bh);
 }
 
+/* Invoke bdrv_co_do_readv/bdrv_co_do_writev */
+static void coroutine_fn bdrv_co_do_rw(void *opaque)
+{
+    BlockDriverAIOCBCoroutine *acb = opaque;
+    BlockDriverState *bs = acb->common.bs;
+
+    if (!acb->is_write) {
+        acb->req.error = bdrv_co_do_readv(bs, acb->req.sector,
+            acb->req.nb_sectors, acb->req.qiov);
+    } else {
+        acb->req.error = bdrv_co_do_writev(bs, acb->req.sector,
+            acb->req.nb_sectors, acb->req.qiov);
+    }
+
+    acb->bh = qemu_bh_new(bdrv_co_rw_bh, acb);
+    qemu_bh_schedule(acb->bh);
+}
+
 static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
                                                int64_t sector_num,
                                                QEMUIOVector *qiov,
                                                int nb_sectors,
                                                BlockDriverCompletionFunc *cb,
                                                void *opaque,
-                                               bool is_write)
+                                               bool is_write,
+                                               CoroutineEntry *entry)
 {
     Coroutine *co;
     BlockDriverAIOCBCoroutine *acb;
@@ -2837,7 +2859,7 @@ static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
     acb->req.qiov = qiov;
     acb->is_write = is_write;
 
-    co = qemu_coroutine_create(bdrv_co_rw);
+    co = qemu_coroutine_create(entry);
     qemu_coroutine_enter(co, acb);
 
     return &acb->common;
@@ -2848,7 +2870,7 @@ static BlockDriverAIOCB *bdrv_co_aio_readv_em(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
     return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque,
-                                 false);
+                                 false, bdrv_co_rw);
 }
 
 static BlockDriverAIOCB *bdrv_co_aio_writev_em(BlockDriverState *bs,
@@ -2856,7 +2878,7 @@ static BlockDriverAIOCB *bdrv_co_aio_writev_em(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
     return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque,
-                                 true);
+                                 true, bdrv_co_rw);
 }
 
 static BlockDriverAIOCB *bdrv_aio_flush_em(BlockDriverState *bs,
-- 
1.7.6.3

Re: [Qemu-devel] [PATCH 4/6] block: switch bdrv_aio_readv() to coroutines

[Kevin Wolf]

Am 05.10.2011 18:17, schrieb Stefan Hajnoczi:
> More sync, aio, and coroutine unification.  Make bdrv_aio_readv() go
> through coroutine request processing.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c |   48 +++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 90c29db..b83e911 100644
> --- a/block.c
> +++ b/block.c
> @@ -78,6 +78,15 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
>      int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
>  static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>      int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
> +static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
> +                                               int64_t sector_num,
> +                                               QEMUIOVector *qiov,
> +                                               int nb_sectors,
> +                                               BlockDriverCompletionFunc *cb,
> +                                               void *opaque,
> +                                               bool is_write,
> +                                               CoroutineEntry *entry);
> +static void coroutine_fn bdrv_co_do_rw(void *opaque);
>  
>  static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
>      QTAILQ_HEAD_INITIALIZER(bdrv_states);
> @@ -2346,17 +2355,10 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
>                                   QEMUIOVector *qiov, int nb_sectors,
>                                   BlockDriverCompletionFunc *cb, void *opaque)
>  {
> -    BlockDriver *drv = bs->drv;
> -
>      trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
>  
> -    if (!drv)
> -        return NULL;
> -    if (bdrv_check_request(bs, sector_num, nb_sectors))
> -        return NULL;
> -
> -    return drv->bdrv_aio_readv(bs, sector_num, qiov, nb_sectors,
> -                               cb, opaque);
> +    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors,
> +                                 cb, opaque, false, bdrv_co_do_rw);
>  }
>  
>  typedef struct BlockCompleteData {
> @@ -2803,6 +2805,7 @@ static void bdrv_co_rw_bh(void *opaque)
>      qemu_aio_release(acb);
>  }
>  
> +/* Invoke .bdrv_co_readv/.bdrv_co_writev */
>  static void coroutine_fn bdrv_co_rw(void *opaque)
>  {
>      BlockDriverAIOCBCoroutine *acb = opaque;
> @@ -2820,13 +2823,32 @@ static void coroutine_fn bdrv_co_rw(void *opaque)
>      qemu_bh_schedule(acb->bh);
>  }
>  
> +/* Invoke bdrv_co_do_readv/bdrv_co_do_writev */
> +static void coroutine_fn bdrv_co_do_rw(void *opaque)
> +{
> +    BlockDriverAIOCBCoroutine *acb = opaque;
> +    BlockDriverState *bs = acb->common.bs;
> +
> +    if (!acb->is_write) {
> +        acb->req.error = bdrv_co_do_readv(bs, acb->req.sector,
> +            acb->req.nb_sectors, acb->req.qiov);
> +    } else {
> +        acb->req.error = bdrv_co_do_writev(bs, acb->req.sector,
> +            acb->req.nb_sectors, acb->req.qiov);
> +    }
> +
> +    acb->bh = qemu_bh_new(bdrv_co_rw_bh, acb);
> +    qemu_bh_schedule(acb->bh);
> +}

The difference between the existing bdrv_co_rw and the new bdrv_co_do_rw
is that the former directly calls drv->... whereas the latter does some
checks first.

I think you could just switch bdrv_co_rw to do the checks. If I'm not
mistaken, the other path is dead code anyway after this change.
Actually, it looks like this series leaves quite a bit of dead code
behind, but I need to apply all patches and check the result to be sure.

Kevin

[Qemu-devel] [PATCH 5/6] block: mark blocks dirty on coroutine write completion

[Stefan Hajnoczi]

The aio write operation marks blocks dirty when the write operation
completes.  The coroutine write operation marks blocks dirty before
issuing the write operation.

It seems safest to mark the block dirty when the operation completes so
that anything tracking dirty blocks will not act before the change has
been made to the image file.

Make the coroutine write operation dirty blocks on write completion.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/block.c b/block.c
index b83e911..5d6e17f 100644
--- a/block.c
+++ b/block.c
@@ -1307,6 +1307,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
     BlockDriver *drv = bs->drv;
+    int ret;
 
     if (!bs->drv) {
         return -ENOMEDIUM;
@@ -1318,6 +1319,8 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
         return -EIO;
     }
 
+    ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
+
     if (bs->dirty_bitmap) {
         set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
     }
@@ -1326,7 +1329,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
         bs->wr_highest_sector = sector_num + nb_sectors - 1;
     }
 
-    return drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
+    return ret;
 }
 
 int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
-- 
1.7.6.3

[Qemu-devel] [PATCH 6/6] block: switch bdrv_aio_writev() to coroutines

[Stefan Hajnoczi]

More sync, aio, and coroutine unification.  Make bdrv_aio_writev() go
through coroutine request processing.

Remove the dirty block callback mechanism which was needed only for aio
processing and can be done more naturally in coroutine context.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |   66 +-------------------------------------------------------------
 1 files changed, 2 insertions(+), 64 deletions(-)

diff --git a/block.c b/block.c
index 5d6e17f..e80121b 100644
--- a/block.c
+++ b/block.c
@@ -2364,76 +2364,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
                                  cb, opaque, false, bdrv_co_do_rw);
 }
 
-typedef struct BlockCompleteData {
-    BlockDriverCompletionFunc *cb;
-    void *opaque;
-    BlockDriverState *bs;
-    int64_t sector_num;
-    int nb_sectors;
-} BlockCompleteData;
-
-static void block_complete_cb(void *opaque, int ret)
-{
-    BlockCompleteData *b = opaque;
-
-    if (b->bs->dirty_bitmap) {
-        set_dirty_bitmap(b->bs, b->sector_num, b->nb_sectors, 1);
-    }
-    b->cb(b->opaque, ret);
-    g_free(b);
-}
-
-static BlockCompleteData *blk_dirty_cb_alloc(BlockDriverState *bs,
-                                             int64_t sector_num,
-                                             int nb_sectors,
-                                             BlockDriverCompletionFunc *cb,
-                                             void *opaque)
-{
-    BlockCompleteData *blkdata = g_malloc0(sizeof(BlockCompleteData));
-
-    blkdata->bs = bs;
-    blkdata->cb = cb;
-    blkdata->opaque = opaque;
-    blkdata->sector_num = sector_num;
-    blkdata->nb_sectors = nb_sectors;
-
-    return blkdata;
-}
-
 BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
                                   QEMUIOVector *qiov, int nb_sectors,
                                   BlockDriverCompletionFunc *cb, void *opaque)
 {
-    BlockDriver *drv = bs->drv;
-    BlockDriverAIOCB *ret;
-    BlockCompleteData *blk_cb_data;
-
     trace_bdrv_aio_writev(bs, sector_num, nb_sectors, opaque);
 
-    if (!drv)
-        return NULL;
-    if (bs->read_only)
-        return NULL;
-    if (bdrv_check_request(bs, sector_num, nb_sectors))
-        return NULL;
-
-    if (bs->dirty_bitmap) {
-        blk_cb_data = blk_dirty_cb_alloc(bs, sector_num, nb_sectors, cb,
-                                         opaque);
-        cb = &block_complete_cb;
-        opaque = blk_cb_data;
-    }
-
-    ret = drv->bdrv_aio_writev(bs, sector_num, qiov, nb_sectors,
-                               cb, opaque);
-
-    if (ret) {
-        if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
-            bs->wr_highest_sector = sector_num + nb_sectors - 1;
-        }
-    }
-
-    return ret;
+    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors,
+                                 cb, opaque, true, bdrv_co_do_rw);
 }
 
 
-- 
1.7.6.3

Re: [Qemu-devel] [PATCH 6/6] block: switch bdrv_aio_writev() to coroutines

[Zhi Yong Wu]

On Thu, Oct 6, 2011 at 12:17 AM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> More sync, aio, and coroutine unification.  Make bdrv_aio_writev() go
> through coroutine request processing.
>
> Remove the dirty block callback mechanism which was needed only for aio
> processing and can be done more naturally in coroutine context.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c |   66 +-------------------------------------------------------------
>  1 files changed, 2 insertions(+), 64 deletions(-)
>
> diff --git a/block.c b/block.c
> index 5d6e17f..e80121b 100644
> --- a/block.c
> +++ b/block.c
> @@ -2364,76 +2364,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
>                                  cb, opaque, false, bdrv_co_do_rw);
>  }
>
> -typedef struct BlockCompleteData {
> -    BlockDriverCompletionFunc *cb;
> -    void *opaque;
> -    BlockDriverState *bs;
> -    int64_t sector_num;
> -    int nb_sectors;
> -} BlockCompleteData;
> -
> -static void block_complete_cb(void *opaque, int ret)
> -{
> -    BlockCompleteData *b = opaque;
> -
> -    if (b->bs->dirty_bitmap) {
> -        set_dirty_bitmap(b->bs, b->sector_num, b->nb_sectors, 1);
> -    }
> -    b->cb(b->opaque, ret);
> -    g_free(b);
> -}
> -
> -static BlockCompleteData *blk_dirty_cb_alloc(BlockDriverState *bs,
> -                                             int64_t sector_num,
> -                                             int nb_sectors,
> -                                             BlockDriverCompletionFunc *cb,
> -                                             void *opaque)
> -{
> -    BlockCompleteData *blkdata = g_malloc0(sizeof(BlockCompleteData));
> -
> -    blkdata->bs = bs;
> -    blkdata->cb = cb;
> -    blkdata->opaque = opaque;
> -    blkdata->sector_num = sector_num;
> -    blkdata->nb_sectors = nb_sectors;
> -
> -    return blkdata;
> -}
> -
>  BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>                                   QEMUIOVector *qiov, int nb_sectors,
>                                   BlockDriverCompletionFunc *cb, void *opaque)
>  {
> -    BlockDriver *drv = bs->drv;
> -    BlockDriverAIOCB *ret;
> -    BlockCompleteData *blk_cb_data;
> -
>     trace_bdrv_aio_writev(bs, sector_num, nb_sectors, opaque);
>
> -    if (!drv)
> -        return NULL;
> -    if (bs->read_only)
> -        return NULL;
> -    if (bdrv_check_request(bs, sector_num, nb_sectors))
> -        return NULL;
> -
> -    if (bs->dirty_bitmap) {
> -        blk_cb_data = blk_dirty_cb_alloc(bs, sector_num, nb_sectors, cb,
> -                                         opaque);
> -        cb = &block_complete_cb;
> -        opaque = blk_cb_data;
> -    }
> -
> -    ret = drv->bdrv_aio_writev(bs, sector_num, qiov, nb_sectors,
> -                               cb, opaque);
> -
> -    if (ret) {
> -        if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
> -            bs->wr_highest_sector = sector_num + nb_sectors - 1;
> -        }
> -    }
As what is said in patch #3.
> -
> -    return ret;
> +    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors,
> +                                 cb, opaque, true, bdrv_co_do_rw);
>  }
>
>
> --
> 1.7.6.3
>
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 0/6] block: do request processing in a coroutine

[Zhi Yong Wu]

On Thu, Oct 6, 2011 at 12:17 AM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> Block layer features like dirty block tracing, I/O throttling, and live block
> copy are forced to duplicate code due to the three different interfaces:
> synchronous, asynchronous, and coroutines.
>
> Since there are bdrv_read(), bdrv_aio_readv(), and bdrv_co_readv() interfaces
> for read (and similar for write), per-request processing needs to be duplicated
> for each of these execution contexts.  For example, dirty block tracking code
> is duplicated across these three interfaces.
>
> This patch series unifies request processing so that there is only one code
> path.  I see this as a prerequisite to the live block copy (image streaming)
> code I am working on, so I'm pushing it now.
>
> The short-term win from this series is that it becomes easy to add live block
> copy and other features.  We now have a single code path where the perf-request
> processing is done.
>
> The longer-term win will be dropping the BlockDriver .bdrv_read(),
> .bdrv_write(), .bdrv_aio_readv(), and .bdrv_aio_writev() interfaces.  By doing
> that we can bring all BlockDrivers onto a common interface, namely
> .bdrv_co_readv() and .bdrv_co_writev().  It will also allow us to drop most of
> the sync and aio emulation code.
>
> A consequence of this patch series is that every I/O request goes through at
> least one coroutine.  There is no longer a direct .bdrv_read(), .bdrv_write(),
> .bdrv_aio_readv(), or .bdrv_aio_writev() call - we're trying to phase out those
> interfaces.  I have not noticed performance degradation in correctness tests
> but we need to confirm that there has not been a performance regression.
>
> Stefan Hajnoczi (6):
>  block: directly invoke .bdrv_aio_*() in bdrv_co_io_em()
>  block: split out bdrv_co_do_readv() and bdrv_co_do_writev()
>  block: switch bdrv_read()/bdrv_write() to coroutines
>  block: switch bdrv_aio_readv() to coroutines
>  block: mark blocks dirty on coroutine write completion
>  block: switch bdrv_aio_writev() to coroutines
>
>  block.c |  273 +++++++++++++++++++++++++++++++--------------------------------
>  1 files changed, 134 insertions(+), 139 deletions(-)
OK. When i am available, i will play with it.
>
> --
> 1.7.6.3
>
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 0/6] block: do request processing in a coroutine

[Kevin Wolf]

Am 05.10.2011 18:17, schrieb Stefan Hajnoczi:
> Block layer features like dirty block tracing, I/O throttling, and live block
> copy are forced to duplicate code due to the three different interfaces:
> synchronous, asynchronous, and coroutines.
> 
> Since there are bdrv_read(), bdrv_aio_readv(), and bdrv_co_readv() interfaces
> for read (and similar for write), per-request processing needs to be duplicated
> for each of these execution contexts.  For example, dirty block tracking code
> is duplicated across these three interfaces.
> 
> This patch series unifies request processing so that there is only one code
> path.  I see this as a prerequisite to the live block copy (image streaming)
> code I am working on, so I'm pushing it now.
> 
> The short-term win from this series is that it becomes easy to add live block
> copy and other features.  We now have a single code path where the perf-request
> processing is done.
> 
> The longer-term win will be dropping the BlockDriver .bdrv_read(),
> .bdrv_write(), .bdrv_aio_readv(), and .bdrv_aio_writev() interfaces.  By doing
> that we can bring all BlockDrivers onto a common interface, namely
> .bdrv_co_readv() and .bdrv_co_writev().  It will also allow us to drop most of
> the sync and aio emulation code.
> 
> A consequence of this patch series is that every I/O request goes through at
> least one coroutine.  There is no longer a direct .bdrv_read(), .bdrv_write(),
> .bdrv_aio_readv(), or .bdrv_aio_writev() call - we're trying to phase out those
> interfaces.  I have not noticed performance degradation in correctness tests
> but we need to confirm that there has not been a performance regression.
> 
> Stefan Hajnoczi (6):
>   block: directly invoke .bdrv_aio_*() in bdrv_co_io_em()
>   block: split out bdrv_co_do_readv() and bdrv_co_do_writev()
>   block: switch bdrv_read()/bdrv_write() to coroutines
>   block: switch bdrv_aio_readv() to coroutines
>   block: mark blocks dirty on coroutine write completion
>   block: switch bdrv_aio_writev() to coroutines
> 
>  block.c |  273 +++++++++++++++++++++++++++++++--------------------------------
>  1 files changed, 134 insertions(+), 139 deletions(-)
> 

Except for breaking synchronous drivers this looks good.

I have applied the first two patches to the block branch, so if you
don't need to change them, submitting patches 3-6 for v2 will be enough.

Kevin