qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC)
@ 2015-01-13 17:02 Vladimir Sementsov-Ogievskiy
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
                   ` (9 more replies)
  0 siblings, 10 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-13 17:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, pbonzini, den, jsnow

The bitmaps are saved into qcow2 file format. It provides both
'internal' and 'external' dirty bitmaps feature:
 - for qcow2 drives we can store bitmaps in the same file
 - for other formats we can store bitmaps in the separate qcow2 file

QCow2 header is extended by fields 'nb_dirty_bitmaps' and
'dirty_bitmaps_offset' like with snapshots.

Proposed command line syntax is the following:

-dirty-bitmap [option1=val1][,option2=val2]...
    Available options are:
    name         The name for the bitmap (necessary).

    file         The file to load the bitmap from.

    file_id      When specified with 'file' option, then this file will
                 be available through this id for other -dirty-bitmap
                 options when specified without 'file' option, then it
                 is a reference to 'file', specified with another
                 -dirty-bitmap option, and it will be used to load the
                 bitmap from.

    drive        The drive to bind the bitmap to. It should be specified
                 as 'id' suboption of one of -drive options. If nor
                 'file' neither 'file_id' are specified, then the bitmap
                 will be loaded from that drive (internal dirty bitmap).

    granularity  The granularity for the bitmap. Not necessary, the
                 default value may be used.

    enabled      on|off. Default is 'on'. Disabled bitmaps are not
                 changing regardless of writes to corresponding drive.

Examples:

qemu -drive file=a.qcow2,id=disk -dirty-bitmap name=b,drive=disk
qemu -drive file=a.raw,id=disk \
     -dirty-bitmap name=b,drive=disk,file=b.qcow2,enabled=off

Vladimir Sementsov-Ogievskiy (8):
  spec: add qcow2-dirty-bitmaps specification
  hbitmap: store / restore
  qcow2: add dirty-bitmaps feature
  block: store persistent dirty bitmaps
  block: add bdrv_load_dirty_bitmap
  qemu: command line option for dirty bitmaps
  qmp: print dirty bitmap
  iotests: test internal persistent dirty bitmap

 block.c                    | 113 ++++++++++
 block/Makefile.objs        |   2 +-
 block/qcow2-dirty-bitmap.c | 514 +++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c              |  26 +++
 block/qcow2.h              |  48 +++++
 blockdev.c                 |  51 +++++
 docs/specs/qcow2.txt       |  59 ++++++
 hmp-commands.hx            |  15 ++
 hmp.c                      |   8 +
 hmp.h                      |   1 +
 include/block/block.h      |   9 +
 include/block/block_int.h  |  10 +
 include/qemu/hbitmap.h     |  49 +++++
 include/sysemu/blockdev.h  |   1 +
 include/sysemu/sysemu.h    |   1 +
 qapi-schema.json           |   3 +-
 qapi/block-core.json       |   3 +
 qemu-options.hx            |  37 ++++
 qmp-commands.hx            |   5 +
 tests/qemu-iotests/115     |  96 +++++++++
 tests/qemu-iotests/115.out |  64 ++++++
 tests/qemu-iotests/group   |   1 +
 util/hbitmap.c             |  87 ++++++++
 vl.c                       | 100 +++++++++
 24 files changed, 1301 insertions(+), 2 deletions(-)
 create mode 100644 block/qcow2-dirty-bitmap.c
 create mode 100755 tests/qemu-iotests/115
 create mode 100644 tests/qemu-iotests/115.out

-- 
1.9.1

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
@ 2015-01-13 17:02 ` Vladimir Sementsov-Ogievskiy
  2015-01-27 15:39   ` Eric Blake
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 2/8] hbitmap: store / restore Vladimir Sementsov-Ogievskiy
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-13 17:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, pbonzini, den, jsnow

Persistent dirty bitmaps will be saved into qcow2 files. It may be used
as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
other drives (there may be qcow2 file with zero disk size but with
several dirty bitmaps for other drives).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
---
 docs/specs/qcow2.txt | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 121dfc8..b29de40 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -116,6 +116,13 @@ in the description of a field.
                     Length of the header structure in bytes. For version 2
                     images, the length is always assumed to be 72 bytes.
 
+        104 - 107:  nb_dirty_bitmaps
+                    Number of dirty bitmaps contained in the image
+
+        108 - 115:  dirty_bitmaps_offset
+                    Offset into the image file at which the dirty bitmaps table
+                    starts. Must be aligned to a cluster boundary.
+
 Directly after the image header, optional sections called header extensions can
 be stored. Each extension has a structure like the following:
 
@@ -360,3 +367,55 @@ Snapshot table entry:
 
         variable:   Padding to round up the snapshot table entry size to the
                     next multiple of 8.
+
+
+== Dirty bitmaps ==
+
+The feature supports storing several dirty bitmaps in the qcow2 file.
+
+=== Cluster mapping ===
+
+Dirty bitmaps are stored using a ONE-level structure for the mapping of
+bitmaps to host clusters. There only an L1 table.
+
+The L1 table has a variable size (stored in the Bitmap table entry) and may
+use multiple clusters, however it must be contiguous in the image file.
+
+Given an offset into the bitmap, the offset into the image file can be
+obtained as follows:
+
+    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
+
+L1 table entry:
+
+    Bit  0 -  61:   Standard cluster descriptor
+
+        62 -  63:   Reserved
+
+=== Bitmap table ===
+
+A directory of all bitmaps is stored in the bitmap table, a contiguous area in
+the image file, whose starting offset and length are given by the header fields
+dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
+variable length, depending on the length of name and extra data.
+
+Bitmap table entry:
+
+    Byte 0 -  7:    Offset into the image file at which the L1 table for the
+                    bitmap starts. Must be aligned to a cluster boundary.
+
+         8 - 11:    Number of entries in the L1 table of the bitmap
+
+        12 - 15:    Bitmap granularity in bytes
+
+        16 - 23:    Bitmap size in sectors
+
+        24 - 25:    Size of the bitmap name
+
+        variable:   The name of the bitmap (not null terminated)
+
+        variable:   Padding to round up the bitmap table entry size to the
+                    next multiple of 8.
+
+The fields "size", "granularity" and "name" are corresponding with the fields
+in struct BdrvDirtyBitmap.
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Qemu-devel] [PATCH 2/8] hbitmap: store / restore
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
@ 2015-01-13 17:02 ` Vladimir Sementsov-Ogievskiy
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 3/8] qcow2: add dirty-bitmaps feature Vladimir Sementsov-Ogievskiy
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-13 17:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, pbonzini, den, jsnow

Functions to store / restore HBitmap. HBitmap should be saved to linear
bitmap format independently of endianess.

These functions are appropriate for dirty bitmap migration, retoring the
bitmap in several steps is available.  To save performance, every step
writes only the last level of the bitmap. All other levels are restored
by hbitmap_restore_finish as a last step of restoring. So, HBitmap is
inconsistent while restoring.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
---
 include/qemu/hbitmap.h | 49 ++++++++++++++++++++++++++++
 util/hbitmap.c         | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index c48c50a..f432f7f 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -137,6 +137,55 @@ void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count);
 bool hbitmap_get(const HBitmap *hb, uint64_t item);
 
 /**
+ * hbitmap_data_size:
+ * @hb: HBitmap to operate on.
+ * @count: Number of bits
+ *
+ * Return amount of bytes hbitmap_store_data needs
+ */
+uint64_t hbitmap_data_size(const HBitmap *hb, uint64_t count);
+
+/**
+ * hbitmap_store_data
+ * @hb: HBitmap to oprate on.
+ * @buf: Buffer to store bitmap data.
+ * @start: First bit to store.
+ * @count: Number of bits to store.
+ *
+ * Stores HBitmap data corresponding to given region. The format of saved data
+ * is linear sequence of bits, so it can be used by hbitmap_restore_data
+ * independently of endianess
+ */
+void hbitmap_store_data(const HBitmap *hb, uint8_t *buf,
+                        uint64_t start, uint64_t count);
+
+/**
+ * hbitmap_restore_data
+ * @hb: HBitmap to oprate on.
+ * @buf: Buffer to restore bitmap data from.
+ * @start: First bit to restore.
+ * @count: Number of bits to restore.
+ *
+ * Retores HBitmap data corresponding to given region. The format is the same
+ * as for hbitmap_store_data.
+ *
+ * ! The bitmap becomes inconsistent after this operation.
+ * hbitmap_restore_finish should be called before using the bitmap after
+ * data restoring.
+ */
+void hbitmap_restore_data(HBitmap *hb, uint8_t *buf,
+                          uint64_t start, uint64_t count);
+
+/**
+ * hbitmap_restore_finish
+ * @hb: HBitmap to operate on.
+ *
+ * Repair HBitmap after calling hbitmap_restore_data. Actuall all HBitmap
+ * layers are restore here.
+ */
+void hbitmap_restore_finish(HBitmap *hb);
+
+/**
  * hbitmap_free:
  * @hb: HBitmap to operate on.
  *
diff --git a/util/hbitmap.c b/util/hbitmap.c
index f400dcb..5d1a776 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -366,6 +366,93 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
     return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) != 0;
 }
 
+uint64_t hbitmap_data_size(const HBitmap *hb, uint64_t count)
+{
+    uint64_t size;
+
+    if (count == 0) {
+        return 0;
+    }
+
+    size = (((count - 1) >> hb->granularity) >> BITS_PER_LEVEL) + 1;
+
+    return size * sizeof(unsigned long);
+}
+
+void hbitmap_store_data(const HBitmap *hb, uint8_t *buf,
+                        uint64_t start, uint64_t count)
+{
+    uint64_t last = start + count - 1;
+    unsigned long *out = (unsigned long *)buf;
+
+    if (count == 0) {
+        return;
+    }
+
+    start = (start >> hb->granularity) >> BITS_PER_LEVEL;
+    last = (last >> hb->granularity) >> BITS_PER_LEVEL;
+    count = last - start + 1;
+
+#ifdef __BIG_ENDIAN_BITFIELD
+    for (i = start; i <= last; ++i) {
+        unsigned long el = hb->levels[HBITMAP_LEVELS - 1][i];
+        out[i] = (BITS_PER_LONG == 32 ? cpu_to_le32(el) : cpu_to_le64(el));
+    }
+#else
+    memcpy(out, &hb->levels[HBITMAP_LEVELS - 1][start],
+           count * sizeof(unsigned long));
+#endif
+}
+
+void hbitmap_restore_data(HBitmap *hb, uint8_t *buf,
+                          uint64_t start, uint64_t count)
+{
+    uint64_t last = start + count - 1;
+    unsigned long *in = (unsigned long *)buf;
+
+    if (count == 0) {
+        return;
+    }
+
+    start = (start >> hb->granularity) >> BITS_PER_LEVEL;
+    last = (last >> hb->granularity) >> BITS_PER_LEVEL;
+    count = last - start + 1;
+
+#ifdef __BIG_ENDIAN_BITFIELD
+    for (i = start; i <= last; ++i) {
+        hb->levels[HBITMAP_LEVELS - 1][i] =
+            (BITS_PER_LONG == 32 ? be32_to_cpu(in[i]) : be64_to_cpu(in[i]));
+    }
+#else
+    memcpy(&hb->levels[HBITMAP_LEVELS - 1][start], in,
+           count * sizeof(unsigned long));
+#endif
+}
+
+void hbitmap_restore_finish(HBitmap *bitmap)
+{
+    int64_t i, size, prev_size;
+    int lev;
+
+    /* restore levels starting from penultimate to zero level, assuming
+     * that the last level is ok */
+    size = MAX((bitmap->size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+    for (lev = HBITMAP_LEVELS - 1; lev-- > 0; ) {
+        prev_size = size;
+        size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+        memset(bitmap->levels[lev], 0, size * sizeof(unsigned long));
+
+        for (i = 0; i < prev_size; ++i) {
+            if (bitmap->levels[lev + 1][i]) {
+                bitmap->levels[lev][i >> BITS_PER_LEVEL] |=
+                    1 << (i & (BITS_PER_LONG - 1));
+            }
+        }
+    }
+
+    bitmap->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
+}
+
 void hbitmap_free(HBitmap *hb)
 {
     unsigned i;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Qemu-devel] [PATCH 3/8] qcow2: add dirty-bitmaps feature
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 2/8] hbitmap: store / restore Vladimir Sementsov-Ogievskiy
@ 2015-01-13 17:02 ` Vladimir Sementsov-Ogievskiy
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 4/8] block: store persistent dirty bitmaps Vladimir Sementsov-Ogievskiy
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-13 17:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, pbonzini, den, jsnow

Adds dirty-bitmaps feature to qcow2 format as specified in
docs/specs/qcow2.txt

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
---
 block/Makefile.objs        |   2 +-
 block/qcow2-dirty-bitmap.c | 514 +++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c              |  26 +++
 block/qcow2.h              |  48 +++++
 include/block/block_int.h  |  10 +
 5 files changed, 599 insertions(+), 1 deletion(-)
 create mode 100644 block/qcow2-dirty-bitmap.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 04b0e43..eebd1c9 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,5 +1,5 @@
 block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o
-block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
+block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o qcow2-dirty-bitmap.o
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c
new file mode 100644
index 0000000..b3d114f
--- /dev/null
+++ b/block/qcow2-dirty-bitmap.c
@@ -0,0 +1,514 @@
+/*
+ * Dirty bitmpas for the QCOW version 2 format
+ *
+ * Copyright (c) 2014-2015 Vladimir Sementsov-Ogievskiy
+ *
+ * This file is derived from qcow2-snapshot.c, original copyright:
+ * Copyright (c) 2004-2006 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu-common.h"
+#include "block/block_int.h"
+#include "block/qcow2.h"
+
+void qcow2_free_dirty_bitmaps(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    int i;
+
+    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
+        g_free(s->dirty_bitmaps[i].name);
+    }
+    g_free(s->dirty_bitmaps);
+    s->dirty_bitmaps = NULL;
+    s->nb_dirty_bitmaps = 0;
+}
+
+int qcow2_read_dirty_bitmaps(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    QCowDirtyBitmapHeader h;
+    QCowDirtyBitmap *bm;
+    int i, name_size;
+    int64_t offset;
+    int ret;
+
+    if (!s->nb_dirty_bitmaps) {
+        s->dirty_bitmaps = NULL;
+        s->dirty_bitmaps_size = 0;
+        return 0;
+    }
+
+    offset = s->dirty_bitmaps_offset;
+    s->dirty_bitmaps = g_new0(QCowDirtyBitmap, s->nb_dirty_bitmaps);
+
+    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
+        /* Read statically sized part of the dirty_bitmap header */
+        offset = align_offset(offset, 8);
+        ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
+        if (ret < 0) {
+            goto fail;
+        }
+
+        offset += sizeof(h);
+        bm = s->dirty_bitmaps + i;
+        bm->l1_table_offset = be64_to_cpu(h.l1_table_offset);
+        bm->l1_size = be32_to_cpu(h.l1_size);
+        bm->bitmap_granularity = be32_to_cpu(h.bitmap_granularity);
+        bm->bitmap_size = be64_to_cpu(h.bitmap_size);
+
+        name_size = be16_to_cpu(h.name_size);
+
+        /* Read dirty_bitmap name */
+        bm->name = g_malloc(name_size + 1);
+        ret = bdrv_pread(bs->file, offset, bm->name, name_size);
+        if (ret < 0) {
+            goto fail;
+        }
+        offset += name_size;
+        bm->name[name_size] = '\0';
+
+        if (offset - s->dirty_bitmaps_offset > QCOW_MAX_DIRTY_BITMAPS_SIZE) {
+            ret = -EFBIG;
+            goto fail;
+        }
+    }
+
+    assert(offset - s->dirty_bitmaps_offset <= INT_MAX);
+    s->dirty_bitmaps_size = offset - s->dirty_bitmaps_offset;
+    return 0;
+
+fail:
+    qcow2_free_dirty_bitmaps(bs);
+    return ret;
+}
+
+/* add at the end of the file a new list of dirty bitmaps */
+static int qcow2_write_dirty_bitmaps(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    QCowDirtyBitmap *bm;
+    QCowDirtyBitmapHeader h;
+    int i, name_size, dirty_bitmaps_size;
+    struct {
+        uint32_t nb_dirty_bitmaps;
+        uint64_t dirty_bitmaps_offset;
+    } QEMU_PACKED header_data;
+    int64_t offset, dirty_bitmaps_offset = 0;
+    int ret;
+
+    /* compute the size of the dirty bitmaps */
+    offset = 0;
+    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
+        bm = s->dirty_bitmaps + i;
+        offset = align_offset(offset, 8);
+        offset += sizeof(h);
+        offset += strlen(bm->name);
+
+        if (offset > QCOW_MAX_DIRTY_BITMAPS_SIZE) {
+            ret = -EFBIG;
+            goto fail;
+        }
+    }
+
+    assert(offset <= INT_MAX);
+    dirty_bitmaps_size = offset;
+
+    /* Allocate space for the new dirty bitmap list */
+    dirty_bitmaps_offset = qcow2_alloc_clusters(bs, dirty_bitmaps_size);
+    offset = dirty_bitmaps_offset;
+    if (offset < 0) {
+        ret = offset;
+        goto fail;
+    }
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* The dirty bitmap list position has not yet been updated, so these
+     * clusters must indeed be completely free */
+    ret = qcow2_pre_write_overlap_check(bs, 0, offset, dirty_bitmaps_size);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* Write all dirty bitmaps to the new list */
+    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
+        bm = s->dirty_bitmaps + i;
+        memset(&h, 0, sizeof(h));
+        h.l1_table_offset = cpu_to_be64(bm->l1_table_offset);
+        h.l1_size = cpu_to_be32(bm->l1_size);
+        h.bitmap_granularity = cpu_to_be32(bm->bitmap_granularity);
+        h.bitmap_size = cpu_to_be64(bm->bitmap_size);
+
+        name_size = strlen(bm->name);
+        assert(name_size <= UINT16_MAX);
+        h.name_size = cpu_to_be16(name_size);
+        offset = align_offset(offset, 8);
+
+        ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
+        if (ret < 0) {
+            goto fail;
+        }
+        offset += sizeof(h);
+
+        ret = bdrv_pwrite(bs->file, offset, bm->name, name_size);
+        if (ret < 0) {
+            goto fail;
+        }
+        offset += name_size;
+    }
+
+    /*
+     * Update the header to point to the new dirty bitmap table. This requires
+     * the new table and its refcounts to be stable on disk.
+     */
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    QEMU_BUILD_BUG_ON(offsetof(QCowHeader, dirty_bitmaps_offset) !=
+                      offsetof(QCowHeader, nb_dirty_bitmaps) +
+                      sizeof(header_data.nb_dirty_bitmaps));
+
+    header_data.nb_dirty_bitmaps        = cpu_to_be32(s->nb_dirty_bitmaps);
+    header_data.dirty_bitmaps_offset    = cpu_to_be64(dirty_bitmaps_offset);
+
+    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_dirty_bitmaps),
+                           &header_data, sizeof(header_data));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* free the old dirty bitmap table */
+    qcow2_free_clusters(bs, s->dirty_bitmaps_offset, s->dirty_bitmaps_size,
+                        QCOW2_DISCARD_ALWAYS);
+    s->dirty_bitmaps_offset = dirty_bitmaps_offset;
+    s->dirty_bitmaps_size = dirty_bitmaps_size;
+    return 0;
+
+fail:
+    if (dirty_bitmaps_offset > 0) {
+        qcow2_free_clusters(bs, dirty_bitmaps_offset, dirty_bitmaps_size,
+                            QCOW2_DISCARD_ALWAYS);
+    }
+    return ret;
+}
+
+static int find_dirty_bitmap_by_name(BlockDriverState *bs,
+                                     const char *name)
+{
+    BDRVQcowState *s = bs->opaque;
+    int i;
+
+    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
+        if (!strcmp(s->dirty_bitmaps[i].name, name)) {
+            return i;
+        }
+    }
+
+    return -1;
+}
+
+uint8_t *qcow2_dirty_bitmap_load(BlockDriverState *bs,
+                            const char *name, uint64_t size,
+                            int granularity)
+{
+    BDRVQcowState *s = bs->opaque;
+    int i, dirty_bitmap_index, ret;
+    uint64_t offset;
+    QCowDirtyBitmap *bm;
+    uint64_t *l1_table;
+    uint8_t *buf;
+
+    dirty_bitmap_index = find_dirty_bitmap_by_name(bs, name);
+    if (dirty_bitmap_index < 0) {
+        return NULL;
+    }
+    bm = &s->dirty_bitmaps[dirty_bitmap_index];
+
+    if (size != bm->bitmap_size || granularity != bm->bitmap_granularity) {
+        return NULL;
+    }
+
+    l1_table = g_malloc(bm->l1_size * sizeof(uint64_t));
+    ret = bdrv_pread(bs->file, bm->l1_table_offset, l1_table,
+                     bm->l1_size * sizeof(uint64_t));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    buf = g_malloc0(bm->l1_size * s->cluster_size);
+    for (i = 0; i < bm->l1_size; ++i) {
+        offset = be64_to_cpu(l1_table[i]);
+        if (!(offset & 1)) {
+            ret = bdrv_pread(bs->file, offset, buf + i * s->cluster_size,
+                             s->cluster_size);
+            if (ret < 0) {
+                goto fail;
+            }
+        }
+    }
+
+    g_free(l1_table);
+    return buf;
+
+fail:
+    g_free(l1_table);
+    return NULL;
+}
+
+int qcow2_dirty_bitmap_store(BlockDriverState *bs, uint8_t *buf,
+                            const char *name, uint64_t size,
+                            int granularity)
+{
+    BDRVQcowState *s = bs->opaque;
+    int cl_size = s->cluster_size;
+    int i, dirty_bitmap_index, ret = 0, n;
+    uint64_t *l1_table;
+    QCowDirtyBitmap *bm;
+    uint64_t buf_size;
+    uint8_t *p;
+    int sector_granularity = granularity >> BDRV_SECTOR_BITS;
+
+    /* find/create dirty bitmap */
+    dirty_bitmap_index = find_dirty_bitmap_by_name(bs, name);
+    if (dirty_bitmap_index >= 0) {
+        bm = s->dirty_bitmaps + dirty_bitmap_index;
+
+        if (size != bm->bitmap_size ||
+            granularity != bm->bitmap_granularity) {
+            qcow2_dirty_bitmap_delete(bs, name, NULL);
+            dirty_bitmap_index = -1;
+        }
+    }
+    if (dirty_bitmap_index < 0) {
+        qcow2_dirty_bitmap_create(bs, name, size, granularity);
+        dirty_bitmap_index = s->nb_dirty_bitmaps - 1;
+    }
+    bm = s->dirty_bitmaps + dirty_bitmap_index;
+
+    /* read l1 table */
+    l1_table = g_malloc(bm->l1_size * sizeof(uint64_t));
+    ret = bdrv_pread(bs->file, bm->l1_table_offset, l1_table,
+                     bm->l1_size * sizeof(uint64_t));
+    if (ret < 0) {
+        goto finish;
+    }
+
+    buf_size = (((size - 1) / sector_granularity) >> 3) + 1;
+    buf_size = align_offset(buf_size, 4);
+    n = buf_size / cl_size;
+    p = buf;
+    for (i = 0; i < bm->l1_size; ++i) {
+        uint64_t addr = be64_to_cpu(l1_table[i]) & ~511;
+        int write_size = (i == n ? (buf_size % cl_size) : cl_size);
+
+        if (buffer_is_zero(p, write_size)) {
+            if (addr) {
+                qcow2_free_clusters(bs, addr, cl_size,
+                                    QCOW2_DISCARD_ALWAYS);
+            }
+            l1_table[i] = cpu_to_be64(1);
+        } else {
+            if (!addr) {
+                addr = qcow2_alloc_clusters(bs, cl_size);
+                l1_table[i] = cpu_to_be64(addr);
+            }
+
+            ret = bdrv_pwrite(bs->file, addr, p, write_size);
+            if (ret < 0) {
+                goto finish;
+            }
+        }
+
+        p += cl_size;
+    }
+
+    ret = bdrv_pwrite(bs->file, bm->l1_table_offset, l1_table,
+                      bm->l1_size * sizeof(uint64_t));
+    if (ret < 0) {
+        goto finish;
+    }
+
+finish:
+    g_free(l1_table);
+    return ret;
+}
+/* if no id is provided, a new one is constructed */
+int qcow2_dirty_bitmap_create(BlockDriverState *bs, const char *name,
+                              uint64_t size, int granularity)
+{
+    BDRVQcowState *s = bs->opaque;
+    QCowDirtyBitmap *new_dirty_bitmap_list = NULL;
+    QCowDirtyBitmap *old_dirty_bitmap_list = NULL;
+    QCowDirtyBitmap sn1, *bm = &sn1;
+    int i, ret;
+    uint64_t *l1_table = NULL;
+    int64_t l1_table_offset;
+    int sector_granularity = granularity >> BDRV_SECTOR_BITS;
+
+    if (s->nb_dirty_bitmaps >= QCOW_MAX_DIRTY_BITMAPS) {
+        return -EFBIG;
+    }
+
+    memset(bm, 0, sizeof(*bm));
+
+    /* Check that the ID is unique */
+    if (find_dirty_bitmap_by_name(bs, name) >= 0) {
+        return -EEXIST;
+    }
+
+    /* Populate bm with passed data */
+    bm->name = g_strdup(name);
+    bm->bitmap_granularity = granularity;
+    bm->bitmap_size = size;
+
+    bm->l1_size =
+        size_to_clusters(s, (((size - 1) / sector_granularity) >> 3) + 1);
+    l1_table_offset =
+        qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
+    if (l1_table_offset < 0) {
+        ret = l1_table_offset;
+        goto fail;
+    }
+    bm->l1_table_offset = l1_table_offset;
+
+    l1_table = g_try_new(uint64_t, bm->l1_size);
+    if (l1_table == NULL) {
+        ret = -ENOMEM;
+        goto fail;
+    }
+
+    /* initialize with zero clusters */
+    for (i = 0; i < s->l1_size; i++) {
+        l1_table[i] = cpu_to_be64(1);
+    }
+
+    ret = qcow2_pre_write_overlap_check(bs, 0, bm->l1_table_offset,
+                                        s->l1_size * sizeof(uint64_t));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    ret = bdrv_pwrite(bs->file, bm->l1_table_offset, l1_table,
+                      s->l1_size * sizeof(uint64_t));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    g_free(l1_table);
+    l1_table = NULL;
+
+    /* Append the new dirty bitmap to the dirty bitmap list */
+    new_dirty_bitmap_list = g_new(QCowDirtyBitmap, s->nb_dirty_bitmaps + 1);
+    if (s->dirty_bitmaps) {
+        memcpy(new_dirty_bitmap_list, s->dirty_bitmaps,
+               s->nb_dirty_bitmaps * sizeof(QCowDirtyBitmap));
+        old_dirty_bitmap_list = s->dirty_bitmaps;
+    }
+    s->dirty_bitmaps = new_dirty_bitmap_list;
+    s->dirty_bitmaps[s->nb_dirty_bitmaps++] = *bm;
+
+    ret = qcow2_write_dirty_bitmaps(bs);
+    if (ret < 0) {
+        g_free(s->dirty_bitmaps);
+        s->dirty_bitmaps = old_dirty_bitmap_list;
+        s->nb_dirty_bitmaps--;
+        goto fail;
+    }
+
+    g_free(old_dirty_bitmap_list);
+
+    return 0;
+
+fail:
+    g_free(bm->name);
+    g_free(l1_table);
+
+    return ret;
+}
+
+int qcow2_dirty_bitmap_delete(BlockDriverState *bs,
+                              const char *name,
+                              Error **errp)
+{
+    BDRVQcowState *s = bs->opaque;
+    QCowDirtyBitmap bm;
+    int dirty_bitmap_index, ret = 0, i;
+    uint64_t *l1_table;
+
+    /* Search the dirty_bitmap */
+    dirty_bitmap_index = find_dirty_bitmap_by_name(bs, name);
+    if (dirty_bitmap_index < 0) {
+        error_setg(errp, "Can't find the dirty bitmap");
+        return -ENOENT;
+    }
+    bm = s->dirty_bitmaps[dirty_bitmap_index];
+
+    /* Remove it from the dirty_bitmap list */
+    memmove(s->dirty_bitmaps + dirty_bitmap_index,
+            s->dirty_bitmaps + dirty_bitmap_index + 1,
+            (s->nb_dirty_bitmaps - dirty_bitmap_index - 1) * sizeof(bm));
+    s->nb_dirty_bitmaps--;
+    ret = qcow2_write_dirty_bitmaps(bs);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed to remove dirty bitmap"
+                         " from dirty bitmap list");
+        return ret;
+    }
+
+    /*
+     * The dirty_bitmap is now unused, clean up. If we fail after this point, we
+     * won't recover but just leak clusters.
+     */
+    g_free(bm.name);
+
+    /*
+     * Now decrease the refcounts of clusters referenced by the dirty_bitmap and
+     * free the L1 table.
+     */
+    l1_table = g_try_new(uint64_t, bm.l1_size);
+    if (l1_table == NULL) {
+        ret = -ENOMEM;
+        goto finish;
+    }
+    ret = bdrv_pread(bs->file, bm.l1_table_offset, l1_table,
+                     s->l1_size * sizeof(uint64_t));
+    if (ret < 0) {
+        goto finish;
+    }
+
+    for (i = 0; i < bm.l1_size; ++i) {
+        uint64_t addr = be64_to_cpu(l1_table[i]);
+        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_ALWAYS);
+    }
+
+    qcow2_free_clusters(bs, bm.l1_table_offset, bm.l1_size * sizeof(uint64_t),
+                        QCOW2_DISCARD_ALWAYS);
+
+finish:
+    g_free(l1_table);
+    return ret;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index e4e690a..6512788 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -570,6 +570,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     be32_to_cpus(&header.refcount_table_clusters);
     be64_to_cpus(&header.snapshots_offset);
     be32_to_cpus(&header.nb_snapshots);
+    be64_to_cpus(&header.dirty_bitmaps_offset);
+    be32_to_cpus(&header.nb_dirty_bitmaps);
 
     if (header.magic != QCOW_MAGIC) {
         error_setg(errp, "Image is not in qcow2 format");
@@ -892,6 +894,16 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    /* Internal bitmaps */
+    s->dirty_bitmaps_offset = header.dirty_bitmaps_offset;
+    s->nb_dirty_bitmaps = header.nb_dirty_bitmaps;
+
+    ret = qcow2_read_dirty_bitmaps(bs);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read dirty bitmaps");
+        goto fail;
+    }
+
     /* Clear unknown autoclear feature bits */
     if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->autoclear_features) {
         s->autoclear_features = 0;
@@ -994,6 +1006,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     g_free(s->unknown_header_fields);
     cleanup_unknown_header_ext(bs);
     qcow2_free_snapshots(bs);
+    qcow2_free_dirty_bitmaps(bs);
     qcow2_refcount_close(bs);
     qemu_vfree(s->l1_table);
     /* else pre-write overlap checks in cache_destroy may crash */
@@ -1457,6 +1470,7 @@ static void qcow2_close(BlockDriverState *bs)
     qemu_vfree(s->cluster_data);
     qcow2_refcount_close(bs);
     qcow2_free_snapshots(bs);
+    qcow2_free_dirty_bitmaps(bs);
 }
 
 static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
@@ -1579,6 +1593,8 @@ int qcow2_update_header(BlockDriverState *bs)
         .refcount_table_clusters = cpu_to_be32(refcount_table_clusters),
         .nb_snapshots           = cpu_to_be32(s->nb_snapshots),
         .snapshots_offset       = cpu_to_be64(s->snapshots_offset),
+        .nb_dirty_bitmaps           = cpu_to_be32(s->nb_dirty_bitmaps),
+        .dirty_bitmaps_offset       = cpu_to_be64(s->dirty_bitmaps_offset),
 
         /* Version 3 fields */
         .incompatible_features  = cpu_to_be64(s->incompatible_features),
@@ -2123,6 +2139,12 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     }
 
+    /* cannot proceed if image has dirty_bitmaps */
+    if (s->nb_dirty_bitmaps) {
+        error_report("Can't resize an image which has dirty bitmaps");
+        return -ENOTSUP;
+    }
+
     /* shrinking is currently not supported */
     if (offset < bs->total_sectors * 512) {
         error_report("qcow2 doesn't support shrinking images yet");
@@ -2888,6 +2910,10 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_get_info          = qcow2_get_info,
     .bdrv_get_specific_info = qcow2_get_specific_info,
 
+    .bdrv_dirty_bitmap_load = qcow2_dirty_bitmap_load,
+    .bdrv_dirty_bitmap_store = qcow2_dirty_bitmap_store,
+    .bdrv_dirty_bitmap_delete = qcow2_dirty_bitmap_delete,
+
     .bdrv_save_vmstate    = qcow2_save_vmstate,
     .bdrv_load_vmstate    = qcow2_load_vmstate,
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 6e39a1b..45a166d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -39,6 +39,7 @@
 
 #define QCOW_MAX_CRYPT_CLUSTERS 32
 #define QCOW_MAX_SNAPSHOTS 65536
+#define QCOW_MAX_DIRTY_BITMAPS 65536
 
 /* 8 MB refcount table is enough for 2 PB images at 64k cluster size
  * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */
@@ -52,6 +53,8 @@
  * space for snapshot names and IDs */
 #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
 
+#define QCOW_MAX_DIRTY_BITMAPS_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS)
+
 /* indicate that the refcount of the referenced cluster is exactly one. */
 #define QCOW_OFLAG_COPIED     (1ULL << 63)
 /* indicate that the cluster is compressed (they never have the copied flag) */
@@ -116,6 +119,9 @@ typedef struct QCowHeader {
 
     uint32_t refcount_order;
     uint32_t header_length;
+
+    uint32_t nb_dirty_bitmaps;
+    uint64_t dirty_bitmaps_offset;
 } QEMU_PACKED QCowHeader;
 
 typedef struct QEMU_PACKED QCowSnapshotHeader {
@@ -138,6 +144,19 @@ typedef struct QEMU_PACKED QCowSnapshotHeader {
     /* name follows  */
 } QCowSnapshotHeader;
 
+typedef struct QEMU_PACKED QCowDirtyBitmapHeader {
+    /* header is 8 byte aligned */
+    uint64_t l1_table_offset;
+
+    uint32_t l1_size;
+    uint32_t bitmap_granularity;
+
+    uint64_t bitmap_size;
+    uint16_t name_size;
+
+    /* name follows  */
+} QCowDirtyBitmapHeader;
+
 typedef struct QEMU_PACKED QCowSnapshotExtraData {
     uint64_t vm_state_size_large;
     uint64_t disk_size;
@@ -156,6 +175,14 @@ typedef struct QCowSnapshot {
     uint64_t vm_clock_nsec;
 } QCowSnapshot;
 
+typedef struct QCowDirtyBitmap {
+    uint64_t l1_table_offset;
+    uint32_t l1_size;
+    char *name;
+    int bitmap_granularity;
+    uint64_t bitmap_size;
+} QCowDirtyBitmap;
+
 struct Qcow2Cache;
 typedef struct Qcow2Cache Qcow2Cache;
 
@@ -254,6 +281,11 @@ typedef struct BDRVQcowState {
     unsigned int nb_snapshots;
     QCowSnapshot *snapshots;
 
+    uint64_t dirty_bitmaps_offset;
+    int dirty_bitmaps_size;
+    unsigned int nb_dirty_bitmaps;
+    QCowDirtyBitmap *dirty_bitmaps;
+
     int flags;
     int qcow_version;
     bool use_lazy_refcounts;
@@ -558,6 +590,22 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
 void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs);
 
+/* qcow2-dirty-bitmap.c functions */
+int qcow2_dirty_bitmap_store(BlockDriverState *bs, uint8_t *buf,
+                             const char *name, uint64_t size,
+                             int granularity);
+uint8_t *qcow2_dirty_bitmap_load(BlockDriverState *bs,
+                                 const char *name, uint64_t size,
+                                 int granularity);
+int qcow2_dirty_bitmap_create(BlockDriverState *bs, const char *name,
+                              uint64_t size, int granularity);
+int qcow2_dirty_bitmap_delete(BlockDriverState *bs,
+                              const char *name,
+                              Error **errp);
+
+void qcow2_free_dirty_bitmaps(BlockDriverState *bs);
+int qcow2_read_dirty_bitmaps(BlockDriverState *bs);
+
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
 int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index cb1e4a1..0e3b5b3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -204,6 +204,16 @@ struct BlockDriver {
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
     ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
 
+    int (*bdrv_dirty_bitmap_store)(BlockDriverState *bs, uint8_t *buf,
+                                   const char *name, uint64_t size,
+                                   int granularity);
+    uint8_t *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs,
+                                       const char *name, uint64_t size,
+                                       int granularity);
+    int (*bdrv_dirty_bitmap_delete)(BlockDriverState *bs,
+                                    const char *name,
+                                    Error **errp);
+
     int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
                              int64_t pos);
     int (*bdrv_load_vmstate)(BlockDriverState *bs, uint8_t *buf,
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Qemu-devel] [PATCH 4/8] block: store persistent dirty bitmaps
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
                   ` (2 preceding siblings ...)
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 3/8] qcow2: add dirty-bitmaps feature Vladimir Sementsov-Ogievskiy
@ 2015-01-13 17:02 ` Vladimir Sementsov-Ogievskiy
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 5/8] block: add bdrv_load_dirty_bitmap Vladimir Sementsov-Ogievskiy
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-13 17:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, pbonzini, den, jsnow

Persistent dirty bitmaps are the bitmaps, for which the new field
BdrvDirtyBitmap.file is not NULL. We save all persistent dirty bitmaps
owned by BlockDriverState in corresponding bdrv_close().
BdrvDirtyBitmap.file is a BlockDriverState, where we want to save the
bitmap. It may be set in bdrv_dirty_bitmap_set_file() only once.
bdrv_ref/bdrv_unref are used for BdrvDirtyBitmap.file to be sure that
files will be closed and resources will be freed.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
---
 block.c               | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  3 +++
 2 files changed, 46 insertions(+)

diff --git a/block.c b/block.c
index 2466ba8..7237b95 100644
--- a/block.c
+++ b/block.c
@@ -54,6 +54,7 @@
 struct BdrvDirtyBitmap {
     HBitmap *bitmap;
     BdrvDirtyBitmap *originator;
+    BlockDriverState *file;
     int64_t size;
     int64_t granularity;
     char *name;
@@ -1840,6 +1841,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
 void bdrv_close(BlockDriverState *bs)
 {
     BdrvAioNotifier *ban, *ban_next;
+    BdrvDirtyBitmap *bm, *bm_next;
 
     if (bs->job) {
         block_job_cancel_sync(bs->job);
@@ -1849,6 +1851,15 @@ void bdrv_close(BlockDriverState *bs)
     bdrv_drain_all(); /* in case flush left pending I/O */
     notifier_list_notify(&bs->close_notifiers, bs);
 
+    /* save and release persistent dirty bitmaps */
+    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, bm_next) {
+        if (bm->file) {
+            bdrv_store_dirty_bitmap(bm);
+            bdrv_unref(bm->file);
+            bdrv_release_dirty_bitmap(bs, bm);
+        }
+    }
+
     if (bs->drv) {
         if (bs->backing_hd) {
             BlockDriverState *backing_hd = bs->backing_hd;
@@ -5373,6 +5384,29 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
     return originator;
 }
 
+int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+    BlockDriverState *bs = bitmap->file;
+    uint8_t *buf;
+    uint64_t size;
+    assert(bs);
+    assert(bs->drv);
+    assert(bs->drv->bdrv_dirty_bitmap_store);
+
+    size = hbitmap_data_size(bitmap->bitmap, bitmap->size);
+    size = (size + 3) & ~3;
+    buf = g_malloc(size);
+
+    hbitmap_store_data(bitmap->bitmap, buf, 0, bitmap->size);
+
+    int res = bs->drv->bdrv_dirty_bitmap_store(bs, buf,
+                                               bitmap->name,
+                                               bitmap->size,
+                                               bitmap->granularity);
+
+    g_free(buf);
+    return res;
+}
 
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           int granularity,
@@ -5421,6 +5455,15 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
     }
 }
 
+void bdrv_dirty_bitmap_set_file(BdrvDirtyBitmap *bitmap, BlockDriverState *file)
+{
+    assert(bitmap->file == NULL);
+    bitmap->file = file;
+    if (file != NULL) {
+        bdrv_ref(file);
+    }
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     bitmap->enabled = false;
diff --git a/include/block/block.h b/include/block/block.h
index cb1f28d..0dfefe3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -442,6 +442,8 @@ BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
 BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
                                            BdrvDirtyBitmap *failed);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_set_file(BdrvDirtyBitmap *bitmap,
+                                BlockDriverState *file);
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
@@ -458,6 +460,7 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
                           BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
 void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
 void bdrv_disable_copy_on_read(BlockDriverState *bs);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Qemu-devel] [PATCH 5/8] block: add bdrv_load_dirty_bitmap
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
                   ` (3 preceding siblings ...)
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 4/8] block: store persistent dirty bitmaps Vladimir Sementsov-Ogievskiy
@ 2015-01-13 17:02 ` Vladimir Sementsov-Ogievskiy
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 6/8] qemu: command line option for dirty bitmaps Vladimir Sementsov-Ogievskiy
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-13 17:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, pbonzini, den, jsnow

The funcion loads dirty bitmap from file, using underlying driver
function.

Note: the function doesn't change BdrvDirtyBitmap.file field. This field
is only used by bdrv_store_dirty_bitmap() function and is ONLY written
by bdrv_dirty_bitmap_set_file() function.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
---
 block.c               | 37 +++++++++++++++++++++++++++++++++++++
 include/block/block.h |  5 +++++
 2 files changed, 42 insertions(+)

diff --git a/block.c b/block.c
index 7237b95..77419e9 100644
--- a/block.c
+++ b/block.c
@@ -5384,6 +5384,43 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
     return originator;
 }
 
+BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState *bs,
+                                        BlockDriverState *file,
+                                        int granularity,
+                                        const char *name,
+                                        Error **errp)
+{
+    BlockDriver *drv = file->drv;
+    if (!drv) {
+        return NULL;
+    }
+    if (drv->bdrv_dirty_bitmap_load) {
+        BdrvDirtyBitmap *bitmap;
+        uint64_t bitmap_size = bdrv_nb_sectors(bs);
+        uint8_t *buf = drv->bdrv_dirty_bitmap_load(file, name, bitmap_size,
+                                                   granularity);
+        if (buf == NULL) {
+            return NULL;
+        }
+
+        bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+        if (bitmap == NULL) {
+            g_free(buf);
+            return NULL;
+        }
+
+        hbitmap_restore_data(bitmap->bitmap, buf, 0, bitmap_size);
+        hbitmap_restore_finish(bitmap->bitmap);
+
+        return bitmap;
+    }
+    if (file->file)  {
+        return bdrv_load_dirty_bitmap(bs, file->file, granularity, name,
+                                      errp);
+    }
+    return NULL;
+}
+
 int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     BlockDriverState *bs = bitmap->file;
diff --git a/include/block/block.h b/include/block/block.h
index 0dfefe3..f36557f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -460,6 +460,11 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
                           BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
 void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState *bs,
+                                        BlockDriverState *file,
+                                        int granularity,
+                                        const char *name,
+                                        Error **errp);
 int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Qemu-devel] [PATCH 6/8] qemu: command line option for dirty bitmaps
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
                   ` (4 preceding siblings ...)
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 5/8] block: add bdrv_load_dirty_bitmap Vladimir Sementsov-Ogievskiy
@ 2015-01-13 17:02 ` Vladimir Sementsov-Ogievskiy
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap Vladimir Sementsov-Ogievskiy
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-13 17:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, pbonzini, den, jsnow

The patch adds the following command line option:

-dirty-bitmap [option1=val1][,option2=val2]...
    Available options are:
    name         The name for the bitmap (necessary).

    file         The file to load the bitmap from.

    file_id      When specified with 'file' option, then this file will
                 be available through this id for other -dirty-bitmap
                 options when specified without 'file' option, then it
                 is a reference to 'file', specified with another
                 -dirty-bitmap option, and it will be used to load the
                 bitmap from.

    drive        The drive to bind the bitmap to. It should be specified
                 as 'id' suboption of one of -drive options. If nor
                 'file' neither 'file_id' are specified, then the bitmap
                 will be loaded from that drive (internal dirty bitmap).

    granularity  The granularity for the bitmap. Not necessary, the
                 default value may be used.

    enabled      on|off. Default is 'on'. Disabled bitmaps are not
                 changing regardless of writes to corresponding drive.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
---
 blockdev.c                |  38 ++++++++++++++++++
 include/sysemu/blockdev.h |   1 +
 include/sysemu/sysemu.h   |   1 +
 qemu-options.hx           |  37 +++++++++++++++++
 vl.c                      | 100 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 177 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 209fedd..8a9be08 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -176,6 +176,11 @@ QemuOpts *drive_def(const char *optstr)
     return qemu_opts_parse(qemu_find_opts("drive"), optstr, 0);
 }
 
+QemuOpts *dirty_bitmap_def(const char *optstr)
+{
+    return qemu_opts_parse(qemu_find_opts("dirty-bitmap"), optstr, 0);
+}
+
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr)
 {
@@ -3044,6 +3049,39 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
     return head;
 }
 
+QemuOptsList qemu_dirty_bitmap_opts = {
+    .name = "dirty-bitmap",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_dirty_bitmap_opts.head),
+    .desc = {
+        {
+            .name = "name",
+            .type = QEMU_OPT_STRING,
+            .help = "Name of the dirty bitmap",
+        },{
+            .name = "file",
+            .type = QEMU_OPT_STRING,
+            .help = "file name to load the bitmap from",
+        },{
+            .name = "file_id",
+            .type = QEMU_OPT_STRING,
+            .help = "node name to load the bitmap from (or to set id for"
+                    " for file, opened by previous option)",
+        },{
+            .name = "drive",
+            .type = QEMU_OPT_STRING,
+            .help = "drive id to bind the bitmap to",
+        },{
+            .name = "granularity",
+            .type = QEMU_OPT_NUMBER,
+            .help = "granularity",
+        },{
+            .name = "enabled",
+            .type = QEMU_OPT_BOOL,
+            .help = "enabled flag (default is 'on')",
+        }
+    }
+};
+
 QemuOptsList qemu_common_drive_opts = {
     .name = "drive",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 09d1e30..48cce5c 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -57,6 +57,7 @@ int drive_get_max_devs(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
 
 QemuOpts *drive_def(const char *optstr);
+QemuOpts *dirty_bitmap_def(const char *optstr);
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr);
 DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 503e5a4..c984a52 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -231,6 +231,7 @@ bool usb_enabled(bool default_usb);
 
 extern QemuOptsList qemu_legacy_drive_opts;
 extern QemuOptsList qemu_common_drive_opts;
+extern QemuOptsList qemu_dirty_bitmap_opts;
 extern QemuOptsList qemu_drive_opts;
 extern QemuOptsList qemu_chardev_opts;
 extern QemuOptsList qemu_device_opts;
diff --git a/qemu-options.hx b/qemu-options.hx
index 10b9568..3a5bfde 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -592,6 +592,43 @@ qemu-system-i386 -hda a -hdb b
 @end example
 ETEXI
 
+DEF("dirty-bitmap", HAS_ARG, QEMU_OPTION_dirty_bitmap,
+    "-dirty-bitmap name=name[,file=file][,file_id=file_id][,drive=@var{id}]\n"
+    "              [,granularity=granularity][,enabled=on|off]\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -dirty-bitmap @var{option}[,@var{option}[,@var{option}[,...]]]
+@findex -dirty-bitmap
+
+Define a dirty-bitmap. Valid options are:
+
+@table @option
+@item name=@var{name}
+The name of the bitmap. Should be unique per @var{file}/@var{drive} and per
+@var{for_drive}.
+@item file=@var{file}
+The separate qcow2 file for loading the bitmap @var{name} from it.
+@item file_id=@var{file_id}
+When specified with @var{file} option, then this @var{file} will be available
+through this @var{file_id} for other @option{-dirty-bitmap} options.
+When specified without @var{file} option, then it is a reference to @var{file},
+specified with another @option{-dirty-bitmap} option, and it will be used to
+load the bitmap from.
+@item drive=@var{drive}
+The drive to bind the bitmap to. It should be specified as @var{id} suboption
+of one of @option{-drive} options.
+If nor @var{file} neither @var{file_id} are specified, then the bitmap will be
+loaded from that drive (internal dirty bitmap).
+@item granularity=@var{granularity}
+Granularity (in bytes) for created dirty bitmap. If the bitmap is already
+exists in specified @var{file}/@var{file_id}/@var{device} it's granularity will
+not be changed but only checked (an error will be generated if this check
+fails).
+@item enabled=@var{enabled}
+Enabled flag for the bitmap. By default the bitmap will be enabled.
+@end table
+ETEXI
+
 DEF("mtdblock", HAS_ARG, QEMU_OPTION_mtdblock,
     "-mtdblock file  use 'file' as on-board Flash memory image\n",
     QEMU_ARCH_ALL)
diff --git a/vl.c b/vl.c
index a824a7d..092771e 100644
--- a/vl.c
+++ b/vl.c
@@ -1157,6 +1157,95 @@ static int cleanup_add_fd(QemuOpts *opts, void *opaque)
 #define MTD_OPTS ""
 #define SD_OPTS ""
 
+static int dirty_bitmap_func(QemuOpts *opts, void *opaque)
+{
+    Error *local_err = NULL;
+    Error **errp = &local_err;
+    BlockDriverState *file_bs = NULL, *for_bs = NULL;
+    BdrvDirtyBitmap *bitmap = NULL;
+
+    const char *name = qemu_opt_get(opts, "name");
+    const char *drive = qemu_opt_get(opts, "drive");
+    const char *file = qemu_opt_get(opts, "file");
+    const char *file_id = qemu_opt_get(opts, "file_id");
+
+    uint64_t granularity = qemu_opt_get_number(opts, "granularity", 0);
+    bool enabled = qemu_opt_get_bool(opts, "enabled", true);
+
+    if (name == NULL) {
+        error_setg(errp, "'name' option is necessary");
+        goto fail;
+    }
+
+    if (drive == NULL) {
+        error_setg(errp, "'drive' option is necessary");
+        goto fail;
+    }
+
+    for_bs = bdrv_lookup_bs(drive, NULL, errp);
+    if (for_bs == NULL) {
+        goto fail;
+    }
+
+    if (file != NULL) {
+        QDict *options = NULL;
+        if (file_id != NULL) {
+            options = qdict_new();
+            qdict_put(options, "node-name", qstring_from_str(file_id));
+        }
+
+        bdrv_open(&file_bs, file, NULL, options, 0, NULL, errp);
+        if (options) {
+            QDECREF(options);
+        }
+        if (file_bs == NULL) {
+            goto fail;
+        }
+    } else if (file_id != NULL) {
+        file_bs = bdrv_find_node(file_id);
+        if (file_bs == NULL) {
+            error_setg(errp, "node '%s' is not found", drive);
+            goto fail;
+        }
+    } else {
+        file_bs = for_bs;
+    }
+
+    if (granularity == 0) {
+        granularity = bdrv_get_default_bitmap_granularity(for_bs);
+    }
+
+    bitmap = bdrv_load_dirty_bitmap(for_bs, file_bs, granularity, name,
+                                    errp);
+    if (*errp != NULL) {
+        goto fail;
+    }
+
+    if (bitmap == NULL) {
+        /* bitmap is not found in file_bs */
+        bitmap = bdrv_create_dirty_bitmap(for_bs, granularity, name, errp);
+        if (!bitmap) {
+            goto fail;
+        }
+    }
+
+    bdrv_dirty_bitmap_set_file(bitmap, file_bs);
+
+    if (!enabled) {
+        bdrv_disable_dirty_bitmap(bitmap);
+    }
+
+    return 0;
+
+fail:
+    error_report("-dirty-bitmap: %s", error_get_pretty(local_err));
+    error_free(local_err);
+    if (file_bs != NULL) {
+        bdrv_close(file_bs);
+    }
+    return -1;
+}
+
 static int drive_init_func(QemuOpts *opts, void *opaque)
 {
     BlockInterfaceType *block_default_type = opaque;
@@ -2741,6 +2830,7 @@ int main(int argc, char **argv, char **envp)
     module_call_init(MODULE_INIT_QOM);
 
     qemu_add_opts(&qemu_drive_opts);
+    qemu_add_opts(&qemu_dirty_bitmap_opts);
     qemu_add_drive_opts(&qemu_legacy_drive_opts);
     qemu_add_drive_opts(&qemu_common_drive_opts);
     qemu_add_drive_opts(&qemu_drive_opts);
@@ -2881,6 +2971,11 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
 	        break;
+            case QEMU_OPTION_dirty_bitmap:
+                if (dirty_bitmap_def(optarg) == NULL) {
+                    exit(1);
+                }
+            break;
             case QEMU_OPTION_set:
                 if (qemu_set_option(optarg) != 0)
                     exit(1);
@@ -4205,6 +4300,11 @@ int main(int argc, char **argv, char **envp)
     default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
     default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
 
+    if (qemu_opts_foreach(qemu_find_opts("dirty-bitmap"), dirty_bitmap_func,
+                          NULL, 1) != 0) {
+        exit(1);
+    }
+
     if (qemu_opts_foreach(qemu_find_opts("numa"), numa_init_func,
                           NULL, 1) != 0) {
         exit(1);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
                   ` (5 preceding siblings ...)
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 6/8] qemu: command line option for dirty bitmaps Vladimir Sementsov-Ogievskiy
@ 2015-01-13 17:02 ` Vladimir Sementsov-Ogievskiy
  2015-01-27 15:53   ` Eric Blake
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 8/8] iotests: test internal persistent " Vladimir Sementsov-Ogievskiy
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-13 17:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, pbonzini, den, jsnow

Adds qmp and hmp commands to print dirty bitmap. This is needed only for
testing persistent dirty bitmap feature.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
---
 block.c               | 33 +++++++++++++++++++++++++++++++++
 blockdev.c            | 13 +++++++++++++
 hmp-commands.hx       | 15 +++++++++++++++
 hmp.c                 |  8 ++++++++
 hmp.h                 |  1 +
 include/block/block.h |  1 +
 qapi-schema.json      |  3 ++-
 qapi/block-core.json  |  3 +++
 qmp-commands.hx       |  5 +++++
 9 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 77419e9..3e6dedf 100644
--- a/block.c
+++ b/block.c
@@ -5445,6 +5445,39 @@ int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap)
     return res;
 }
 
+void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+    unsigned long a = 0, b = 0;
+
+    printf("bitmap '%s'\n", bitmap->name ? bitmap->name : "no name");
+    printf("enabled: %s\n", bitmap->enabled ? "true" : "false");
+    printf("size: %" PRId64 "\n", bitmap->size);
+    printf("granularity: %" PRId64 "\n", bitmap->granularity);
+    printf("dirty regions begin:\n");
+
+    while (true) {
+        for (a = b; a < bitmap->size && !hbitmap_get(bitmap->bitmap, a); ++a) {
+            ;
+        }
+        if (a >= bitmap->size) {
+            break;
+        }
+
+        for (b = a + 1;
+             b < bitmap->size && hbitmap_get(bitmap->bitmap, b);
+             ++b) {
+            ;
+        }
+
+        printf("%ld -> %ld\n", a, b - 1);
+        if (b >= bitmap->size) {
+            break;
+        }
+    }
+
+    printf("dirty regions end\n");
+}
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           int granularity,
                                           const char *name,
diff --git a/blockdev.c b/blockdev.c
index 8a9be08..8b58c2e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2079,6 +2079,19 @@ void qmp_block_dirty_bitmap_add(const char *node_ref, const char *name,
     aio_context_release(aio_context);
 }
 
+void qmp_block_dirty_bitmap_print(const char *node_ref, const char *name,
+                                  Error **errp)
+{
+    BdrvDirtyBitmap *bitmap;
+
+    bitmap = block_dirty_bitmap_lookup(node_ref, name, NULL, errp);
+    if (!bitmap) {
+        return;
+    }
+
+    bdrv_print_dirty_bitmap(bitmap);
+}
+
 void qmp_block_dirty_bitmap_remove(const char *node_ref, const char *name,
                                    Error **errp)
 {
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e37bc8b..a9be506 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -58,6 +58,21 @@ Quit the emulator.
 ETEXI
 
     {
+        .name       = "print_dirty_bitmap",
+        .args_type  = "device:B,bitmap:s",
+        .params     = "device bitmap",
+        .help       = "print dirty bitmap",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd = hmp_print_dirty_bitmap,
+    },
+
+STEXI
+@item print_dirty_bitmap device_id bitmap_name
+@findex print_dirty_bitmap
+Print dirty bitmap meta information and dirty regions.
+ETEXI
+
+    {
         .name       = "block_resize",
         .args_type  = "device:B,size:o",
         .params     = "device size",
diff --git a/hmp.c b/hmp.c
index 63b19c7..a269145 100644
--- a/hmp.c
+++ b/hmp.c
@@ -782,6 +782,14 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
     qapi_free_TPMInfoList(info_list);
 }
 
+void hmp_print_dirty_bitmap(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_str(qdict, "bitmap");
+
+    qmp_block_dirty_bitmap_print(device, name, NULL);
+}
+
 void hmp_quit(Monitor *mon, const QDict *qdict)
 {
     monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 4bb5dca..6bbbc33 100644
--- a/hmp.h
+++ b/hmp.h
@@ -19,6 +19,7 @@
 #include "qapi-types.h"
 #include "qapi/qmp/qdict.h"
 
+void hmp_print_dirty_bitmap(Monitor *mon, const QDict *qdict);
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
 void hmp_info_kvm(Monitor *mon, const QDict *qdict);
diff --git a/include/block/block.h b/include/block/block.h
index f36557f..7188791 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -466,6 +466,7 @@ BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState *bs,
                                         const char *name,
                                         Error **errp);
 int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap);
+void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
 void bdrv_disable_copy_on_read(BlockDriverState *bs);
diff --git a/qapi-schema.json b/qapi-schema.json
index 85f55d9..1475f69 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1263,7 +1263,8 @@
        'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
        'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
        'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
-       'block-dirty-bitmap-disable': 'BlockDirtyBitmap'
+       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-print': 'BlockDirtyBitmap'
    } }
 
 ##
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1892b50..3e1edb1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -982,6 +982,9 @@
 {'command': 'block-dirty-bitmap-disable',
   'data': 'BlockDirtyBitmap' }
 
+{'command': 'block-dirty-bitmap-print',
+  'data': 'BlockDirtyBitmap' }
+
 ##
 # @block_set_io_throttle:
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 479d4f5..8065715 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1222,6 +1222,11 @@ EQMP
         .args_type  = "node-ref:B,name:s",
         .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_disable,
     },
+    {
+        .name       = "block-dirty-bitmap-print",
+        .args_type  = "node-ref:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_print,
+    },
 
 SQMP
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Qemu-devel] [PATCH 8/8] iotests: test internal persistent dirty bitmap
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
                   ` (6 preceding siblings ...)
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap Vladimir Sementsov-Ogievskiy
@ 2015-01-13 17:02 ` Vladimir Sementsov-Ogievskiy
  2015-01-27 11:06 ` [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
  2015-02-04 15:07 ` Vladimir Sementsov-Ogievskiy
  9 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-13 17:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, pbonzini, den, jsnow

The test performs several vm reloads with checking and updating dirty
bitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
---
 tests/qemu-iotests/115     | 96 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/115.out | 64 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 161 insertions(+)
 create mode 100755 tests/qemu-iotests/115
 create mode 100644 tests/qemu-iotests/115.out

diff --git a/tests/qemu-iotests/115 b/tests/qemu-iotests/115
new file mode 100755
index 0000000..ef853b1
--- /dev/null
+++ b/tests/qemu-iotests/115
@@ -0,0 +1,96 @@
+#!/bin/bash
+#
+# Persistent dirty bitmap test
+#
+# Performs several vm reloads with checking and updating dirty bitmap.
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# 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=vsementsov@parallels.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1    # failure is the default!
+
+_cleanup()
+{
+    _cleanup_qemu
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_os Linux
+
+size=1G
+_make_test_img $size
+
+qemu_comm_method="monitor"
+
+launch() {
+    _launch_qemu -drive file="${TEST_IMG}",id=disk\
+        -dirty-bitmap name=bitmap,drive=disk
+    echo
+    echo START VM
+}
+
+print_bitmap() {
+    _send_qemu_cmd $QEMU_HANDLE 'print_dirty_bitmap disk bitmap'\
+        "dirty regions end" | sed 's/^/    /'
+}
+
+write() {
+    _send_qemu_cmd $QEMU_HANDLE 'qemu-io disk "write ' $@ '"' "(qemu)"
+    echo
+    echo write $@
+}
+
+quit() {
+    _send_qemu_cmd $QEMU_HANDLE 'qemu-io disk flush' "(qemu)"
+    _send_qemu_cmd $QEMU_HANDLE 'quit' ""
+    echo QUIT
+    wait ${QEMU_PID[$QEMU_HANDLE]} 2>/dev/null
+}
+
+{
+    launch
+    print_bitmap
+    write 50m 1m
+    print_bitmap
+    quit
+
+    launch
+    print_bitmap
+    write 0m 10m
+    write 700m 200m
+    print_bitmap
+    quit
+
+    launch
+    print_bitmap
+    quit
+} | sed '/(qemu)/d'
+
+status=0
diff --git a/tests/qemu-iotests/115.out b/tests/qemu-iotests/115.out
new file mode 100644
index 0000000..d570c9f
--- /dev/null
+++ b/tests/qemu-iotests/115.out
@@ -0,0 +1,64 @@
+QA output created by 115
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+
+START VM
+QEMU X.Y.Z monitor - type 'help' for more information
+    bitmap 'bitmap'
+    enabled: true
+    size: 2097152
+    granularity: 65536
+    dirty regions begin:
+    dirty regions end
+
+write 50m 1m
+wrote 1048576/1048576 bytes at offset 52428800
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+    bitmap 'bitmap'
+    enabled: true
+    size: 2097152
+    granularity: 65536
+    dirty regions begin:
+    102400 -> 104447
+    dirty regions end
+QUIT
+
+START VM
+QEMU X.Y.Z monitor - type 'help' for more information
+    bitmap 'bitmap'
+    enabled: true
+    size: 2097152
+    granularity: 65536
+    dirty regions begin:
+    102400 -> 104447
+    dirty regions end
+
+write 0m 10m
+wrote 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+write 700m 200m
+wrote 209715200/209715200 bytes at offset 734003200
+200 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+    bitmap 'bitmap'
+    enabled: true
+    size: 2097152
+    granularity: 65536
+    dirty regions begin:
+    0 -> 20479
+    102400 -> 104447
+    1433600 -> 1843199
+    dirty regions end
+QUIT
+
+START VM
+QEMU X.Y.Z monitor - type 'help' for more information
+    bitmap 'bitmap'
+    enabled: true
+    size: 2097152
+    granularity: 65536
+    dirty regions begin:
+    0 -> 20479
+    102400 -> 104447
+    1433600 -> 1843199
+    dirty regions end
+QUIT
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index a4742c6..77377b3 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -115,3 +115,4 @@
 111 rw auto quick
 113 rw auto quick
 114 rw auto quick
+115 rw auto quick
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC)
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
                   ` (7 preceding siblings ...)
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 8/8] iotests: test internal persistent " Vladimir Sementsov-Ogievskiy
@ 2015-01-27 11:06 ` Vladimir Sementsov-Ogievskiy
  2015-02-04 15:07 ` Vladimir Sementsov-Ogievskiy
  9 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-27 11:06 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, stefanha, pbonzini, den, jsnow

ping

Best regards,
Vladimir

On 13.01.2015 20:02, Vladimir Sementsov-Ogievskiy wrote:
> The bitmaps are saved into qcow2 file format. It provides both
> 'internal' and 'external' dirty bitmaps feature:
>   - for qcow2 drives we can store bitmaps in the same file
>   - for other formats we can store bitmaps in the separate qcow2 file
>
> QCow2 header is extended by fields 'nb_dirty_bitmaps' and
> 'dirty_bitmaps_offset' like with snapshots.
>
> Proposed command line syntax is the following:
>
> -dirty-bitmap [option1=val1][,option2=val2]...
>      Available options are:
>      name         The name for the bitmap (necessary).
>
>      file         The file to load the bitmap from.
>
>      file_id      When specified with 'file' option, then this file will
>                   be available through this id for other -dirty-bitmap
>                   options when specified without 'file' option, then it
>                   is a reference to 'file', specified with another
>                   -dirty-bitmap option, and it will be used to load the
>                   bitmap from.
>
>      drive        The drive to bind the bitmap to. It should be specified
>                   as 'id' suboption of one of -drive options. If nor
>                   'file' neither 'file_id' are specified, then the bitmap
>                   will be loaded from that drive (internal dirty bitmap).
>
>      granularity  The granularity for the bitmap. Not necessary, the
>                   default value may be used.
>
>      enabled      on|off. Default is 'on'. Disabled bitmaps are not
>                   changing regardless of writes to corresponding drive.
>
> Examples:
>
> qemu -drive file=a.qcow2,id=disk -dirty-bitmap name=b,drive=disk
> qemu -drive file=a.raw,id=disk \
>       -dirty-bitmap name=b,drive=disk,file=b.qcow2,enabled=off
>
> Vladimir Sementsov-Ogievskiy (8):
>    spec: add qcow2-dirty-bitmaps specification
>    hbitmap: store / restore
>    qcow2: add dirty-bitmaps feature
>    block: store persistent dirty bitmaps
>    block: add bdrv_load_dirty_bitmap
>    qemu: command line option for dirty bitmaps
>    qmp: print dirty bitmap
>    iotests: test internal persistent dirty bitmap
>
>   block.c                    | 113 ++++++++++
>   block/Makefile.objs        |   2 +-
>   block/qcow2-dirty-bitmap.c | 514 +++++++++++++++++++++++++++++++++++++++++++++
>   block/qcow2.c              |  26 +++
>   block/qcow2.h              |  48 +++++
>   blockdev.c                 |  51 +++++
>   docs/specs/qcow2.txt       |  59 ++++++
>   hmp-commands.hx            |  15 ++
>   hmp.c                      |   8 +
>   hmp.h                      |   1 +
>   include/block/block.h      |   9 +
>   include/block/block_int.h  |  10 +
>   include/qemu/hbitmap.h     |  49 +++++
>   include/sysemu/blockdev.h  |   1 +
>   include/sysemu/sysemu.h    |   1 +
>   qapi-schema.json           |   3 +-
>   qapi/block-core.json       |   3 +
>   qemu-options.hx            |  37 ++++
>   qmp-commands.hx            |   5 +
>   tests/qemu-iotests/115     |  96 +++++++++
>   tests/qemu-iotests/115.out |  64 ++++++
>   tests/qemu-iotests/group   |   1 +
>   util/hbitmap.c             |  87 ++++++++
>   vl.c                       | 100 +++++++++
>   24 files changed, 1301 insertions(+), 2 deletions(-)
>   create mode 100644 block/qcow2-dirty-bitmap.c
>   create mode 100755 tests/qemu-iotests/115
>   create mode 100644 tests/qemu-iotests/115.out
>

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
@ 2015-01-27 15:39   ` Eric Blake
  0 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2015-01-27 15:39 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, den, jsnow, stefanha, pbonzini

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

On 01/13/2015 10:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
> other drives (there may be qcow2 file with zero disk size but with
> several dirty bitmaps for other drives).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> ---
>  docs/specs/qcow2.txt | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..b29de40 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -116,6 +116,13 @@ in the description of a field.
>                      Length of the header structure in bytes. For version 2
>                      images, the length is always assumed to be 72 bytes.
>  
> +        104 - 107:  nb_dirty_bitmaps
> +                    Number of dirty bitmaps contained in the image
> +
> +        108 - 115:  dirty_bitmaps_offset
> +                    Offset into the image file at which the dirty bitmaps table
> +                    starts. Must be aligned to a cluster boundary.

NACK.  You cannot add new fields directly into the header without
bumping the version number; but that is overkill.  Instead, stick the
dirty bitmaps into a new header extension.  The existing fields of the
header are sufficient to record that a new extension is present.  Also,
you need to decide whether dirty bitmaps are an ignorable feature or a
mandatory feature, and set the appropriate feature bit in the header
flag fields (thus when an older qemu that doesn't know about dirty
bitmaps opens the file, it will either reject the open due to not
knowing how to keep dirty bitmaps sane, or will accept the open but
clear the ignorable bit so that the next time a new qemu opens the file
it will know that the dirty bitmap is no longer reliable).

> +=== Cluster mapping ===
> +
> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
> +bitmaps to host clusters. There only an L1 table.

s/There/There is/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap
  2015-01-13 17:02 ` [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap Vladimir Sementsov-Ogievskiy
@ 2015-01-27 15:53   ` Eric Blake
  2015-01-27 16:29     ` Markus Armbruster
  2015-01-30  9:06     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 2 replies; 41+ messages in thread
From: Eric Blake @ 2015-01-27 15:53 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, den, jsnow, stefanha, pbonzini

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

On 01/13/2015 10:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> Adds qmp and hmp commands to print dirty bitmap. This is needed only for
> testing persistent dirty bitmap feature.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> ---

> +++ b/block.c
> @@ -5445,6 +5445,39 @@ int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>      return res;
>  }
>  
> +void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> +{
> +    unsigned long a = 0, b = 0;
> +
> +    printf("bitmap '%s'\n", bitmap->name ? bitmap->name : "no name");
> +    printf("enabled: %s\n", bitmap->enabled ? "true" : "false");
> +    printf("size: %" PRId64 "\n", bitmap->size);
> +    printf("granularity: %" PRId64 "\n", bitmap->granularity);
> +    printf("dirty regions begin:\n");

> +++ b/blockdev.c
> @@ -2079,6 +2079,19 @@ void qmp_block_dirty_bitmap_add(const char *node_ref, const char *name,
>      aio_context_release(aio_context);
>  }
>  
> +void qmp_block_dirty_bitmap_print(const char *node_ref, const char *name,
> +                                  Error **errp)
> +{
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bitmap = block_dirty_bitmap_lookup(node_ref, name, NULL, errp);
> +    if (!bitmap) {
> +        return;
> +    }
> +
> +    bdrv_print_dirty_bitmap(bitmap);

Eww.  This assumes that stdout is usable.  But that is not the case for
QMP commands.  It would be better to package up the output in a
structured format and return it to the caller, and let the caller decide
how to print it.

> +++ b/qapi/block-core.json
> @@ -982,6 +982,9 @@
>  {'command': 'block-dirty-bitmap-disable',
>    'data': 'BlockDirtyBitmap' }
>  
> +{'command': 'block-dirty-bitmap-print',
> +  'data': 'BlockDirtyBitmap' }

Missing documentation, if we even want this command.  As mentioned
above, this should return ALL of the information necessary for the
client to then decide how to print the information, rather than directly
printing information to stdout itself.  And it might be better to name
this command in the 'query-' namespace.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap
  2015-01-27 15:53   ` Eric Blake
@ 2015-01-27 16:29     ` Markus Armbruster
  2015-01-30  9:06     ` Vladimir Sementsov-Ogievskiy
  1 sibling, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2015-01-27 16:29 UTC (permalink / raw)
  To: Eric Blake
  Cc: kwolf, qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha,
	pbonzini, den, jsnow

Eric Blake <eblake@redhat.com> writes:

> On 01/13/2015 10:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Adds qmp and hmp commands to print dirty bitmap. This is needed only for
>> testing persistent dirty bitmap feature.
>> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>> ---
>
>> +++ b/block.c
>> @@ -5445,6 +5445,39 @@ int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>      return res;
>>  }
>>  
>> +void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>> +{
>> +    unsigned long a = 0, b = 0;
>> +
>> +    printf("bitmap '%s'\n", bitmap->name ? bitmap->name : "no name");
>> +    printf("enabled: %s\n", bitmap->enabled ? "true" : "false");
>> +    printf("size: %" PRId64 "\n", bitmap->size);
>> +    printf("granularity: %" PRId64 "\n", bitmap->granularity);
>> +    printf("dirty regions begin:\n");
>
>> +++ b/blockdev.c
>> @@ -2079,6 +2079,19 @@ void qmp_block_dirty_bitmap_add(const char *node_ref, const char *name,
>>      aio_context_release(aio_context);
>>  }
>>  
>> +void qmp_block_dirty_bitmap_print(const char *node_ref, const char *name,
>> +                                  Error **errp)
>> +{
>> +    BdrvDirtyBitmap *bitmap;
>> +
>> +    bitmap = block_dirty_bitmap_lookup(node_ref, name, NULL, errp);
>> +    if (!bitmap) {
>> +        return;
>> +    }
>> +
>> +    bdrv_print_dirty_bitmap(bitmap);
>
> Eww.  This assumes that stdout is usable.  But that is not the case for
> QMP commands.  It would be better to package up the output in a
> structured format and return it to the caller, and let the caller decide
> how to print it.

This isn't a case of "would be better", actually.

HMP commands may print to the *monitor*.  Printing to stdout is wrong.

QMP commands return their result as a JSON object.  Printing results
anywhere is wrong.

We commonly implement the HMP command handler on top of the QMP command
handler.  The QMP command handler returns an object.  The QMP core sends
this object in JSON format to the QMP client.  The HMP command prints it
to the monitor in a suitable human-readable format.

>> +++ b/qapi/block-core.json
>> @@ -982,6 +982,9 @@
>>  {'command': 'block-dirty-bitmap-disable',
>>    'data': 'BlockDirtyBitmap' }
>>  
>> +{'command': 'block-dirty-bitmap-print',
>> +  'data': 'BlockDirtyBitmap' }
>
> Missing documentation, if we even want this command.  As mentioned
> above, this should return ALL of the information necessary for the
> client to then decide how to print the information, rather than directly
> printing information to stdout itself.  And it might be better to name
> this command in the 'query-' namespace.

Yup.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap
  2015-01-27 15:53   ` Eric Blake
  2015-01-27 16:29     ` Markus Armbruster
@ 2015-01-30  9:06     ` Vladimir Sementsov-Ogievskiy
  2015-01-30 17:51       ` Eric Blake
  1 sibling, 1 reply; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-01-30  9:06 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, den, jsnow, stefanha, pbonzini

is it better to add qmp_query_dirty_bitmap with underlying 
bdrv_query_dirty_bitmap, or to modify (add dirty regions information) 
existing qmp_query_block/qmp_query_dirty_bitmapS?

Best regards,
Vladimir

On 27.01.2015 18:53, Eric Blake wrote:
> On 01/13/2015 10:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Adds qmp and hmp commands to print dirty bitmap. This is needed only for
>> testing persistent dirty bitmap feature.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>> ---
>> +++ b/block.c
>> @@ -5445,6 +5445,39 @@ int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>       return res;
>>   }
>>   
>> +void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>> +{
>> +    unsigned long a = 0, b = 0;
>> +
>> +    printf("bitmap '%s'\n", bitmap->name ? bitmap->name : "no name");
>> +    printf("enabled: %s\n", bitmap->enabled ? "true" : "false");
>> +    printf("size: %" PRId64 "\n", bitmap->size);
>> +    printf("granularity: %" PRId64 "\n", bitmap->granularity);
>> +    printf("dirty regions begin:\n");
>> +++ b/blockdev.c
>> @@ -2079,6 +2079,19 @@ void qmp_block_dirty_bitmap_add(const char *node_ref, const char *name,
>>       aio_context_release(aio_context);
>>   }
>>   
>> +void qmp_block_dirty_bitmap_print(const char *node_ref, const char *name,
>> +                                  Error **errp)
>> +{
>> +    BdrvDirtyBitmap *bitmap;
>> +
>> +    bitmap = block_dirty_bitmap_lookup(node_ref, name, NULL, errp);
>> +    if (!bitmap) {
>> +        return;
>> +    }
>> +
>> +    bdrv_print_dirty_bitmap(bitmap);
> Eww.  This assumes that stdout is usable.  But that is not the case for
> QMP commands.  It would be better to package up the output in a
> structured format and return it to the caller, and let the caller decide
> how to print it.
>
>> +++ b/qapi/block-core.json
>> @@ -982,6 +982,9 @@
>>   {'command': 'block-dirty-bitmap-disable',
>>     'data': 'BlockDirtyBitmap' }
>>   
>> +{'command': 'block-dirty-bitmap-print',
>> +  'data': 'BlockDirtyBitmap' }
> Missing documentation, if we even want this command.  As mentioned
> above, this should return ALL of the information necessary for the
> client to then decide how to print the information, rather than directly
> printing information to stdout itself.  And it might be better to name
> this command in the 'query-' namespace.
>

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap
  2015-01-30  9:06     ` Vladimir Sementsov-Ogievskiy
@ 2015-01-30 17:51       ` Eric Blake
  0 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2015-01-30 17:51 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, den, jsnow, stefanha, pbonzini

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

On 01/30/2015 02:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> is it better to add qmp_query_dirty_bitmap with underlying
> bdrv_query_dirty_bitmap, or to modify (add dirty regions information)
> existing qmp_query_block/qmp_query_dirty_bitmapS?

[please don't top-post on technical lists]

Extending an existing command may be reasonable, if the amount of
information being added is compact (if you are adding several kilobytes
of information, that might justify a new command, if only so that old
callers don't waste time receiving large amounts of data on the wire
just to sift through and discard it all to get at the older content they
cared about).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC)
  2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
                   ` (8 preceding siblings ...)
  2015-01-27 11:06 ` [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
@ 2015-02-04 15:07 ` Vladimir Sementsov-Ogievskiy
  2015-02-04 15:20   ` John Snow
  9 siblings, 1 reply; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-02-04 15:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, stefanha, pbonzini, den, jsnow

On 13.01.2015 20:02, Vladimir Sementsov-Ogievskiy wrote:
> The bitmaps are saved into qcow2 file format. It provides both
> 'internal' and 'external' dirty bitmaps feature:
>   - for qcow2 drives we can store bitmaps in the same file
>   - for other formats we can store bitmaps in the separate qcow2 file
>
> QCow2 header is extended by fields 'nb_dirty_bitmaps' and
> 'dirty_bitmaps_offset' like with snapshots.
>
> Proposed command line syntax is the following:
>
> -dirty-bitmap [option1=val1][,option2=val2]...
>      Available options are:
>      name         The name for the bitmap (necessary).
>
>      file         The file to load the bitmap from.
>
>      file_id      When specified with 'file' option, then this file will
>                   be available through this id for other -dirty-bitmap
>                   options when specified without 'file' option, then it
>                   is a reference to 'file', specified with another
>                   -dirty-bitmap option, and it will be used to load the
>                   bitmap from.
>
>      drive        The drive to bind the bitmap to. It should be specified
>                   as 'id' suboption of one of -drive options. If nor
>                   'file' neither 'file_id' are specified, then the bitmap
>                   will be loaded from that drive (internal dirty bitmap).
>
>      granularity  The granularity for the bitmap. Not necessary, the
>                   default value may be used.
>
>      enabled      on|off. Default is 'on'. Disabled bitmaps are not
>                   changing regardless of writes to corresponding drive.
>
> Examples:
>
> qemu -drive file=a.qcow2,id=disk -dirty-bitmap name=b,drive=disk
> qemu -drive file=a.raw,id=disk \
>       -dirty-bitmap name=b,drive=disk,file=b.qcow2,enabled=off
>
> Vladimir Sementsov-Ogievskiy (8):
>    spec: add qcow2-dirty-bitmaps specification
>    hbitmap: store / restore
>    qcow2: add dirty-bitmaps feature
>    block: store persistent dirty bitmaps
>    block: add bdrv_load_dirty_bitmap
>    qemu: command line option for dirty bitmaps
>    qmp: print dirty bitmap
>    iotests: test internal persistent dirty bitmap
>
>   block.c                    | 113 ++++++++++
>   block/Makefile.objs        |   2 +-
>   block/qcow2-dirty-bitmap.c | 514 +++++++++++++++++++++++++++++++++++++++++++++
>   block/qcow2.c              |  26 +++
>   block/qcow2.h              |  48 +++++
>   blockdev.c                 |  51 +++++
>   docs/specs/qcow2.txt       |  59 ++++++
>   hmp-commands.hx            |  15 ++
>   hmp.c                      |   8 +
>   hmp.h                      |   1 +
>   include/block/block.h      |   9 +
>   include/block/block_int.h  |  10 +
>   include/qemu/hbitmap.h     |  49 +++++
>   include/sysemu/blockdev.h  |   1 +
>   include/sysemu/sysemu.h    |   1 +
>   qapi-schema.json           |   3 +-
>   qapi/block-core.json       |   3 +
>   qemu-options.hx            |  37 ++++
>   qmp-commands.hx            |   5 +
>   tests/qemu-iotests/115     |  96 +++++++++
>   tests/qemu-iotests/115.out |  64 ++++++
>   tests/qemu-iotests/group   |   1 +
>   util/hbitmap.c             |  87 ++++++++
>   vl.c                       | 100 +++++++++
>   24 files changed, 1301 insertions(+), 2 deletions(-)
>   create mode 100644 block/qcow2-dirty-bitmap.c
>   create mode 100755 tests/qemu-iotests/115
>   create mode 100644 tests/qemu-iotests/115.out
>

Ping. I've already done (locally):
1) using qcow2 header extension instead of changing the header
2) normal qmp query request instead of "print dirty bitmap"
  - thanks to Eric and Markus

Now I'm waiting for some comments on the concept and it's realization to 
roll v3.

-- 
Best regards,
Vladimir

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC)
  2015-02-04 15:07 ` Vladimir Sementsov-Ogievskiy
@ 2015-02-04 15:20   ` John Snow
  2015-02-04 15:40     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-02-04 15:20 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel; +Cc: kwolf, den, stefanha, pbonzini



On 02/04/2015 10:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> On 13.01.2015 20:02, Vladimir Sementsov-Ogievskiy wrote:
>> The bitmaps are saved into qcow2 file format. It provides both
>> 'internal' and 'external' dirty bitmaps feature:
>>   - for qcow2 drives we can store bitmaps in the same file
>>   - for other formats we can store bitmaps in the separate qcow2 file
>>
>> QCow2 header is extended by fields 'nb_dirty_bitmaps' and
>> 'dirty_bitmaps_offset' like with snapshots.
>>
>> Proposed command line syntax is the following:
>>
>> -dirty-bitmap [option1=val1][,option2=val2]...
>>      Available options are:
>>      name         The name for the bitmap (necessary).
>>
>>      file         The file to load the bitmap from.
>>
>>      file_id      When specified with 'file' option, then this file will
>>                   be available through this id for other -dirty-bitmap
>>                   options when specified without 'file' option, then it
>>                   is a reference to 'file', specified with another
>>                   -dirty-bitmap option, and it will be used to load the
>>                   bitmap from.
>>
>>      drive        The drive to bind the bitmap to. It should be specified
>>                   as 'id' suboption of one of -drive options. If nor
>>                   'file' neither 'file_id' are specified, then the bitmap
>>                   will be loaded from that drive (internal dirty bitmap).
>>
>>      granularity  The granularity for the bitmap. Not necessary, the
>>                   default value may be used.
>>
>>      enabled      on|off. Default is 'on'. Disabled bitmaps are not
>>                   changing regardless of writes to corresponding drive.
>>
>> Examples:
>>
>> qemu -drive file=a.qcow2,id=disk -dirty-bitmap name=b,drive=disk
>> qemu -drive file=a.raw,id=disk \
>>       -dirty-bitmap name=b,drive=disk,file=b.qcow2,enabled=off
>>
>> Vladimir Sementsov-Ogievskiy (8):
>>    spec: add qcow2-dirty-bitmaps specification
>>    hbitmap: store / restore
>>    qcow2: add dirty-bitmaps feature
>>    block: store persistent dirty bitmaps
>>    block: add bdrv_load_dirty_bitmap
>>    qemu: command line option for dirty bitmaps
>>    qmp: print dirty bitmap
>>    iotests: test internal persistent dirty bitmap
>>
>>   block.c                    | 113 ++++++++++
>>   block/Makefile.objs        |   2 +-
>>   block/qcow2-dirty-bitmap.c | 514
>> +++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c              |  26 +++
>>   block/qcow2.h              |  48 +++++
>>   blockdev.c                 |  51 +++++
>>   docs/specs/qcow2.txt       |  59 ++++++
>>   hmp-commands.hx            |  15 ++
>>   hmp.c                      |   8 +
>>   hmp.h                      |   1 +
>>   include/block/block.h      |   9 +
>>   include/block/block_int.h  |  10 +
>>   include/qemu/hbitmap.h     |  49 +++++
>>   include/sysemu/blockdev.h  |   1 +
>>   include/sysemu/sysemu.h    |   1 +
>>   qapi-schema.json           |   3 +-
>>   qapi/block-core.json       |   3 +
>>   qemu-options.hx            |  37 ++++
>>   qmp-commands.hx            |   5 +
>>   tests/qemu-iotests/115     |  96 +++++++++
>>   tests/qemu-iotests/115.out |  64 ++++++
>>   tests/qemu-iotests/group   |   1 +
>>   util/hbitmap.c             |  87 ++++++++
>>   vl.c                       | 100 +++++++++
>>   24 files changed, 1301 insertions(+), 2 deletions(-)
>>   create mode 100644 block/qcow2-dirty-bitmap.c
>>   create mode 100755 tests/qemu-iotests/115
>>   create mode 100644 tests/qemu-iotests/115.out
>>
>
> Ping. I've already done (locally):
> 1) using qcow2 header extension instead of changing the header
> 2) normal qmp query request instead of "print dirty bitmap"
>   - thanks to Eric and Markus
>
> Now I'm waiting for some comments on the concept and it's realization to
> roll v3.
>

My apologies, I've been sick for a while. I'll get going on finishing 
review of these two series.

I assume you'd like to merge the bitmaps migration first?

--js

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC)
  2015-02-04 15:20   ` John Snow
@ 2015-02-04 15:40     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-02-04 15:40 UTC (permalink / raw)
  To: John Snow, qemu-devel; +Cc: kwolf, den, stefanha, pbonzini

On 04.02.2015 18:20, John Snow wrote:
>
>
> On 02/04/2015 10:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> On 13.01.2015 20:02, Vladimir Sementsov-Ogievskiy wrote:
>>> The bitmaps are saved into qcow2 file format. It provides both
>>> 'internal' and 'external' dirty bitmaps feature:
>>>   - for qcow2 drives we can store bitmaps in the same file
>>>   - for other formats we can store bitmaps in the separate qcow2 file
>>>
>>> QCow2 header is extended by fields 'nb_dirty_bitmaps' and
>>> 'dirty_bitmaps_offset' like with snapshots.
>>>
>>> Proposed command line syntax is the following:
>>>
>>> -dirty-bitmap [option1=val1][,option2=val2]...
>>>      Available options are:
>>>      name         The name for the bitmap (necessary).
>>>
>>>      file         The file to load the bitmap from.
>>>
>>>      file_id      When specified with 'file' option, then this file 
>>> will
>>>                   be available through this id for other -dirty-bitmap
>>>                   options when specified without 'file' option, then it
>>>                   is a reference to 'file', specified with another
>>>                   -dirty-bitmap option, and it will be used to load the
>>>                   bitmap from.
>>>
>>>      drive        The drive to bind the bitmap to. It should be 
>>> specified
>>>                   as 'id' suboption of one of -drive options. If nor
>>>                   'file' neither 'file_id' are specified, then the 
>>> bitmap
>>>                   will be loaded from that drive (internal dirty 
>>> bitmap).
>>>
>>>      granularity  The granularity for the bitmap. Not necessary, the
>>>                   default value may be used.
>>>
>>>      enabled      on|off. Default is 'on'. Disabled bitmaps are not
>>>                   changing regardless of writes to corresponding drive.
>>>
>>> Examples:
>>>
>>> qemu -drive file=a.qcow2,id=disk -dirty-bitmap name=b,drive=disk
>>> qemu -drive file=a.raw,id=disk \
>>>       -dirty-bitmap name=b,drive=disk,file=b.qcow2,enabled=off
>>>
>>> Vladimir Sementsov-Ogievskiy (8):
>>>    spec: add qcow2-dirty-bitmaps specification
>>>    hbitmap: store / restore
>>>    qcow2: add dirty-bitmaps feature
>>>    block: store persistent dirty bitmaps
>>>    block: add bdrv_load_dirty_bitmap
>>>    qemu: command line option for dirty bitmaps
>>>    qmp: print dirty bitmap
>>>    iotests: test internal persistent dirty bitmap
>>>
>>>   block.c                    | 113 ++++++++++
>>>   block/Makefile.objs        |   2 +-
>>>   block/qcow2-dirty-bitmap.c | 514
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>   block/qcow2.c              |  26 +++
>>>   block/qcow2.h              |  48 +++++
>>>   blockdev.c                 |  51 +++++
>>>   docs/specs/qcow2.txt       |  59 ++++++
>>>   hmp-commands.hx            |  15 ++
>>>   hmp.c                      |   8 +
>>>   hmp.h                      |   1 +
>>>   include/block/block.h      |   9 +
>>>   include/block/block_int.h  |  10 +
>>>   include/qemu/hbitmap.h     |  49 +++++
>>>   include/sysemu/blockdev.h  |   1 +
>>>   include/sysemu/sysemu.h    |   1 +
>>>   qapi-schema.json           |   3 +-
>>>   qapi/block-core.json       |   3 +
>>>   qemu-options.hx            |  37 ++++
>>>   qmp-commands.hx            |   5 +
>>>   tests/qemu-iotests/115     |  96 +++++++++
>>>   tests/qemu-iotests/115.out |  64 ++++++
>>>   tests/qemu-iotests/group   |   1 +
>>>   util/hbitmap.c             |  87 ++++++++
>>>   vl.c                       | 100 +++++++++
>>>   24 files changed, 1301 insertions(+), 2 deletions(-)
>>>   create mode 100644 block/qcow2-dirty-bitmap.c
>>>   create mode 100755 tests/qemu-iotests/115
>>>   create mode 100644 tests/qemu-iotests/115.out
>>>
>>
>> Ping. I've already done (locally):
>> 1) using qcow2 header extension instead of changing the header
>> 2) normal qmp query request instead of "print dirty bitmap"
>>   - thanks to Eric and Markus
>>
>> Now I'm waiting for some comments on the concept and it's realization to
>> roll v3.
>>
>
> My apologies, I've been sick for a while. I'll get going on finishing 
> review of these two series.
>
> I assume you'd like to merge the bitmaps migration first?
>
> --js

The sequence is not important. These series are necessary for making a 
complete backup solution - don't lose named dirty bitmaps on shutdown 
and migration. Shutdown is more often task, and dirty bitmaps migration 
was appeared as a subtask when we were discussed the format for saving 
dirty bitmaps. But they both are needed, then, ok, let's start with 
migration.

-- 
Best regards,
Vladimir

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-08 15:21 [Qemu-devel] [PATCH v2 RFC 0/8] block: persistent dirty bitmaps Vladimir Sementsov-Ogievskiy
@ 2015-06-08 15:21 ` Vladimir Sementsov-Ogievskiy
  2015-06-09 16:01   ` John Snow
                     ` (5 more replies)
  0 siblings, 6 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-06-08 15:21 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, vsementsov, Vladimir Sementsov-Ogievskiy, stefanha,
	pbonzini, den, jsnow

From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>

Persistent dirty bitmaps will be saved into qcow2 files. It may be used
as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
other drives (there may be qcow2 file with zero disk size but with
several dirty bitmaps for other drives).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 121dfc8..0fffba2 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -123,6 +123,7 @@ be stored. Each extension has a structure like the following:
                         0x00000000 - End of the header extension area
                         0xE2792ACA - Backing file format name
                         0x6803f857 - Feature name table
+                        0x23852875 - Dirty bitmaps
                         other      - Unknown header extension, can be safely
                                      ignored
 
@@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
                     terminated if it has full length)
 
 
+== Dirty bitmaps ==
+
+Dirty bitmaps is an optional header extension. It provides a possibility of
+storing dirty bitmaps in qcow2 image. The fields are:
+
+          0 -  3:  nb_dirty_bitmaps
+                   Number of dirty bitmaps contained in the image
+
+          4 - 11:  dirty_bitmaps_offset
+                   Offset into the image file at which the dirty bitmaps table
+                   starts. Must be aligned to a cluster boundary.
+
+
 == Host cluster management ==
 
 qcow2 manages the allocation of host clusters by maintaining a reference count
@@ -360,3 +374,55 @@ Snapshot table entry:
 
         variable:   Padding to round up the snapshot table entry size to the
                     next multiple of 8.
+
+
+== Dirty bitmaps ==
+
+The feature supports storing several dirty bitmaps in the qcow2 file.
+
+=== Cluster mapping ===
+
+Dirty bitmaps are stored using a ONE-level structure for the mapping of
+bitmaps to host clusters. There is only an L1 table.
+
+The L1 table has a variable size (stored in the Bitmap table entry) and may
+use multiple clusters, however it must be contiguous in the image file.
+
+Given an offset into the bitmap, the offset into the image file can be
+obtained as follows:
+
+    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
+
+L1 table entry:
+
+    Bit  0 -  61:   Standard cluster descriptor
+
+        62 -  63:   Reserved
+
+=== Bitmap table ===
+
+A directory of all bitmaps is stored in the bitmap table, a contiguous area in
+the image file, whose starting offset and length are given by the header fields
+dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
+variable length, depending on the length of name and extra data.
+
+Bitmap table entry:
+
+    Byte 0 -  7:    Offset into the image file at which the L1 table for the
+                    bitmap starts. Must be aligned to a cluster boundary.
+
+         8 - 11:    Number of entries in the L1 table of the bitmap
+
+        12 - 15:    Bitmap granularity in bytes
+
+        16 - 23:    Bitmap size in sectors
+
+        24 - 25:    Size of the bitmap name
+
+        variable:   The name of the bitmap (not null terminated)
+
+        variable:   Padding to round up the bitmap table entry size to the
+                    next multiple of 8.
+
+The fields "size", "granularity" and "name" are corresponding with the fields
+in struct BdrvDirtyBitmap.
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-08 15:21 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
@ 2015-06-09 16:01   ` John Snow
  2015-06-09 17:03   ` Stefan Hajnoczi
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-06-09 16:01 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, pbonzini, Vladimir Sementsov-Ogievskiy, stefanha, den



On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> 
> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
> other drives (there may be qcow2 file with zero disk size but with
> several dirty bitmaps for other drives).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..0fffba2 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like the following:
>                          0x00000000 - End of the header extension area
>                          0xE2792ACA - Backing file format name
>                          0x6803f857 - Feature name table
> +                        0x23852875 - Dirty bitmaps
>                          other      - Unknown header extension, can be safely
>                                       ignored
>  
> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>                      terminated if it has full length)
>  
>  
> +== Dirty bitmaps ==
> +
> +Dirty bitmaps is an optional header extension. It provides a possibility of
> +storing dirty bitmaps in qcow2 image. The fields are:
> +

I would say "It provides the ability to store dirty bitmaps in a qcow2
image." We have more than the possibility now :)

> +          0 -  3:  nb_dirty_bitmaps
> +                   Number of dirty bitmaps contained in the image
> +

Most fields seem to be documented as complete sentences, so maybe "The
number of dirty bitmaps contained in the image." and add the period.

> +          4 - 11:  dirty_bitmaps_offset
> +                   Offset into the image file at which the dirty bitmaps table
> +                   starts. Must be aligned to a cluster boundary.
> +
> +
>  == Host cluster management ==
>  
>  qcow2 manages the allocation of host clusters by maintaining a reference count
> @@ -360,3 +374,55 @@ Snapshot table entry:
>  
>          variable:   Padding to round up the snapshot table entry size to the
>                      next multiple of 8.
> +
> +
> +== Dirty bitmaps ==
> +
> +The feature supports storing several dirty bitmaps in the qcow2 file.

You could drop "several" from this sentence, we support an arbitrary number.

> +
> +=== Cluster mapping ===
> +
> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
> +bitmaps to host clusters. There is only an L1 table.
> +
> +The L1 table has a variable size (stored in the Bitmap table entry) and may
> +use multiple clusters, however it must be contiguous in the image file.
> +
> +Given an offset into the bitmap, the offset into the image file can be
> +obtained as follows:
> +
> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
> +
> +L1 table entry:
> +
> +    Bit  0 -  61:   Standard cluster descriptor
> +
> +        62 -  63:   Reserved
> +
> +=== Bitmap table ===
> +
> +A directory of all bitmaps is stored in the bitmap table, a contiguous area in
> +the image file, whose starting offset and length are given by the header fields
> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
> +variable length, depending on the length of name and extra data.
> +
> +Bitmap table entry:
> +
> +    Byte 0 -  7:    Offset into the image file at which the L1 table for the
> +                    bitmap starts. Must be aligned to a cluster boundary.
> +
> +         8 - 11:    Number of entries in the L1 table of the bitmap
> +
> +        12 - 15:    Bitmap granularity in bytes
> +
> +        16 - 23:    Bitmap size in sectors
> +
> +        24 - 25:    Size of the bitmap name
> +

This is, believe it or not, the first place in code that I am aware of
that actually places a limit on how big a bitmap name can be (64K!) --
we should probably clamp this value to something even lower (1024 is
probably graciously sufficient) and enforce that in the various bitmap
add/create routines.

> +        variable:   The name of the bitmap (not null terminated)
> +
> +        variable:   Padding to round up the bitmap table entry size to the
> +                    next multiple of 8.
> +
> +The fields "size", "granularity" and "name" are corresponding with the fields
> +in struct BdrvDirtyBitmap.
> 

Not yet being intricately familiar with qcow2, this looks good to me.

--js

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-08 15:21 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
  2015-06-09 16:01   ` John Snow
@ 2015-06-09 17:03   ` Stefan Hajnoczi
  2015-06-10  8:19     ` Vladimir Sementsov-Ogievskiy
  2015-06-10 15:34   ` Kevin Wolf
                     ` (3 subsequent siblings)
  5 siblings, 1 reply; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-06-09 17:03 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: kwolf, qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha, den,
	pbonzini, jsnow

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

On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> 
> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
> other drives (there may be qcow2 file with zero disk size but with
> several dirty bitmaps for other drives).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..0fffba2 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like the following:
>                          0x00000000 - End of the header extension area
>                          0xE2792ACA - Backing file format name
>                          0x6803f857 - Feature name table
> +                        0x23852875 - Dirty bitmaps
>                          other      - Unknown header extension, can be safely
>                                       ignored
>  
> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>                      terminated if it has full length)
>  
>  
> +== Dirty bitmaps ==
> +
> +Dirty bitmaps is an optional header extension. It provides a possibility of
> +storing dirty bitmaps in qcow2 image. The fields are:
> +
> +          0 -  3:  nb_dirty_bitmaps
> +                   Number of dirty bitmaps contained in the image

Is there a maximum?

> +
> +          4 - 11:  dirty_bitmaps_offset
> +                   Offset into the image file at which the dirty bitmaps table
> +                   starts. Must be aligned to a cluster boundary.

The autoclear feature bit is undocumented.

>  == Host cluster management ==
>  
>  qcow2 manages the allocation of host clusters by maintaining a reference count
> @@ -360,3 +374,55 @@ Snapshot table entry:
>  
>          variable:   Padding to round up the snapshot table entry size to the
>                      next multiple of 8.
> +
> +
> +== Dirty bitmaps ==
> +
> +The feature supports storing several dirty bitmaps in the qcow2 file.
> +
> +=== Cluster mapping ===
> +
> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
> +bitmaps to host clusters. There is only an L1 table.
> +
> +The L1 table has a variable size (stored in the Bitmap table entry) and may
> +use multiple clusters, however it must be contiguous in the image file.

The use of "L1 table" could be confusing.  The refcount metadata uses
"refcount table" and "refcount block" to describe a one-level table.

> +
> +Given an offset into the bitmap, the offset into the image file can be
> +obtained as follows:
> +
> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)

It might help to add granularity to this formula.

Instead of "offset", "bit_number" or "bitnr" might be clearer since
"offset" means something different in other parts of the document.

> +
> +L1 table entry:
> +
> +    Bit  0 -  61:   Standard cluster descriptor
> +
> +        62 -  63:   Reserved

Do you really want to use the standard cluster descriptor with it's zero
bit?

Since bitmaps don't honor backing files there doesn't seem much point in
using the zero bit, things are simpler if just bits 9-55 are contain the
host cluster offset and 0 means the cluster is unallocated.

By honoring the zero bit there are three states:
1. Zero bit set, read zeroes
2. Zero bit not set, host cluster offset != 0, bits valid
3. Zero bit not set, host cluster offset == 0, unallocated

State 1 is not useful.

> +=== Bitmap table ===
> +
> +A directory of all bitmaps is stored in the bitmap table, a contiguous area in
> +the image file, whose starting offset and length are given by the header fields
> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
> +variable length, depending on the length of name and extra data.
> +
> +Bitmap table entry:
> +
> +    Byte 0 -  7:    Offset into the image file at which the L1 table for the
> +                    bitmap starts. Must be aligned to a cluster boundary.
> +
> +         8 - 11:    Number of entries in the L1 table of the bitmap
> +
> +        12 - 15:    Bitmap granularity in bytes
> +
> +        16 - 23:    Bitmap size in sectors
> +
> +        24 - 25:    Size of the bitmap name
> +
> +        variable:   The name of the bitmap (not null terminated)
> +
> +        variable:   Padding to round up the bitmap table entry size to the
> +                    next multiple of 8.
> +
> +The fields "size", "granularity" and "name" are corresponding with the fields
> +in struct BdrvDirtyBitmap.

Referring to the internals of a C struct in QEMU is not appropriate for
a file format specification.  Please document the fields fully including
their constraints, minimums, maximums, etc.

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-09 17:03   ` Stefan Hajnoczi
@ 2015-06-10  8:19     ` Vladimir Sementsov-Ogievskiy
  2015-06-10  8:49       ` Vladimir Sementsov-Ogievskiy
                         ` (2 more replies)
  0 siblings, 3 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-06-10  8:19 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: kwolf, qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha, den,
	pbonzini, jsnow

On 09.06.2015 20:03, Stefan Hajnoczi wrote:
> On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>
>> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
>> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
>> other drives (there may be qcow2 file with zero disk size but with
>> several dirty bitmaps for other drives).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>
>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>> index 121dfc8..0fffba2 100644
>> --- a/docs/specs/qcow2.txt
>> +++ b/docs/specs/qcow2.txt
>> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like the following:
>>                           0x00000000 - End of the header extension area
>>                           0xE2792ACA - Backing file format name
>>                           0x6803f857 - Feature name table
>> +                        0x23852875 - Dirty bitmaps
>>                           other      - Unknown header extension, can be safely
>>                                        ignored
>>   
>> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>>                       terminated if it has full length)
>>   
>>   
>> +== Dirty bitmaps ==
>> +
>> +Dirty bitmaps is an optional header extension. It provides a possibility of
>> +storing dirty bitmaps in qcow2 image. The fields are:
>> +
>> +          0 -  3:  nb_dirty_bitmaps
>> +                   Number of dirty bitmaps contained in the image
> Is there a maximum?
hmm. any proposals for this?
>
>> +
>> +          4 - 11:  dirty_bitmaps_offset
>> +                   Offset into the image file at which the dirty bitmaps table
>> +                   starts. Must be aligned to a cluster boundary.
> The autoclear feature bit is undocumented.
>
>>   == Host cluster management ==
>>   
>>   qcow2 manages the allocation of host clusters by maintaining a reference count
>> @@ -360,3 +374,55 @@ Snapshot table entry:
>>   
>>           variable:   Padding to round up the snapshot table entry size to the
>>                       next multiple of 8.
>> +
>> +
>> +== Dirty bitmaps ==
>> +
>> +The feature supports storing several dirty bitmaps in the qcow2 file.
>> +
>> +=== Cluster mapping ===
>> +
>> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
>> +bitmaps to host clusters. There is only an L1 table.
>> +
>> +The L1 table has a variable size (stored in the Bitmap table entry) and may
>> +use multiple clusters, however it must be contiguous in the image file.
> The use of "L1 table" could be confusing.  The refcount metadata uses
> "refcount table" and "refcount block" to describe a one-level table.
I agree. Hmm.. dirty bitmaps table? ok?
>
>> +
>> +Given an offset into the bitmap, the offset into the image file can be
>> +obtained as follows:
>> +
>> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
> It might help to add granularity to this formula.
>
> Instead of "offset", "bit_number" or "bitnr" might be clearer since
> "offset" means something different in other parts of the document.
Hmm. In my opinion, the bitmap here is stored as raw data. And 
granularity is an additional parameter (for deserializing this data). 
So, it is an offset in bytes for this data. The format is not for 
accessing bitmap bits, it's only for loading the whole bitmap one time.
>
>> +
>> +L1 table entry:
>> +
>> +    Bit  0 -  61:   Standard cluster descriptor
>> +
>> +        62 -  63:   Reserved
> Do you really want to use the standard cluster descriptor with it's zero
> bit?
>
> Since bitmaps don't honor backing files there doesn't seem much point in
> using the zero bit, things are simpler if just bits 9-55 are contain the
> host cluster offset and 0 means the cluster is unallocated.
>
> By honoring the zero bit there are three states:
> 1. Zero bit set, read zeroes
> 2. Zero bit not set, host cluster offset != 0, bits valid
> 3. Zero bit not set, host cluster offset == 0, unallocated
>
> State 1 is not useful.
>
>> +=== Bitmap table ===
>> +
>> +A directory of all bitmaps is stored in the bitmap table, a contiguous area in
>> +the image file, whose starting offset and length are given by the header fields
>> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
>> +variable length, depending on the length of name and extra data.
>> +
>> +Bitmap table entry:
>> +
>> +    Byte 0 -  7:    Offset into the image file at which the L1 table for the
>> +                    bitmap starts. Must be aligned to a cluster boundary.
>> +
>> +         8 - 11:    Number of entries in the L1 table of the bitmap
>> +
>> +        12 - 15:    Bitmap granularity in bytes
>> +
>> +        16 - 23:    Bitmap size in sectors
>> +
>> +        24 - 25:    Size of the bitmap name
>> +
>> +        variable:   The name of the bitmap (not null terminated)
>> +
>> +        variable:   Padding to round up the bitmap table entry size to the
>> +                    next multiple of 8.
>> +
>> +The fields "size", "granularity" and "name" are corresponding with the fields
>> +in struct BdrvDirtyBitmap.
> Referring to the internals of a C struct in QEMU is not appropriate for
> a file format specification.  Please document the fields fully including
> their constraints, minimums, maximums, etc.


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-10  8:19     ` Vladimir Sementsov-Ogievskiy
@ 2015-06-10  8:49       ` Vladimir Sementsov-Ogievskiy
  2015-06-10 13:00       ` Eric Blake
  2015-06-10 13:24       ` Stefan Hajnoczi
  2 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-06-10  8:49 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: kwolf, qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha, den,
	pbonzini, jsnow

On 10.06.2015 11:19, Vladimir Sementsov-Ogievskiy wrote:
> On 09.06.2015 20:03, Stefan Hajnoczi wrote:
>> On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir 
>> Sementsov-Ogievskiy wrote:
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>>
>>> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
>>> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
>>> other drives (there may be qcow2 file with zero disk size but with
>>> several dirty bitmaps for other drives).
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   docs/specs/qcow2.txt | 66 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 66 insertions(+)
>>>
>>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>>> index 121dfc8..0fffba2 100644
>>> --- a/docs/specs/qcow2.txt
>>> +++ b/docs/specs/qcow2.txt
>>> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like 
>>> the following:
>>>                           0x00000000 - End of the header extension area
>>>                           0xE2792ACA - Backing file format name
>>>                           0x6803f857 - Feature name table
>>> +                        0x23852875 - Dirty bitmaps
>>>                           other      - Unknown header extension, can 
>>> be safely
>>>                                        ignored
>>>   @@ -166,6 +167,19 @@ the header extension data. Each entry look 
>>> like this:
>>>                       terminated if it has full length)
>>>     +== Dirty bitmaps ==
>>> +
>>> +Dirty bitmaps is an optional header extension. It provides a 
>>> possibility of
>>> +storing dirty bitmaps in qcow2 image. The fields are:
>>> +
>>> +          0 -  3:  nb_dirty_bitmaps
>>> +                   Number of dirty bitmaps contained in the image
>> Is there a maximum?
> hmm. any proposals for this?
>>
>>> +
>>> +          4 - 11:  dirty_bitmaps_offset
>>> +                   Offset into the image file at which the dirty 
>>> bitmaps table
>>> +                   starts. Must be aligned to a cluster boundary.
>> The autoclear feature bit is undocumented.
>>
>>>   == Host cluster management ==
>>>     qcow2 manages the allocation of host clusters by maintaining a 
>>> reference count
>>> @@ -360,3 +374,55 @@ Snapshot table entry:
>>>             variable:   Padding to round up the snapshot table entry 
>>> size to the
>>>                       next multiple of 8.
>>> +
>>> +
>>> +== Dirty bitmaps ==
>>> +
>>> +The feature supports storing several dirty bitmaps in the qcow2 file.
>>> +
>>> +=== Cluster mapping ===
>>> +
>>> +Dirty bitmaps are stored using a ONE-level structure for the 
>>> mapping of
>>> +bitmaps to host clusters. There is only an L1 table.
>>> +
>>> +The L1 table has a variable size (stored in the Bitmap table entry) 
>>> and may
>>> +use multiple clusters, however it must be contiguous in the image 
>>> file.
>> The use of "L1 table" could be confusing.  The refcount metadata uses
>> "refcount table" and "refcount block" to describe a one-level table.
> I agree. Hmm.. dirty bitmaps table? ok?
oh, no, bad idea. dirty bitmaps table is other thing))
>>
>>> +
>>> +Given an offset into the bitmap, the offset into the image file can be
>>> +obtained as follows:
>>> +
>>> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
>> It might help to add granularity to this formula.
>>
>> Instead of "offset", "bit_number" or "bitnr" might be clearer since
>> "offset" means something different in other parts of the document.
> Hmm. In my opinion, the bitmap here is stored as raw data. And 
> granularity is an additional parameter (for deserializing this data). 
> So, it is an offset in bytes for this data. The format is not for 
> accessing bitmap bits, it's only for loading the whole bitmap one time.
>>
>>> +
>>> +L1 table entry:
>>> +
>>> +    Bit  0 -  61:   Standard cluster descriptor
>>> +
>>> +        62 -  63:   Reserved
>> Do you really want to use the standard cluster descriptor with it's zero
>> bit?
>>
>> Since bitmaps don't honor backing files there doesn't seem much point in
>> using the zero bit, things are simpler if just bits 9-55 are contain the
>> host cluster offset and 0 means the cluster is unallocated.
>>
>> By honoring the zero bit there are three states:
>> 1. Zero bit set, read zeroes
>> 2. Zero bit not set, host cluster offset != 0, bits valid
>> 3. Zero bit not set, host cluster offset == 0, unallocated
>>
>> State 1 is not useful.
>>
>>> +=== Bitmap table ===
>>> +
>>> +A directory of all bitmaps is stored in the bitmap table, a 
>>> contiguous area in
>>> +the image file, whose starting offset and length are given by the 
>>> header fields
>>> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the 
>>> bitmap table have
>>> +variable length, depending on the length of name and extra data.
>>> +
>>> +Bitmap table entry:
>>> +
>>> +    Byte 0 -  7:    Offset into the image file at which the L1 
>>> table for the
>>> +                    bitmap starts. Must be aligned to a cluster 
>>> boundary.
>>> +
>>> +         8 - 11:    Number of entries in the L1 table of the bitmap
>>> +
>>> +        12 - 15:    Bitmap granularity in bytes
>>> +
>>> +        16 - 23:    Bitmap size in sectors
>>> +
>>> +        24 - 25:    Size of the bitmap name
>>> +
>>> +        variable:   The name of the bitmap (not null terminated)
>>> +
>>> +        variable:   Padding to round up the bitmap table entry size 
>>> to the
>>> +                    next multiple of 8.
>>> +
>>> +The fields "size", "granularity" and "name" are corresponding with 
>>> the fields
>>> +in struct BdrvDirtyBitmap.
>> Referring to the internals of a C struct in QEMU is not appropriate for
>> a file format specification.  Please document the fields fully including
>> their constraints, minimums, maximums, etc.
>
>


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-10  8:19     ` Vladimir Sementsov-Ogievskiy
  2015-06-10  8:49       ` Vladimir Sementsov-Ogievskiy
@ 2015-06-10 13:00       ` Eric Blake
  2015-06-11 10:16         ` Vladimir Sementsov-Ogievskiy
  2015-06-10 13:24       ` Stefan Hajnoczi
  2 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2015-06-10 13:00 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, Stefan Hajnoczi
  Cc: kwolf, qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha,
	pbonzini, den, jsnow

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

On 06/10/2015 02:19 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> +Dirty bitmaps is an optional header extension. It provides a
>>> possibility of
>>> +storing dirty bitmaps in qcow2 image. The fields are:
>>> +
>>> +          0 -  3:  nb_dirty_bitmaps
>>> +                   Number of dirty bitmaps contained in the image
>> Is there a maximum?
> hmm. any proposals for this?
>>
>>> +
>>> +          4 - 11:  dirty_bitmaps_offset

I'm not sure if there is a reasonable cap on the number of dirty
bitmaps; I doubt that anyone will actually supply all 4G possible images
allowed by the four-byte field, but don't have a suggestion on a smaller
limit that doesn't feel arbitrary.

[meta-comment] It's very hard to pick out the new content in your reply
if you do not separate your new text with a newline both before and
after (as I'm doing here).


>>> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
>>> +bitmaps to host clusters. There is only an L1 table.
>>> +
>>> +The L1 table has a variable size (stored in the Bitmap table entry)
>>> and may
>>> +use multiple clusters, however it must be contiguous in the image file.
>> The use of "L1 table" could be confusing.  The refcount metadata uses
>> "refcount table" and "refcount block" to describe a one-level table.
> I agree. Hmm.. dirty bitmaps table? ok?

"dirty bitmaps table" works for me, as a name for the one-level table.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-10  8:19     ` Vladimir Sementsov-Ogievskiy
  2015-06-10  8:49       ` Vladimir Sementsov-Ogievskiy
  2015-06-10 13:00       ` Eric Blake
@ 2015-06-10 13:24       ` Stefan Hajnoczi
  2015-06-11 10:19         ` Vladimir Sementsov-Ogievskiy
  2 siblings, 1 reply; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-06-10 13:24 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: kwolf, Stefan Hajnoczi, qemu-devel, Vladimir Sementsov-Ogievskiy,
	pbonzini, den, jsnow

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

On Wed, Jun 10, 2015 at 11:19:30AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 09.06.2015 20:03, Stefan Hajnoczi wrote:
> >On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >>@@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
> >>                      terminated if it has full length)
> >>+== Dirty bitmaps ==
> >>+
> >>+Dirty bitmaps is an optional header extension. It provides a possibility of
> >>+storing dirty bitmaps in qcow2 image. The fields are:
> >>+
> >>+          0 -  3:  nb_dirty_bitmaps
> >>+                   Number of dirty bitmaps contained in the image
> >Is there a maximum?
> hmm. any proposals for this?

65535 seems practical.

> >>+=== Cluster mapping ===
> >>+
> >>+Dirty bitmaps are stored using a ONE-level structure for the mapping of
> >>+bitmaps to host clusters. There is only an L1 table.
> >>+
> >>+The L1 table has a variable size (stored in the Bitmap table entry) and may
> >>+use multiple clusters, however it must be contiguous in the image file.
> >The use of "L1 table" could be confusing.  The refcount metadata uses
> >"refcount table" and "refcount block" to describe a one-level table.
> I agree. Hmm.. dirty bitmaps table? ok?

Yes, that is good.

> >>+
> >>+Given an offset into the bitmap, the offset into the image file can be
> >>+obtained as follows:
> >>+
> >>+    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
> >It might help to add granularity to this formula.
> >
> >Instead of "offset", "bit_number" or "bitnr" might be clearer since
> >"offset" means something different in other parts of the document.
> Hmm. In my opinion, the bitmap here is stored as raw data. And granularity
> is an additional parameter (for deserializing this data). So, it is an
> offset in bytes for this data. The format is not for accessing bitmap bits,
> it's only for loading the whole bitmap one time.

You are right, it wasn't clear when I read this the first time.  My
problem was the "offset into the bitmap" doesn't have any units.  So
let's make this more explicit.  Can you document how to go from a bit
number down to the offset?

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-08 15:21 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
  2015-06-09 16:01   ` John Snow
  2015-06-09 17:03   ` Stefan Hajnoczi
@ 2015-06-10 15:34   ` Kevin Wolf
  2015-06-11 10:25     ` Vladimir Sementsov-Ogievskiy
  2015-08-24 10:46     ` Vladimir Sementsov-Ogievskiy
  2015-08-24 13:30   ` Vladimir Sementsov-Ogievskiy
                     ` (2 subsequent siblings)
  5 siblings, 2 replies; 41+ messages in thread
From: Kevin Wolf @ 2015-06-10 15:34 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha, pbonzini, den,
	jsnow

Am 08.06.2015 um 17:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> 
> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
> other drives (there may be qcow2 file with zero disk size but with
> several dirty bitmaps for other drives).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..0fffba2 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like the following:
>                          0x00000000 - End of the header extension area
>                          0xE2792ACA - Backing file format name
>                          0x6803f857 - Feature name table
> +                        0x23852875 - Dirty bitmaps
>                          other      - Unknown header extension, can be safely
>                                       ignored
>  
> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>                      terminated if it has full length)
>  
>  
> +== Dirty bitmaps ==
> +
> +Dirty bitmaps is an optional header extension. It provides a possibility of
> +storing dirty bitmaps in qcow2 image. The fields are:
> +
> +          0 -  3:  nb_dirty_bitmaps
> +                   Number of dirty bitmaps contained in the image
> +
> +          4 - 11:  dirty_bitmaps_offset
> +                   Offset into the image file at which the dirty bitmaps table
> +                   starts. Must be aligned to a cluster boundary.
> +
> +
>  == Host cluster management ==

You need to use a compatibility flag because for old qemu versions, the
dirty bitmaps (and associated metadata) are leaked clusters and qemu-img
check would "repair" them by resetting the refcount to 0.

At second sight, I see that your patches add an autoclear flag.
Presumably the contents of the dirty bitmaps is outdated when you
accessed the image with an older version, so this seems right. We just
need to document it.

>  qcow2 manages the allocation of host clusters by maintaining a reference count
> @@ -360,3 +374,55 @@ Snapshot table entry:
>  
>          variable:   Padding to round up the snapshot table entry size to the
>                      next multiple of 8.
> +
> +
> +== Dirty bitmaps ==
> +
> +The feature supports storing several dirty bitmaps in the qcow2 file.
> +
> +=== Cluster mapping ===
> +
> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
> +bitmaps to host clusters. There is only an L1 table.
> +
> +The L1 table has a variable size (stored in the Bitmap table entry) and may
> +use multiple clusters, however it must be contiguous in the image file.
> +
> +Given an offset into the bitmap, the offset into the image file can be
> +obtained as follows:
> +
> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
> +
> +L1 table entry:
> +
> +    Bit  0 -  61:   Standard cluster descriptor
> +
> +        62 -  63:   Reserved

Stefan already mentioned that we don't have a "L1" when there is only
one level, and that you shouldn't reuse the cluster descriptors from L2
tables.

> +=== Bitmap table ===
> +
> +A directory of all bitmaps is stored in the bitmap table, a contiguous area in
> +the image file, whose starting offset and length are given by the header fields
> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
> +variable length, depending on the length of name and extra data.
> +
> +Bitmap table entry:
> +
> +    Byte 0 -  7:    Offset into the image file at which the L1 table for the
> +                    bitmap starts. Must be aligned to a cluster boundary.
> +
> +         8 - 11:    Number of entries in the L1 table of the bitmap

Worth using 64 bits here? This can only cover 4 * 512 GB = 2 TB for the
smallest possible cluster size. Though it's 65536 * 512 = 32 PB for the
default, which might be enough for a while.

> +        12 - 15:    Bitmap granularity in bytes
> +
> +        16 - 23:    Bitmap size in sectors

Please don't use sectors, that's a meaningless unit. Bytes is better.

> +        24 - 25:    Size of the bitmap name

We should use a smaller limit than the possible 64k to avoid too large
memory allocations. Nobody needs really long bitmap names.

> +
> +        variable:   The name of the bitmap (not null terminated)
> +
> +        variable:   Padding to round up the bitmap table entry size to the
> +                    next multiple of 8.

Kevin

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-10 13:00       ` Eric Blake
@ 2015-06-11 10:16         ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-06-11 10:16 UTC (permalink / raw)
  To: Eric Blake, Stefan Hajnoczi
  Cc: kwolf, qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha,
	pbonzini, den, jsnow

On 10.06.2015 16:00, Eric Blake wrote:
> On 06/10/2015 02:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>> +Dirty bitmaps is an optional header extension. It provides a
>>>> possibility of
>>>> +storing dirty bitmaps in qcow2 image. The fields are:
>>>> +
>>>> +          0 -  3:  nb_dirty_bitmaps
>>>> +                   Number of dirty bitmaps contained in the image
>>> Is there a maximum?
>> hmm. any proposals for this?
>>>> +
>>>> +          4 - 11:  dirty_bitmaps_offset
> I'm not sure if there is a reasonable cap on the number of dirty
> bitmaps; I doubt that anyone will actually supply all 4G possible images
> allowed by the four-byte field, but don't have a suggestion on a smaller
> limit that doesn't feel arbitrary.
>
> [meta-comment] It's very hard to pick out the new content in your reply
> if you do not separate your new text with a newline both before and
> after (as I'm doing here).
>
>
>>>> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
>>>> +bitmaps to host clusters. There is only an L1 table.
>>>> +
>>>> +The L1 table has a variable size (stored in the Bitmap table entry)
>>>> and may
>>>> +use multiple clusters, however it must be contiguous in the image file.
>>> The use of "L1 table" could be confusing.  The refcount metadata uses
>>> "refcount table" and "refcount block" to describe a one-level table.
>> I agree. Hmm.. dirty bitmaps table? ok?
> "dirty bitmaps table" works for me, as a name for the one-level table.
>

for now, dirty bitmaps table is the table with bitmap descriptors, and 
each bitmap descriptor contains its own l1 table..
What about dirty bitmap directory for descriptors and dirty bitmap table 
for l1? like pde pte)

-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-10 13:24       ` Stefan Hajnoczi
@ 2015-06-11 10:19         ` Vladimir Sementsov-Ogievskiy
  2015-06-11 13:03           ` Stefan Hajnoczi
  0 siblings, 1 reply; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-06-11 10:19 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: kwolf, Stefan Hajnoczi, qemu-devel, Vladimir Sementsov-Ogievskiy,
	pbonzini, den, jsnow

On 10.06.2015 16:24, Stefan Hajnoczi wrote:
> On Wed, Jun 10, 2015 at 11:19:30AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 09.06.2015 20:03, Stefan Hajnoczi wrote:
>>> On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>>>>                       terminated if it has full length)
>>>> +== Dirty bitmaps ==
>>>> +
>>>> +Dirty bitmaps is an optional header extension. It provides a possibility of
>>>> +storing dirty bitmaps in qcow2 image. The fields are:
>>>> +
>>>> +          0 -  3:  nb_dirty_bitmaps
>>>> +                   Number of dirty bitmaps contained in the image
>>> Is there a maximum?
>> hmm. any proposals for this?
> 65535 seems practical.

So, you suggest to reduce this field width to 2b? And additional 2 bytes 
reserved field, to achieve 8b-alignment?

>
>>>> +=== Cluster mapping ===
>>>> +
>>>> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
>>>> +bitmaps to host clusters. There is only an L1 table.
>>>> +
>>>> +The L1 table has a variable size (stored in the Bitmap table entry) and may
>>>> +use multiple clusters, however it must be contiguous in the image file.
>>> The use of "L1 table" could be confusing.  The refcount metadata uses
>>> "refcount table" and "refcount block" to describe a one-level table.
>> I agree. Hmm.. dirty bitmaps table? ok?
> Yes, that is good.
>
>>>> +
>>>> +Given an offset into the bitmap, the offset into the image file can be
>>>> +obtained as follows:
>>>> +
>>>> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
>>> It might help to add granularity to this formula.
>>>
>>> Instead of "offset", "bit_number" or "bitnr" might be clearer since
>>> "offset" means something different in other parts of the document.
>> Hmm. In my opinion, the bitmap here is stored as raw data. And granularity
>> is an additional parameter (for deserializing this data). So, it is an
>> offset in bytes for this data. The format is not for accessing bitmap bits,
>> it's only for loading the whole bitmap one time.
> You are right, it wasn't clear when I read this the first time.  My
> problem was the "offset into the bitmap" doesn't have any units.  So
> let's make this more explicit.  Can you document how to go from a bit
> number down to the offset?


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-10 15:34   ` Kevin Wolf
@ 2015-06-11 10:25     ` Vladimir Sementsov-Ogievskiy
  2015-06-11 16:30       ` John Snow
  2015-08-24 10:46     ` Vladimir Sementsov-Ogievskiy
  1 sibling, 1 reply; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-06-11 10:25 UTC (permalink / raw)
  To: Kevin Wolf
  Cc: qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha, pbonzini, den,
	jsnow

On 10.06.2015 18:34, Kevin Wolf wrote:
> Am 08.06.2015 um 17:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>
>> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
>> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
>> other drives (there may be qcow2 file with zero disk size but with
>> several dirty bitmaps for other drives).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>
>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>> index 121dfc8..0fffba2 100644
>> --- a/docs/specs/qcow2.txt
>> +++ b/docs/specs/qcow2.txt
>> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like the following:
>>                           0x00000000 - End of the header extension area
>>                           0xE2792ACA - Backing file format name
>>                           0x6803f857 - Feature name table
>> +                        0x23852875 - Dirty bitmaps
>>                           other      - Unknown header extension, can be safely
>>                                        ignored
>>   
>> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>>                       terminated if it has full length)
>>   
>>   
>> +== Dirty bitmaps ==
>> +
>> +Dirty bitmaps is an optional header extension. It provides a possibility of
>> +storing dirty bitmaps in qcow2 image. The fields are:
>> +
>> +          0 -  3:  nb_dirty_bitmaps
>> +                   Number of dirty bitmaps contained in the image
>> +
>> +          4 - 11:  dirty_bitmaps_offset
>> +                   Offset into the image file at which the dirty bitmaps table
>> +                   starts. Must be aligned to a cluster boundary.
>> +
>> +
>>   == Host cluster management ==
> You need to use a compatibility flag because for old qemu versions, the
> dirty bitmaps (and associated metadata) are leaked clusters and qemu-img
> check would "repair" them by resetting the refcount to 0.
>
> At second sight, I see that your patches add an autoclear flag.
> Presumably the contents of the dirty bitmaps is outdated when you
> accessed the image with an older version, so this seems right. We just
> need to document it.
>
>>   qcow2 manages the allocation of host clusters by maintaining a reference count
>> @@ -360,3 +374,55 @@ Snapshot table entry:
>>   
>>           variable:   Padding to round up the snapshot table entry size to the
>>                       next multiple of 8.
>> +
>> +
>> +== Dirty bitmaps ==
>> +
>> +The feature supports storing several dirty bitmaps in the qcow2 file.
>> +
>> +=== Cluster mapping ===
>> +
>> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
>> +bitmaps to host clusters. There is only an L1 table.
>> +
>> +The L1 table has a variable size (stored in the Bitmap table entry) and may
>> +use multiple clusters, however it must be contiguous in the image file.
>> +
>> +Given an offset into the bitmap, the offset into the image file can be
>> +obtained as follows:
>> +
>> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
>> +
>> +L1 table entry:
>> +
>> +    Bit  0 -  61:   Standard cluster descriptor
>> +
>> +        62 -  63:   Reserved
> Stefan already mentioned that we don't have a "L1" when there is only
> one level, and that you shouldn't reuse the cluster descriptors from L2
> tables.
>
>> +=== Bitmap table ===
>> +
>> +A directory of all bitmaps is stored in the bitmap table, a contiguous area in
>> +the image file, whose starting offset and length are given by the header fields
>> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
>> +variable length, depending on the length of name and extra data.
>> +
>> +Bitmap table entry:
>> +
>> +    Byte 0 -  7:    Offset into the image file at which the L1 table for the
>> +                    bitmap starts. Must be aligned to a cluster boundary.
>> +
>> +         8 - 11:    Number of entries in the L1 table of the bitmap
> Worth using 64 bits here? This can only cover 4 * 512 GB = 2 TB for the
> smallest possible cluster size. Though it's 65536 * 512 = 32 PB for the
> default, which might be enough for a while.
>
>> +        12 - 15:    Bitmap granularity in bytes
>> +
>> +        16 - 23:    Bitmap size in sectors
> Please don't use sectors, that's a meaningless unit. Bytes is better.
Just bad description. Actually it is ~ (number of bits in bitmap * 
granularity), and it is corresponding to number of sectors in the image.
>
>> +        24 - 25:    Size of the bitmap name
> We should use a smaller limit than the possible 64k to avoid too large
> memory allocations. Nobody needs really long bitmap names.
>
>> +
>> +        variable:   The name of the bitmap (not null terminated)
>> +
>> +        variable:   Padding to round up the bitmap table entry size to the
>> +                    next multiple of 8.
> Kevin


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-11 10:19         ` Vladimir Sementsov-Ogievskiy
@ 2015-06-11 13:03           ` Stefan Hajnoczi
  2015-06-11 16:21             ` John Snow
  0 siblings, 1 reply; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-06-11 13:03 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: kwolf, qemu-devel, Vladimir Sementsov-Ogievskiy, Stefan Hajnoczi,
	den, pbonzini, jsnow

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

On Thu, Jun 11, 2015 at 01:19:24PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 10.06.2015 16:24, Stefan Hajnoczi wrote:
> >On Wed, Jun 10, 2015 at 11:19:30AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >>On 09.06.2015 20:03, Stefan Hajnoczi wrote:
> >>>On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >>>>@@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
> >>>>                      terminated if it has full length)
> >>>>+== Dirty bitmaps ==
> >>>>+
> >>>>+Dirty bitmaps is an optional header extension. It provides a possibility of
> >>>>+storing dirty bitmaps in qcow2 image. The fields are:
> >>>>+
> >>>>+          0 -  3:  nb_dirty_bitmaps
> >>>>+                   Number of dirty bitmaps contained in the image
> >>>Is there a maximum?
> >>hmm. any proposals for this?
> >65535 seems practical.
> 
> So, you suggest to reduce this field width to 2b? And additional 2 bytes
> reserved field, to achieve 8b-alignment?

No, I would leave it 32-bit but impose a little (which can be increased
later if necessary).  That's how nb_snapshots works too.

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-11 13:03           ` Stefan Hajnoczi
@ 2015-06-11 16:21             ` John Snow
  2015-06-12 10:28               ` Stefan Hajnoczi
  0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-06-11 16:21 UTC (permalink / raw)
  To: Stefan Hajnoczi, Vladimir Sementsov-Ogievskiy
  Cc: kwolf, qemu-devel, Vladimir Sementsov-Ogievskiy, Stefan Hajnoczi,
	den, pbonzini

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/11/2015 09:03 AM, Stefan Hajnoczi wrote:
> On Thu, Jun 11, 2015 at 01:19:24PM +0300, Vladimir
> Sementsov-Ogievskiy wrote:
>> On 10.06.2015 16:24, Stefan Hajnoczi wrote:
>>> On Wed, Jun 10, 2015 at 11:19:30AM +0300, Vladimir
>>> Sementsov-Ogievskiy wrote:
>>>> On 09.06.2015 20:03, Stefan Hajnoczi wrote:
>>>>> On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir
>>>>> Sementsov-Ogievskiy wrote:
>>>>>> @@ -166,6 +167,19 @@ the header extension data. Each
>>>>>> entry look like this: terminated if it has full length) 
>>>>>> +== Dirty bitmaps == + +Dirty bitmaps is an optional
>>>>>> header extension. It provides a possibility of +storing
>>>>>> dirty bitmaps in qcow2 image. The fields are: + +
>>>>>> 0 -  3:  nb_dirty_bitmaps +                   Number of
>>>>>> dirty bitmaps contained in the image
>>>>> Is there a maximum?
>>>> hmm. any proposals for this?
>>> 65535 seems practical.
>> 
>> So, you suggest to reduce this field width to 2b? And additional
>> 2 bytes reserved field, to achieve 8b-alignment?
> 
> No, I would leave it 32-bit but impose a little (which can be
> increased later if necessary).  That's how nb_snapshots works too.
> 

Doesn't the code already limit the number of bitmaps via +#define
QCOW_MAX_DIRTY_BITMAPS 65536, from patch 2?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVebWLAAoJEH3vgQaq/DkOJCYP/jUSJT+jhb3+GvAtddCssyYR
u1BHZacXyTsDTwX4WDZQ5eGEZJZeSwu7++w5N+m+62yDxervarfEE0G/nuGRSNWx
0zYF0RrlYZFdDqed18rgXJJjCtNo1jp67ojk+xpEBUMx9cgFa6s+BTkrY0h+4hiO
V3mvU0H1+8by1Ss5lvziKCHdrksGyBIS4gw+WZNshdOc46/nBZfSlh6CWmtOO/5S
XZwXLKE7QMJMzigdcLJBOlymRwnF094Myklf8fZQILgbdoHoKhEEj9gVWkSpoNk9
FkMDDS1qN5vtYy5Ehzwy9QpbsN5ZEhuHoj5N8k0vDfFHgB9KKvOChvxf2lVhgbz7
fvGpqUb4eEdTvRno9V+8KoEcs99JXLvhed8LrfcZzq05WKbLeAdXYj18QrDw8pdY
Fl4kV5Ca4dpvDAcNZDlCKERv+STLh56hYXEYtjzNEXL+ryQwUyHetY/M6Qodq0j2
FtJq21aj68vEOovQQcX2QxqRxkPzDEvNPbM+phBOh2FjQkbvB6I5bs/ueloyi2q9
UtXWhR6ImUgA6LN25OIc6GS9xYJsFiQlLh1uI/bJoDEpQvVnMojAXE7SohyTya89
2+HIGJsdkbBZsc4SN1INqcsRCeN1at8KiwdIbAijrciF9WIsv0kUEvCvmA93UVYp
s2Os9g5QgMXrK1icCK5J
=CIuZ
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-11 10:25     ` Vladimir Sementsov-Ogievskiy
@ 2015-06-11 16:30       ` John Snow
  2015-06-12  8:33         ` Kevin Wolf
  0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-06-11 16:30 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, Kevin Wolf
  Cc: qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha, pbonzini, den



On 06/11/2015 06:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> On 10.06.2015 18:34, Kevin Wolf wrote:
>> Am 08.06.2015 um 17:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>>
>>> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
>>> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
>>> other drives (there may be qcow2 file with zero disk size but with
>>> several dirty bitmaps for other drives).
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   docs/specs/qcow2.txt | 66
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 66 insertions(+)
>>>
>>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>>> index 121dfc8..0fffba2 100644
>>> --- a/docs/specs/qcow2.txt
>>> +++ b/docs/specs/qcow2.txt
>>> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like
>>> the following:
>>>                           0x00000000 - End of the header extension area
>>>                           0xE2792ACA - Backing file format name
>>>                           0x6803f857 - Feature name table
>>> +                        0x23852875 - Dirty bitmaps
>>>                           other      - Unknown header extension, can
>>> be safely
>>>                                        ignored
>>>   @@ -166,6 +167,19 @@ the header extension data. Each entry look
>>> like this:
>>>                       terminated if it has full length)
>>>     +== Dirty bitmaps ==
>>> +
>>> +Dirty bitmaps is an optional header extension. It provides a
>>> possibility of
>>> +storing dirty bitmaps in qcow2 image. The fields are:
>>> +
>>> +          0 -  3:  nb_dirty_bitmaps
>>> +                   Number of dirty bitmaps contained in the image
>>> +
>>> +          4 - 11:  dirty_bitmaps_offset
>>> +                   Offset into the image file at which the dirty
>>> bitmaps table
>>> +                   starts. Must be aligned to a cluster boundary.
>>> +
>>> +
>>>   == Host cluster management ==
>> You need to use a compatibility flag because for old qemu versions, the
>> dirty bitmaps (and associated metadata) are leaked clusters and qemu-img
>> check would "repair" them by resetting the refcount to 0.
>>
>> At second sight, I see that your patches add an autoclear flag.
>> Presumably the contents of the dirty bitmaps is outdated when you
>> accessed the image with an older version, so this seems right. We just
>> need to document it.
>>
>>>   qcow2 manages the allocation of host clusters by maintaining a
>>> reference count
>>> @@ -360,3 +374,55 @@ Snapshot table entry:
>>>             variable:   Padding to round up the snapshot table entry
>>> size to the
>>>                       next multiple of 8.
>>> +
>>> +
>>> +== Dirty bitmaps ==
>>> +
>>> +The feature supports storing several dirty bitmaps in the qcow2 file.
>>> +
>>> +=== Cluster mapping ===
>>> +
>>> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
>>> +bitmaps to host clusters. There is only an L1 table.
>>> +
>>> +The L1 table has a variable size (stored in the Bitmap table entry)
>>> and may
>>> +use multiple clusters, however it must be contiguous in the image file.
>>> +
>>> +Given an offset into the bitmap, the offset into the image file can be
>>> +obtained as follows:
>>> +
>>> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
>>> +
>>> +L1 table entry:
>>> +
>>> +    Bit  0 -  61:   Standard cluster descriptor
>>> +
>>> +        62 -  63:   Reserved
>> Stefan already mentioned that we don't have a "L1" when there is only
>> one level, and that you shouldn't reuse the cluster descriptors from L2
>> tables.
>>
>>> +=== Bitmap table ===
>>> +
>>> +A directory of all bitmaps is stored in the bitmap table, a
>>> contiguous area in
>>> +the image file, whose starting offset and length are given by the
>>> header fields
>>> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap
>>> table have
>>> +variable length, depending on the length of name and extra data.
>>> +
>>> +Bitmap table entry:
>>> +
>>> +    Byte 0 -  7:    Offset into the image file at which the L1 table
>>> for the
>>> +                    bitmap starts. Must be aligned to a cluster
>>> boundary.
>>> +
>>> +         8 - 11:    Number of entries in the L1 table of the bitmap
>> Worth using 64 bits here? This can only cover 4 * 512 GB = 2 TB for the
>> smallest possible cluster size. Though it's 65536 * 512 = 32 PB for the
>> default, which might be enough for a while.
>>
>>> +        12 - 15:    Bitmap granularity in bytes
>>> +
>>> +        16 - 23:    Bitmap size in sectors
>> Please don't use sectors, that's a meaningless unit. Bytes is better.
> Just bad description. Actually it is ~ (number of bits in bitmap *
> granularity), and it is corresponding to number of sectors in the image.

In defense of this, it does happen to be sectors, but what it /really/
represents is the virtual addressable range of the bitmap (its 'size'),
which just-so-happens to be a sector bitmap.

We could just remove the word "sectors" entirely, and just flatly call
it the bitmap size -- but this does reveal the internal nature of the
block layer, which uses sector bitmaps.

If you wish, we can rework this field to use bytes and just convert on
every load/store into the format that we actually require. I suppose
it'd match the QMP interface in that way.

>>
>>> +        24 - 25:    Size of the bitmap name
>> We should use a smaller limit than the possible 64k to avoid too large
>> memory allocations. Nobody needs really long bitmap names.
>>
>>> +
>>> +        variable:   The name of the bitmap (not null terminated)
>>> +
>>> +        variable:   Padding to round up the bitmap table entry size
>>> to the
>>> +                    next multiple of 8.
>> Kevin
> 
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-11 16:30       ` John Snow
@ 2015-06-12  8:33         ` Kevin Wolf
  0 siblings, 0 replies; 41+ messages in thread
From: Kevin Wolf @ 2015-06-12  8:33 UTC (permalink / raw)
  To: John Snow
  Cc: Vladimir Sementsov-Ogievskiy, qemu-devel,
	Vladimir Sementsov-Ogievskiy, stefanha, pbonzini, den

Am 11.06.2015 um 18:30 hat John Snow geschrieben:
> On 06/11/2015 06:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> > On 10.06.2015 18:34, Kevin Wolf wrote:
> >> Am 08.06.2015 um 17:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>> +=== Bitmap table ===
> >>> +
> >>> +A directory of all bitmaps is stored in the bitmap table, a
> >>> contiguous area in
> >>> +the image file, whose starting offset and length are given by the
> >>> header fields
> >>> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap
> >>> table have
> >>> +variable length, depending on the length of name and extra data.
> >>> +
> >>> +Bitmap table entry:
> >>> +
> >>> +    Byte 0 -  7:    Offset into the image file at which the L1 table
> >>> for the
> >>> +                    bitmap starts. Must be aligned to a cluster
> >>> boundary.
> >>> +
> >>> +         8 - 11:    Number of entries in the L1 table of the bitmap
> >> Worth using 64 bits here? This can only cover 4 * 512 GB = 2 TB for the
> >> smallest possible cluster size. Though it's 65536 * 512 = 32 PB for the
> >> default, which might be enough for a while.
> >>
> >>> +        12 - 15:    Bitmap granularity in bytes
> >>> +
> >>> +        16 - 23:    Bitmap size in sectors
> >> Please don't use sectors, that's a meaningless unit. Bytes is better.
> > Just bad description. Actually it is ~ (number of bits in bitmap *
> > granularity), and it is corresponding to number of sectors in the image.
> 
> In defense of this, it does happen to be sectors, but what it /really/
> represents is the virtual addressable range of the bitmap (its 'size'),
> which just-so-happens to be a sector bitmap.

So not the size of the bitmap, but the size of (range in) the image that
is covered by the bitmap?

> We could just remove the word "sectors" entirely, and just flatly call
> it the bitmap size -- but this does reveal the internal nature of the
> block layer, which uses sector bitmaps.
> 
> If you wish, we can rework this field to use bytes and just convert on
> every load/store into the format that we actually require. I suppose
> it'd match the QMP interface in that way.

Internally we can do whatever we want, but what is stored in the image
format can't be changed later on, so it should be kept as generic as
possible.

How about "number of bits in the bitmap" as the unit for the size? And
possibly require that it's a multiple of 8.

Kevin

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-11 16:21             ` John Snow
@ 2015-06-12 10:28               ` Stefan Hajnoczi
  2015-06-12 15:19                 ` John Snow
  0 siblings, 1 reply; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-06-12 10:28 UTC (permalink / raw)
  To: John Snow
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, Stefan Hajnoczi, qemu-devel,
	Vladimir Sementsov-Ogievskiy, den, pbonzini

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

On Thu, Jun 11, 2015 at 12:21:31PM -0400, John Snow wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> 
> 
> On 06/11/2015 09:03 AM, Stefan Hajnoczi wrote:
> > On Thu, Jun 11, 2015 at 01:19:24PM +0300, Vladimir
> > Sementsov-Ogievskiy wrote:
> >> On 10.06.2015 16:24, Stefan Hajnoczi wrote:
> >>> On Wed, Jun 10, 2015 at 11:19:30AM +0300, Vladimir
> >>> Sementsov-Ogievskiy wrote:
> >>>> On 09.06.2015 20:03, Stefan Hajnoczi wrote:
> >>>>> On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir
> >>>>> Sementsov-Ogievskiy wrote:
> >>>>>> @@ -166,6 +167,19 @@ the header extension data. Each
> >>>>>> entry look like this: terminated if it has full length) 
> >>>>>> +== Dirty bitmaps == + +Dirty bitmaps is an optional
> >>>>>> header extension. It provides a possibility of +storing
> >>>>>> dirty bitmaps in qcow2 image. The fields are: + +
> >>>>>> 0 -  3:  nb_dirty_bitmaps +                   Number of
> >>>>>> dirty bitmaps contained in the image
> >>>>> Is there a maximum?
> >>>> hmm. any proposals for this?
> >>> 65535 seems practical.
> >> 
> >> So, you suggest to reduce this field width to 2b? And additional
> >> 2 bytes reserved field, to achieve 8b-alignment?
> > 
> > No, I would leave it 32-bit but impose a little (which can be

s/little/limit/

> > increased later if necessary).  That's how nb_snapshots works too.
> > 
> 
> Doesn't the code already limit the number of bitmaps via +#define
> QCOW_MAX_DIRTY_BITMAPS 65536, from patch 2?

It needs to be in the specification.

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-12 10:28               ` Stefan Hajnoczi
@ 2015-06-12 15:19                 ` John Snow
  0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-06-12 15:19 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, Stefan Hajnoczi, qemu-devel,
	Vladimir Sementsov-Ogievskiy, den, pbonzini



On 06/12/2015 06:28 AM, Stefan Hajnoczi wrote:
> On Thu, Jun 11, 2015 at 12:21:31PM -0400, John Snow wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA256
>>
>>
>>
>> On 06/11/2015 09:03 AM, Stefan Hajnoczi wrote:
>>> On Thu, Jun 11, 2015 at 01:19:24PM +0300, Vladimir
>>> Sementsov-Ogievskiy wrote:
>>>> On 10.06.2015 16:24, Stefan Hajnoczi wrote:
>>>>> On Wed, Jun 10, 2015 at 11:19:30AM +0300, Vladimir
>>>>> Sementsov-Ogievskiy wrote:
>>>>>> On 09.06.2015 20:03, Stefan Hajnoczi wrote:
>>>>>>> On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir
>>>>>>> Sementsov-Ogievskiy wrote:
>>>>>>>> @@ -166,6 +167,19 @@ the header extension data. Each
>>>>>>>> entry look like this: terminated if it has full length) 
>>>>>>>> +== Dirty bitmaps == + +Dirty bitmaps is an optional
>>>>>>>> header extension. It provides a possibility of +storing
>>>>>>>> dirty bitmaps in qcow2 image. The fields are: + +
>>>>>>>> 0 -  3:  nb_dirty_bitmaps +                   Number of
>>>>>>>> dirty bitmaps contained in the image
>>>>>>> Is there a maximum?
>>>>>> hmm. any proposals for this?
>>>>> 65535 seems practical.
>>>>
>>>> So, you suggest to reduce this field width to 2b? And additional
>>>> 2 bytes reserved field, to achieve 8b-alignment?
>>>
>>> No, I would leave it 32-bit but impose a little (which can be
> 
> s/little/limit/
> 
>>> increased later if necessary).  That's how nb_snapshots works too.
>>>
>>
>> Doesn't the code already limit the number of bitmaps via +#define
>> QCOW_MAX_DIRTY_BITMAPS 65536, from patch 2?
> 
> It needs to be in the specification.
> 

Yes, but the way the replies read made it sound like we hadn't decided
on what the limit *was*, so I was just trying to clarify for myself, here.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-10 15:34   ` Kevin Wolf
  2015-06-11 10:25     ` Vladimir Sementsov-Ogievskiy
@ 2015-08-24 10:46     ` Vladimir Sementsov-Ogievskiy
  1 sibling, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-08-24 10:46 UTC (permalink / raw)
  To: Kevin Wolf
  Cc: qemu-devel, Vladimir Sementsov-Ogievskiy, stefanha, pbonzini, den,
	jsnow

On 10.06.2015 18:34, Kevin Wolf wrote:
> Am 08.06.2015 um 17:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>
>> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
>> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
>> other drives (there may be qcow2 file with zero disk size but with
>> several dirty bitmaps for other drives).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>
>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>> index 121dfc8..0fffba2 100644
>> --- a/docs/specs/qcow2.txt
>> +++ b/docs/specs/qcow2.txt
>> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like the following:
>>                           0x00000000 - End of the header extension area
>>                           0xE2792ACA - Backing file format name
>>                           0x6803f857 - Feature name table
>> +                        0x23852875 - Dirty bitmaps
>>                           other      - Unknown header extension, can be safely
>>                                        ignored
>>   
>> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>>                       terminated if it has full length)
>>   
>>   
>> +== Dirty bitmaps ==
>> +
>> +Dirty bitmaps is an optional header extension. It provides a possibility of
>> +storing dirty bitmaps in qcow2 image. The fields are:
>> +
>> +          0 -  3:  nb_dirty_bitmaps
>> +                   Number of dirty bitmaps contained in the image
>> +
>> +          4 - 11:  dirty_bitmaps_offset
>> +                   Offset into the image file at which the dirty bitmaps table
>> +                   starts. Must be aligned to a cluster boundary.
>> +
>> +
>>   == Host cluster management ==
> You need to use a compatibility flag because for old qemu versions, the
> dirty bitmaps (and associated metadata) are leaked clusters and qemu-img
> check would "repair" them by resetting the refcount to 0.
>
> At second sight, I see that your patches add an autoclear flag.
> Presumably the contents of the dirty bitmaps is outdated when you
> accessed the image with an older version, so this seems right. We just
> need to document it.
>
>>   qcow2 manages the allocation of host clusters by maintaining a reference count
>> @@ -360,3 +374,55 @@ Snapshot table entry:
>>   
>>           variable:   Padding to round up the snapshot table entry size to the
>>                       next multiple of 8.
>> +
>> +
>> +== Dirty bitmaps ==
>> +
>> +The feature supports storing several dirty bitmaps in the qcow2 file.
>> +
>> +=== Cluster mapping ===
>> +
>> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
>> +bitmaps to host clusters. There is only an L1 table.
>> +
>> +The L1 table has a variable size (stored in the Bitmap table entry) and may
>> +use multiple clusters, however it must be contiguous in the image file.
>> +
>> +Given an offset into the bitmap, the offset into the image file can be
>> +obtained as follows:
>> +
>> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
>> +
>> +L1 table entry:
>> +
>> +    Bit  0 -  61:   Standard cluster descriptor
>> +
>> +        62 -  63:   Reserved
> Stefan already mentioned that we don't have a "L1" when there is only
> one level, and that you shouldn't reuse the cluster descriptors from L2
> tables.
>
>> +=== Bitmap table ===
>> +
>> +A directory of all bitmaps is stored in the bitmap table, a contiguous area in
>> +the image file, whose starting offset and length are given by the header fields
>> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
>> +variable length, depending on the length of name and extra data.
>> +
>> +Bitmap table entry:
>> +
>> +    Byte 0 -  7:    Offset into the image file at which the L1 table for the
>> +                    bitmap starts. Must be aligned to a cluster boundary.
>> +
>> +         8 - 11:    Number of entries in the L1 table of the bitmap
> Worth using 64 bits here? This can only cover 4 * 512 GB = 2 TB for the
> smallest possible cluster size. Though it's 65536 * 512 = 32 PB for the
> default, which might be enough for a while.

We store the bitmap in RAM.. I think 2 TB bitmap should not appear, 
larger granularity should be used for big disks.

>
>> +        12 - 15:    Bitmap granularity in bytes
>> +
>> +        16 - 23:    Bitmap size in sectors
> Please don't use sectors, that's a meaningless unit. Bytes is better.
>
>> +        24 - 25:    Size of the bitmap name
> We should use a smaller limit than the possible 64k to avoid too large
> memory allocations. Nobody needs really long bitmap names.
>
>> +
>> +        variable:   The name of the bitmap (not null terminated)
>> +
>> +        variable:   Padding to round up the bitmap table entry size to the
>> +                    next multiple of 8.
> Kevin


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-08 15:21 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
                     ` (2 preceding siblings ...)
  2015-06-10 15:34   ` Kevin Wolf
@ 2015-08-24 13:30   ` Vladimir Sementsov-Ogievskiy
  2015-08-24 14:08     ` Vladimir Sementsov-Ogievskiy
  2015-08-24 14:04   ` Vladimir Sementsov-Ogievskiy
  2015-08-31 22:21   ` Eric Blake
  5 siblings, 1 reply; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-08-24 13:30 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, stefanha, pbonzini, den,
	jsnow

About structs and constraints:

Optional Header:

64bit nb_dirty_bitmaps
     valid: 1 - 65536. I think here should not be 0, in this case 
dirty-bitmap-optional-header should not exist at all. Should it instead 
be 0 - 65536
64bit dirty_bitmaps_offset
     valid: any, but dirty_bitmaps_offset % cluster_size = 0

Dirty BItmap Directory Enrty ( = bitmap header):

64bit dirty_bitmap_table_offset
     valid: any, but dirty_bitmaps_offset % cluster_size = 0
64bit nb_virtual_bits (before it was called bitmap_size)
     valid: no direct constraints (as for disk size), but it should be 
less then dirty_bitmap_table_size * cluster_size * 8 * bitmap_granularity
32bit dirty_bitmap_table_size
     ? The bitmap will take ~ dirty_bitmap_table_size * cluster_size 
bytes in RAM. What the limit should be for it?
32bit bitmap_granularity_bits ( before it was bitmap_granularity)
     valid; 0 - 63 (as for HBitmap)
     (1 << bitmap_granularity_bits) is number of virtual bits in one 
physical bit. Not related to sectors/bytes, etc. Let this format be 
closer to HBitmap than to BdrvDirtyBitmap
16bit name_size
     valid: 1 - 1023. // should it be 0 - 1023 ?
/* name follows */
/* offset to 8 bytes boundary follows */

On 08.06.2015 18:21, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>
> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
> other drives (there may be qcow2 file with zero disk size but with
> several dirty bitmaps for other drives).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 66 insertions(+)
>
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..0fffba2 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like the following:
>                           0x00000000 - End of the header extension area
>                           0xE2792ACA - Backing file format name
>                           0x6803f857 - Feature name table
> +                        0x23852875 - Dirty bitmaps
>                           other      - Unknown header extension, can be safely
>                                        ignored
>   
> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>                       terminated if it has full length)
>   
>   
> +== Dirty bitmaps ==
> +
> +Dirty bitmaps is an optional header extension. It provides a possibility of
> +storing dirty bitmaps in qcow2 image. The fields are:
> +
> +          0 -  3:  nb_dirty_bitmaps
> +                   Number of dirty bitmaps contained in the image
> +
> +          4 - 11:  dirty_bitmaps_offset
> +                   Offset into the image file at which the dirty bitmaps table
> +                   starts. Must be aligned to a cluster boundary.
> +
> +
>   == Host cluster management ==
>   
>   qcow2 manages the allocation of host clusters by maintaining a reference count
> @@ -360,3 +374,55 @@ Snapshot table entry:
>   
>           variable:   Padding to round up the snapshot table entry size to the
>                       next multiple of 8.
> +
> +
> +== Dirty bitmaps ==
> +
> +The feature supports storing several dirty bitmaps in the qcow2 file.
> +
> +=== Cluster mapping ===
> +
> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
> +bitmaps to host clusters. There is only an L1 table.
> +
> +The L1 table has a variable size (stored in the Bitmap table entry) and may
> +use multiple clusters, however it must be contiguous in the image file.
> +
> +Given an offset into the bitmap, the offset into the image file can be
> +obtained as follows:
> +
> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
> +
> +L1 table entry:
> +
> +    Bit  0 -  61:   Standard cluster descriptor
> +
> +        62 -  63:   Reserved
> +
> +=== Bitmap table ===
> +
> +A directory of all bitmaps is stored in the bitmap table, a contiguous area in
> +the image file, whose starting offset and length are given by the header fields
> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
> +variable length, depending on the length of name and extra data.
> +
> +Bitmap table entry:
> +
> +    Byte 0 -  7:    Offset into the image file at which the L1 table for the
> +                    bitmap starts. Must be aligned to a cluster boundary.
> +
> +         8 - 11:    Number of entries in the L1 table of the bitmap
> +
> +        12 - 15:    Bitmap granularity in bytes
> +
> +        16 - 23:    Bitmap size in sectors
> +
> +        24 - 25:    Size of the bitmap name
> +
> +        variable:   The name of the bitmap (not null terminated)
> +
> +        variable:   Padding to round up the bitmap table entry size to the
> +                    next multiple of 8.
> +
> +The fields "size", "granularity" and "name" are corresponding with the fields
> +in struct BdrvDirtyBitmap.


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-08 15:21 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
                     ` (3 preceding siblings ...)
  2015-08-24 13:30   ` Vladimir Sementsov-Ogievskiy
@ 2015-08-24 14:04   ` Vladimir Sementsov-Ogievskiy
  2015-08-31 22:21   ` Eric Blake
  5 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-08-24 14:04 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, stefanha, pbonzini, den,
	jsnow

About structs and constraints:

== Optional Header ==

64bit nb_dirty_bitmaps
     valid: 1 - 65536. I think here should not be 0, in this case 
dirty-bitmap-optional-header should not exist at all. Should it instead 
be 0 - 65536

64bit dirty_bitmaps_offset
     valid: any, but dirty_bitmaps_offset % cluster_size = 0

== Dirty BItmap Directory Enrty ( = bitmap header) ==

64bit dirty_bitmap_table_offset
     valid: any, but dirty_bitmap_table_offset % cluster_size = 0

64bit nb_virtual_bits (before it was called bitmap_size)
     valid: no direct constraints (as for disk size), but it should be 
<= dirty_bitmap_table_size * cluster_size * 8 * bitmap_granularity

32bit dirty_bitmap_table_size
     ? The bitmap will take ~ dirty_bitmap_table_size * cluster_size 
bytes in RAM. What the limit should be for it?
     for bdrv dirty bitmaps
           max disk size = QCOW_MAX_L1_SIZE / 8 * (cluster_size / 8) * 
cluster_size = 524288 * cluster_size ^ 2,
           dirty bitmap covers the following size: 
dirty_bitmap_table_size * cluster_size * 8 * byte_granularity
           therefore, to cover the whole disk,
           dirty_bitmap_table_size * cluster_size * 8 * byte_granularity 
 >= 524288 * cluster_size ^ 2
           i.e. dirty_bitmap_table_size >= 65536 * cluster_size / 
byte_granularity
           max cluster size is 0x200000 ( = 1 << 21 ), min granularity = 
sector_size = 512, so
           dirty_bitmap_table_size >= 0x10000000, and this limit should 
be enough for any current disk configuration, but, with cluster of size 
1 << 21
           and such dirty_bitmap_table_size we will have tooo huge 
bitmap in RAM and it is unreal..

32bit bitmap_granularity_bits ( before it was bitmap_granularity)
     valid; 0 - 63 (as for HBitmap)
     (1 << bitmap_granularity_bits) is number of virtual bits in one 
physical bit. Not related to sectors/bytes, etc. Let this format be 
closer to HBitmap than to BdrvDirtyBitmap

16bit name_size
     valid: 1 - 1023. // should it be 0 - 1023 ?

/* name follows */
/* offset to 8 bytes boundary follows */

==About granularity==
To make things more general, granularity is not related to 
disk/bytes/sectors. Granularity is just number of virtual bits in one 
physical bit.

Then, for bdrv dirty bitmaps (let's assume, that it may be not only one 
type of store dirty bitmaps), let establish, that one virtual bit is 
corresponding to one sector of the disk, i.e., byte granularity would be:
byte_granularity = (1 << bitmap_granularity_bits) * 512
for ex:
65536 = (1 << 7) * 512

On 08.06.2015 18:21, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>
> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
> other drives (there may be qcow2 file with zero disk size but with
> several dirty bitmaps for other drives).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 66 insertions(+)
>
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..0fffba2 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like the following:
>                           0x00000000 - End of the header extension area
>                           0xE2792ACA - Backing file format name
>                           0x6803f857 - Feature name table
> +                        0x23852875 - Dirty bitmaps
>                           other      - Unknown header extension, can be safely
>                                        ignored
>   
> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>                       terminated if it has full length)
>   
>   
> +== Dirty bitmaps ==
> +
> +Dirty bitmaps is an optional header extension. It provides a possibility of
> +storing dirty bitmaps in qcow2 image. The fields are:
> +
> +          0 -  3:  nb_dirty_bitmaps
> +                   Number of dirty bitmaps contained in the image
> +
> +          4 - 11:  dirty_bitmaps_offset
> +                   Offset into the image file at which the dirty bitmaps table
> +                   starts. Must be aligned to a cluster boundary.
> +
> +
>   == Host cluster management ==
>   
>   qcow2 manages the allocation of host clusters by maintaining a reference count
> @@ -360,3 +374,55 @@ Snapshot table entry:
>   
>           variable:   Padding to round up the snapshot table entry size to the
>                       next multiple of 8.
> +
> +
> +== Dirty bitmaps ==
> +
> +The feature supports storing several dirty bitmaps in the qcow2 file.
> +
> +=== Cluster mapping ===
> +
> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
> +bitmaps to host clusters. There is only an L1 table.
> +
> +The L1 table has a variable size (stored in the Bitmap table entry) and may
> +use multiple clusters, however it must be contiguous in the image file.
> +
> +Given an offset into the bitmap, the offset into the image file can be
> +obtained as follows:
> +
> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
> +
> +L1 table entry:
> +
> +    Bit  0 -  61:   Standard cluster descriptor
> +
> +        62 -  63:   Reserved
> +
> +=== Bitmap table ===
> +
> +A directory of all bitmaps is stored in the bitmap table, a contiguous area in
> +the image file, whose starting offset and length are given by the header fields
> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table have
> +variable length, depending on the length of name and extra data.
> +
> +Bitmap table entry:
> +
> +    Byte 0 -  7:    Offset into the image file at which the L1 table for the
> +                    bitmap starts. Must be aligned to a cluster boundary.
> +
> +         8 - 11:    Number of entries in the L1 table of the bitmap
> +
> +        12 - 15:    Bitmap granularity in bytes
> +
> +        16 - 23:    Bitmap size in sectors
> +
> +        24 - 25:    Size of the bitmap name
> +
> +        variable:   The name of the bitmap (not null terminated)
> +
> +        variable:   Padding to round up the bitmap table entry size to the
> +                    next multiple of 8.
> +
> +The fields "size", "granularity" and "name" are corresponding with the fields
> +in struct BdrvDirtyBitmap.


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-08-24 13:30   ` Vladimir Sementsov-Ogievskiy
@ 2015-08-24 14:08     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 41+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2015-08-24 14:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, stefanha, pbonzini, den,
	jsnow

Sorry, drop this if you, look at the new version of this litter
On 24.08.2015 16:30, Vladimir Sementsov-Ogievskiy wrote:
> About structs and constraints:
>
> Optional Header:
>
> 64bit nb_dirty_bitmaps
>     valid: 1 - 65536. I think here should not be 0, in this case 
> dirty-bitmap-optional-header should not exist at all. Should it 
> instead be 0 - 65536
> 64bit dirty_bitmaps_offset
>     valid: any, but dirty_bitmaps_offset % cluster_size = 0
>
> Dirty BItmap Directory Enrty ( = bitmap header):
>
> 64bit dirty_bitmap_table_offset
>     valid: any, but dirty_bitmaps_offset % cluster_size = 0
> 64bit nb_virtual_bits (before it was called bitmap_size)
>     valid: no direct constraints (as for disk size), but it should be 
> less then dirty_bitmap_table_size * cluster_size * 8 * bitmap_granularity
> 32bit dirty_bitmap_table_size
>     ? The bitmap will take ~ dirty_bitmap_table_size * cluster_size 
> bytes in RAM. What the limit should be for it?
> 32bit bitmap_granularity_bits ( before it was bitmap_granularity)
>     valid; 0 - 63 (as for HBitmap)
>     (1 << bitmap_granularity_bits) is number of virtual bits in one 
> physical bit. Not related to sectors/bytes, etc. Let this format be 
> closer to HBitmap than to BdrvDirtyBitmap
> 16bit name_size
>     valid: 1 - 1023. // should it be 0 - 1023 ?
> /* name follows */
> /* offset to 8 bytes boundary follows */
>
> On 08.06.2015 18:21, Vladimir Sementsov-Ogievskiy wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>
>> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
>> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
>> other drives (there may be qcow2 file with zero disk size but with
>> several dirty bitmaps for other drives).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/specs/qcow2.txt | 66 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>
>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>> index 121dfc8..0fffba2 100644
>> --- a/docs/specs/qcow2.txt
>> +++ b/docs/specs/qcow2.txt
>> @@ -123,6 +123,7 @@ be stored. Each extension has a structure like 
>> the following:
>>                           0x00000000 - End of the header extension area
>>                           0xE2792ACA - Backing file format name
>>                           0x6803f857 - Feature name table
>> +                        0x23852875 - Dirty bitmaps
>>                           other      - Unknown header extension, can 
>> be safely
>>                                        ignored
>>   @@ -166,6 +167,19 @@ the header extension data. Each entry look 
>> like this:
>>                       terminated if it has full length)
>>     +== Dirty bitmaps ==
>> +
>> +Dirty bitmaps is an optional header extension. It provides a 
>> possibility of
>> +storing dirty bitmaps in qcow2 image. The fields are:
>> +
>> +          0 -  3:  nb_dirty_bitmaps
>> +                   Number of dirty bitmaps contained in the image
>> +
>> +          4 - 11:  dirty_bitmaps_offset
>> +                   Offset into the image file at which the dirty 
>> bitmaps table
>> +                   starts. Must be aligned to a cluster boundary.
>> +
>> +
>>   == Host cluster management ==
>>     qcow2 manages the allocation of host clusters by maintaining a 
>> reference count
>> @@ -360,3 +374,55 @@ Snapshot table entry:
>>             variable:   Padding to round up the snapshot table entry 
>> size to the
>>                       next multiple of 8.
>> +
>> +
>> +== Dirty bitmaps ==
>> +
>> +The feature supports storing several dirty bitmaps in the qcow2 file.
>> +
>> +=== Cluster mapping ===
>> +
>> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
>> +bitmaps to host clusters. There is only an L1 table.
>> +
>> +The L1 table has a variable size (stored in the Bitmap table entry) 
>> and may
>> +use multiple clusters, however it must be contiguous in the image file.
>> +
>> +Given an offset into the bitmap, the offset into the image file can be
>> +obtained as follows:
>> +
>> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
>> +
>> +L1 table entry:
>> +
>> +    Bit  0 -  61:   Standard cluster descriptor
>> +
>> +        62 -  63:   Reserved
>> +
>> +=== Bitmap table ===
>> +
>> +A directory of all bitmaps is stored in the bitmap table, a 
>> contiguous area in
>> +the image file, whose starting offset and length are given by the 
>> header fields
>> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap 
>> table have
>> +variable length, depending on the length of name and extra data.
>> +
>> +Bitmap table entry:
>> +
>> +    Byte 0 -  7:    Offset into the image file at which the L1 table 
>> for the
>> +                    bitmap starts. Must be aligned to a cluster 
>> boundary.
>> +
>> +         8 - 11:    Number of entries in the L1 table of the bitmap
>> +
>> +        12 - 15:    Bitmap granularity in bytes
>> +
>> +        16 - 23:    Bitmap size in sectors
>> +
>> +        24 - 25:    Size of the bitmap name
>> +
>> +        variable:   The name of the bitmap (not null terminated)
>> +
>> +        variable:   Padding to round up the bitmap table entry size 
>> to the
>> +                    next multiple of 8.
>> +
>> +The fields "size", "granularity" and "name" are corresponding with 
>> the fields
>> +in struct BdrvDirtyBitmap.
>
>


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-06-08 15:21 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
                     ` (4 preceding siblings ...)
  2015-08-24 14:04   ` Vladimir Sementsov-Ogievskiy
@ 2015-08-31 22:21   ` Eric Blake
  2015-08-31 22:24     ` John Snow
  5 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2015-08-31 22:21 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, stefanha, pbonzini, den,
	jsnow

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

On 06/08/2015 09:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> 
> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
> other drives (there may be qcow2 file with zero disk size but with
> several dirty bitmaps for other drives).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 

> +== Dirty bitmaps ==
> +
> +Dirty bitmaps is an optional header extension. It provides a possibility of
> +storing dirty bitmaps in qcow2 image. The fields are:
> +
> +          0 -  3:  nb_dirty_bitmaps
> +                   Number of dirty bitmaps contained in the image
> +
> +          4 - 11:  dirty_bitmaps_offset
> +                   Offset into the image file at which the dirty bitmaps table
> +                   starts. Must be aligned to a cluster boundary.

To date, all 8-byte fields in qcow2 have been 8-byte aligned; this would
break that nice feature.  You could keep that nice property by swapping
the order of these two fields.

[Note that the spec on header extensions already requires clients to
recognize that a header extension of 12 bytes implicitly pads out an
additional 4 bytes, so that the next header extension type field once
again lands on 8-byte alignment]

> +== Dirty bitmaps ==
> +
> +The feature supports storing several dirty bitmaps in the qcow2 file.

Is it possible to have a qcow2 file that stores JUST dirty bitmap(s) and
no guest data (that is, no L1 table, no backing file)?  It might make
sense, if we intend to allow persistent bitmap files that can be
associated with a raw disk.  But right now, the spec seems to require
that l1_table_offset must be non-zero.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification
  2015-08-31 22:21   ` Eric Blake
@ 2015-08-31 22:24     ` John Snow
  0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-08-31 22:24 UTC (permalink / raw)
  To: Eric Blake, Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, den, Vladimir Sementsov-Ogievskiy, stefanha, pbonzini



On 08/31/2015 06:21 PM, Eric Blake wrote:
> On 06/08/2015 09:21 AM, Vladimir Sementsov-Ogievskiy wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>
>> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
>> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
>> other drives (there may be qcow2 file with zero disk size but with
>> several dirty bitmaps for other drives).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  docs/specs/qcow2.txt | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
> 
>> +== Dirty bitmaps ==
>> +
>> +Dirty bitmaps is an optional header extension. It provides a possibility of
>> +storing dirty bitmaps in qcow2 image. The fields are:
>> +
>> +          0 -  3:  nb_dirty_bitmaps
>> +                   Number of dirty bitmaps contained in the image
>> +
>> +          4 - 11:  dirty_bitmaps_offset
>> +                   Offset into the image file at which the dirty bitmaps table
>> +                   starts. Must be aligned to a cluster boundary.
> 
> To date, all 8-byte fields in qcow2 have been 8-byte aligned; this would
> break that nice feature.  You could keep that nice property by swapping
> the order of these two fields.
> 
> [Note that the spec on header extensions already requires clients to
> recognize that a header extension of 12 bytes implicitly pads out an
> additional 4 bytes, so that the next header extension type field once
> again lands on 8-byte alignment]
> 
>> +== Dirty bitmaps ==
>> +
>> +The feature supports storing several dirty bitmaps in the qcow2 file.
> 
> Is it possible to have a qcow2 file that stores JUST dirty bitmap(s) and
> no guest data (that is, no L1 table, no backing file)?  It might make
> sense, if we intend to allow persistent bitmap files that can be
> associated with a raw disk.  But right now, the spec seems to require
> that l1_table_offset must be non-zero.
> 

To my knowledge, this is a consequence of the decision that bitmaps do
not have to be related to the data contained within the .qcow2.

Ultimately, it's fine (for our purposes) there's no data in the qcow2. I
have been testing with a 0-length qcow2 to test this patchset.

--js

^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, other threads:[~2015-08-31 22:24 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-13 17:02 [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
2015-01-13 17:02 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
2015-01-27 15:39   ` Eric Blake
2015-01-13 17:02 ` [Qemu-devel] [PATCH 2/8] hbitmap: store / restore Vladimir Sementsov-Ogievskiy
2015-01-13 17:02 ` [Qemu-devel] [PATCH 3/8] qcow2: add dirty-bitmaps feature Vladimir Sementsov-Ogievskiy
2015-01-13 17:02 ` [Qemu-devel] [PATCH 4/8] block: store persistent dirty bitmaps Vladimir Sementsov-Ogievskiy
2015-01-13 17:02 ` [Qemu-devel] [PATCH 5/8] block: add bdrv_load_dirty_bitmap Vladimir Sementsov-Ogievskiy
2015-01-13 17:02 ` [Qemu-devel] [PATCH 6/8] qemu: command line option for dirty bitmaps Vladimir Sementsov-Ogievskiy
2015-01-13 17:02 ` [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap Vladimir Sementsov-Ogievskiy
2015-01-27 15:53   ` Eric Blake
2015-01-27 16:29     ` Markus Armbruster
2015-01-30  9:06     ` Vladimir Sementsov-Ogievskiy
2015-01-30 17:51       ` Eric Blake
2015-01-13 17:02 ` [Qemu-devel] [PATCH 8/8] iotests: test internal persistent " Vladimir Sementsov-Ogievskiy
2015-01-27 11:06 ` [Qemu-devel] [PATCH 0/8] block: persistent dirty bitmaps (RFC) Vladimir Sementsov-Ogievskiy
2015-02-04 15:07 ` Vladimir Sementsov-Ogievskiy
2015-02-04 15:20   ` John Snow
2015-02-04 15:40     ` Vladimir Sementsov-Ogievskiy
  -- strict thread matches above, loose matches on Subject: below --
2015-06-08 15:21 [Qemu-devel] [PATCH v2 RFC 0/8] block: persistent dirty bitmaps Vladimir Sementsov-Ogievskiy
2015-06-08 15:21 ` [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification Vladimir Sementsov-Ogievskiy
2015-06-09 16:01   ` John Snow
2015-06-09 17:03   ` Stefan Hajnoczi
2015-06-10  8:19     ` Vladimir Sementsov-Ogievskiy
2015-06-10  8:49       ` Vladimir Sementsov-Ogievskiy
2015-06-10 13:00       ` Eric Blake
2015-06-11 10:16         ` Vladimir Sementsov-Ogievskiy
2015-06-10 13:24       ` Stefan Hajnoczi
2015-06-11 10:19         ` Vladimir Sementsov-Ogievskiy
2015-06-11 13:03           ` Stefan Hajnoczi
2015-06-11 16:21             ` John Snow
2015-06-12 10:28               ` Stefan Hajnoczi
2015-06-12 15:19                 ` John Snow
2015-06-10 15:34   ` Kevin Wolf
2015-06-11 10:25     ` Vladimir Sementsov-Ogievskiy
2015-06-11 16:30       ` John Snow
2015-06-12  8:33         ` Kevin Wolf
2015-08-24 10:46     ` Vladimir Sementsov-Ogievskiy
2015-08-24 13:30   ` Vladimir Sementsov-Ogievskiy
2015-08-24 14:08     ` Vladimir Sementsov-Ogievskiy
2015-08-24 14:04   ` Vladimir Sementsov-Ogievskiy
2015-08-31 22:21   ` Eric Blake
2015-08-31 22:24     ` John Snow

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).