[Qemu-devel] [PATCH v3 0/27] write/create for Parallels images with reasonable performance

[Qemu-devel] [PATCH v3 0/27] write/create for Parallels images with reasonable performance

[Denis V. Lunev]

This patchset provides an ability to create of/write to Parallels
images and some testing of the new code. Readings and writings are
optimized out and I expect the same or slightly better performance
as qcow2.

This patchset consists of not problematic part of the previous
patchset aka
   [PATCH v4 0/16] parallels format support improvements
and new write/create code but it does not contradict questionable code
with XML handling there.

Changes from v3:
- fixed checkpatch warnings even in just moved code. I am tired of them :)
- fixed contingency check in patch 18

Changes from v2:
- read performance is almost doubled (up to 360 Mb/sec), write performance
  is improved by 15-20%
- bat caching approach changed completely. bat_bitmap now contains the data
  in on-disk format, which allows to use this data for metadata cache
- incorrect close detection code is added (inuse field in the header)
- very basic check consistency code added

Changes from v1:
- patches 13-19 added, which boosts performance from 800 KiB/sec to
  near native performance

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Roman Kagan <rkagan@parallels.com>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH 01/27] iotests, parallels: quote TEST_IMG in 076 test to be path-safe

[Denis V. Lunev]

suggested by Jeff Cody

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/076 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/076 b/tests/qemu-iotests/076
index ed2be35..0139976 100755
--- a/tests/qemu-iotests/076
+++ b/tests/qemu-iotests/076
@@ -49,31 +49,31 @@ nb_sectors_offset=$((0x24))
 echo
 echo "== Read from a valid v1 image =="
 _use_sample_img parallels-v1.bz2
-{ $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 0 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 echo
 echo "== Negative catalog size =="
 _use_sample_img parallels-v1.bz2
 poke_file "$TEST_IMG" "$catalog_entries_offset" "\xff\xff\xff\xff"
-{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read 0 512" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 echo
 echo "== Overflow in catalog allocation =="
 _use_sample_img parallels-v1.bz2
 poke_file "$TEST_IMG" "$nb_sectors_offset" "\xff\xff\xff\xff"
 poke_file "$TEST_IMG" "$catalog_entries_offset" "\x01\x00\x00\x40"
-{ $QEMU_IO -c "read 64M 64M" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read 64M 64M" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 echo
 echo "== Zero sectors per track =="
 _use_sample_img parallels-v1.bz2
 poke_file "$TEST_IMG" "$tracks_offset" "\x00\x00\x00\x00"
-{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read 0 512" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 echo
 echo "== Read from a valid v2 image =="
 _use_sample_img parallels-v2.bz2
-{ $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 0 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 # success, all done
 echo "*** done"
-- 
1.9.1

[Qemu-devel] [PATCH 02/27] block/parallels: rename parallels_header to ParallelsHeader

[Denis V. Lunev]

this follows QEMU coding convention

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 4f9cd8d..dca0df6 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -35,7 +35,7 @@
 #define HEADER_SIZE 64
 
 // always little-endian
-struct parallels_header {
+typedef struct ParallelsHeader {
     char magic[16]; // "WithoutFreeSpace"
     uint32_t version;
     uint32_t heads;
@@ -46,7 +46,7 @@ struct parallels_header {
     uint32_t inuse;
     uint32_t data_off;
     char padding[12];
-} QEMU_PACKED;
+} QEMU_PACKED ParallelsHeader;
 
 typedef struct BDRVParallelsState {
     CoMutex lock;
@@ -61,7 +61,7 @@ typedef struct BDRVParallelsState {
 
 static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
-    const struct parallels_header *ph = (const void *)buf;
+    const ParallelsHeader *ph = (const void *)buf;
 
     if (buf_size < HEADER_SIZE)
         return 0;
@@ -79,7 +79,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVParallelsState *s = bs->opaque;
     int i;
-    struct parallels_header ph;
+    ParallelsHeader ph;
     int ret;
 
     bs->read_only = 1; // no write support yet
-- 
1.9.1

[Qemu-devel] [PATCH 03/27] block/parallels: switch to bdrv_read

[Denis V. Lunev]

From: Roman Kagan <rkagan@parallels.com>

Switch the .bdrv_read method implementation from using bdrv_pread() to
bdrv_read() on the underlying file, since the latter is subject to i/o
throttling while the former is not.

Besides, since bdrv_read() operates in sectors rather than bytes, adjust
the helper functions to do so too.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index dca0df6..baefd3e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -146,9 +146,8 @@ fail:
     return ret;
 }
 
-static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
+static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
 {
-    BDRVParallelsState *s = bs->opaque;
     uint32_t index, offset;
 
     index = sector_num / s->tracks;
@@ -157,24 +156,27 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
     /* not allocated */
     if ((index >= s->catalog_size) || (s->catalog_bitmap[index] == 0))
         return -1;
-    return
-        ((uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset) * 512;
+    return (uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset;
 }
 
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
+    BDRVParallelsState *s = bs->opaque;
+
     while (nb_sectors > 0) {
-        int64_t position = seek_to_sector(bs, sector_num);
+        int64_t position = seek_to_sector(s, sector_num);
         if (position >= 0) {
-            if (bdrv_pread(bs->file, position, buf, 512) != 512)
-                return -1;
+            int ret = bdrv_read(bs->file, position, buf, 1);
+            if (ret < 0) {
+                return ret;
+            }
         } else {
-            memset(buf, 0, 512);
+            memset(buf, 0, BDRV_SECTOR_SIZE);
         }
         nb_sectors--;
         sector_num++;
-        buf += 512;
+        buf += BDRV_SECTOR_SIZE;
     }
     return 0;
 }
-- 
1.9.1

[Qemu-devel] [PATCH 04/27] block/parallels: read up to cluster end in one go

[Denis V. Lunev]

From: Roman Kagan <rkagan@parallels.com>

Teach parallels_read() to do reads in coarser granularity than just a
single sector: if requested, read up to the cluster end in one go.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index baefd3e..8770c82 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -159,6 +159,13 @@ static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
     return (uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset;
 }
 
+static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
+        int nb_sectors)
+{
+    int ret = s->tracks - sector_num % s->tracks;
+    return MIN(nb_sectors, ret);
+}
+
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
@@ -166,17 +173,18 @@ static int parallels_read(BlockDriverState *bs, int64_t sector_num,
 
     while (nb_sectors > 0) {
         int64_t position = seek_to_sector(s, sector_num);
+        int n = cluster_remainder(s, sector_num, nb_sectors);
         if (position >= 0) {
-            int ret = bdrv_read(bs->file, position, buf, 1);
+            int ret = bdrv_read(bs->file, position, buf, n);
             if (ret < 0) {
                 return ret;
             }
         } else {
-            memset(buf, 0, BDRV_SECTOR_SIZE);
+            memset(buf, 0, n << BDRV_SECTOR_BITS);
         }
-        nb_sectors--;
-        sector_num++;
-        buf += BDRV_SECTOR_SIZE;
+        nb_sectors -= n;
+        sector_num += n;
+        buf += n << BDRV_SECTOR_BITS;
     }
     return 0;
 }
-- 
1.9.1

[Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status

[Denis V. Lunev]

From: Roman Kagan <rkagan@parallels.com>

Implement VFS method for get_block_status to Parallels format driver.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 8770c82..b469984 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -166,6 +166,26 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
     return MIN(nb_sectors, ret);
 }
 
+static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *pnum)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t offset;
+
+    qemu_co_mutex_lock(&s->lock);
+    offset = seek_to_sector(s, sector_num);
+    qemu_co_mutex_unlock(&s->lock);
+
+    *pnum = cluster_remainder(s, sector_num, nb_sectors);
+
+    if (offset < 0) {
+        return 0;
+    }
+
+    return (offset << BDRV_SECTOR_BITS) |
+        BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+}
+
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
@@ -213,6 +233,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_open		= parallels_open,
     .bdrv_read          = parallels_co_read,
     .bdrv_close		= parallels_close,
+    .bdrv_co_get_block_status = parallels_co_get_block_status,
 };
 
 static void bdrv_parallels_init(void)
-- 
1.9.1

[Qemu-devel] [PATCH 06/27] block/parallels: provide _co_readv routine for parallels format driver

[Denis V. Lunev]

Main approach is taken from qcow2_co_readv.

The patch drops coroutine lock for the duration of IO operation and
peforms normal scatter-gather IO using standard QEMU backend.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index b469984..64b169b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -186,37 +186,45 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
         BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
 }
 
-static int parallels_read(BlockDriverState *bs, int64_t sector_num,
-                    uint8_t *buf, int nb_sectors)
+static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
     BDRVParallelsState *s = bs->opaque;
+    uint64_t bytes_done = 0;
+    QEMUIOVector hd_qiov;
+    int ret = 0;
 
+    qemu_iovec_init(&hd_qiov, qiov->niov);
+
+    qemu_co_mutex_lock(&s->lock);
     while (nb_sectors > 0) {
         int64_t position = seek_to_sector(s, sector_num);
         int n = cluster_remainder(s, sector_num, nb_sectors);
-        if (position >= 0) {
-            int ret = bdrv_read(bs->file, position, buf, n);
+        int nbytes = n << BDRV_SECTOR_BITS;
+
+        if (position < 0) {
+            qemu_iovec_memset(qiov, bytes_done, 0, nbytes);
+        } else {
+            qemu_iovec_reset(&hd_qiov);
+            qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
+
+            qemu_co_mutex_unlock(&s->lock);
+            ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
+            qemu_co_mutex_lock(&s->lock);
+
             if (ret < 0) {
-                return ret;
+                goto fail;
             }
-        } else {
-            memset(buf, 0, n << BDRV_SECTOR_BITS);
         }
+
         nb_sectors -= n;
         sector_num += n;
-        buf += n << BDRV_SECTOR_BITS;
+        bytes_done += nbytes;
     }
-    return 0;
-}
-
-static coroutine_fn int parallels_co_read(BlockDriverState *bs, int64_t sector_num,
-                                          uint8_t *buf, int nb_sectors)
-{
-    int ret;
-    BDRVParallelsState *s = bs->opaque;
-    qemu_co_mutex_lock(&s->lock);
-    ret = parallels_read(bs, sector_num, buf, nb_sectors);
     qemu_co_mutex_unlock(&s->lock);
+
+fail:
+    qemu_iovec_destroy(&hd_qiov);
     return ret;
 }
 
@@ -231,9 +239,9 @@ static BlockDriver bdrv_parallels = {
     .instance_size	= sizeof(BDRVParallelsState),
     .bdrv_probe		= parallels_probe,
     .bdrv_open		= parallels_open,
-    .bdrv_read          = parallels_co_read,
     .bdrv_close		= parallels_close,
     .bdrv_co_get_block_status = parallels_co_get_block_status,
+    .bdrv_co_readv  = parallels_co_readv,
 };
 
 static void bdrv_parallels_init(void)
-- 
1.9.1

[Qemu-devel] [PATCH 07/27] block/parallels: replace magic constants 4, 64 with proper sizeofs

[Denis V. Lunev]

simple purification..

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 64b169b..306f2e3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -32,7 +32,6 @@
 #define HEADER_MAGIC "WithoutFreeSpace"
 #define HEADER_MAGIC2 "WithouFreSpacExt"
 #define HEADER_VERSION 2
-#define HEADER_SIZE 64
 
 // always little-endian
 typedef struct ParallelsHeader {
@@ -63,7 +62,7 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
 {
     const ParallelsHeader *ph = (const void *)buf;
 
-    if (buf_size < HEADER_SIZE)
+    if (buf_size < sizeof(ParallelsHeader))
         return 0;
 
     if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
@@ -116,7 +115,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->catalog_size = le32_to_cpu(ph.catalog_entries);
-    if (s->catalog_size > INT_MAX / 4) {
+    if (s->catalog_size > INT_MAX / sizeof(uint32_t)) {
         error_setg(errp, "Catalog too large");
         ret = -EFBIG;
         goto fail;
@@ -127,7 +126,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_pread(bs->file, 64, s->catalog_bitmap, s->catalog_size * 4);
+    ret = bdrv_pread(bs->file, sizeof(ParallelsHeader),
+                     s->catalog_bitmap, s->catalog_size * sizeof(uint32_t));
     if (ret < 0) {
         goto fail;
     }
-- 
1.9.1

[Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Denis V. Lunev]

Support write on Parallels images. The code is almost the same as one
in the previous patch implemented scatter-gather IO for read.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 306f2e3..61d7da7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -81,8 +81,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     ParallelsHeader ph;
     int ret;
 
-    bs->read_only = 1; // no write support yet
-
     ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
     if (ret < 0) {
         goto fail;
@@ -166,6 +164,37 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
     return MIN(nb_sectors, ret);
 }
 
+static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t idx, offset, tmp;
+    int64_t pos;
+    int ret;
+
+    idx = sector_num / s->tracks;
+    offset = sector_num % s->tracks;
+
+    if (idx >= s->catalog_size) {
+        return -EINVAL;
+    }
+    if (s->catalog_bitmap[idx] != 0) {
+        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
+    }
+
+    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
+    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
+    s->catalog_bitmap[idx] = pos / s->off_multiplier;
+
+    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
+
+    ret = bdrv_pwrite_sync(bs->file,
+            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
+    if (ret < 0) {
+        return ret;
+    }
+    return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
+}
+
 static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -186,6 +215,49 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
         BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
 }
 
+static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint64_t bytes_done = 0;
+    QEMUIOVector hd_qiov;
+    int ret = 0;
+
+    qemu_iovec_init(&hd_qiov, qiov->niov);
+
+    qemu_co_mutex_lock(&s->lock);
+    while (nb_sectors > 0) {
+        int64_t position = allocate_cluster(bs, sector_num);
+        int n = cluster_remainder(s, sector_num, nb_sectors);
+        int nbytes = n << BDRV_SECTOR_BITS;
+
+        if (position < 0) {
+            ret = (int)position;
+            break;
+        }
+
+        qemu_iovec_reset(&hd_qiov);
+        qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
+
+        qemu_co_mutex_unlock(&s->lock);
+        ret = bdrv_co_writev(bs->file, position, n, &hd_qiov);
+        qemu_co_mutex_lock(&s->lock);
+
+        if (ret < 0) {
+            goto fail;
+        }
+
+        nb_sectors -= n;
+        sector_num += n;
+        bytes_done += nbytes;
+    }
+    qemu_co_mutex_unlock(&s->lock);
+
+fail:
+    qemu_iovec_destroy(&hd_qiov);
+    return ret;
+}
+
 static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
@@ -242,6 +314,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_close		= parallels_close,
     .bdrv_co_get_block_status = parallels_co_get_block_status,
     .bdrv_co_readv  = parallels_co_readv,
+    .bdrv_co_writev = parallels_co_writev,
 };
 
 static void bdrv_parallels_init(void)
-- 
1.9.1

[Qemu-devel] [PATCH 09/27] iotests, parallels: test for write into Parallels image

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/076     |  5 +++++
 tests/qemu-iotests/076.out | 10 ++++++++++
 2 files changed, 15 insertions(+)

diff --git a/tests/qemu-iotests/076 b/tests/qemu-iotests/076
index 0139976..c9b55a9 100755
--- a/tests/qemu-iotests/076
+++ b/tests/qemu-iotests/076
@@ -74,6 +74,11 @@ echo
 echo "== Read from a valid v2 image =="
 _use_sample_img parallels-v2.bz2
 { $QEMU_IO -c "read -P 0x11 0 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -P 0x21 1024k 1k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -P 0x22 1025k 1k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x21 1024k 1k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x22 1025k 1k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 1026k 62k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/076.out b/tests/qemu-iotests/076.out
index 32ade08..b0000ae 100644
--- a/tests/qemu-iotests/076.out
+++ b/tests/qemu-iotests/076.out
@@ -19,4 +19,14 @@ no file open, try 'help open'
 == Read from a valid v2 image ==
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 1048576
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 1049600
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1024/1024 bytes at offset 1048576
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1024/1024 bytes at offset 1049600
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 63488/63488 bytes at offset 1050624
+62 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
1.9.1

[Qemu-devel] [PATCH 10/27] block/parallels: support parallels image creation

[Denis V. Lunev]

Do not even care to create WithoutFreeSpace image, it is obsolete.
Always create WithouFreSpacExt one.

The code also does not spend a lot of efforts to fill cylinders and
heads fields, they are not used actually in a real life neither in
QEMU nor in Parallels products.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviwed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 61d7da7..28338ec 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -33,6 +33,9 @@
 #define HEADER_MAGIC2 "WithouFreSpacExt"
 #define HEADER_VERSION 2
 
+#define DEFAULT_CLUSTER_SIZE 1048576        /* 1 MiB */
+
+
 // always little-endian
 typedef struct ParallelsHeader {
     char magic[16]; // "WithoutFreeSpace"
@@ -300,12 +303,103 @@ fail:
     return ret;
 }
 
+static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
+{
+    int64_t total_size, cl_size;
+    uint8_t tmp[BDRV_SECTOR_SIZE];
+    Error *local_err = NULL;
+    BlockDriverState *file;
+    uint32_t cat_entries, cat_sectors;
+    ParallelsHeader header;
+    int ret;
+
+    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+                          BDRV_SECTOR_SIZE);
+    cl_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
+                          DEFAULT_CLUSTER_SIZE), BDRV_SECTOR_SIZE);
+
+    ret = bdrv_create_file(filename, opts, &local_err);
+    if (ret < 0) {
+        error_propagate(errp, local_err);
+        return ret;
+    }
+
+    file = NULL;
+    ret = bdrv_open(&file, filename, NULL, NULL,
+                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
+    if (ret < 0) {
+        error_propagate(errp, local_err);
+        return ret;
+    }
+    ret = bdrv_truncate(file, 0);
+    if (ret < 0) {
+        goto exit;
+    }
+
+    cat_entries = DIV_ROUND_UP(total_size, cl_size);
+    cat_sectors = DIV_ROUND_UP(cat_entries * sizeof(uint32_t) +
+                               sizeof(ParallelsHeader), cl_size);
+    cat_sectors = (cat_sectors *  cl_size) >> BDRV_SECTOR_BITS;
+
+    memset(&header, 0, sizeof(header));
+    memcpy(header.magic, HEADER_MAGIC2, sizeof(header.magic));
+    header.version = cpu_to_le32(HEADER_VERSION);
+    /* don't care much about geometry, it is not used on image level */
+    header.heads = cpu_to_le32(16);
+    header.cylinders = cpu_to_le32(total_size / BDRV_SECTOR_SIZE / 16 / 32);
+    header.tracks = cpu_to_le32(cl_size >> BDRV_SECTOR_BITS);
+    header.catalog_entries = cpu_to_le32(cat_entries);
+    header.nb_sectors = cpu_to_le64(DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE));
+    header.data_off = cpu_to_le32(cat_sectors);
+
+    /* write all the data */
+    memset(tmp, 0, sizeof(tmp));
+    memcpy(tmp, &header, sizeof(header));
+
+    ret = bdrv_pwrite(file, 0, tmp, BDRV_SECTOR_SIZE);
+    if (ret < 0) {
+        goto exit;
+    }
+    ret = bdrv_write_zeroes(file, 1, cat_sectors - 1, 0);
+    if (ret < 0) {
+        goto exit;
+    }
+    ret = 0;
+
+done:
+    bdrv_unref(file);
+    return ret;
+
+exit:
+    error_setg_errno(errp, -ret, "Failed to create Parallels image");
+    goto done;
+}
+
 static void parallels_close(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
     g_free(s->catalog_bitmap);
 }
 
+static QemuOptsList parallels_create_opts = {
+    .name = "parallels-create-opts",
+    .head = QTAILQ_HEAD_INITIALIZER(parallels_create_opts.head),
+    .desc = {
+        {
+            .name = BLOCK_OPT_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "Virtual disk size",
+        },
+        {
+            .name = BLOCK_OPT_CLUSTER_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "Parallels image cluster size",
+            .def_value_str = stringify(DEFAULT_CLUSTER_SIZE),
+        },
+        { /* end of list */ }
+    }
+};
+
 static BlockDriver bdrv_parallels = {
     .format_name	= "parallels",
     .instance_size	= sizeof(BDRVParallelsState),
@@ -315,6 +409,9 @@ static BlockDriver bdrv_parallels = {
     .bdrv_co_get_block_status = parallels_co_get_block_status,
     .bdrv_co_readv  = parallels_co_readv,
     .bdrv_co_writev = parallels_co_writev,
+
+    .bdrv_create    = parallels_create,
+    .create_opts    = &parallels_create_opts,
 };
 
 static void bdrv_parallels_init(void)
-- 
1.9.1

[Qemu-devel] [PATCH 11/27] iotests, parallels: test for newly created parallels image via qemu-img

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviwed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/115     | 68 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/115.out | 24 ++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 93 insertions(+)
 create mode 100755 tests/qemu-iotests/115
 create mode 100644 tests/qemu-iotests/115.out

diff --git a/tests/qemu-iotests/115 b/tests/qemu-iotests/115
new file mode 100755
index 0000000..f45afa7
--- /dev/null
+++ b/tests/qemu-iotests/115
@@ -0,0 +1,68 @@
+#!/bin/bash
+#
+# parallels format validation tests (created by QEMU)
+#
+# Copyright (C) 2014 Denis V. Lunev <den@openvz.org>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=den@openvz.org
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt parallels
+_supported_proto file
+_supported_os Linux
+
+size=64M
+CLUSTER_SIZE=64k
+IMGFMT=parallels
+_make_test_img $size
+
+echo == read empty image ==
+{ $QEMU_IO -c "read -P 0 32k 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo == write more than 1 block in a row ==
+{ $QEMU_IO -c "write -P 0x11 32k 128k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo == read less than block ==
+{ $QEMU_IO -c "read -P 0x11 32k 32k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo == read exactly 1 block ==
+{ $QEMU_IO -c "read -P 0x11 64k 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo == read more than 1 block ==
+{ $QEMU_IO -c "read -P 0x11 32k 128k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo == check that there is no trash after written ==
+{ $QEMU_IO -c "read -P 0 160k 32k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo == check that there is no trash before written ==
+{ $QEMU_IO -c "read -P 0 0 32k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/115.out b/tests/qemu-iotests/115.out
new file mode 100644
index 0000000..a06f1a2
--- /dev/null
+++ b/tests/qemu-iotests/115.out
@@ -0,0 +1,24 @@
+QA output created by 115
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+== read empty image ==
+read 65536/65536 bytes at offset 32768
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write more than 1 block in a row ==
+wrote 131072/131072 bytes at offset 32768
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== read less than block ==
+read 32768/32768 bytes at offset 32768
+32 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== read exactly 1 block ==
+read 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== read more than 1 block ==
+read 131072/131072 bytes at offset 32768
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check that there is no trash after written ==
+read 32768/32768 bytes at offset 163840
+32 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check that there is no trash before written ==
+read 32768/32768 bytes at offset 0
+32 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0d3b95c..3794af7 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -118,5 +118,6 @@
 111 rw auto quick
 113 rw auto quick
 114 rw auto quick
+115 rw auto quick
 116 rw auto quick
 123 rw auto quick
-- 
1.9.1

[Qemu-devel] [PATCH 12/27] parallels: change copyright information in the image header

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviwed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index 28338ec..7ef3136 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -2,8 +2,12 @@
  * Block driver for Parallels disk image format
  *
  * Copyright (c) 2007 Alex Beregszaszi
+ * Copyright (c) 2014 Denis V. Lunev <den@openvz.org>
  *
- * This code is based on comparing different disk images created by Parallels.
+ * This code was originally based on comparing different disk images created
+ * by Parallels. Currently it is based on opened OpenVZ sources
+ * available at
+ *     http://git.openvz.org/?p=ploop;a=summary
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
-- 
1.9.1

[Qemu-devel] [PATCH 13/27] block/parallels: rename catalog_ names to bat_

[Denis V. Lunev]

BAT means 'block allocation table'. Thus this name is clean and shorter
on writing.

Some obvious formatting fixes in the old code were made to make checkpatch
happy.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 56 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 7ef3136..a7c66dc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -47,7 +47,7 @@ typedef struct ParallelsHeader {
     uint32_t heads;
     uint32_t cylinders;
     uint32_t tracks;
-    uint32_t catalog_entries;
+    uint32_t bat_entries;
     uint64_t nb_sectors;
     uint32_t inuse;
     uint32_t data_off;
@@ -57,8 +57,8 @@ typedef struct ParallelsHeader {
 typedef struct BDRVParallelsState {
     CoMutex lock;
 
-    uint32_t *catalog_bitmap;
-    unsigned int catalog_size;
+    uint32_t *bat_bitmap;
+    unsigned int bat_size;
 
     unsigned int tracks;
 
@@ -119,26 +119,27 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    s->catalog_size = le32_to_cpu(ph.catalog_entries);
-    if (s->catalog_size > INT_MAX / sizeof(uint32_t)) {
+    s->bat_size = le32_to_cpu(ph.bat_entries);
+    if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
         error_setg(errp, "Catalog too large");
         ret = -EFBIG;
         goto fail;
     }
-    s->catalog_bitmap = g_try_new(uint32_t, s->catalog_size);
-    if (s->catalog_size && s->catalog_bitmap == NULL) {
+    s->bat_bitmap = g_try_new(uint32_t, s->bat_size);
+    if (s->bat_size && s->bat_bitmap == NULL) {
         ret = -ENOMEM;
         goto fail;
     }
 
     ret = bdrv_pread(bs->file, sizeof(ParallelsHeader),
-                     s->catalog_bitmap, s->catalog_size * sizeof(uint32_t));
+                     s->bat_bitmap, s->bat_size * sizeof(uint32_t));
     if (ret < 0) {
         goto fail;
     }
 
-    for (i = 0; i < s->catalog_size; i++)
-        le32_to_cpus(&s->catalog_bitmap[i]);
+    for (i = 0; i < s->bat_size; i++) {
+        le32_to_cpus(&s->bat_bitmap[i]);
+    }
 
     qemu_co_mutex_init(&s->lock);
     return 0;
@@ -147,7 +148,7 @@ fail_format:
     error_setg(errp, "Image not in Parallels format");
     ret = -EINVAL;
 fail:
-    g_free(s->catalog_bitmap);
+    g_free(s->bat_bitmap);
     return ret;
 }
 
@@ -159,9 +160,10 @@ static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
     offset = sector_num % s->tracks;
 
     /* not allocated */
-    if ((index >= s->catalog_size) || (s->catalog_bitmap[index] == 0))
+    if ((index >= s->bat_size) || (s->bat_bitmap[index] == 0)) {
         return -1;
-    return (uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset;
+    }
+    return (uint64_t)s->bat_bitmap[index] * s->off_multiplier + offset;
 }
 
 static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
@@ -181,25 +183,25 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     idx = sector_num / s->tracks;
     offset = sector_num % s->tracks;
 
-    if (idx >= s->catalog_size) {
+    if (idx >= s->bat_size) {
         return -EINVAL;
     }
-    if (s->catalog_bitmap[idx] != 0) {
-        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
+    if (s->bat_bitmap[idx] != 0) {
+        return (uint64_t)s->bat_bitmap[idx] * s->off_multiplier + offset;
     }
 
     pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
     bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
-    s->catalog_bitmap[idx] = pos / s->off_multiplier;
+    s->bat_bitmap[idx] = pos / s->off_multiplier;
 
-    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
+    tmp = cpu_to_le32(s->bat_bitmap[idx]);
 
     ret = bdrv_pwrite_sync(bs->file,
             sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
     if (ret < 0) {
         return ret;
     }
-    return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
+    return (uint64_t)s->bat_bitmap[idx] * s->off_multiplier + offset;
 }
 
 static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
@@ -313,7 +315,7 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
     uint8_t tmp[BDRV_SECTOR_SIZE];
     Error *local_err = NULL;
     BlockDriverState *file;
-    uint32_t cat_entries, cat_sectors;
+    uint32_t bat_entries, bat_sectors;
     ParallelsHeader header;
     int ret;
 
@@ -340,10 +342,10 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
         goto exit;
     }
 
-    cat_entries = DIV_ROUND_UP(total_size, cl_size);
-    cat_sectors = DIV_ROUND_UP(cat_entries * sizeof(uint32_t) +
+    bat_entries = DIV_ROUND_UP(total_size, cl_size);
+    bat_sectors = DIV_ROUND_UP(bat_entries * sizeof(uint32_t) +
                                sizeof(ParallelsHeader), cl_size);
-    cat_sectors = (cat_sectors *  cl_size) >> BDRV_SECTOR_BITS;
+    bat_sectors = (bat_sectors *  cl_size) >> BDRV_SECTOR_BITS;
 
     memset(&header, 0, sizeof(header));
     memcpy(header.magic, HEADER_MAGIC2, sizeof(header.magic));
@@ -352,9 +354,9 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
     header.heads = cpu_to_le32(16);
     header.cylinders = cpu_to_le32(total_size / BDRV_SECTOR_SIZE / 16 / 32);
     header.tracks = cpu_to_le32(cl_size >> BDRV_SECTOR_BITS);
-    header.catalog_entries = cpu_to_le32(cat_entries);
+    header.bat_entries = cpu_to_le32(bat_entries);
     header.nb_sectors = cpu_to_le64(DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE));
-    header.data_off = cpu_to_le32(cat_sectors);
+    header.data_off = cpu_to_le32(bat_sectors);
 
     /* write all the data */
     memset(tmp, 0, sizeof(tmp));
@@ -364,7 +366,7 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
     if (ret < 0) {
         goto exit;
     }
-    ret = bdrv_write_zeroes(file, 1, cat_sectors - 1, 0);
+    ret = bdrv_write_zeroes(file, 1, bat_sectors - 1, 0);
     if (ret < 0) {
         goto exit;
     }
@@ -382,7 +384,7 @@ exit:
 static void parallels_close(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
-    g_free(s->catalog_bitmap);
+    g_free(s->bat_bitmap);
 }
 
 static QemuOptsList parallels_create_opts = {
-- 
1.9.1

[Qemu-devel] [PATCH 14/27] block/parallels: create bat2sect helper

[Denis V. Lunev]

deduplicate copy/paste arithmetcs

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index a7c66dc..6bc5e62 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -152,6 +152,12 @@ fail:
     return ret;
 }
 
+
+static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx)
+{
+    return (uint64_t)s->bat_bitmap[idx] * s->off_multiplier;
+}
+
 static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
 {
     uint32_t index, offset;
@@ -163,7 +169,7 @@ static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
     if ((index >= s->bat_size) || (s->bat_bitmap[index] == 0)) {
         return -1;
     }
-    return (uint64_t)s->bat_bitmap[index] * s->off_multiplier + offset;
+    return bat2sect(s, index) + offset;
 }
 
 static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
@@ -187,7 +193,7 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
         return -EINVAL;
     }
     if (s->bat_bitmap[idx] != 0) {
-        return (uint64_t)s->bat_bitmap[idx] * s->off_multiplier + offset;
+        return bat2sect(s, idx) + offset;
     }
 
     pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
@@ -201,7 +207,7 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     if (ret < 0) {
         return ret;
     }
-    return (uint64_t)s->bat_bitmap[idx] * s->off_multiplier + offset;
+    return bat2sect(s, idx) + offset;
 }
 
 static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
-- 
1.9.1

[Qemu-devel] [PATCH 15/27] block/parallels: keep BAT bitmap data in little endian in memory

[Denis V. Lunev]

This will allow to use this data as buffer to BAT update directly
without any intermediate buffers.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 6bc5e62..8301de4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -84,7 +84,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
     BDRVParallelsState *s = bs->opaque;
-    int i;
     ParallelsHeader ph;
     int ret;
 
@@ -137,10 +136,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    for (i = 0; i < s->bat_size; i++) {
-        le32_to_cpus(&s->bat_bitmap[i]);
-    }
-
     qemu_co_mutex_init(&s->lock);
     return 0;
 
@@ -155,7 +150,7 @@ fail:
 
 static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx)
 {
-    return (uint64_t)s->bat_bitmap[idx] * s->off_multiplier;
+    return (uint64_t)le32_to_cpu(s->bat_bitmap[idx]) * s->off_multiplier;
 }
 
 static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
@@ -182,7 +177,7 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
 static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
 {
     BDRVParallelsState *s = bs->opaque;
-    uint32_t idx, offset, tmp;
+    uint32_t idx, offset;
     int64_t pos;
     int ret;
 
@@ -198,12 +193,11 @@ static int64_t allocate_cluster(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_bitmap[idx] = pos / s->off_multiplier;
-
-    tmp = cpu_to_le32(s->bat_bitmap[idx]);
 
+    s->bat_bitmap[idx] = cpu_to_le32(pos / s->off_multiplier);
     ret = bdrv_pwrite_sync(bs->file,
-            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
+            sizeof(ParallelsHeader) + idx * sizeof(s->bat_bitmap[idx]),
+            s->bat_bitmap + idx, sizeof(s->bat_bitmap[idx]));
     if (ret < 0) {
         return ret;
     }
-- 
1.9.1

[Qemu-devel] [PATCH 16/27] block/parallels: read parallels image header and BAT into single buffer

[Denis V. Lunev]

This metadata cache would allow to properly batch BAT updates to disk
in next patches. These updates will be properly aligned to avoid
read-modify-write transactions on block level.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 8301de4..3d515dd 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -57,6 +57,8 @@ typedef struct ParallelsHeader {
 typedef struct BDRVParallelsState {
     CoMutex lock;
 
+    ParallelsHeader *header;
+    uint32_t header_size;
     uint32_t *bat_bitmap;
     unsigned int bat_size;
 
@@ -85,7 +87,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
-    int ret;
+    int ret, size;
 
     ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
     if (ret < 0) {
@@ -124,17 +126,25 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EFBIG;
         goto fail;
     }
-    s->bat_bitmap = g_try_new(uint32_t, s->bat_size);
-    if (s->bat_size && s->bat_bitmap == NULL) {
+
+    size = sizeof(ParallelsHeader) + sizeof(uint32_t) * s->bat_size;
+    s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file));
+    s->header = qemu_try_blockalign(bs->file, s->header_size);
+    if (s->header == NULL) {
         ret = -ENOMEM;
         goto fail;
     }
+    if (le32_to_cpu(ph.data_off) < s->header_size) {
+        /* there is not enough unused space to fit to block align between BAT
+           and actual data. We can't avoid read-modify-write... */
+        s->header_size = size;
+    }
 
-    ret = bdrv_pread(bs->file, sizeof(ParallelsHeader),
-                     s->bat_bitmap, s->bat_size * sizeof(uint32_t));
+    ret = bdrv_pread(bs->file, 0, s->header, s->header_size);
     if (ret < 0) {
         goto fail;
     }
+    s->bat_bitmap = (uint32_t *)(s->header + 1);
 
     qemu_co_mutex_init(&s->lock);
     return 0;
@@ -143,7 +153,7 @@ fail_format:
     error_setg(errp, "Image not in Parallels format");
     ret = -EINVAL;
 fail:
-    g_free(s->bat_bitmap);
+    qemu_vfree(s->header);
     return ret;
 }
 
@@ -384,7 +394,7 @@ exit:
 static void parallels_close(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
-    g_free(s->bat_bitmap);
+    qemu_vfree(s->header);
 }
 
 static QemuOptsList parallels_create_opts = {
-- 
1.9.1

[Qemu-devel] [PATCH 17/27] block/parallels: move parallels_open/probe to the very end of the file

[Denis V. Lunev]

This will help to avoid forward declarations for upcoming parallels_check

Some very obvious formatting fixes were made to the moved code to make
checkpatch happy.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 185 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 95 insertions(+), 90 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 3d515dd..3d71624 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -67,96 +67,6 @@ typedef struct BDRVParallelsState {
     unsigned int off_multiplier;
 } BDRVParallelsState;
 
-static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
-{
-    const ParallelsHeader *ph = (const void *)buf;
-
-    if (buf_size < sizeof(ParallelsHeader))
-        return 0;
-
-    if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
-        !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
-        (le32_to_cpu(ph->version) == HEADER_VERSION))
-        return 100;
-
-    return 0;
-}
-
-static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
-                          Error **errp)
-{
-    BDRVParallelsState *s = bs->opaque;
-    ParallelsHeader ph;
-    int ret, size;
-
-    ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
-    if (ret < 0) {
-        goto fail;
-    }
-
-    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
-
-    if (le32_to_cpu(ph.version) != HEADER_VERSION) {
-        goto fail_format;
-    }
-    if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
-        s->off_multiplier = 1;
-        bs->total_sectors = 0xffffffff & bs->total_sectors;
-    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
-        s->off_multiplier = le32_to_cpu(ph.tracks);
-    } else {
-        goto fail_format;
-    }
-
-    s->tracks = le32_to_cpu(ph.tracks);
-    if (s->tracks == 0) {
-        error_setg(errp, "Invalid image: Zero sectors per track");
-        ret = -EINVAL;
-        goto fail;
-    }
-    if (s->tracks > INT32_MAX/513) {
-        error_setg(errp, "Invalid image: Too big cluster");
-        ret = -EFBIG;
-        goto fail;
-    }
-
-    s->bat_size = le32_to_cpu(ph.bat_entries);
-    if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
-        error_setg(errp, "Catalog too large");
-        ret = -EFBIG;
-        goto fail;
-    }
-
-    size = sizeof(ParallelsHeader) + sizeof(uint32_t) * s->bat_size;
-    s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file));
-    s->header = qemu_try_blockalign(bs->file, s->header_size);
-    if (s->header == NULL) {
-        ret = -ENOMEM;
-        goto fail;
-    }
-    if (le32_to_cpu(ph.data_off) < s->header_size) {
-        /* there is not enough unused space to fit to block align between BAT
-           and actual data. We can't avoid read-modify-write... */
-        s->header_size = size;
-    }
-
-    ret = bdrv_pread(bs->file, 0, s->header, s->header_size);
-    if (ret < 0) {
-        goto fail;
-    }
-    s->bat_bitmap = (uint32_t *)(s->header + 1);
-
-    qemu_co_mutex_init(&s->lock);
-    return 0;
-
-fail_format:
-    error_setg(errp, "Image not in Parallels format");
-    ret = -EINVAL;
-fail:
-    qemu_vfree(s->header);
-    return ret;
-}
-
 
 static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx)
 {
@@ -391,6 +301,101 @@ exit:
     goto done;
 }
 
+
+static int parallels_probe(const uint8_t *buf, int buf_size,
+                           const char *filename)
+{
+    const ParallelsHeader *ph = (const void *)buf;
+
+    if (buf_size < sizeof(ParallelsHeader)) {
+        return 0;
+    }
+
+    if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
+        !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
+        (le32_to_cpu(ph->version) == HEADER_VERSION)) {
+        return 100;
+    }
+
+    return 0;
+}
+
+static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
+                          Error **errp)
+{
+    BDRVParallelsState *s = bs->opaque;
+    ParallelsHeader ph;
+    int ret, size;
+
+    ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
+
+    if (le32_to_cpu(ph.version) != HEADER_VERSION) {
+        goto fail_format;
+    }
+    if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
+        s->off_multiplier = 1;
+        bs->total_sectors = 0xffffffff & bs->total_sectors;
+    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
+        s->off_multiplier = le32_to_cpu(ph.tracks);
+    } else {
+        goto fail_format;
+    }
+
+    s->tracks = le32_to_cpu(ph.tracks);
+    if (s->tracks == 0) {
+        error_setg(errp, "Invalid image: Zero sectors per track");
+        ret = -EINVAL;
+        goto fail;
+    }
+    if (s->tracks > INT32_MAX/513) {
+        error_setg(errp, "Invalid image: Too big cluster");
+        ret = -EFBIG;
+        goto fail;
+    }
+
+    s->bat_size = le32_to_cpu(ph.bat_entries);
+    if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
+        error_setg(errp, "Catalog too large");
+        ret = -EFBIG;
+        goto fail;
+    }
+
+    size = sizeof(ParallelsHeader) + sizeof(uint32_t) * s->bat_size;
+    s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file));
+    s->header = qemu_try_blockalign(bs->file, s->header_size);
+    if (s->header == NULL) {
+        ret = -ENOMEM;
+        goto fail;
+    }
+    if (le32_to_cpu(ph.data_off) < s->header_size) {
+        /* there is not enough unused space to fit to block align between BAT
+           and actual data. We can't avoid read-modify-write... */
+        s->header_size = size;
+    }
+
+    ret = bdrv_pread(bs->file, 0, s->header, s->header_size);
+    if (ret < 0) {
+        goto fail;
+    }
+    s->bat_bitmap = (uint32_t *)(s->header + 1);
+
+    qemu_co_mutex_init(&s->lock);
+    return 0;
+
+fail_format:
+    error_setg(errp, "Image not in Parallels format");
+    ret = -EINVAL;
+fail:
+    qemu_vfree(s->header);
+    return ret;
+}
+
+
 static void parallels_close(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
-- 
1.9.1

[Qemu-devel] [PATCH 18/27] block/parallels: implement parallels_check method of block driver

[Denis V. Lunev]

The check is very simple at the moment. It calculates necessary stats
and fix only the following errors:
- space leak at the end of the image. This would happens due to
  preallocation
- clusters outside the image are zeroed. Nothing else could be done here

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 3d71624..0e5d4c3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -229,6 +229,90 @@ fail:
     return ret;
 }
 
+
+static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
+                           BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t size, prev_off, high_off;
+    int ret;
+    uint32_t i;
+    bool flush_bat = false;
+    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
+
+    size = bdrv_getlength(bs->file);
+    if (size < 0) {
+        res->check_errors++;
+        return size;
+    }
+
+    res->bfi.total_clusters = s->bat_size;
+    res->bfi.compressed_clusters = 0; /* compression is not supported */
+
+    high_off = 0;
+    prev_off = 0;
+    for (i = 0; i < s->bat_size; i++) {
+        int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off == 0) {
+            prev_off = 0;
+            continue;
+        }
+
+        /* cluster outside the image */
+        if (off > size) {
+            fprintf(stderr, "%s cluster %u is outside image\n",
+                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+            res->corruptions++;
+            if (fix & BDRV_FIX_ERRORS) {
+                prev_off = 0;
+                s->bat_bitmap[i] = 0;
+                res->corruptions_fixed++;
+                flush_bat = true;
+                continue;
+            }
+        }
+
+        res->bfi.allocated_clusters++;
+        if (off > high_off) {
+            high_off = off;
+        }
+
+        if (prev_off != 0 && (prev_off + cluster_size) != off) {
+            res->bfi.fragmented_clusters++;
+        }
+        prev_off = off;
+    }
+
+    if (flush_bat) {
+        ret = bdrv_pwrite_sync(bs->file, 0, s->header, s->header_size);
+        if (ret < 0) {
+            res->check_errors++;
+            return ret;
+        }
+    }
+
+    res->image_end_offset = high_off + cluster_size;
+    if (size > res->image_end_offset) {
+        int64_t count;
+        count = DIV_ROUND_UP(size - res->image_end_offset, cluster_size);
+        fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
+                fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
+                size - res->image_end_offset);
+        res->leaks += count;
+        if (fix & BDRV_FIX_LEAKS) {
+            ret = bdrv_truncate(bs->file, res->image_end_offset);
+            if (ret < 0) {
+                res->check_errors++;
+                return ret;
+            }
+            res->leaks_fixed += count;
+        }
+    }
+
+    return 0;
+}
+
+
 static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
 {
     int64_t total_size, cl_size;
@@ -432,6 +516,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_co_writev = parallels_co_writev,
 
     .bdrv_create    = parallels_create,
+    .bdrv_check     = parallels_check,
     .create_opts    = &parallels_create_opts,
 };
 
-- 
1.9.1

[Qemu-devel] [PATCH 19/27] block/parallels: implement incorrect close detection

[Denis V. Lunev]

The software driver must set inuse field in Parallels header to
0x746F6E59 when the image is opened in read-write mode. The presence of
this magic in the header on open forces image consistency check.

There is an unfortunate trick here. We can not check for inuse in
parallels_check as this will happen too late. It is possible to do
that for simple check, but during the fix this would always report
an error as the image was opened in BDRV_O_RDWR mode. Thus we save
the flag in BDRVParallelsState for this.

On the other hand, nothing should be done to clear inuse in
parallels_check. Generic close will do the job right.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 0e5d4c3..bafc74b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -36,6 +36,7 @@
 #define HEADER_MAGIC "WithoutFreeSpace"
 #define HEADER_MAGIC2 "WithouFreSpacExt"
 #define HEADER_VERSION 2
+#define HEADER_INUSE_MAGIC  (0x746F6E59)
 
 #define DEFAULT_CLUSTER_SIZE 1048576        /* 1 MiB */
 
@@ -59,6 +60,8 @@ typedef struct BDRVParallelsState {
 
     ParallelsHeader *header;
     uint32_t header_size;
+    bool header_unclean;
+
     uint32_t *bat_bitmap;
     unsigned int bat_size;
 
@@ -246,6 +249,17 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
         return size;
     }
 
+    if (s->header_unclean) {
+        fprintf(stderr, "%s image was not closed correctly\n",
+                fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
+        res->corruptions++;
+        if (fix & BDRV_FIX_ERRORS) {
+            /* parallels_close will do the job right */
+            res->corruptions_fixed++;
+            s->header_unclean = false;
+        }
+    }
+
     res->bfi.total_clusters = s->bat_size;
     res->bfi.compressed_clusters = 0; /* compression is not supported */
 
@@ -404,6 +418,17 @@ static int parallels_probe(const uint8_t *buf, int buf_size,
     return 0;
 }
 
+static int parallels_update_header(BlockDriverState *bs)
+{
+    BDRVParallelsState *s = bs->opaque;
+    unsigned size = MAX(bdrv_opt_mem_align(bs->file), sizeof(ParallelsHeader));
+
+    if (size > s->header_size) {
+        size = s->header_size;
+    }
+    return bdrv_pwrite_sync(bs->file, 0, s->header, size);
+}
+
 static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
@@ -468,6 +493,25 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->bat_bitmap = (uint32_t *)(s->header + 1);
 
+    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+        /* Image was not closed correctly. The check is mandatory */
+        s->header_unclean = true;
+        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
+            error_setg(errp, "parallels: Image was not closed correctly; "
+                       "cannot be opened read/write");
+            ret = -EACCES;
+            goto fail;
+        }
+    }
+
+    if (flags & BDRV_O_RDWR) {
+        s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
+        ret = parallels_update_header(bs);
+        if (ret < 0) {
+            goto fail;
+        }
+    }
+
     qemu_co_mutex_init(&s->lock);
     return 0;
 
@@ -483,6 +527,12 @@ fail:
 static void parallels_close(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
+
+    if (bs->open_flags & BDRV_O_RDWR) {
+        s->header->inuse = 0;
+        parallels_update_header(bs);
+    }
+
     qemu_vfree(s->header);
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH 20/27] iotests, parallels: check for incorrectly closed image in tests

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviwed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/115     |  9 +++++++++
 tests/qemu-iotests/115.out | 17 +++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/tests/qemu-iotests/115 b/tests/qemu-iotests/115
index f45afa7..4873f40 100755
--- a/tests/qemu-iotests/115
+++ b/tests/qemu-iotests/115
@@ -42,6 +42,8 @@ _supported_fmt parallels
 _supported_proto file
 _supported_os Linux
 
+inuse_offset=$((0x2c))
+
 size=64M
 CLUSTER_SIZE=64k
 IMGFMT=parallels
@@ -62,6 +64,13 @@ echo == check that there is no trash after written ==
 echo == check that there is no trash before written ==
 { $QEMU_IO -c "read -P 0 0 32k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo "== Corrupt image =="
+poke_file "$TEST_IMG" "$inuse_offset" "\x59\x6e\x6f\x74"
+{ $QEMU_IO -c "read -P 0x11 64k 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+_check_test_img
+_check_test_img -r all
+{ $QEMU_IO -c "read -P 0x11 64k 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/115.out b/tests/qemu-iotests/115.out
index a06f1a2..5e93b3b 100644
--- a/tests/qemu-iotests/115.out
+++ b/tests/qemu-iotests/115.out
@@ -21,4 +21,21 @@ read 32768/32768 bytes at offset 163840
 == check that there is no trash before written ==
 read 32768/32768 bytes at offset 0
 32 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== Corrupt image ==
+qemu-io: can't open device TEST_DIR/t.parallels: parallels: Image was not closed correctly; cannot be opened read/write
+no file open, try 'help open'
+ERROR image was not closed correctly
+
+1 errors were found on the image.
+Data may be corrupted, or further writes to the image may corrupt it.
+Repairing image was not closed correctly
+The following inconsistencies were found and repaired:
+
+    0 leaked clusters
+    1 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+read 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
1.9.1

[Qemu-devel] [PATCH 21/27] block/parallels: no need to flush on each block allocation table update

[Denis V. Lunev]

>From the point of guest each write to real disk prior to disk barrier
operation could be lost. Therefore there is no problem that "not synced"
new block is lost due to not updated allocation table if QEMU is crashed.
This situation is properly detected and handled now using inuse magic
and in parallels_check

This patch improves writing performance of
  qemu-img create -f parallels -o cluster_size=64k ./1.hds 64G
  qemu-io -f parallels -c "write -P 0x11 0 1024k" 1.hds
from 45 Mb/sec to 160 Mb/sec on my SSD disk. The gain on rotational media
is much more sufficient, from 800 Kb/sec to 45 Mb/sec.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index bafc74b..2605c1a 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -118,7 +118,7 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
 
     s->bat_bitmap[idx] = cpu_to_le32(pos / s->off_multiplier);
-    ret = bdrv_pwrite_sync(bs->file,
+    ret = bdrv_pwrite(bs->file,
             sizeof(ParallelsHeader) + idx * sizeof(s->bat_bitmap[idx]),
             s->bat_bitmap + idx, sizeof(s->bat_bitmap[idx]));
     if (ret < 0) {
-- 
1.9.1

[Qemu-devel] [PATCH 22/27] block/parallels: improve image reading performance

[Denis V. Lunev]

Try to perform IO for the biggest continuous block possible.
The performance for sequential read is increased from 220 Gb/sec to
360 Gb/sec for continous image on my SSD HDD.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 2605c1a..0f255fe 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -97,6 +97,35 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
     return MIN(nb_sectors, ret);
 }
 
+static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
+                            int nb_sectors, int *pnum)
+{
+    int64_t start_off = -2, prev_end_off = -2;
+
+    *pnum = 0;
+    while (nb_sectors > 0 || start_off == -2) {
+        int64_t offset = seek_to_sector(s, sector_num);
+        int to_end;
+
+        if (start_off == -2) {
+            start_off = offset;
+            prev_end_off = offset;
+        } else if (offset != prev_end_off) {
+            break;
+        }
+
+        to_end = cluster_remainder(s, sector_num, nb_sectors);
+        nb_sectors -= to_end;
+        sector_num += to_end;
+        *pnum += to_end;
+
+        if (offset > 0) {
+            prev_end_off += to_end;
+        }
+    }
+    return start_off;
+}
+
 static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
 {
     BDRVParallelsState *s = bs->opaque;
@@ -134,11 +163,9 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
     int64_t offset;
 
     qemu_co_mutex_lock(&s->lock);
-    offset = seek_to_sector(s, sector_num);
+    offset = block_status(s, sector_num, nb_sectors, pnum);
     qemu_co_mutex_unlock(&s->lock);
 
-    *pnum = cluster_remainder(s, sector_num, nb_sectors);
-
     if (offset < 0) {
         return 0;
     }
@@ -202,8 +229,8 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
 
     qemu_co_mutex_lock(&s->lock);
     while (nb_sectors > 0) {
-        int64_t position = seek_to_sector(s, sector_num);
-        int n = cluster_remainder(s, sector_num, nb_sectors);
+        int n;
+        int64_t position = block_status(s, sector_num, nb_sectors, &n);
         int nbytes = n << BDRV_SECTOR_BITS;
 
         if (position < 0) {
-- 
1.9.1

[Qemu-devel] [PATCH 23/27] block/parallels: create bat_entry_off helper

[Denis V. Lunev]

calculate offset of the BAT entry in the image file.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 0f255fe..f0518be 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -76,6 +76,11 @@ static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx)
     return (uint64_t)le32_to_cpu(s->bat_bitmap[idx]) * s->off_multiplier;
 }
 
+static uint32_t bat_entry_off(uint32_t idx)
+{
+    return sizeof(ParallelsHeader) + sizeof(uint32_t) * idx;
+}
+
 static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
 {
     uint32_t index, offset;
@@ -147,9 +152,8 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
 
     s->bat_bitmap[idx] = cpu_to_le32(pos / s->off_multiplier);
-    ret = bdrv_pwrite(bs->file,
-            sizeof(ParallelsHeader) + idx * sizeof(s->bat_bitmap[idx]),
-            s->bat_bitmap + idx, sizeof(s->bat_bitmap[idx]));
+    ret = bdrv_pwrite(bs->file, bat_entry_off(idx), s->bat_bitmap + idx,
+                      sizeof(s->bat_bitmap[idx]));
     if (ret < 0) {
         return ret;
     }
@@ -388,8 +392,7 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
     }
 
     bat_entries = DIV_ROUND_UP(total_size, cl_size);
-    bat_sectors = DIV_ROUND_UP(bat_entries * sizeof(uint32_t) +
-                               sizeof(ParallelsHeader), cl_size);
+    bat_sectors = DIV_ROUND_UP(bat_entry_off(bat_entries), cl_size);
     bat_sectors = (bat_sectors *  cl_size) >> BDRV_SECTOR_BITS;
 
     memset(&header, 0, sizeof(header));
@@ -501,7 +504,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    size = sizeof(ParallelsHeader) + sizeof(uint32_t) * s->bat_size;
+    size = bat_entry_off(s->bat_size);
     s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file));
     s->header = qemu_try_blockalign(bs->file, s->header_size);
     if (s->header == NULL) {
-- 
1.9.1

[Qemu-devel] [PATCH 24/27] 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 uses loaded BAT data as a cache and performs
actual on-disk metadata updates in parallels_co_flush_to_os callback.

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 <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index f0518be..6e7f000 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -30,6 +30,7 @@
 #include "qemu-common.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
+#include "qemu/bitmap.h"
 
 /**************************************************************/
 
@@ -62,6 +63,9 @@ typedef struct BDRVParallelsState {
     uint32_t header_size;
     bool header_unclean;
 
+    unsigned long *bat_dirty_bmap;
+    unsigned int  bat_dirty_block;
+
     uint32_t *bat_bitmap;
     unsigned int bat_size;
 
@@ -136,7 +140,6 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     BDRVParallelsState *s = bs->opaque;
     uint32_t idx, offset;
     int64_t pos;
-    int ret;
 
     idx = sector_num / s->tracks;
     offset = sector_num % s->tracks;
@@ -152,14 +155,43 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
 
     s->bat_bitmap[idx] = cpu_to_le32(pos / s->off_multiplier);
-    ret = bdrv_pwrite(bs->file, bat_entry_off(idx), s->bat_bitmap + idx,
-                      sizeof(s->bat_bitmap[idx]));
-    if (ret < 0) {
-        return ret;
-    }
+
+    bitmap_set(s->bat_dirty_bmap, bat_entry_off(idx) / s->bat_dirty_block, 1);
     return bat2sect(s, idx) + offset;
 }
 
+
+static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs)
+{
+    BDRVParallelsState *s = bs->opaque;
+    unsigned long size = DIV_ROUND_UP(s->header_size, s->bat_dirty_block);
+    unsigned long bit;
+
+    qemu_co_mutex_lock(&s->lock);
+
+    bit = find_first_bit(s->bat_dirty_bmap, size);
+    while (bit < size) {
+        uint32_t off = bit * s->bat_dirty_block;
+        uint32_t to_write = s->bat_dirty_block;
+        int ret;
+
+        if (off + to_write > s->header_size) {
+            to_write = s->header_size - off;
+        }
+        ret = bdrv_pwrite(bs->file, off, (uint8_t *)s->header + off, to_write);
+        if (ret < 0) {
+            qemu_co_mutex_unlock(&s->lock);
+            return ret;
+        }
+        bit = find_next_bit(s->bat_dirty_bmap, size, bit + 1);
+    }
+    bitmap_zero(s->bat_dirty_bmap, size);
+
+    qemu_co_mutex_unlock(&s->lock);
+    return 0;
+}
+
+
 static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -542,6 +574,10 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
+    s->bat_dirty_block = 4 * getpagesize();
+    s->bat_dirty_bmap =
+        bitmap_new(DIV_ROUND_UP(s->header_size, s->bat_dirty_block));
+
     qemu_co_mutex_init(&s->lock);
     return 0;
 
@@ -563,6 +599,7 @@ static void parallels_close(BlockDriverState *bs)
         parallels_update_header(bs);
     }
 
+    g_free(s->bat_dirty_bmap);
     qemu_vfree(s->header);
 }
 
@@ -592,6 +629,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

[Qemu-devel] [PATCH 25/27] block/parallels: add prealloc-mode and prealloc-size open paramemets

[Denis V. Lunev]

This is preparational commit for tweaks in Parallels image expansion.
The idea is that enlarge via truncate by one data block is slow. It
would be much better to use fallocate via bdrv_write_zeroes and
expand by some significant amount at once.

This patch just adds proper parameters into BDRVParallelsState and
performs options parsing in parallels_open.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 6e7f000..6f25907 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -31,6 +31,7 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "qemu/bitmap.h"
+#include "qapi/util.h"
 
 /**************************************************************/
 
@@ -56,6 +57,20 @@ typedef struct ParallelsHeader {
     char padding[12];
 } QEMU_PACKED ParallelsHeader;
 
+
+typedef enum ParallelsPreallocMode {
+    PRL_PREALLOC_MODE_FALLOCATE = 0,
+    PRL_PREALLOC_MODE_TRUNCATE = 1,
+    PRL_PREALLOC_MODE_MAX = 2,
+} ParallelsPreallocMode;
+
+static const char *prealloc_mode_lookup[] = {
+    "falloc",
+    "truncate",
+    NULL,
+};
+
+
 typedef struct BDRVParallelsState {
     CoMutex lock;
 
@@ -69,12 +84,40 @@ typedef struct BDRVParallelsState {
     uint32_t *bat_bitmap;
     unsigned int bat_size;
 
+    uint64_t prealloc_size;
+    ParallelsPreallocMode prealloc_mode;
+
     unsigned int tracks;
 
     unsigned int off_multiplier;
 } BDRVParallelsState;
 
 
+#define PARALLELS_OPT_PREALLOC_MODE     "prealloc-mode"
+#define PARALLELS_OPT_PREALLOC_SIZE     "prealloc-size"
+
+static QemuOptsList parallels_runtime_opts = {
+    .name = "parallels",
+    .head = QTAILQ_HEAD_INITIALIZER(parallels_runtime_opts.head),
+    .desc = {
+        {
+            .name = PARALLELS_OPT_PREALLOC_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "Preallocation size on image expansion",
+            .def_value_str = "128MiB",
+        },
+        {
+            .name = PARALLELS_OPT_PREALLOC_MODE,
+            .type = QEMU_OPT_STRING,
+            .help = "Preallocation mode on image expansion "
+                    "(allowed values: falloc, truncate)",
+            .def_value_str = "falloc",
+        },
+        { /* end of list */ },
+    },
+};
+
+
 static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx)
 {
     return (uint64_t)le32_to_cpu(s->bat_bitmap[idx]) * s->off_multiplier;
@@ -497,6 +540,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
     int ret, size;
+    QemuOpts *opts = NULL;
+    Error *local_err = NULL;
+    char *buf;
 
     ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
     if (ret < 0) {
@@ -566,6 +612,27 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
+    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, &local_err);
+    if (local_err != NULL) {
+        goto fail_options;
+    }
+
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err != NULL) {
+        goto fail_options;
+    }
+
+    s->prealloc_size =
+        qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
+    s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
+    buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
+    s->prealloc_mode = qapi_enum_parse(prealloc_mode_lookup, buf,
+            PRL_PREALLOC_MODE_MAX, PRL_PREALLOC_MODE_FALLOCATE, &local_err);
+    g_free(buf);
+    if (local_err != NULL) {
+        goto fail_options;
+    }
+
     if (flags & BDRV_O_RDWR) {
         s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
         ret = parallels_update_header(bs);
@@ -587,6 +654,11 @@ fail_format:
 fail:
     qemu_vfree(s->header);
     return ret;
+
+fail_options:
+    error_propagate(errp, local_err);
+    ret = -EINVAL;
+    goto fail;
 }
 
 
-- 
1.9.1

[Qemu-devel] [PATCH 26/27] block/parallels: optimize linear image expansion

[Denis V. Lunev]

Plain image expansion spends a lot of time to update image file size.
This seriously affects the performance. The following simple test
  qemu_img create -f parallels -o cluster_size=64k ./1.hds 64G
  qemu_io -n -c "write -P 0x11 0 1024M" ./1.hds
could be improved if the format driver will pre-allocate some space
in the image file with a reasonable chunk.

This patch preallocates 128 Mb using bdrv_write_zeroes, which should
normally use fallocate() call inside. Fallback to older truncate()
could be used as a fallback using image open options thanks to the
previous patch.

The benefit is around 15%.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Karan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 6f25907..2a1e31f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -84,6 +84,7 @@ typedef struct BDRVParallelsState {
     uint32_t *bat_bitmap;
     unsigned int bat_size;
 
+    int64_t  data_end;
     uint64_t prealloc_size;
     ParallelsPreallocMode prealloc_mode;
 
@@ -195,7 +196,21 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     }
 
     pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
-    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
+    if (s->data_end + s->tracks > pos) {
+        int ret;
+        if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
+            ret = bdrv_write_zeroes(bs->file, s->data_end,
+                                    s->prealloc_size, 0);
+        } else {
+            ret = bdrv_truncate(bs->file,
+                    (s->data_end + s->prealloc_size) << BDRV_SECTOR_BITS);
+        }
+        if (ret < 0) {
+            return ret;
+        }
+    }
+    pos = s->data_end;
+    s->data_end += s->tracks;
 
     s->bat_bitmap[idx] = cpu_to_le32(pos / s->off_multiplier);
 
@@ -539,7 +554,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
-    int ret, size;
+    int ret, size, i;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -589,7 +604,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -ENOMEM;
         goto fail;
     }
-    if (le32_to_cpu(ph.data_off) < s->header_size) {
+    s->data_end = le32_to_cpu(ph.data_off);
+    if (s->data_end == 0) {
+        s->data_end = ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
+    }
+    if (s->data_end < s->header_size) {
         /* there is not enough unused space to fit to block align between BAT
            and actual data. We can't avoid read-modify-write... */
         s->header_size = size;
@@ -601,6 +620,13 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->bat_bitmap = (uint32_t *)(s->header + 1);
 
+    for (i = 0; i < s->bat_size; i++) {
+        int64_t off = bat2sect(s, i);
+        if (off >= s->data_end) {
+            s->data_end = off + s->tracks;
+        }
+    }
+
     if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
         /* Image was not closed correctly. The check is mandatory */
         s->header_unclean = true;
@@ -671,6 +697,10 @@ static void parallels_close(BlockDriverState *bs)
         parallels_update_header(bs);
     }
 
+    if (bs->open_flags & BDRV_O_RDWR) {
+        bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS);
+    }
+
     g_free(s->bat_dirty_bmap);
     qemu_vfree(s->header);
 }
-- 
1.9.1

[Qemu-devel] [PATCH 27/27] block/parallels: improve image writing performance further

[Denis V. Lunev]

Try to perform IO for the biggest continuous block possible.
All blocks abscent in the image are accounted in the same type
and preallocation is made for all of them at once.

The performance for sequential write is increased from 200 Gb/sec to
235 Gb/sec on my SSD HDD.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 2a1e31f..7531340 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -179,43 +179,47 @@ static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
     return start_off;
 }
 
-static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
+static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
+                                 int nb_sectors, int *pnum)
 {
     BDRVParallelsState *s = bs->opaque;
-    uint32_t idx, offset;
-    int64_t pos;
+    uint32_t idx, to_allocate, i;
+    int64_t pos, space;
 
-    idx = sector_num / s->tracks;
-    offset = sector_num % s->tracks;
+    pos = block_status(s, sector_num, nb_sectors, pnum);
+    if (pos > 0) {
+        return pos;
+    }
 
+    idx = sector_num / s->tracks;
     if (idx >= s->bat_size) {
         return -EINVAL;
     }
-    if (s->bat_bitmap[idx] != 0) {
-        return bat2sect(s, idx) + offset;
-    }
 
-    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
-    if (s->data_end + s->tracks > pos) {
+    to_allocate = (sector_num + *pnum + s->tracks - 1) / s->tracks - idx;
+    space = to_allocate * s->tracks;
+    if (s->data_end + space > bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS) {
         int ret;
+        space += s->prealloc_size;
         if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-            ret = bdrv_write_zeroes(bs->file, s->data_end,
-                                    s->prealloc_size, 0);
+            ret = bdrv_write_zeroes(bs->file, s->data_end, space, 0);
         } else {
             ret = bdrv_truncate(bs->file,
-                    (s->data_end + s->prealloc_size) << BDRV_SECTOR_BITS);
+                                (s->data_end + space) << BDRV_SECTOR_BITS);
         }
         if (ret < 0) {
             return ret;
         }
     }
-    pos = s->data_end;
-    s->data_end += s->tracks;
 
-    s->bat_bitmap[idx] = cpu_to_le32(pos / s->off_multiplier);
+    for (i = 0; i < to_allocate; i++) {
+        s->bat_bitmap[idx + i] = cpu_to_le32(s->data_end / s->off_multiplier);
+        s->data_end += s->tracks;
+        bitmap_set(s->bat_dirty_bmap,
+                   bat_entry_off(idx) / s->bat_dirty_block, 1);
+    }
 
-    bitmap_set(s->bat_dirty_bmap, bat_entry_off(idx) / s->bat_dirty_block, 1);
-    return bat2sect(s, idx) + offset;
+    return bat2sect(s, idx) + sector_num % s->tracks;
 }
 
 
@@ -280,8 +284,8 @@ static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
 
     qemu_co_mutex_lock(&s->lock);
     while (nb_sectors > 0) {
-        int64_t position = allocate_cluster(bs, sector_num);
-        int n = cluster_remainder(s, sector_num, nb_sectors);
+        int n;
+        int64_t position = allocate_clusters(bs, sector_num, nb_sectors, &n);
         int nbytes = n << BDRV_SECTOR_BITS;
 
         if (position < 0) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 18/27] block/parallels: implement parallels_check method of block driver

[Roman Kagan]

On Wed, Mar 11, 2015 at 01:28:12PM +0300, Denis V. Lunev wrote:
> The check is very simple at the moment. It calculates necessary stats
> and fix only the following errors:
> - space leak at the end of the image. This would happens due to
>   preallocation
> - clusters outside the image are zeroed. Nothing else could be done here
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)

Reviewed-by: Roman Kagan <rkagan@parallels.com>

Roman.

Re: [Qemu-devel] [PATCH 01/27] iotests, parallels: quote TEST_IMG in 076 test to be path-safe

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:27:55PM +0300, Denis V. Lunev wrote:
> suggested by Jeff Cody
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/076 | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 02/27] block/parallels: rename parallels_header to ParallelsHeader

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:27:56PM +0300, Denis V. Lunev wrote:
> this follows QEMU coding convention
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 03/27] block/parallels: switch to bdrv_read

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:27:57PM +0300, Denis V. Lunev wrote:
> From: Roman Kagan <rkagan@parallels.com>
> 
> Switch the .bdrv_read method implementation from using bdrv_pread() to
> bdrv_read() on the underlying file, since the latter is subject to i/o
> throttling while the former is not.
> 
> Besides, since bdrv_read() operates in sectors rather than bytes, adjust
> the helper functions to do so too.

Can this patch be dropped since "[PATCH 06/27] block/parallels: provide
_co_readv routine for parallels format driver" switches to
.bdrv_co_readv() anyway?

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 04/27] block/parallels: read up to cluster end in one go

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:27:58PM +0300, Denis V. Lunev wrote:
> From: Roman Kagan <rkagan@parallels.com>
> 
> Teach parallels_read() to do reads in coarser granularity than just a
> single sector: if requested, read up to the cluster end in one go.
> 
> Signed-off-by: Roman Kagan <rkagan@parallels.com>
> Reviewed-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 03/27] block/parallels: switch to bdrv_read

[Denis V. Lunev]

On 22/04/15 15:23, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:27:57PM +0300, Denis V. Lunev wrote:
>> From: Roman Kagan <rkagan@parallels.com>
>>
>> Switch the .bdrv_read method implementation from using bdrv_pread() to
>> bdrv_read() on the underlying file, since the latter is subject to i/o
>> throttling while the former is not.
>>
>> Besides, since bdrv_read() operates in sectors rather than bytes, adjust
>> the helper functions to do so too.
> Can this patch be dropped since "[PATCH 06/27] block/parallels: provide
> _co_readv routine for parallels format driver" switches to
> .bdrv_co_readv() anyway?
>
> Stefan
this is not that easy. This patch changes seek_to_sector call and
in reality to drop it I have to merge patches 3, 4 and 6.
Besides that this patch has been written by my colleague and
I feel comfortable to preserve original Signed-off-by of him.

Den

Re: [Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:27:59PM +0300, Denis V. Lunev wrote:
> +static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
> +        int64_t sector_num, int nb_sectors, int *pnum)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int64_t offset;
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    offset = seek_to_sector(s, sector_num);
> +    qemu_co_mutex_unlock(&s->lock);

The lock isn't necessary here yet.  It may become necessary when write
support is added, but probably not even then since seek_to_sector()
cannot yield (it's not a coroutine function), so there are no possible
races with other coroutines.

The same also applies for parallels_co_read().  The lock there isn't
necessary since the block driver is read-only and has no mutable state -
there is no shared state that needs lock protection.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 06/27] block/parallels: provide _co_readv routine for parallels format driver

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:00PM +0300, Denis V. Lunev wrote:
> Main approach is taken from qcow2_co_readv.
> 
> The patch drops coroutine lock for the duration of IO operation and
> peforms normal scatter-gather IO using standard QEMU backend.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index b469984..64b169b 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -186,37 +186,45 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
>          BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
>  }
>  
> -static int parallels_read(BlockDriverState *bs, int64_t sector_num,
> -                    uint8_t *buf, int nb_sectors)
> +static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
> +        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
>  {
>      BDRVParallelsState *s = bs->opaque;
> +    uint64_t bytes_done = 0;
> +    QEMUIOVector hd_qiov;
> +    int ret = 0;
>  
> +    qemu_iovec_init(&hd_qiov, qiov->niov);
> +
> +    qemu_co_mutex_lock(&s->lock);
>      while (nb_sectors > 0) {
>          int64_t position = seek_to_sector(s, sector_num);
>          int n = cluster_remainder(s, sector_num, nb_sectors);
> -        if (position >= 0) {
> -            int ret = bdrv_read(bs->file, position, buf, n);
> +        int nbytes = n << BDRV_SECTOR_BITS;
> +
> +        if (position < 0) {
> +            qemu_iovec_memset(qiov, bytes_done, 0, nbytes);
> +        } else {
> +            qemu_iovec_reset(&hd_qiov);
> +            qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
> +
> +            qemu_co_mutex_unlock(&s->lock);
> +            ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
> +            qemu_co_mutex_lock(&s->lock);
> +
>              if (ret < 0) {
> -                return ret;
> +                goto fail;

The lock is never unlocked if the goto is taken.

>              }
> -        } else {
> -            memset(buf, 0, n << BDRV_SECTOR_BITS);
>          }
> +
>          nb_sectors -= n;
>          sector_num += n;
> -        buf += n << BDRV_SECTOR_BITS;
> +        bytes_done += nbytes;
>      }
> -    return 0;
> -}
> -
> -static coroutine_fn int parallels_co_read(BlockDriverState *bs, int64_t sector_num,
> -                                          uint8_t *buf, int nb_sectors)
> -{
> -    int ret;
> -    BDRVParallelsState *s = bs->opaque;
> -    qemu_co_mutex_lock(&s->lock);
> -    ret = parallels_read(bs, sector_num, buf, nb_sectors);
>      qemu_co_mutex_unlock(&s->lock);
> +
> +fail:
> +    qemu_iovec_destroy(&hd_qiov);
>      return ret;
>  }

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status

[Denis V. Lunev]

On 22/04/15 15:39, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:27:59PM +0300, Denis V. Lunev wrote:
>> +static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
>> +        int64_t sector_num, int nb_sectors, int *pnum)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    int64_t offset;
>> +
>> +    qemu_co_mutex_lock(&s->lock);
>> +    offset = seek_to_sector(s, sector_num);
>> +    qemu_co_mutex_unlock(&s->lock);
> The lock isn't necessary here yet.  It may become necessary when write
> support is added, but probably not even then since seek_to_sector()
> cannot yield (it's not a coroutine function), so there are no possible
> races with other coroutines.
>
> The same also applies for parallels_co_read().  The lock there isn't
> necessary since the block driver is read-only and has no mutable state -
> there is no shared state that needs lock protection.
do you propose to drop the lock here and add it later with write
support (patch 08)? I'd prefer to stay on the safe side. Yes, the
lock is not mandatory at the moment but in 2 patches it will be
mandatory.

I can extend the comment to clarify this.

Re: [Qemu-devel] [PATCH 07/27] block/parallels: replace magic constants 4, 64 with proper sizeofs

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:01PM +0300, Denis V. Lunev wrote:
> simple purification..
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 06/27] block/parallels: provide _co_readv routine for parallels format driver

[Denis V. Lunev]

On 22/04/15 15:41, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:28:00PM +0300, Denis V. Lunev wrote:
>> Main approach is taken from qcow2_co_readv.
>>
>> The patch drops coroutine lock for the duration of IO operation and
>> peforms normal scatter-gather IO using standard QEMU backend.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Roman Kagan <rkagan@parallels.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/parallels.c | 46 +++++++++++++++++++++++++++-------------------
>>   1 file changed, 27 insertions(+), 19 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index b469984..64b169b 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -186,37 +186,45 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
>>           BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
>>   }
>>   
>> -static int parallels_read(BlockDriverState *bs, int64_t sector_num,
>> -                    uint8_t *buf, int nb_sectors)
>> +static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
>> +        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>> +    uint64_t bytes_done = 0;
>> +    QEMUIOVector hd_qiov;
>> +    int ret = 0;
>>   
>> +    qemu_iovec_init(&hd_qiov, qiov->niov);
>> +
>> +    qemu_co_mutex_lock(&s->lock);
>>       while (nb_sectors > 0) {
>>           int64_t position = seek_to_sector(s, sector_num);
>>           int n = cluster_remainder(s, sector_num, nb_sectors);
>> -        if (position >= 0) {
>> -            int ret = bdrv_read(bs->file, position, buf, n);
>> +        int nbytes = n << BDRV_SECTOR_BITS;
>> +
>> +        if (position < 0) {
>> +            qemu_iovec_memset(qiov, bytes_done, 0, nbytes);
>> +        } else {
>> +            qemu_iovec_reset(&hd_qiov);
>> +            qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
>> +
>> +            qemu_co_mutex_unlock(&s->lock);
>> +            ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
>> +            qemu_co_mutex_lock(&s->lock);
>> +
>>               if (ret < 0) {
>> -                return ret;
>> +                goto fail;
> The lock is never unlocked if the goto is taken.

nice catch! We have missed that...

>>               }
>> -        } else {
>> -            memset(buf, 0, n << BDRV_SECTOR_BITS);
>>           }
>> +
>>           nb_sectors -= n;
>>           sector_num += n;
>> -        buf += n << BDRV_SECTOR_BITS;
>> +        bytes_done += nbytes;
>>       }
>> -    return 0;
>> -}
>> -
>> -static coroutine_fn int parallels_co_read(BlockDriverState *bs, int64_t sector_num,
>> -                                          uint8_t *buf, int nb_sectors)
>> -{
>> -    int ret;
>> -    BDRVParallelsState *s = bs->opaque;
>> -    qemu_co_mutex_lock(&s->lock);
>> -    ret = parallels_read(bs, sector_num, buf, nb_sectors);
>>       qemu_co_mutex_unlock(&s->lock);
>> +
>> +fail:
>> +    qemu_iovec_destroy(&hd_qiov);
>>       return ret;
>>   }

Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Denis V. Lunev]

On 11/03/15 13:28, Denis V. Lunev wrote:
> Support write on Parallels images. The code is almost the same as one
> in the previous patch implemented scatter-gather IO for read.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/parallels.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 75 insertions(+), 2 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 306f2e3..61d7da7 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -81,8 +81,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       ParallelsHeader ph;
>       int ret;
>   
> -    bs->read_only = 1; // no write support yet
> -
>       ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
>       if (ret < 0) {
>           goto fail;
> @@ -166,6 +164,37 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
>       return MIN(nb_sectors, ret);
>   }
>   
> +static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    uint32_t idx, offset, tmp;
> +    int64_t pos;
> +    int ret;
> +
> +    idx = sector_num / s->tracks;
> +    offset = sector_num % s->tracks;
> +
> +    if (idx >= s->catalog_size) {
> +        return -EINVAL;
> +    }
> +    if (s->catalog_bitmap[idx] != 0) {
> +        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
> +    }
> +
> +    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
> +    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
> +    s->catalog_bitmap[idx] = pos / s->off_multiplier;
> +
> +    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
> +
> +    ret = bdrv_pwrite_sync(bs->file,
> +            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
> +}
> +
>   static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
>           int64_t sector_num, int nb_sectors, int *pnum)
>   {
> @@ -186,6 +215,49 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
>           BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
>   }
>   
> +static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
> +        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    uint64_t bytes_done = 0;
> +    QEMUIOVector hd_qiov;
> +    int ret = 0;
> +
> +    qemu_iovec_init(&hd_qiov, qiov->niov);
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    while (nb_sectors > 0) {
> +        int64_t position = allocate_cluster(bs, sector_num);
> +        int n = cluster_remainder(s, sector_num, nb_sectors);
> +        int nbytes = n << BDRV_SECTOR_BITS;
> +
> +        if (position < 0) {
> +            ret = (int)position;
> +            break;
> +        }
> +
> +        qemu_iovec_reset(&hd_qiov);
> +        qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
> +
> +        qemu_co_mutex_unlock(&s->lock);
> +        ret = bdrv_co_writev(bs->file, position, n, &hd_qiov);
> +        qemu_co_mutex_lock(&s->lock);
> +
> +        if (ret < 0) {
> +            goto fail;
same problem with unlock as in patch 6


> +        }
> +
> +        nb_sectors -= n;
> +        sector_num += n;
> +        bytes_done += nbytes;
> +    }
> +    qemu_co_mutex_unlock(&s->lock);
> +
> +fail:
> +    qemu_iovec_destroy(&hd_qiov);
> +    return ret;
> +}
> +
>   static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
>           int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
>   {
> @@ -242,6 +314,7 @@ static BlockDriver bdrv_parallels = {
>       .bdrv_close		= parallels_close,
>       .bdrv_co_get_block_status = parallels_co_get_block_status,
>       .bdrv_co_readv  = parallels_co_readv,
> +    .bdrv_co_writev = parallels_co_writev,
>   };
>   
>   static void bdrv_parallels_init(void)

Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:02PM +0300, Denis V. Lunev wrote:
> +static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
> +        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    uint64_t bytes_done = 0;
> +    QEMUIOVector hd_qiov;
> +    int ret = 0;
> +
> +    qemu_iovec_init(&hd_qiov, qiov->niov);
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    while (nb_sectors > 0) {
> +        int64_t position = allocate_cluster(bs, sector_num);
> +        int n = cluster_remainder(s, sector_num, nb_sectors);
> +        int nbytes = n << BDRV_SECTOR_BITS;
> +
> +        if (position < 0) {
> +            ret = (int)position;
> +            break;
> +        }
> +
> +        qemu_iovec_reset(&hd_qiov);
> +        qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
> +
> +        qemu_co_mutex_unlock(&s->lock);
> +        ret = bdrv_co_writev(bs->file, position, n, &hd_qiov);
> +        qemu_co_mutex_lock(&s->lock);
> +
> +        if (ret < 0) {
> +            goto fail;

Missing unlock if goto is taken.  I'd suggest dropping the 'fail' label
and using break.

> +        }
> +
> +        nb_sectors -= n;
> +        sector_num += n;
> +        bytes_done += nbytes;
> +    }
> +    qemu_co_mutex_unlock(&s->lock);
> +
> +fail:
> +    qemu_iovec_destroy(&hd_qiov);
> +    return ret;
> +}

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:02PM +0300, Denis V. Lunev wrote:
> +static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    uint32_t idx, offset, tmp;
> +    int64_t pos;
> +    int ret;
> +
> +    idx = sector_num / s->tracks;
> +    offset = sector_num % s->tracks;
> +
> +    if (idx >= s->catalog_size) {
> +        return -EINVAL;
> +    }
> +    if (s->catalog_bitmap[idx] != 0) {
> +        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
> +    }
> +
> +    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
> +    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
> +    s->catalog_bitmap[idx] = pos / s->off_multiplier;
> +
> +    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
> +
> +    ret = bdrv_pwrite_sync(bs->file,
> +            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));

What is the purpose of the sync?

> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
> +}

This function is missing error handling.  If the catalog bitmap update
cannot be written to file then our in-memory copy should also be
reverted back to 0 (unallocated).

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 09/27] iotests, parallels: test for write into Parallels image

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:03PM +0300, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/076     |  5 +++++
>  tests/qemu-iotests/076.out | 10 ++++++++++
>  2 files changed, 15 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 10/27] block/parallels: support parallels image creation

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:04PM +0300, Denis V. Lunev wrote:
> Do not even care to create WithoutFreeSpace image, it is obsolete.
> Always create WithouFreSpacExt one.
> 
> The code also does not spend a lot of efforts to fill cylinders and
> heads fields, they are not used actually in a real life neither in
> QEMU nor in Parallels products.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviwed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Denis V. Lunev]

On 22/04/15 16:08, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:28:02PM +0300, Denis V. Lunev wrote:
>> +static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    uint32_t idx, offset, tmp;
>> +    int64_t pos;
>> +    int ret;
>> +
>> +    idx = sector_num / s->tracks;
>> +    offset = sector_num % s->tracks;
>> +
>> +    if (idx >= s->catalog_size) {
>> +        return -EINVAL;
>> +    }
>> +    if (s->catalog_bitmap[idx] != 0) {
>> +        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
>> +    }
>> +
>> +    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
>> +    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
>> +    s->catalog_bitmap[idx] = pos / s->off_multiplier;
>> +
>> +    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
>> +
>> +    ret = bdrv_pwrite_sync(bs->file,
>> +            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
> What is the purpose of the sync?
This is necessary to preserve image consistency on crash from
my point of view. There is no check consistency at the moment.
The sync will be removed later when proper crash detection
code will be added (patches 19, 20, 21)

On the other hand this _sync  was borrowed from qcow.c:get_cluster_offset
which was used as a base of this work.
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
>> +}
> This function is missing error handling.  If the catalog bitmap update
> cannot be written to file then our in-memory copy should also be
> reverted back to 0 (unallocated).
ok

Re: [Qemu-devel] [PATCH 11/27] iotests, parallels: test for newly created parallels image via qemu-img

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:05PM +0300, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviwed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/115     | 68 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/115.out | 24 ++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 93 insertions(+)
>  create mode 100755 tests/qemu-iotests/115
>  create mode 100644 tests/qemu-iotests/115.out

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 12/27] parallels: change copyright information in the image header

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:06PM +0300, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviwed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 12/27] parallels: change copyright information in the image header

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:06PM +0300, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviwed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 28338ec..7ef3136 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -2,8 +2,12 @@
>   * Block driver for Parallels disk image format
>   *
>   * Copyright (c) 2007 Alex Beregszaszi
> + * Copyright (c) 2014 Denis V. Lunev <den@openvz.org>

By the way, feel free to bump this to 2015.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 13/27] block/parallels: rename catalog_ names to bat_

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:07PM +0300, Denis V. Lunev wrote:
> BAT means 'block allocation table'. Thus this name is clean and shorter
> on writing.
> 
> Some obvious formatting fixes in the old code were made to make checkpatch
> happy.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 56 ++++++++++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 14/27] block/parallels: create bat2sect helper

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:08PM +0300, Denis V. Lunev wrote:
> deduplicate copy/paste arithmetcs
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 15/27] block/parallels: keep BAT bitmap data in little endian in memory

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:09PM +0300, Denis V. Lunev wrote:
> This will allow to use this data as buffer to BAT update directly
> without any intermediate buffers.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 16/27] block/parallels: read parallels image header and BAT into single buffer

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:10PM +0300, Denis V. Lunev wrote:
> This metadata cache would allow to properly batch BAT updates to disk
> in next patches. These updates will be properly aligned to avoid
> read-modify-write transactions on block level.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 17/27] block/parallels: move parallels_open/probe to the very end of the file

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:11PM +0300, Denis V. Lunev wrote:
> This will help to avoid forward declarations for upcoming parallels_check
> 
> Some very obvious formatting fixes were made to the moved code to make
> checkpatch happy.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 185 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 95 insertions(+), 90 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 18/27] block/parallels: implement parallels_check method of block driver

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:12PM +0300, Denis V. Lunev wrote:
> The check is very simple at the moment. It calculates necessary stats
> and fix only the following errors:
> - space leak at the end of the image. This would happens due to
>   preallocation
> - clusters outside the image are zeroed. Nothing else could be done here
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 19/27] block/parallels: implement incorrect close detection

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:13PM +0300, Denis V. Lunev wrote:
> The software driver must set inuse field in Parallels header to
> 0x746F6E59 when the image is opened in read-write mode. The presence of
> this magic in the header on open forces image consistency check.
> 
> There is an unfortunate trick here. We can not check for inuse in
> parallels_check as this will happen too late. It is possible to do
> that for simple check, but during the fix this would always report
> an error as the image was opened in BDRV_O_RDWR mode. Thus we save
> the flag in BDRVParallelsState for this.
> 
> On the other hand, nothing should be done to clear inuse in
> parallels_check. Generic close will do the job right.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 20/27] iotests, parallels: check for incorrectly closed image in tests

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:14PM +0300, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviwed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/115     |  9 +++++++++
>  tests/qemu-iotests/115.out | 17 +++++++++++++++++
>  2 files changed, 26 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 21/27] block/parallels: no need to flush on each block allocation table update

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:15PM +0300, Denis V. Lunev wrote:
> From the point of guest each write to real disk prior to disk barrier
> operation could be lost. Therefore there is no problem that "not synced"
> new block is lost due to not updated allocation table if QEMU is crashed.
> This situation is properly detected and handled now using inuse magic
> and in parallels_check
> 
> This patch improves writing performance of
>   qemu-img create -f parallels -o cluster_size=64k ./1.hds 64G
>   qemu-io -f parallels -c "write -P 0x11 0 1024k" 1.hds
> from 45 Mb/sec to 160 Mb/sec on my SSD disk. The gain on rotational media
> is much more sufficient, from 800 Kb/sec to 45 Mb/sec.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index bafc74b..2605c1a 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -118,7 +118,7 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
>      bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
>  
>      s->bat_bitmap[idx] = cpu_to_le32(pos / s->off_multiplier);
> -    ret = bdrv_pwrite_sync(bs->file,
> +    ret = bdrv_pwrite(bs->file,
>              sizeof(ParallelsHeader) + idx * sizeof(s->bat_bitmap[idx]),
>              s->bat_bitmap + idx, sizeof(s->bat_bitmap[idx]));
>      if (ret < 0) {

Please squash this into the write support patch.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 21/27] block/parallels: no need to flush on each block allocation table update

[Denis V. Lunev]

On 22/04/15 17:05, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:28:15PM +0300, Denis V. Lunev wrote:
>>  From the point of guest each write to real disk prior to disk barrier
>> operation could be lost. Therefore there is no problem that "not synced"
>> new block is lost due to not updated allocation table if QEMU is crashed.
>> This situation is properly detected and handled now using inuse magic
>> and in parallels_check
>>
>> This patch improves writing performance of
>>    qemu-img create -f parallels -o cluster_size=64k ./1.hds 64G
>>    qemu-io -f parallels -c "write -P 0x11 0 1024k" 1.hds
>> from 45 Mb/sec to 160 Mb/sec on my SSD disk. The gain on rotational media
>> is much more sufficient, from 800 Kb/sec to 45 Mb/sec.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Roman Kagan <rkagan@parallels.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/parallels.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index bafc74b..2605c1a 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -118,7 +118,7 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
>>       bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
>>   
>>       s->bat_bitmap[idx] = cpu_to_le32(pos / s->off_multiplier);
>> -    ret = bdrv_pwrite_sync(bs->file,
>> +    ret = bdrv_pwrite(bs->file,
>>               sizeof(ParallelsHeader) + idx * sizeof(s->bat_bitmap[idx]),
>>               s->bat_bitmap + idx, sizeof(s->bat_bitmap[idx]));
>>       if (ret < 0) {
> Please squash this into the write support patch.
ok

Re: [Qemu-devel] [PATCH 22/27] block/parallels: improve image reading performance

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:16PM +0300, Denis V. Lunev wrote:
> Try to perform IO for the biggest continuous block possible.
> The performance for sequential read is increased from 220 Gb/sec to
> 360 Gb/sec for continous image on my SSD HDD.

Please, can I have your SSD drive? :)

Did you mean MB/sec?

> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 22/27] block/parallels: improve image reading performance

[Denis V. Lunev]

On 22/04/15 17:11, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:28:16PM +0300, Denis V. Lunev wrote:
>> Try to perform IO for the biggest continuous block possible.
>> The performance for sequential read is increased from 220 Gb/sec to
>> 360 Gb/sec for continous image on my SSD HDD.
> Please, can I have your SSD drive? :)
>
> Did you mean MB/sec?
yes... This was so wonderful to write these numbers.
I am dreaming.

OK, will fix that.

>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Roman Kagan <rkagan@parallels.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/parallels.c | 37 ++++++++++++++++++++++++++++++++-----
>>   1 file changed, 32 insertions(+), 5 deletions(-)
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH 23/27] block/parallels: create bat_entry_off helper

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:17PM +0300, Denis V. Lunev wrote:
> calculate offset of the BAT entry in the image file.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 24/27] block/parallels: delay writing to BAT till bdrv_co_flush_to_os

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:18PM +0300, Denis V. Lunev wrote:
> 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 uses loaded BAT data as a cache and performs
> actual on-disk metadata updates in parallels_co_flush_to_os callback.
> 
> 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 <den@openvz.org>
> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 44 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 26/27] block/parallels: optimize linear image expansion

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:28:20PM +0300, Denis V. Lunev wrote:
> Plain image expansion spends a lot of time to update image file size.
> This seriously affects the performance. The following simple test
>   qemu_img create -f parallels -o cluster_size=64k ./1.hds 64G
>   qemu_io -n -c "write -P 0x11 0 1024M" ./1.hds
> could be improved if the format driver will pre-allocate some space
> in the image file with a reasonable chunk.
> 
> This patch preallocates 128 Mb using bdrv_write_zeroes, which should
> normally use fallocate() call inside. Fallback to older truncate()
> could be used as a fallback using image open options thanks to the
> previous patch.
> 
> The benefit is around 15%.

qcow2 doesn't use bdrv_truncate() at all.  It simply writes past the end
of bs->file to grow the file.  Can you use this approach instead?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 0/27] write/create for Parallels images with reasonable performance

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2015 at 01:27:54PM +0300, Denis V. Lunev wrote:
> This patchset provides an ability to create of/write to Parallels
> images and some testing of the new code. Readings and writings are
> optimized out and I expect the same or slightly better performance
> as qcow2.
> 
> This patchset consists of not problematic part of the previous
> patchset aka
>    [PATCH v4 0/16] parallels format support improvements
> and new write/create code but it does not contradict questionable code
> with XML handling there.
> 
> Changes from v3:
> - fixed checkpatch warnings even in just moved code. I am tired of them :)
> - fixed contingency check in patch 18
> 
> Changes from v2:
> - read performance is almost doubled (up to 360 Mb/sec), write performance
>   is improved by 15-20%
> - bat caching approach changed completely. bat_bitmap now contains the data
>   in on-disk format, which allows to use this data for metadata cache
> - incorrect close detection code is added (inuse field in the header)
> - very basic check consistency code added
> 
> Changes from v1:
> - patches 13-19 added, which boosts performance from 800 KiB/sec to
>   near native performance
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Roman Kagan <rkagan@parallels.com>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>

I have done a first pass review and left comments.  Thanks!

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 26/27] block/parallels: optimize linear image expansion

[Denis V. Lunev]

On 22/04/15 17:18, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:28:20PM +0300, Denis V. Lunev wrote:
>> Plain image expansion spends a lot of time to update image file size.
>> This seriously affects the performance. The following simple test
>>    qemu_img create -f parallels -o cluster_size=64k ./1.hds 64G
>>    qemu_io -n -c "write -P 0x11 0 1024M" ./1.hds
>> could be improved if the format driver will pre-allocate some space
>> in the image file with a reasonable chunk.
>>
>> This patch preallocates 128 Mb using bdrv_write_zeroes, which should
>> normally use fallocate() call inside. Fallback to older truncate()
>> could be used as a fallback using image open options thanks to the
>> previous patch.
>>
>> The benefit is around 15%.
> qcow2 doesn't use bdrv_truncate() at all.  It simply writes past the end
> of bs->file to grow the file.  Can you use this approach instead?
this is worse from performance point of view.

OK, there is no difference if big write will come from guest. In
this case single write will do the job just fine. Though if the
file will be extended by several different write the situation
will be different. Each write will update inode metadata.
Welcome journal write. This metadata update will cost us
even more in the case of network filesystem and much more
in the case of distributed filesystem (additional MDS write
transaction at least).

This is the main reason to follow this approach here.

Re: [Qemu-devel] [PATCH 26/27] block/parallels: optimize linear image expansion

[Denis V. Lunev]

On 22/04/15 17:25, Denis V. Lunev wrote:
> On 22/04/15 17:18, Stefan Hajnoczi wrote:
>> On Wed, Mar 11, 2015 at 01:28:20PM +0300, Denis V. Lunev wrote:
>>> Plain image expansion spends a lot of time to update image file size.
>>> This seriously affects the performance. The following simple test
>>>    qemu_img create -f parallels -o cluster_size=64k ./1.hds 64G
>>>    qemu_io -n -c "write -P 0x11 0 1024M" ./1.hds
>>> could be improved if the format driver will pre-allocate some space
>>> in the image file with a reasonable chunk.
>>>
>>> This patch preallocates 128 Mb using bdrv_write_zeroes, which should
>>> normally use fallocate() call inside. Fallback to older truncate()
>>> could be used as a fallback using image open options thanks to the
>>> previous patch.
>>>
>>> The benefit is around 15%.
>> qcow2 doesn't use bdrv_truncate() at all.  It simply writes past the end
>> of bs->file to grow the file.  Can you use this approach instead?
> this is worse from performance point of view.
>
> OK, there is no difference if big write will come from guest. In
> this case single write will do the job just fine. Though if the
> file will be extended by several different write the situation
> will be different. Each write will update inode metadata.
> Welcome journal write. This metadata update will cost us
> even more in the case of network filesystem and much more
> in the case of distributed filesystem (additional MDS write
> transaction at least).
>
> This is the main reason to follow this approach here.

#define _GNU_SOURCE

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/types.h>
#include <malloc.h>
#include <string.h>

int main(int argc, char *argv[])
{
     int fd = open(argv[1], O_WRONLY | O_CREAT | O_DIRECT | O_TRUNC, 0644);
     void *buf;
     int i = 0;

     buf = memalign(4096, 65536);
     memset(buf, 0x11, 65536);

     for (i = 0; i < 1024 * 64; i++) {
         write(fd, buf, 65536);
     }

     close(fd);
     return 0;
}

# with O_TRUNC in the test
hades /vol $ time for i in `seq 1 10` ; do ./a.out aa ; done

real    3m4.031s
user    0m0.123s
sys    0m18.902s
hades /vol
hades /vol $
hades /vol $

# without O_TRUNC in the test (file exists)
hades /vol $ time for i in `seq 1 10` ; do ./a.out aa ; done

real    2m56.770s
user    0m0.133s
sys    0m10.756s
hades /vol $

The difference for this case (ext4) is 5%.

hades ~ $ uname -a
Linux hades 3.16.0-34-generic #47~14.04.1-Ubuntu SMP Fri Apr 10 17:49:16 
UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
hades ~ $

Re: [Qemu-devel] [PATCH 03/27] block/parallels: switch to bdrv_read

[Stefan Hajnoczi]

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

On Wed, Apr 22, 2015 at 03:30:50PM +0300, Denis V. Lunev wrote:
> On 22/04/15 15:23, Stefan Hajnoczi wrote:
> >On Wed, Mar 11, 2015 at 01:27:57PM +0300, Denis V. Lunev wrote:
> >>From: Roman Kagan <rkagan@parallels.com>
> >>
> >>Switch the .bdrv_read method implementation from using bdrv_pread() to
> >>bdrv_read() on the underlying file, since the latter is subject to i/o
> >>throttling while the former is not.
> >>
> >>Besides, since bdrv_read() operates in sectors rather than bytes, adjust
> >>the helper functions to do so too.
> >Can this patch be dropped since "[PATCH 06/27] block/parallels: provide
> >_co_readv routine for parallels format driver" switches to
> >.bdrv_co_readv() anyway?
> >
> >Stefan
> this is not that easy. This patch changes seek_to_sector call and
> in reality to drop it I have to merge patches 3, 4 and 6.
> Besides that this patch has been written by my colleague and
> I feel comfortable to preserve original Signed-off-by of him.

I understand.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status

[Stefan Hajnoczi]

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

On Wed, Apr 22, 2015 at 03:42:23PM +0300, Denis V. Lunev wrote:
> On 22/04/15 15:39, Stefan Hajnoczi wrote:
> >On Wed, Mar 11, 2015 at 01:27:59PM +0300, Denis V. Lunev wrote:
> >>+static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
> >>+        int64_t sector_num, int nb_sectors, int *pnum)
> >>+{
> >>+    BDRVParallelsState *s = bs->opaque;
> >>+    int64_t offset;
> >>+
> >>+    qemu_co_mutex_lock(&s->lock);
> >>+    offset = seek_to_sector(s, sector_num);
> >>+    qemu_co_mutex_unlock(&s->lock);
> >The lock isn't necessary here yet.  It may become necessary when write
> >support is added, but probably not even then since seek_to_sector()
> >cannot yield (it's not a coroutine function), so there are no possible
> >races with other coroutines.
> >
> >The same also applies for parallels_co_read().  The lock there isn't
> >necessary since the block driver is read-only and has no mutable state -
> >there is no shared state that needs lock protection.
> do you propose to drop the lock here and add it later with write
> support (patch 08)? I'd prefer to stay on the safe side. Yes, the
> lock is not mandatory at the moment but in 2 patches it will be
> mandatory.

This locking approach is very conservative.  At the end of the series,
the only region that needs a lock is allocate_clusters() because
bdrv_write_zeroes() may yield before s->bat_bitmap[] is assigned - we
need to prevent simultaneous allocate_clusters() calls to the same
clusters.

I'm fine with the conservative approach, but please add a doc comment to
BDRVParallelsState.lock explaining what this lock protects.  This way
people reading the code will understand your philosophy and be able to
follow it when modifying the code.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Stefan Hajnoczi]

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

On Wed, Apr 22, 2015 at 04:16:38PM +0300, Denis V. Lunev wrote:
> On 22/04/15 16:08, Stefan Hajnoczi wrote:
> >On Wed, Mar 11, 2015 at 01:28:02PM +0300, Denis V. Lunev wrote:
> >>+static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
> >>+{
> >>+    BDRVParallelsState *s = bs->opaque;
> >>+    uint32_t idx, offset, tmp;
> >>+    int64_t pos;
> >>+    int ret;
> >>+
> >>+    idx = sector_num / s->tracks;
> >>+    offset = sector_num % s->tracks;
> >>+
> >>+    if (idx >= s->catalog_size) {
> >>+        return -EINVAL;
> >>+    }
> >>+    if (s->catalog_bitmap[idx] != 0) {
> >>+        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
> >>+    }
> >>+
> >>+    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
> >>+    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
> >>+    s->catalog_bitmap[idx] = pos / s->off_multiplier;
> >>+
> >>+    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
> >>+
> >>+    ret = bdrv_pwrite_sync(bs->file,
> >>+            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
> >What is the purpose of the sync?
> This is necessary to preserve image consistency on crash from
> my point of view. There is no check consistency at the moment.
> The sync will be removed later when proper crash detection
> code will be added (patches 19, 20, 21)

Let's look at possible orderings in case of failure:

A. BAT update
B. Data write

This sync enforces A, B ordering.  If we can see B, then A must also
have happened thanks to the sync.

But A, B ordering is too conservative.  Imagine B, A ordering and the
failure where we crash before A.  It means we wrote the data but never
linked it into the BAT.

What happens in that case?  We've leaked a cluster in the underlying
image file but it doesn't corrupt the visible disk from the guest
point-of-view.

Because your implementation uses truncate to extend the file size before
A, even the A, B failure case results in a leaked cluster.  So the B, A
case is not worse in any way.

Why do other image formats sync cluster allocation updates?  Because
they support backing files and in that case an A, B ordering results in
data corruption so they enforce B, A ordering (the opposite of what
you're trying to do!).

The reason why A, B ordering results in data corruption when backing
files are in use is because the guest's write request might touch only a
subset of the cluster (a couple of sectors out of the whole cluster).
So the guest needs to copy the remaining sectors from the backing file.
If there is a dangling BAT entry like in the A, B failure case, then the
guest will see a zeroed cluster instead of the contents of the backing
file.  This is a data corruption, but only if a backing file is being
used!

So the sync is not necessary, both A, B and B, A ordering work for
block/parallels.c.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status

[Denis V. Lunev]

On 23/04/15 12:03, Stefan Hajnoczi wrote:
> On Wed, Apr 22, 2015 at 03:42:23PM +0300, Denis V. Lunev wrote:
>> On 22/04/15 15:39, Stefan Hajnoczi wrote:
>>> On Wed, Mar 11, 2015 at 01:27:59PM +0300, Denis V. Lunev wrote:
>>>> +static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
>>>> +        int64_t sector_num, int nb_sectors, int *pnum)
>>>> +{
>>>> +    BDRVParallelsState *s = bs->opaque;
>>>> +    int64_t offset;
>>>> +
>>>> +    qemu_co_mutex_lock(&s->lock);
>>>> +    offset = seek_to_sector(s, sector_num);
>>>> +    qemu_co_mutex_unlock(&s->lock);
>>> The lock isn't necessary here yet.  It may become necessary when write
>>> support is added, but probably not even then since seek_to_sector()
>>> cannot yield (it's not a coroutine function), so there are no possible
>>> races with other coroutines.
>>>
>>> The same also applies for parallels_co_read().  The lock there isn't
>>> necessary since the block driver is read-only and has no mutable state -
>>> there is no shared state that needs lock protection.
>> do you propose to drop the lock here and add it later with write
>> support (patch 08)? I'd prefer to stay on the safe side. Yes, the
>> lock is not mandatory at the moment but in 2 patches it will be
>> mandatory.
> This locking approach is very conservative.  At the end of the series,
> the only region that needs a lock is allocate_clusters() because
> bdrv_write_zeroes() may yield before s->bat_bitmap[] is assigned - we
> need to prevent simultaneous allocate_clusters() calls to the same
> clusters.
>
> I'm fine with the conservative approach, but please add a doc comment to
> BDRVParallelsState.lock explaining what this lock protects.  This way
> people reading the code will understand your philosophy and be able to
> follow it when modifying the code.
>
> Stefan
ok. sounds good for me.

I would like to implement the code as conservative as possible at the
beginning and place optimizations later step-by-step to have a
clear path to bisect the introductory patch.

At the moment I do not think that this locking scheme will affect
the performance but I can be wrong. There are a lot of stuff to
be implemented from the functional point of view before this will
be needed to tweak from my point of view.

Re: [Qemu-devel] [PATCH 26/27] block/parallels: optimize linear image expansion

[Stefan Hajnoczi]

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

On Wed, Apr 22, 2015 at 05:25:14PM +0300, Denis V. Lunev wrote:
> On 22/04/15 17:18, Stefan Hajnoczi wrote:
> >On Wed, Mar 11, 2015 at 01:28:20PM +0300, Denis V. Lunev wrote:
> >>Plain image expansion spends a lot of time to update image file size.
> >>This seriously affects the performance. The following simple test
> >>   qemu_img create -f parallels -o cluster_size=64k ./1.hds 64G
> >>   qemu_io -n -c "write -P 0x11 0 1024M" ./1.hds
> >>could be improved if the format driver will pre-allocate some space
> >>in the image file with a reasonable chunk.
> >>
> >>This patch preallocates 128 Mb using bdrv_write_zeroes, which should
> >>normally use fallocate() call inside. Fallback to older truncate()
> >>could be used as a fallback using image open options thanks to the
> >>previous patch.
> >>
> >>The benefit is around 15%.
> >qcow2 doesn't use bdrv_truncate() at all.  It simply writes past the end
> >of bs->file to grow the file.  Can you use this approach instead?
> this is worse from performance point of view.
> 
> OK, there is no difference if big write will come from guest. In
> this case single write will do the job just fine. Though if the
> file will be extended by several different write the situation
> will be different. Each write will update inode metadata.
> Welcome journal write. This metadata update will cost us
> even more in the case of network filesystem and much more
> in the case of distributed filesystem (additional MDS write
> transaction at least).
> 
> This is the main reason to follow this approach here.

You are right, this seems like a good approach.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Kevin Wolf]

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

Am 23.04.2015 um 11:20 hat Stefan Hajnoczi geschrieben:
> On Wed, Apr 22, 2015 at 04:16:38PM +0300, Denis V. Lunev wrote:
> > On 22/04/15 16:08, Stefan Hajnoczi wrote:
> > >On Wed, Mar 11, 2015 at 01:28:02PM +0300, Denis V. Lunev wrote:
> > >>+static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
> > >>+{
> > >>+    BDRVParallelsState *s = bs->opaque;
> > >>+    uint32_t idx, offset, tmp;
> > >>+    int64_t pos;
> > >>+    int ret;
> > >>+
> > >>+    idx = sector_num / s->tracks;
> > >>+    offset = sector_num % s->tracks;
> > >>+
> > >>+    if (idx >= s->catalog_size) {
> > >>+        return -EINVAL;
> > >>+    }
> > >>+    if (s->catalog_bitmap[idx] != 0) {
> > >>+        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
> > >>+    }
> > >>+
> > >>+    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
> > >>+    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
> > >>+    s->catalog_bitmap[idx] = pos / s->off_multiplier;
> > >>+
> > >>+    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
> > >>+
> > >>+    ret = bdrv_pwrite_sync(bs->file,
> > >>+            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
> > >What is the purpose of the sync?
> > This is necessary to preserve image consistency on crash from
> > my point of view. There is no check consistency at the moment.
> > The sync will be removed later when proper crash detection
> > code will be added (patches 19, 20, 21)
> 
> Let's look at possible orderings in case of failure:
> 
> A. BAT update
> B. Data write
> 
> This sync enforces A, B ordering.  If we can see B, then A must also
> have happened thanks to the sync.
> 
> But A, B ordering is too conservative.  Imagine B, A ordering and the
> failure where we crash before A.  It means we wrote the data but never
> linked it into the BAT.
> 
> What happens in that case?  We've leaked a cluster in the underlying
> image file but it doesn't corrupt the visible disk from the guest
> point-of-view.
> 
> Because your implementation uses truncate to extend the file size before
> A, even the A, B failure case results in a leaked cluster.  So the B, A
> case is not worse in any way.
> 
> Why do other image formats sync cluster allocation updates?  Because
> they support backing files and in that case an A, B ordering results in
> data corruption so they enforce B, A ordering (the opposite of what
> you're trying to do!).
> 
> The reason why A, B ordering results in data corruption when backing
> files are in use is because the guest's write request might touch only a
> subset of the cluster (a couple of sectors out of the whole cluster).
> So the guest needs to copy the remaining sectors from the backing file.
> If there is a dangling BAT entry like in the A, B failure case, then the
> guest will see a zeroed cluster instead of the contents of the backing
> file.  This is a data corruption, but only if a backing file is being
> used!
> 
> So the sync is not necessary, both A, B and B, A ordering work for
> block/parallels.c.

Actually, I suspect this means that the parallels driver is restricted
to protocols with bdrv_has_zero_init() == true, otherwise zeros can turn
into random data (which means that it can't work e.g. directly on host
block devices).

Do we enforce this?

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Denis V. Lunev]

On 23/04/15 12:20, Stefan Hajnoczi wrote:
> On Wed, Apr 22, 2015 at 04:16:38PM +0300, Denis V. Lunev wrote:
>> On 22/04/15 16:08, Stefan Hajnoczi wrote:
>>> On Wed, Mar 11, 2015 at 01:28:02PM +0300, Denis V. Lunev wrote:
>>>> +static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
>>>> +{
>>>> +    BDRVParallelsState *s = bs->opaque;
>>>> +    uint32_t idx, offset, tmp;
>>>> +    int64_t pos;
>>>> +    int ret;
>>>> +
>>>> +    idx = sector_num / s->tracks;
>>>> +    offset = sector_num % s->tracks;
>>>> +
>>>> +    if (idx >= s->catalog_size) {
>>>> +        return -EINVAL;
>>>> +    }
>>>> +    if (s->catalog_bitmap[idx] != 0) {
>>>> +        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
>>>> +    }
>>>> +
>>>> +    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
>>>> +    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
>>>> +    s->catalog_bitmap[idx] = pos / s->off_multiplier;
>>>> +
>>>> +    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
>>>> +
>>>> +    ret = bdrv_pwrite_sync(bs->file,
>>>> +            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
>>> What is the purpose of the sync?
>> This is necessary to preserve image consistency on crash from
>> my point of view. There is no check consistency at the moment.
>> The sync will be removed later when proper crash detection
>> code will be added (patches 19, 20, 21)
> Let's look at possible orderings in case of failure:
>
> A. BAT update
> B. Data write
>
> This sync enforces A, B ordering.  If we can see B, then A must also
> have happened thanks to the sync.
>
> But A, B ordering is too conservative.  Imagine B, A ordering and the
> failure where we crash before A.  It means we wrote the data but never
> linked it into the BAT.
>
> What happens in that case?  We've leaked a cluster in the underlying
> image file but it doesn't corrupt the visible disk from the guest
> point-of-view.
>
> Because your implementation uses truncate to extend the file size before
> A, even the A, B failure case results in a leaked cluster.  So the B, A
> case is not worse in any way.
>
> Why do other image formats sync cluster allocation updates?  Because
> they support backing files and in that case an A, B ordering results in
> data corruption so they enforce B, A ordering (the opposite of what
> you're trying to do!).
>
> The reason why A, B ordering results in data corruption when backing
> files are in use is because the guest's write request might touch only a
> subset of the cluster (a couple of sectors out of the whole cluster).
> So the guest needs to copy the remaining sectors from the backing file.
> If there is a dangling BAT entry like in the A, B failure case, then the
> guest will see a zeroed cluster instead of the contents of the backing
> file.  This is a data corruption, but only if a backing file is being
> used!
>
> So the sync is not necessary, both A, B and B, A ordering work for
> block/parallels.c.
>
> Stefan
this is very interesting speculation. I have never considered the
problem from this point of view. Thank you very much for this!

Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Denis V. Lunev]

On 23/04/15 12:32, Kevin Wolf wrote:
> Am 23.04.2015 um 11:20 hat Stefan Hajnoczi geschrieben:
>> On Wed, Apr 22, 2015 at 04:16:38PM +0300, Denis V. Lunev wrote:
>>> On 22/04/15 16:08, Stefan Hajnoczi wrote:
>>>> On Wed, Mar 11, 2015 at 01:28:02PM +0300, Denis V. Lunev wrote:
>>>>> +static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
>>>>> +{
>>>>> +    BDRVParallelsState *s = bs->opaque;
>>>>> +    uint32_t idx, offset, tmp;
>>>>> +    int64_t pos;
>>>>> +    int ret;
>>>>> +
>>>>> +    idx = sector_num / s->tracks;
>>>>> +    offset = sector_num % s->tracks;
>>>>> +
>>>>> +    if (idx >= s->catalog_size) {
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +    if (s->catalog_bitmap[idx] != 0) {
>>>>> +        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
>>>>> +    }
>>>>> +
>>>>> +    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
>>>>> +    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
>>>>> +    s->catalog_bitmap[idx] = pos / s->off_multiplier;
>>>>> +
>>>>> +    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
>>>>> +
>>>>> +    ret = bdrv_pwrite_sync(bs->file,
>>>>> +            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
>>>> What is the purpose of the sync?
>>> This is necessary to preserve image consistency on crash from
>>> my point of view. There is no check consistency at the moment.
>>> The sync will be removed later when proper crash detection
>>> code will be added (patches 19, 20, 21)
>> Let's look at possible orderings in case of failure:
>>
>> A. BAT update
>> B. Data write
>>
>> This sync enforces A, B ordering.  If we can see B, then A must also
>> have happened thanks to the sync.
>>
>> But A, B ordering is too conservative.  Imagine B, A ordering and the
>> failure where we crash before A.  It means we wrote the data but never
>> linked it into the BAT.
>>
>> What happens in that case?  We've leaked a cluster in the underlying
>> image file but it doesn't corrupt the visible disk from the guest
>> point-of-view.
>>
>> Because your implementation uses truncate to extend the file size before
>> A, even the A, B failure case results in a leaked cluster.  So the B, A
>> case is not worse in any way.
>>
>> Why do other image formats sync cluster allocation updates?  Because
>> they support backing files and in that case an A, B ordering results in
>> data corruption so they enforce B, A ordering (the opposite of what
>> you're trying to do!).
>>
>> The reason why A, B ordering results in data corruption when backing
>> files are in use is because the guest's write request might touch only a
>> subset of the cluster (a couple of sectors out of the whole cluster).
>> So the guest needs to copy the remaining sectors from the backing file.
>> If there is a dangling BAT entry like in the A, B failure case, then the
>> guest will see a zeroed cluster instead of the contents of the backing
>> file.  This is a data corruption, but only if a backing file is being
>> used!
>>
>> So the sync is not necessary, both A, B and B, A ordering work for
>> block/parallels.c.
> Actually, I suspect this means that the parallels driver is restricted
> to protocols with bdrv_has_zero_init() == true, otherwise zeros can turn
> into random data (which means that it can't work e.g. directly on host
> block devices).
>
> Do we enforce this?
>
> Kevin
this is fixed in the patch 26 when the code is replaced with

+    if (s->data_end + s->tracks > pos) {
+        int ret;
+        if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
+            ret = bdrv_write_zeroes(bs->file, s->data_end,
+                                    s->prealloc_size, 0);
+        } else {
+            ret = bdrv_truncate(bs->file,
+                    (s->data_end + s->prealloc_size) << BDRV_SECTOR_BITS);
+        }
+        if (ret < 0) {
+            return ret;
+        }
+    }

on a default path, but you are correct. Some checking is
necessary to be on a safe side.

Den

Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Kevin Wolf]

Am 23.04.2015 um 11:47 hat Denis V. Lunev geschrieben:
> On 23/04/15 12:32, Kevin Wolf wrote:
> >Am 23.04.2015 um 11:20 hat Stefan Hajnoczi geschrieben:
> >>On Wed, Apr 22, 2015 at 04:16:38PM +0300, Denis V. Lunev wrote:
> >>>On 22/04/15 16:08, Stefan Hajnoczi wrote:
> >>>>On Wed, Mar 11, 2015 at 01:28:02PM +0300, Denis V. Lunev wrote:
> >>>>>+static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
> >>>>>+{
> >>>>>+    BDRVParallelsState *s = bs->opaque;
> >>>>>+    uint32_t idx, offset, tmp;
> >>>>>+    int64_t pos;
> >>>>>+    int ret;
> >>>>>+
> >>>>>+    idx = sector_num / s->tracks;
> >>>>>+    offset = sector_num % s->tracks;
> >>>>>+
> >>>>>+    if (idx >= s->catalog_size) {
> >>>>>+        return -EINVAL;
> >>>>>+    }
> >>>>>+    if (s->catalog_bitmap[idx] != 0) {
> >>>>>+        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
> >>>>>+    }
> >>>>>+
> >>>>>+    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
> >>>>>+    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
> >>>>>+    s->catalog_bitmap[idx] = pos / s->off_multiplier;
> >>>>>+
> >>>>>+    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
> >>>>>+
> >>>>>+    ret = bdrv_pwrite_sync(bs->file,
> >>>>>+            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
> >>>>What is the purpose of the sync?
> >>>This is necessary to preserve image consistency on crash from
> >>>my point of view. There is no check consistency at the moment.
> >>>The sync will be removed later when proper crash detection
> >>>code will be added (patches 19, 20, 21)
> >>Let's look at possible orderings in case of failure:
> >>
> >>A. BAT update
> >>B. Data write
> >>
> >>This sync enforces A, B ordering.  If we can see B, then A must also
> >>have happened thanks to the sync.
> >>
> >>But A, B ordering is too conservative.  Imagine B, A ordering and the
> >>failure where we crash before A.  It means we wrote the data but never
> >>linked it into the BAT.
> >>
> >>What happens in that case?  We've leaked a cluster in the underlying
> >>image file but it doesn't corrupt the visible disk from the guest
> >>point-of-view.
> >>
> >>Because your implementation uses truncate to extend the file size before
> >>A, even the A, B failure case results in a leaked cluster.  So the B, A
> >>case is not worse in any way.
> >>
> >>Why do other image formats sync cluster allocation updates?  Because
> >>they support backing files and in that case an A, B ordering results in
> >>data corruption so they enforce B, A ordering (the opposite of what
> >>you're trying to do!).
> >>
> >>The reason why A, B ordering results in data corruption when backing
> >>files are in use is because the guest's write request might touch only a
> >>subset of the cluster (a couple of sectors out of the whole cluster).
> >>So the guest needs to copy the remaining sectors from the backing file.
> >>If there is a dangling BAT entry like in the A, B failure case, then the
> >>guest will see a zeroed cluster instead of the contents of the backing
> >>file.  This is a data corruption, but only if a backing file is being
> >>used!
> >>
> >>So the sync is not necessary, both A, B and B, A ordering work for
> >>block/parallels.c.
> >Actually, I suspect this means that the parallels driver is restricted
> >to protocols with bdrv_has_zero_init() == true, otherwise zeros can turn
> >into random data (which means that it can't work e.g. directly on host
> >block devices).
> >
> >Do we enforce this?
> >
> >Kevin
> this is fixed in the patch 26 when the code is replaced with
> 
> +    if (s->data_end + s->tracks > pos) {
> +        int ret;
> +        if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
> +            ret = bdrv_write_zeroes(bs->file, s->data_end,
> +                                    s->prealloc_size, 0);
> +        } else {
> +            ret = bdrv_truncate(bs->file,
> +                    (s->data_end + s->prealloc_size) << BDRV_SECTOR_BITS);
> +        }
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> 
> on a default path, but you are correct. Some checking is
> necessary to be on a safe side.

Ah, yes, you're right. That should fix it for the default settings. The
check is only needed when using the bdrv_truncate() path.

Kevin

Re: [Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status

[Stefan Hajnoczi]

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

On Thu, Apr 23, 2015 at 12:23:43PM +0300, Denis V. Lunev wrote:
> On 23/04/15 12:03, Stefan Hajnoczi wrote:
> >On Wed, Apr 22, 2015 at 03:42:23PM +0300, Denis V. Lunev wrote:
> >>On 22/04/15 15:39, Stefan Hajnoczi wrote:
> >>>On Wed, Mar 11, 2015 at 01:27:59PM +0300, Denis V. Lunev wrote:
> >>>>+static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
> >>>>+        int64_t sector_num, int nb_sectors, int *pnum)
> >>>>+{
> >>>>+    BDRVParallelsState *s = bs->opaque;
> >>>>+    int64_t offset;
> >>>>+
> >>>>+    qemu_co_mutex_lock(&s->lock);
> >>>>+    offset = seek_to_sector(s, sector_num);
> >>>>+    qemu_co_mutex_unlock(&s->lock);
> >>>The lock isn't necessary here yet.  It may become necessary when write
> >>>support is added, but probably not even then since seek_to_sector()
> >>>cannot yield (it's not a coroutine function), so there are no possible
> >>>races with other coroutines.
> >>>
> >>>The same also applies for parallels_co_read().  The lock there isn't
> >>>necessary since the block driver is read-only and has no mutable state -
> >>>there is no shared state that needs lock protection.
> >>do you propose to drop the lock here and add it later with write
> >>support (patch 08)? I'd prefer to stay on the safe side. Yes, the
> >>lock is not mandatory at the moment but in 2 patches it will be
> >>mandatory.
> >This locking approach is very conservative.  At the end of the series,
> >the only region that needs a lock is allocate_clusters() because
> >bdrv_write_zeroes() may yield before s->bat_bitmap[] is assigned - we
> >need to prevent simultaneous allocate_clusters() calls to the same
> >clusters.
> >
> >I'm fine with the conservative approach, but please add a doc comment to
> >BDRVParallelsState.lock explaining what this lock protects.  This way
> >people reading the code will understand your philosophy and be able to
> >follow it when modifying the code.
> >
> >Stefan
> ok. sounds good for me.
> 
> I would like to implement the code as conservative as possible at the
> beginning and place optimizations later step-by-step to have a
> clear path to bisect the introductory patch.
> 
> At the moment I do not think that this locking scheme will affect
> the performance but I can be wrong. There are a lot of stuff to
> be implemented from the functional point of view before this will
> be needed to tweak from my point of view.

Okay.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Denis V. Lunev]

Support write on Parallels images. The code is almost the same as one
in the previous patch implemented scatter-gather IO for read.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 306f2e3..61d7da7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -81,8 +81,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     ParallelsHeader ph;
     int ret;
 
-    bs->read_only = 1; // no write support yet
-
     ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
     if (ret < 0) {
         goto fail;
@@ -166,6 +164,37 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
     return MIN(nb_sectors, ret);
 }
 
+static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t idx, offset, tmp;
+    int64_t pos;
+    int ret;
+
+    idx = sector_num / s->tracks;
+    offset = sector_num % s->tracks;
+
+    if (idx >= s->catalog_size) {
+        return -EINVAL;
+    }
+    if (s->catalog_bitmap[idx] != 0) {
+        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
+    }
+
+    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
+    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
+    s->catalog_bitmap[idx] = pos / s->off_multiplier;
+
+    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
+
+    ret = bdrv_pwrite_sync(bs->file,
+            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
+    if (ret < 0) {
+        return ret;
+    }
+    return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
+}
+
 static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -186,6 +215,49 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
         BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
 }
 
+static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint64_t bytes_done = 0;
+    QEMUIOVector hd_qiov;
+    int ret = 0;
+
+    qemu_iovec_init(&hd_qiov, qiov->niov);
+
+    qemu_co_mutex_lock(&s->lock);
+    while (nb_sectors > 0) {
+        int64_t position = allocate_cluster(bs, sector_num);
+        int n = cluster_remainder(s, sector_num, nb_sectors);
+        int nbytes = n << BDRV_SECTOR_BITS;
+
+        if (position < 0) {
+            ret = (int)position;
+            break;
+        }
+
+        qemu_iovec_reset(&hd_qiov);
+        qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
+
+        qemu_co_mutex_unlock(&s->lock);
+        ret = bdrv_co_writev(bs->file, position, n, &hd_qiov);
+        qemu_co_mutex_lock(&s->lock);
+
+        if (ret < 0) {
+            goto fail;
+        }
+
+        nb_sectors -= n;
+        sector_num += n;
+        bytes_done += nbytes;
+    }
+    qemu_co_mutex_unlock(&s->lock);
+
+fail:
+    qemu_iovec_destroy(&hd_qiov);
+    return ret;
+}
+
 static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
@@ -242,6 +314,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_close		= parallels_close,
     .bdrv_co_get_block_status = parallels_co_get_block_status,
     .bdrv_co_readv  = parallels_co_readv,
+    .bdrv_co_writev = parallels_co_writev,
 };
 
 static void bdrv_parallels_init(void)
-- 
1.9.1