[Qemu-devel] [PATCH 17/19] block/parallels: delay writing to BAT till bdrv_co_flush_to_os

[Denis V. Lunev]

The idea is that we do not need to immediately sync BAT to the image as
from the guest point of view there is a possibility that IO is lost
even in the physical controller until flush command was finished.
bdrv_co_flush_to_os is exactly the right place for this purpose.

Technically the patch aligns writes into BAT to MAX(bdrv_align, 4096),
which elliminates read-modify-write transactions on BAT update and
cache ready-to-write content in a special buffer in BDRVParallelsState.

This buffer possibly contains ParallelsHeader if the first page of the
image should be modified. The header occupies first 64 bytes of the image
and the BAT starts immediately after it.

It is also possible that BAT end is not aligned to the cluster size.
ParallelsHeader->data_off is not specified for this case. We should write
only part of the cache in that case.

This patch speed ups
  qemu-img create -f parallels -o cluster_size=64k ./1.hds 64G
  qemu-io -f parallels -c "write -P 0x11 0 1024k" 1.hds
writing from 50-60 Mb/sec to 80-90 Mb/sec on rotational media and
from 160 Mb/sec to 190 Mb/sec on SSD disk.

Signed-off-by: Denis V. Lunev <address@hidden>
CC: Kevin Wolf <address@hidden>
CC: Stefan Hajnoczi <address@hidden>
---
 block/parallels.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 94 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 46cf031..18b9267 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -62,6 +62,11 @@ typedef struct BDRVParallelsState {
     uint32_t *bat;
     unsigned int bat_size;
 
+    uint32_t *bat_cache;
+    unsigned bat_cache_size;
+    int bat_cache_off;
+    int data_off;
+
     unsigned int tracks;
 
     unsigned int off_multiplier;
@@ -148,6 +153,17 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
         le32_to_cpus(&s->bat[i]);
 
     qemu_co_mutex_init(&s->lock);
+
+    s->bat_cache_off = -1;
+    if (bs->open_flags & BDRV_O_RDWR) {
+        s->bat_cache_size = MAX(bdrv_opt_mem_align(bs->file), 4096);
+        s->bat_cache = qemu_blockalign(bs->file, s->bat_cache_size);
+    }
+    s->data_off = le32_to_cpu(s->ph.data_off) * BDRV_SECTOR_SIZE;
+    if (s->data_off == 0) {
+        s->data_off = ROUND_UP(bat_offset(s->bat_size), BDRV_SECTOR_SIZE);
+    }
+
     return 0;
 
 fail_format:
@@ -171,10 +187,71 @@ static int64_t seek_to_sector(BDRVParallelsState *s, 
int64_t sector_num)
     return (uint64_t)s->bat[index] * s->off_multiplier + offset;
 }
 
+static int write_bat_cache(BlockDriverState *bs)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int size, off;
+
+    if (s->bat_cache_off == -1) {
+        /* no cached data */
+        return 0;
+    }
+
+    size = s->bat_cache_size;
+    if (size + s->bat_cache_off > s->data_off) {
+        /* avoid writing to the first data block */
+        size = s->data_off - s->bat_cache_off;
+    }
+
+    off = s->bat_cache_off;
+    s->bat_cache_off = -1;
+    return bdrv_pwrite(bs->file, off, s->bat_cache, size);
+}
+
+static int cache_bat(BlockDriverState *bs, uint32_t idx, uint32_t new_data_off)
+{
+    int ret, i, off, cache_off;
+    int64_t first_idx, last_idx;
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t *cache = s->bat_cache;
+
+    off = bat_offset(idx);
+    cache_off = (off / s->bat_cache_size) * s->bat_cache_size;
+
+    if (s->bat_cache_off != -1 && s->bat_cache_off != cache_off) {
+        ret = write_bat_cache(bs);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    first_idx = idx - (off - cache_off) / sizeof(uint32_t);
+    last_idx = first_idx + s->bat_cache_size / sizeof(uint32_t);
+    if (first_idx < 0) {
+        memcpy(s->bat_cache, &s->ph, sizeof(s->ph));
+        first_idx = 0;
+        cache = s->bat_cache + sizeof(s->ph) / sizeof(uint32_t);
+    }
+
+    if (last_idx > s->bat_size) {
+        memset(cache + s->bat_size - first_idx, 0,
+               sizeof(uint32_t) * (last_idx - s->bat_size));
+    }
+
+    for (i = 0; i < last_idx - first_idx; i++) {
+        cache[i] = cpu_to_le32(s->bat[first_idx + i]);
+    }
+    cache[idx - first_idx] = cpu_to_le32(new_data_off);
+    s->bat[idx] = new_data_off;
+
+    s->bat_cache_off = cache_off;
+    return 0;
+}
+
 static int64_t allocate_sector(BlockDriverState *bs, int64_t sector_num)
 {
     BDRVParallelsState *s = bs->opaque;
-    uint32_t idx, offset, tmp;
+    uint32_t idx, offset;
     int64_t pos;
     int ret;
 
@@ -190,17 +267,27 @@ static int64_t allocate_sector(BlockDriverState *bs, 
int64_t sector_num)
 
     pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
     bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
-    s->bat[idx] = pos / s->off_multiplier;
-
-    tmp = cpu_to_le32(s->bat[idx]);
 
-    ret = bdrv_pwrite(bs->file, bat_offset(idx), &tmp, sizeof(tmp));
+    ret = cache_bat(bs, idx, pos / s->off_multiplier);
     if (ret < 0) {
         return ret;
     }
     return (uint64_t)s->bat[idx] * s->off_multiplier + offset;
 }
 
+static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int ret;
+
+    qemu_co_mutex_lock(&s->lock);
+    ret = write_bat_cache(bs);
+    qemu_co_mutex_unlock(&s->lock);
+
+    return ret;
+}
+
+
 static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
         int nb_sectors)
 {
@@ -387,6 +474,7 @@ exit:
 static void parallels_close(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
+    qemu_vfree(s->bat_cache);
     g_free(s->bat);
 }
 
@@ -416,6 +504,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_open         = parallels_open,
     .bdrv_close                = parallels_close,
     .bdrv_co_get_block_status = parallels_co_get_block_status,
+    .bdrv_co_flush_to_os      = parallels_co_flush_to_os,
     .bdrv_co_readv  = parallels_co_readv,
     .bdrv_co_writev = parallels_co_writev,
 
-- 
1.9.1