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

[Vladimir Sementsov-Ogievskiy]

On 13.06.2015 00:55, John Snow wrote:
> On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> > From: Vladimir Sementsov-Ogievskiy <address@hidden>
> >
> > Adds dirty-bitmaps feature to qcow2 format as specified in
> > docs/specs/qcow2.txt
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
...
> > +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);
> If this fails, we should 'return ret'.
>
> > +            dirty_bitmap_index = -1;
> > +        }
> > +    }
> Oh, find_dirty_bitmap_by_name only looks by name, but then you check to
> make sure the size and granularity matches. If it doesn't, you actually
> create a new bitmap with the *same name* but different attributes, and
> delete the old one.
>
> Is that appropriate? I guess if we're already here in store, it means we
> made it past the add checks... which means for whatever reason we
> definitely want to store *this* bitmap...
>
> I think this code is a little extraneous, it might be best to just issue
> an ultimatum that "You can't have two bitmaps with the same name in a
> file." and let that be that -- finding something with the wrong size
> would just simply be an error.
>
> > +    if (dirty_bitmap_index < 0) {
> > +        qcow2_dirty_bitmap_create(bs, name, size, granularity);
> If this fails, we need to return ret immediately.

Not agree. I think it's ok for qcow2_dirty_bitmap_store to store given 
bitmap if it can. It can in two cases (in next patchset version):

1) found the bitmap with the same name, size and granularity: it is 
assumed to be the previous version and will be rewritten
2) not found the bitmap: it's ok, just save it.. This case works when 
the bitmap was created while qemu runs.



> > +        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,

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