Re: [Qemu-devel] [PATCH 2/8] qcow2: add dirty-bitmaps feature

[John Snow <jsnow@redhat.com>]

On 06/15/2015 10:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> On 12.06.2015 02:04, John Snow wrote:
>>
>> On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>>
>>> Adds dirty-bitmaps feature to qcow2 format as specified in
>>> docs/specs/qcow2.txt
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/Makefile.objs        |   2 +-
>>>   block/qcow2-dirty-bitmap.c | 503
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>   block/qcow2.c              |  56 +++++
>>>   block/qcow2.h              |  50 +++++
>>>   include/block/block_int.h  |  10 +
>>>   5 files changed, 620 insertions(+), 1 deletion(-)
>>>   create mode 100644 block/qcow2-dirty-bitmap.c
>>>

[snip]

>>> +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;
>> I catch a segfault right around here if I do the following:
>>
>> ./x86_64-softmmu/qemu-system-x86_64 --dirty-bitmap
>> file=bitmaps.qcow2,name=bitmap0,drive=drive0 -drive
>> if=none,file=hda.qcow2,id=drive0 -device ide-hd,drive=drive0
>>
>> hda.qcow2 and bitmaps.qcow2 are both empty files, but bitmaps.qcow2 has
>> a size of '0'.
> empty file or qcow2 files of size 0 (with header) ?

Sorry, that was ambiguous. A properly formatted qcow2 file with a size of 0.

I tried two configurations:

1) Saving a bitmap to the file it describes, using e.g. hda.qcow2, which
is an 8GiB qcow2 file that has been formatted, but contains no
allocations yet.

2) Saving a bitmap to a bitmaps.qcow2 file, which is a formatted qcow2
with a size of 0, to describe a file hda.qcow2 which is 8GiB and has no
allocations either.

Both crash, because I think you are confusing s->l1_size with
bm->l1_size during the storage routine.

The following patch fixes my initial issues with the series, at least at
a cursory glance:

commit 824ed0e9f56425c98ec600abb0e31791d12e628f
Author: John Snow <jsnow@redhat.com>
Date:   Mon Jun 15 12:06:43 2015 -0400

    fix persistence

diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c
index 686a121..eaaec14 100644
--- a/block/qcow2-dirty-bitmap.c
+++ b/block/qcow2-dirty-bitmap.c
@@ -300,7 +300,8 @@ int qcow2_dirty_bitmap_store(BlockDriverState *bs,
uint8_t *buf,
         }
     }
     if (dirty_bitmap_index < 0) {
-        qcow2_dirty_bitmap_create(bs, name, size, granularity);
+        ret = qcow2_dirty_bitmap_create(bs, name, size, granularity);
+        if (ret < 0) { return ret; }
         dirty_bitmap_index = s->nb_dirty_bitmaps - 1;
     }
     bm = s->dirty_bitmaps + dirty_bitmap_index;
@@ -384,7 +385,7 @@ int qcow2_dirty_bitmap_create(BlockDriverState *bs,
const char *name,
     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));
+        qcow2_alloc_clusters(bs, bm->l1_size * sizeof(uint64_t));
     if (l1_table_offset < 0) {
         ret = l1_table_offset;
         goto fail;
@@ -398,18 +399,18 @@ int qcow2_dirty_bitmap_create(BlockDriverState
*bs, const char *name,
     }

     /* initialize with zero clusters */
-    for (i = 0; i < s->l1_size; i++) {
+    for (i = 0; i < bm->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));
+                                        bm->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));
+                      bm->l1_size * sizeof(uint64_t));
     if (ret < 0) {
         goto fail;
     }
diff --git a/vl.c b/vl.c
index 4cae8a6..8f6d79f 100644
--- a/vl.c
+++ b/vl.c
@@ -1146,7 +1146,7 @@ static int dirty_bitmap_func(void *opaque,
QemuOpts *opts, Error **errp)
             qdict_put(options, "node-name", qstring_from_str(file_id));
         }

-        bdrv_open(&file_bs, file, NULL, options, 0, NULL, errp);
+        bdrv_open(&file_bs, file, NULL, options, BDRV_O_RDWR, NULL, errp);
         if (options) {
             QDECREF(options);
         }