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

[Qemu-devel] [PATCH v4 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.

Changes from v4:
- parallels format driver marked as bdrv_has_zero_init_1
- added missed unlocks to parallels_co_readv/writev on error path, locking
  is shortened and simplified
- changed test number for created images
- added check for bdrv_has_zero_init() and availability of bdrv_truncate()
  in parallels_open() and proper error handling in alloc_cluster
- some patch comments are improved

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: 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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@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

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

[Stefan Hajnoczi]

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

On Tue, Apr 28, 2015 at 10:46:36AM +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.
> 
> 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(-)

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

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

[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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@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.

qemu_co_mutex_lock is not necessary yet (the driver is read-only) but
will be necessary very soon when write will be supported.

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

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

[Stefan Hajnoczi]

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

On Tue, Apr 28, 2015 at 10:46:38AM +0300, Denis V. Lunev wrote:
> From: Roman Kagan <rkagan@parallels.com>
> 
> Implement VFS method for get_block_status to Parallels format driver.
> 
> qemu_co_mutex_lock is not necessary yet (the driver is read-only) but
> will be necessary very soon when write will be supported.
> 
> 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(+)

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

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

[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.

The patch also adds comment about locking considerations in the driver.

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 | 54 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index b469984..f3ffece 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -49,6 +49,10 @@ typedef struct ParallelsHeader {
 } QEMU_PACKED ParallelsHeader;
 
 typedef struct BDRVParallelsState {
+    /** Locking is conservative, the lock protects
+     *   - image file extending (truncate, fallocate)
+     *   - any access to block allocation table
+     */
     CoMutex lock;
 
     uint32_t *catalog_bitmap;
@@ -186,37 +190,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);
 
     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);
+        int64_t position;
+        int n, nbytes;
+
+        qemu_co_mutex_lock(&s->lock);
+        position = seek_to_sector(s, sector_num);
+        qemu_co_mutex_unlock(&s->lock);
+
+        n = cluster_remainder(s, sector_num, nb_sectors);
+        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);
+
+            ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
             if (ret < 0) {
-                return ret;
+                break;
             }
-        } 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);
+    qemu_iovec_destroy(&hd_qiov);
     return ret;
 }
 
@@ -231,9 +243,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

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

[Roman Kagan]

On Tue, Apr 28, 2015 at 10:46:39AM +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.
> 
> The patch also adds comment about locking considerations in the driver.
> 
> 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 | 54 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 21 deletions(-)

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

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

[Stefan Hajnoczi]

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

On Tue, Apr 28, 2015 at 10:46:39AM +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.
> 
> The patch also adds comment about locking considerations in the driver.
> 
> 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 | 54 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 21 deletions(-)

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

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

[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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index f3ffece..138e618 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 {
@@ -67,7 +66,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) ||
@@ -120,7 +119,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;
@@ -131,7 +130,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: mark parallels format driver as zero inited

[Denis V. Lunev]

>From the guest point of view unallocated blocks are zeroed.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/parallels.c b/block/parallels.c
index 138e618..ae64ce5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -245,6 +245,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_open		= parallels_open,
     .bdrv_close		= parallels_close,
     .bdrv_co_get_block_status = parallels_co_get_block_status,
+    .bdrv_has_zero_init       = bdrv_has_zero_init_1,
     .bdrv_co_readv  = parallels_co_readv,
 };
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 08/27] block/parallels: mark parallels format driver as zero inited

[Roman Kagan]

On Tue, Apr 28, 2015 at 10:46:41AM +0300, Denis V. Lunev wrote:
> From the guest point of view unallocated blocks are zeroed.
> 
> 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 | 1 +
>  1 file changed, 1 insertion(+)

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

Re: [Qemu-devel] [PATCH 08/27] block/parallels: mark parallels format driver as zero inited

[Stefan Hajnoczi]

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

On Tue, Apr 28, 2015 at 10:46:41AM +0300, Denis V. Lunev wrote:
> From the guest point of view unallocated blocks are zeroed.
> 
> 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 | 1 +
>  1 file changed, 1 insertion(+)

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

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

[Qemu-devel] [PATCH 09/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>
CC: Roman Kagan <rkagan@parallels.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 88 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index ae64ce5..8d73803 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -60,6 +60,8 @@ typedef struct BDRVParallelsState {
     unsigned int tracks;
 
     unsigned int off_multiplier;
+
+    bool has_truncate;
 } BDRVParallelsState;
 
 static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
@@ -85,8 +87,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;
@@ -139,6 +139,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     for (i = 0; i < s->catalog_size; i++)
         le32_to_cpus(&s->catalog_bitmap[i]);
 
+    s->has_truncate = bdrv_has_zero_init(bs->file) &&
+                      bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
+
     qemu_co_mutex_init(&s->lock);
     return 0;
 
@@ -170,6 +173,46 @@ 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;
+    if (s->has_truncate) {
+        ret = bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
+    } else {
+        ret = bdrv_write_zeroes(bs->file, pos, s->tracks, 0);
+    }
+    if (ret < 0) {
+        return ret;
+    }
+
+    s->catalog_bitmap[idx] = pos / s->off_multiplier;
+
+    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
+
+    ret = bdrv_pwrite(bs->file,
+            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
+    if (ret < 0) {
+        s->catalog_bitmap[idx] = 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)
 {
@@ -190,6 +233,48 @@ 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);
+
+    while (nb_sectors > 0) {
+        int64_t position;
+        int n, nbytes;
+
+        qemu_co_mutex_lock(&s->lock);
+        position = allocate_cluster(bs, sector_num);
+        qemu_co_mutex_unlock(&s->lock);
+        if (position < 0) {
+            ret = (int)position;
+            break;
+        }
+
+        n = cluster_remainder(s, sector_num, nb_sectors);
+        nbytes = n << BDRV_SECTOR_BITS;
+
+        qemu_iovec_reset(&hd_qiov);
+        qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
+
+        ret = bdrv_co_writev(bs->file, position, n, &hd_qiov);
+        if (ret < 0) {
+            break;
+        }
+
+        nb_sectors -= n;
+        sector_num += n;
+        bytes_done += nbytes;
+    }
+
+    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)
 {
@@ -247,6 +332,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_co_get_block_status = parallels_co_get_block_status,
     .bdrv_has_zero_init       = bdrv_has_zero_init_1,
     .bdrv_co_readv  = parallels_co_readv,
+    .bdrv_co_writev = parallels_co_writev,
 };
 
 static void bdrv_parallels_init(void)
-- 
1.9.1

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

[Roman Kagan]

On Tue, Apr 28, 2015 at 10:46:42AM +0300, 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>
> CC: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 88 insertions(+), 2 deletions(-)

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

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

[Stefan Hajnoczi]

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

On Tue, Apr 28, 2015 at 10:46:42AM +0300, 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>
> CC: Roman Kagan <rkagan@parallels.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 88 insertions(+), 2 deletions(-)

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

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

[Qemu-devel] [PATCH 10/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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@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 11/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>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 8d73803..15f6cb3 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"
@@ -317,12 +320,103 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
     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),
@@ -333,6 +427,9 @@ static BlockDriver bdrv_parallels = {
     .bdrv_has_zero_init       = bdrv_has_zero_init_1,
     .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 12/27] iotests, parallels: test for newly created parallels image via qemu-img

[Denis V. Lunev]

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

diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
new file mode 100755
index 0000000..f45afa7
--- /dev/null
+++ b/tests/qemu-iotests/131
@@ -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/131.out b/tests/qemu-iotests/131.out
new file mode 100644
index 0000000..4158a2f
--- /dev/null
+++ b/tests/qemu-iotests/131.out
@@ -0,0 +1,24 @@
+QA output created by 131
+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 bcf2578..4318df0 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -125,3 +125,4 @@
 123 rw auto quick
 128 rw auto quick
 130 rw auto quick
+131 rw auto quick
-- 
1.9.1

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

[Denis V. Lunev]

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

diff --git a/block/parallels.c b/block/parallels.c
index 15f6cb3..f615f44 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) 2015 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 14/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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 58 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index f615f44..16fbdf4 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;
@@ -61,8 +61,8 @@ typedef struct BDRVParallelsState {
      */
     CoMutex lock;
 
-    uint32_t *catalog_bitmap;
-    unsigned int catalog_size;
+    uint32_t *bat_bitmap;
+    unsigned int bat_size;
 
     unsigned int tracks;
 
@@ -125,26 +125,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]);
+    }
 
     s->has_truncate = bdrv_has_zero_init(bs->file) &&
                       bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
@@ -156,7 +157,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;
 }
 
@@ -168,9 +169,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,
@@ -190,11 +192,11 @@ 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;
@@ -207,17 +209,17 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
         return ret;
     }
 
-    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(bs->file,
             sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
     if (ret < 0) {
-        s->catalog_bitmap[idx] = 0;
+        s->bat_bitmap[idx] = 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,
@@ -330,7 +332,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;
 
@@ -357,10 +359,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));
@@ -369,9 +371,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));
@@ -381,7 +383,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;
     }
@@ -399,7 +401,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 15/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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 16fbdf4..1540c21 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -161,6 +161,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;
@@ -172,7 +178,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,
@@ -196,7 +202,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;
@@ -219,7 +225,7 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
         s->bat_bitmap[idx] = 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 16/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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 1540c21..431adf1 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -90,7 +90,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
     BDRVParallelsState *s = bs->opaque;
-    int i;
     ParallelsHeader ph;
     int ret;
 
@@ -143,10 +142,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]);
-    }
-
     s->has_truncate = bdrv_has_zero_init(bs->file) &&
                       bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
 
@@ -164,7 +159,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)
@@ -191,7 +186,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;
 
@@ -215,12 +210,10 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
         return ret;
     }
 
-    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(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) {
         s->bat_bitmap[idx] = 0;
         return ret;
-- 
1.9.1

[Qemu-devel] [PATCH 17/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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 431adf1..f8a9981 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -61,6 +61,8 @@ typedef struct BDRVParallelsState {
      */
     CoMutex lock;
 
+    ParallelsHeader *header;
+    uint32_t header_size;
     uint32_t *bat_bitmap;
     unsigned int bat_size;
 
@@ -91,7 +93,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) {
@@ -130,17 +132,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);
 
     s->has_truncate = bdrv_has_zero_init(bs->file) &&
                       bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
@@ -152,7 +162,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;
 }
 
@@ -400,7 +410,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 18/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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 191 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 98 insertions(+), 93 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index f8a9981..35ffb6f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -73,99 +73,6 @@ typedef struct BDRVParallelsState {
     bool has_truncate;
 } 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);
-
-    s->has_truncate = bdrv_has_zero_init(bs->file) &&
-                      bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
-
-    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)
 {
@@ -407,6 +314,104 @@ 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);
+
+    s->has_truncate = bdrv_has_zero_init(bs->file) &&
+                      bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
+
+    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 19/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>
Reviewed-by: Roman Kagan <rkagan@parallels.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 35ffb6f..35f231a 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -242,6 +242,90 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
     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;
@@ -449,6 +533,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 20/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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 35f231a..76e3a4e 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 */
 
@@ -63,6 +64,8 @@ typedef struct BDRVParallelsState {
 
     ParallelsHeader *header;
     uint32_t header_size;
+    bool header_unclean;
+
     uint32_t *bat_bitmap;
     unsigned int bat_size;
 
@@ -259,6 +262,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 */
 
@@ -417,6 +431,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)
 {
@@ -484,6 +509,25 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     s->has_truncate = bdrv_has_zero_init(bs->file) &&
                       bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
 
+    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;
 
@@ -499,6 +543,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 21/27] iotests, parallels: check for incorrectly closed image in tests

[Denis V. Lunev]

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

diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index f45afa7..4873f40 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -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/131.out b/tests/qemu-iotests/131.out
index 4158a2f..021a04c 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.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 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 Mb/sec to
360 Mb/sec for continous image on my SSD HDD.

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

diff --git a/block/parallels.c b/block/parallels.c
index 76e3a4e..5ff74e8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -103,6 +103,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;
@@ -148,11 +177,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;
     }
@@ -218,10 +245,9 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
         int n, nbytes;
 
         qemu_co_mutex_lock(&s->lock);
-        position = seek_to_sector(s, sector_num);
+        position = block_status(s, sector_num, nb_sectors, &n);
         qemu_co_mutex_unlock(&s->lock);
 
-        n = cluster_remainder(s, sector_num, nb_sectors);
         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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 5ff74e8..4d8a0d4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -82,6 +82,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;
@@ -160,9 +165,8 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     }
 
     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) {
         s->bat_bitmap[idx] = 0;
         return ret;
@@ -400,8 +404,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));
@@ -513,7 +516,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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 4d8a0d4..05fe030 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"
 
 /**************************************************************/
 
@@ -66,6 +67,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;
 
@@ -165,15 +169,43 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     }
 
     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) {
-        s->bat_bitmap[idx] = 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)
 {
@@ -557,6 +589,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;
 
@@ -578,6 +614,7 @@ static void parallels_close(BlockDriverState *bs)
         parallels_update_header(bs);
     }
 
+    g_free(s->bat_dirty_bmap);
     qemu_vfree(s->header);
 }
 
@@ -608,6 +645,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_close		= parallels_close,
     .bdrv_co_get_block_status = parallels_co_get_block_status,
     .bdrv_has_zero_init       = bdrv_has_zero_init_1,
+    .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.

Original idea with sequential file writing to the end of the file without
fallocate/truncate would be slower than this approach if the image is
expanded with several operations:
- each image expanding means file metadata update, i.e. filesystem
  journal write. Truncate/write to newly truncated space update file
  metadata twice thus truncate removal helps. With fallocate call
  inside bdrv_write_zeroes file metadata is updated only once and
  this should happen infrequently thus this approach is the best one
  for the image expansion
- tail writes are ordered, i.e. the guest IO queue could not be sent
  immediately to the host introducing additional IO delays

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

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 | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 77 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 05fe030..440938e 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 {
     /** Locking is conservative, the lock protects
      *   - image file extending (truncate, fallocate)
@@ -73,14 +88,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;
-
-    bool has_truncate;
 } 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;
@@ -159,7 +200,7 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     }
 
     pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
-    if (s->has_truncate) {
+    if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
         ret = bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
     } else {
         ret = bdrv_write_zeroes(bs->file, pos, s->tracks, 0);
@@ -509,6 +550,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) {
@@ -567,9 +611,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->bat_bitmap = (uint32_t *)(s->header + 1);
 
-    s->has_truncate = bdrv_has_zero_init(bs->file) &&
-                      bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
-
     if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
         /* Image was not closed correctly. The check is mandatory */
         s->header_unclean = true;
@@ -581,6 +622,31 @@ 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 (!bdrv_has_zero_init(bs->file) ||
+            bdrv_truncate(bs->file, bdrv_getlength(bs->file)) != 0) {
+        s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
+    }
+
     if (flags & BDRV_O_RDWR) {
         s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
         ret = parallels_update_header(bs);
@@ -602,6 +668,11 @@ fail_format:
 fail:
     qemu_vfree(s->header);
     return ret;
+
+fail_options:
+    error_propagate(errp, local_err);
+    ret = -EINVAL;
+    goto fail;
 }
 
 
-- 
1.9.1

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

[Roman Kagan]

On Tue, Apr 28, 2015 at 10:46:58AM +0300, Denis V. Lunev wrote:
> 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.
> 
> Original idea with sequential file writing to the end of the file without
> fallocate/truncate would be slower than this approach if the image is
> expanded with several operations:
> - each image expanding means file metadata update, i.e. filesystem
>   journal write. Truncate/write to newly truncated space update file
>   metadata twice thus truncate removal helps. With fallocate call
>   inside bdrv_write_zeroes file metadata is updated only once and
>   this should happen infrequently thus this approach is the best one
>   for the image expansion
> - tail writes are ordered, i.e. the guest IO queue could not be sent
>   immediately to the host introducing additional IO delays
> 
> This patch just adds proper parameters into BDRVParallelsState and
> performs options parsing in parallels_open.
> 
> 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 77 insertions(+), 6 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 05fe030..440938e 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 {
>      /** Locking is conservative, the lock protects
>       *   - image file extending (truncate, fallocate)
> @@ -73,14 +88,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;
> -
> -    bool has_truncate;
>  } 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;
> @@ -159,7 +200,7 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
>      }
>  
>      pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
> -    if (s->has_truncate) {
> +    if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
>          ret = bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
>      } else {
>          ret = bdrv_write_zeroes(bs->file, pos, s->tracks, 0);
> @@ -509,6 +550,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) {
> @@ -567,9 +611,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>      s->bat_bitmap = (uint32_t *)(s->header + 1);
>  
> -    s->has_truncate = bdrv_has_zero_init(bs->file) &&
> -                      bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
> -
>      if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>          /* Image was not closed correctly. The check is mandatory */
>          s->header_unclean = true;

It may be slightly more logical to leave truncate support out of patch
09 sticking with bdrv_write_zeros there, and introduce it all in this
patch.

Otherwise looks good to me.

Roman.

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

[Roman Kagan]

On Tue, Apr 28, 2015 at 01:59:56PM +0300, Roman Kagan wrote:
> On Tue, Apr 28, 2015 at 10:46:58AM +0300, Denis V. Lunev wrote:
> > 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.
> > 
> > Original idea with sequential file writing to the end of the file without
> > fallocate/truncate would be slower than this approach if the image is
> > expanded with several operations:
> > - each image expanding means file metadata update, i.e. filesystem
> >   journal write. Truncate/write to newly truncated space update file
> >   metadata twice thus truncate removal helps. With fallocate call
> >   inside bdrv_write_zeroes file metadata is updated only once and
> >   this should happen infrequently thus this approach is the best one
> >   for the image expansion
> > - tail writes are ordered, i.e. the guest IO queue could not be sent
> >   immediately to the host introducing additional IO delays
> > 
> > This patch just adds proper parameters into BDRVParallelsState and
> > performs options parsing in parallels_open.
> > 
> > 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 77 insertions(+), 6 deletions(-)
> 
> It may be slightly more logical to leave truncate support out of patch
> 09 sticking with bdrv_write_zeros there, and introduce it all in this
> patch.
> 
> Otherwise looks good to me.

I mean,

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

Roman.

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

[Stefan Hajnoczi]

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

On Tue, Apr 28, 2015 at 10:46:58AM +0300, Denis V. Lunev wrote:
> 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.
> 
> Original idea with sequential file writing to the end of the file without
> fallocate/truncate would be slower than this approach if the image is
> expanded with several operations:
> - each image expanding means file metadata update, i.e. filesystem
>   journal write. Truncate/write to newly truncated space update file
>   metadata twice thus truncate removal helps. With fallocate call
>   inside bdrv_write_zeroes file metadata is updated only once and
>   this should happen infrequently thus this approach is the best one
>   for the image expansion
> - tail writes are ordered, i.e. the guest IO queue could not be sent
>   immediately to the host introducing additional IO delays
> 
> This patch just adds proper parameters into BDRVParallelsState and
> performs options parsing in parallels_open.
> 
> 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 77 insertions(+), 6 deletions(-)

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

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

[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 | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 440938e..e7124d9 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -88,6 +88,7 @@ typedef struct BDRVParallelsState {
     uint32_t *bat_bitmap;
     unsigned int bat_size;
 
+    int64_t  data_end;
     uint64_t prealloc_size;
     ParallelsPreallocMode prealloc_mode;
 
@@ -187,7 +188,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;
@@ -200,14 +200,21 @@ static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
     }
 
     pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
-    if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
-        ret = bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
-    } else {
-        ret = bdrv_write_zeroes(bs->file, pos, s->tracks, 0);
-    }
-    if (ret < 0) {
-        return ret;
+    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);
 
@@ -549,7 +556,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;
@@ -599,7 +606,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;
@@ -611,6 +622,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;
@@ -685,6 +703,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

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

[Roman Kagan]

On Tue, Apr 28, 2015 at 10:46:59AM +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%.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Roman Karan <rkagan@parallels.com>

s/Karan/Kagan/

> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 42 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)

Roman.

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

[Stefan Hajnoczi]

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

On Tue, Apr 28, 2015 at 10:46:59AM +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%.
> 
> 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 | 42 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)

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

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

[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 Mb/sec to
235 Mb/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 | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index e7124d9..046b568 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -183,43 +183,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;
 }
 
 
@@ -287,14 +291,13 @@ static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
         int n, nbytes;
 
         qemu_co_mutex_lock(&s->lock);
-        position = allocate_cluster(bs, sector_num);
+        position = allocate_clusters(bs, sector_num, nb_sectors, &n);
         qemu_co_mutex_unlock(&s->lock);
         if (position < 0) {
             ret = (int)position;
             break;
         }
 
-        n = cluster_remainder(s, sector_num, nb_sectors);
         nbytes = n << BDRV_SECTOR_BITS;
 
         qemu_iovec_reset(&hd_qiov);
-- 
1.9.1

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

[Stefan Hajnoczi]

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

On Tue, Apr 28, 2015 at 10:47:00AM +0300, Denis V. Lunev wrote:
> 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 Mb/sec to
> 235 Mb/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 | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)

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

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

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

[Denis V. Lunev]

On 28/04/15 10:46, 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.
>
> Changes from v4:
> - parallels format driver marked as bdrv_has_zero_init_1
> - added missed unlocks to parallels_co_readv/writev on error path, locking
>    is shortened and simplified
> - changed test number for created images
> - added check for bdrv_has_zero_init() and availability of bdrv_truncate()
>    in parallels_open() and proper error handling in alloc_cluster
> - some patch comments are improved
>
> 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: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
>
ping

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

[Denis V. Lunev]

On 28/04/15 10:46, 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.
>
> Changes from v4:
> - parallels format driver marked as bdrv_has_zero_init_1
> - added missed unlocks to parallels_co_readv/writev on error path, locking
>    is shortened and simplified
> - changed test number for created images
> - added check for bdrv_has_zero_init() and availability of bdrv_truncate()
>    in parallels_open() and proper error handling in alloc_cluster
> - some patch comments are improved
>
> 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: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
>
ping

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

[Stefan Hajnoczi]

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

On Tue, Apr 28, 2015 at 10:46:33AM +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.
> 
> Changes from v4:
> - parallels format driver marked as bdrv_has_zero_init_1
> - added missed unlocks to parallels_co_readv/writev on error path, locking
>   is shortened and simplified
> - changed test number for created images
> - added check for bdrv_has_zero_init() and availability of bdrv_truncate()
>   in parallels_open() and proper error handling in alloc_cluster
> - some patch comments are improved
> 
> 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: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>

Thanks, applied to my master tree:
https://github.com/stefanha/qemu/commits/master

Stefan

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

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

[Kevin Wolf]

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

Am 18.05.2015 um 18:45 hat Stefan Hajnoczi geschrieben:
> On Tue, Apr 28, 2015 at 10:46:33AM +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.
> > 
> > Changes from v4:
> > - parallels format driver marked as bdrv_has_zero_init_1
> > - added missed unlocks to parallels_co_readv/writev on error path, locking
> >   is shortened and simplified
> > - changed test number for created images
> > - added check for bdrv_has_zero_init() and availability of bdrv_truncate()
> >   in parallels_open() and proper error handling in alloc_cluster
> > - some patch comments are improved
> > 
> > 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: Kevin Wolf <kwolf@redhat.com>
> > CC: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Thanks, applied to my master tree:
> https://github.com/stefanha/qemu/commits/master

Are you sure you didn't intend to apply it to 'block' rather than
'master'? (Just trying to make sure it's not missing from the next pull
request.)

Kevin

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