Re: [Qemu-devel] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Max Reitz <mreitz@redhat.com>]

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

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are

*with the AUTO flag set

> loaded at image open and becomes BdrvDirtyBitmap's for corresponding

"loaded when the image is opened and become BdrvDirtyBitmaps for the
corresponding drive."

> drive. These bitmaps are deleted from Qcow2 image after loading to avoid

*from the Qcow2 image

> conflicts.
> 
> Extra data in bitmaps is not supported for now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c | 518 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  block/qcow2.c        |   5 +
>  block/qcow2.h        |   3 +
>  3 files changed, 525 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index cd18b07..760a047 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -25,6 +25,12 @@

[...]

> +static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
> +                               uint32_t bitmap_table_size)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    int cl_size = s->cluster_size;
> +    int i;
> +
> +    for (i = 0; i < bitmap_table_size; ++i) {
> +        uint64_t addr = bitmap_table[i];
> +        if (addr <= 1) {

I'd rather add a BME_TABLE_ENTRY_OFFSET_MASK
(0x00fffffffffffe00ull) and then use:

uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK;
if (!addr) {

> +            continue;
> +        }
> +
> +        qcow2_free_clusters(bs, addr, cl_size, QCOW2_DISCARD_ALWAYS);

I think this should rather be QCOW2_DISCARD_OTHER; or you introduce a
new QCOW2_DISCARD_* flag.

(The same applies to all the other QCOW2_DISCARD_ALWAYS users here.)

Also, I don't really see the point of introducing cl_size just for this
place; it doesn't even save you from a line wrap.

> +        bitmap_table[i] = 0;
> +    }
> +}
> +
> +static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapDirEntry *entry,
> +                             uint64_t **table)
> +{
> +    int ret;
> +
> +    *table = g_try_new(uint64_t, entry->bitmap_table_size);
> +    if (*table == NULL) {

Not that g_try_new() will return NULL if you try to allocate a buffer
with size 0.

Normally, entry->bitmap_table_size shouldn't be 0, but I don't think
you're catching that case anywhere.

> +        return -ENOMEM;
> +    }
> +

I wouldn't mind an

assert(entry->bitmap_table_size <= UINT32_MAX / sizeof(uint64_t));

here because of the following multiplication (which is extended to 64
bit on 64 bit machines, but stays in 32 bit on 32 bit machines).

(Of course,

assert(entry->bitmap_table_size <= BME_MAX_TABLE_SIZE);

would be fine, too.)

> +    ret = bdrv_pread(bs->file, entry->bitmap_table_offset,
> +                     *table, entry->bitmap_table_size * sizeof(uint64_t));
> +    if (ret < 0) {
> +        g_free(*table);
> +        *table = NULL;
> +        return ret;
> +    }
> +
> +    bitmap_table_to_cpu(*table, entry->bitmap_table_size);
> +
> +    return 0;
> +}
> +
> +static void do_free_bitmap_clusters(BlockDriverState *bs,
> +                                    uint64_t table_offset,
> +                                    uint32_t table_size,
> +                                    uint64_t *bitmap_table)
> +{
> +    clear_bitmap_table(bs, bitmap_table, table_size);
> +
> +    qcow2_free_clusters(bs, table_offset, table_size * sizeof(uint64_t),
> +                        QCOW2_DISCARD_ALWAYS);
> +}
> +
> +static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
> +{
> +    int ret;
> +    uint64_t *bitmap_table;
> +
> +    ret = bitmap_table_load(bs, entry, &bitmap_table);
> +    if (ret < 0 || bitmap_table == NULL) {

That should be:

if (ret < 0) {
    assert(bitmap_table == NULL);

Or the other way around:

if (bitmap_table == NULL) {
    assert(ret < 0);

Otherwise, it looks like bitmap_table may be NULL with ret being 0, and
the next statement would make this function return success even though
it actually failed.

> +        return ret;
> +    }
> +
> +    do_free_bitmap_clusters(bs, entry->bitmap_table_offset,
> +                            entry->bitmap_table_size, bitmap_table);
> +
> +    return 0;

You're leaking bitmap_table here.

> +}
> +
> +/* dirty sectors in cluster is a number of sectors in the image, corresponding
> + * to one cluster of bitmap data */

How about:

This function returns the number of sectors covered by a single cluster
of dirty bitmap data.

> +static uint64_t dirty_sectors_in_cluster(const BDRVQcow2State *s,
> +                                         const BdrvDirtyBitmap *bitmap)
> +{
> +    uint32_t sector_granularity =
> +            bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
> +
> +    return (uint64_t)sector_granularity * (s->cluster_size << 3);
> +}
> +
> +static int load_bitmap_data(BlockDriverState *bs,
> +                            const uint64_t *dirty_bitmap_table,
> +                            uint32_t dirty_bitmap_table_size,
> +                            BdrvDirtyBitmap *bitmap)
> +{
> +    int ret = 0;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t sector, dsc;
> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +    int cl_size = s->cluster_size;

Once more, I don't really see the reason to put this into a dedicated
variable (I feel like it makes the code harder to read because you have
to keep in mind what cl_size is).

> +    uint8_t *buf = NULL;
> +    uint32_t i, tab_size =
> +            size_to_clusters(s,
> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
> +
> +    if (tab_size != dirty_bitmap_table_size) {
> +        return -EINVAL;
> +    }

You should also check that tab_size does not exceed BME_MAX_TABLE_SIZE.

And maybe you should check that the physical size of the bitmap data
does not exceed BME_MAX_PHYS_SIZE, but I'm not so sure why you have that
constant at all because it's basically superseded by the existence of
BME_MAX_TABLE_SIZE.

> +
> +    bdrv_clear_dirty_bitmap(bitmap, NULL);
> +
> +    buf = g_malloc0(cl_size);

g_malloc() would suffice as well, since you'll be overwriting all of it
anyway.

> +    dsc = dirty_sectors_in_cluster(s, bitmap);
> +    for (i = 0, sector = 0; i < tab_size; ++i, sector += dsc) {
> +        uint64_t end = MIN(bm_size, sector + dsc);
> +        uint64_t offset = dirty_bitmap_table[i];
> +
> +        if (offset == 1) {
> +            bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, end, false);
> +        } else if (offset > 1) {

As before, I'd organize this differently, e.g. like:

uint64_t table_entry = dirty_bitmap_table[i];
uint64_t offset = table_entry & BME_TABLE_ENTRY_OFFSET_MASK;

if (table_entry & BME_TABLE_ENTRY_RESERVED_MASK) {
    ret = -EINVAL;
    goto finish;
}

if (!offset) {
    if (table_entry & 1) {
        bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, end, false);
    } else {
        /* No need to deserialize zeroes because the dirty bitmap is
         * already cleared */
    }
} else {
    ret = bdrv_pread(...);
    ...
}

It's more code, yes, but I find it easier to read because it reflects
the specification better.

> +            ret = bdrv_pread(bs->file, offset, buf, cl_size);
> +            if (ret < 0) {
> +                goto finish;
> +            }
> +            bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, end, false);
> +        }
> +    }
> +    ret = 0;
> +
> +    bdrv_dirty_bitmap_deserialize_finish(bitmap);
> +
> +finish:
> +    g_free(buf);
> +
> +    return ret;
> +}
> +
> +static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
> +                                    Qcow2BitmapDirEntry *entry, Error **errp)
> +{
> +    int ret;
> +    uint64_t *bitmap_table = NULL;
> +    uint32_t granularity;
> +    BdrvDirtyBitmap *bitmap = NULL;
> +    char *name = g_strndup(dir_entry_name_notcstr(entry), entry->name_size);

Maybe you should limit entry->name_size to BME_MAX_NAME_SIZE.

> +
> +    ret = bitmap_table_load(bs, entry, &bitmap_table);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "Could not read bitmap_table table from image");
> +        goto fail;
> +    }
> +
> +    granularity = 1U << entry->granularity_bits;

You should be making sure that entry->granularity_bits does not exceed
[BME_MIN_GRANULARITY_BITS, BME_MAX_GRANULARITY_BITS] before this point.

> +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +    if (bitmap == NULL) {
> +        goto fail;
> +    }
> +
> +    ret = load_bitmap_data(bs, bitmap_table, entry->bitmap_table_size,
> +                           bitmap);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not read bitmap from image");
> +        goto fail;
> +    }
> +
> +    g_free(name);
> +    g_free(bitmap_table);
> +    return bitmap;
> +
> +fail:
> +    g_free(name);
> +    g_free(bitmap_table);
> +    if (bitmap != NULL) {
> +        bdrv_release_dirty_bitmap(bs, bitmap);
> +    }
> +
> +    return NULL;
> +}
> +
> +/* directory_read
> + * Read bitmaps directory from bs by @offset and @size. Convert it to cpu
> + * format from BE.
> + */
> +static uint8_t *directory_read(BlockDriverState *bs,
> +                               uint64_t offset, uint64_t size, Error **errp)
> +{
> +    int ret;
> +    uint8_t *dir, *dir_end;
> +    Qcow2BitmapDirEntry *e;

I think you should return an error if size >
QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE.

> +
> +    dir = g_try_malloc(size);
> +    if (dir == NULL) {
> +        error_setg(errp, "Can't allocate space for bitmap directory.");

Please drop the full stop at the end of the message.

(And I personally don't like contractions ("can't") in user-facing
messages too much, but that is just my taste.)

> +        return NULL;
> +    }
> +    dir_end = dir + size;
> +
> +    ret = bdrv_pread(bs->file, offset, dir, size);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Can't read bitmap directory.");

Again, no full stop at the end of error messages, please.

> +        goto fail;
> +    }
> +
> +    /* loop is safe because next entry offset is calculated after conversion to

Should it be s/safe/unsafe/?

> +     * cpu format */
> +    for_each_bitmap_dir_entry_unsafe(e, dir, size) {
> +        if ((uint8_t *)(e + 1) > dir_end) {
> +            goto broken_dir;
> +        }
> +
> +        bitmap_dir_entry_to_cpu(e);
> +
> +        if ((uint8_t *)next_dir_entry(e) > dir_end) {
> +            goto broken_dir;
> +        }
> +
> +        if (e->extra_data_size != 0) {
> +            error_setg(errp, "Can't load bitmap '%.*s' from '%s':"
> +                       "extra data not supported.", e->name_size,

Full stop again.

Also, I'm not quite sure why you give the device/node name here. You
don't do that anywhere else and I think if we want to emit the
information where something failed, it should be added at some previous
point in the call chain.

> +                       dir_entry_name_notcstr(e),
> +                       bdrv_get_device_or_node_name(bs));
> +            goto fail;
> +        }
> +    }

Maybe you should count the number of dirty bitmaps and return an error
if it exceeds QCOW_MAX_DIRTY_BITMAPS.

> +
> +    assert((uint8_t *)e == dir_end);

I think this should rather go to broken_dir if they're not equal.

> +
> +    return dir;
> +
> +broken_dir:
> +    error_setg(errp, "Broken bitmap directory.");

And again.

> +
> +fail:
> +    g_free(dir);
> +
> +    return NULL;
> +}
> +
> +static int update_header_sync(BlockDriverState *bs)
> +{
> +    int ret;
> +
> +    ret = qcow2_update_header(bs);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    ret = bdrv_flush(bs);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +/* write bitmap directory from the state to new allocated clusters */
> +static int64_t directory_write(BlockDriverState *bs, const uint8_t *dir,
> +                               size_t size)
> +{
> +    int ret = 0;
> +    uint8_t *dir_be = NULL;
> +    int64_t dir_offset = 0;
> +
> +    dir_be = g_try_malloc(size);
> +    if (dir_be == NULL) {
> +        return -ENOMEM;
> +    }
> +    memcpy(dir_be, dir, size);
> +    bitmap_directory_to_be(dir_be, size);
> +
> +    /* Allocate space for the new bitmap directory */
> +    dir_offset = qcow2_alloc_clusters(bs, size);
> +    if (dir_offset < 0) {
> +        ret = dir_offset;
> +        goto out;
> +    }
> +
> +    /* The bitmap directory position has not yet been updated, so these
> +     * clusters must indeed be completely free */
> +    ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, size);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
> +    ret = bdrv_pwrite(bs->file, dir_offset, dir_be, size);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
> +out:
> +    g_free(dir_be);
> +
> +    if (ret < 0) {
> +        if (dir_offset > 0) {
> +            qcow2_free_clusters(bs, dir_offset, size, QCOW2_DISCARD_ALWAYS);
> +        }
> +
> +        return ret;
> +    }
> +
> +    return dir_offset;
> +}
> +
> +static int directory_update(BlockDriverState *bs, uint8_t *new_dir,
> +                            size_t new_size, uint32_t new_nb_bitmaps)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    int ret;
> +    int64_t new_offset = 0;
> +    uint64_t old_offset = s->bitmap_directory_offset;
> +    uint64_t old_size = s->bitmap_directory_size;
> +    uint32_t old_nb_bitmaps = s->nb_bitmaps;
> +    uint64_t old_autocl = s->autoclear_features;
> +
> +    if (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
> +        return -EINVAL;
> +    }

Also, you should probably check new_nb_bitmaps against
QCOW_MAX_DIRTY_BITMAPS, but maybe you'll be adding that check somewhere
else in a future patch. In that case, an assertion wouldn't be too bad
here, though.

> +
> +    if ((new_size == 0) != (new_nb_bitmaps == 0)) {
> +        return -EINVAL;
> +    }
> +
> +    if (new_nb_bitmaps > 0) {
> +        new_offset = directory_write(bs, new_dir, new_size);
> +        if (new_offset < 0) {
> +            return new_offset;
> +        }
> +
> +        ret = bdrv_flush(bs);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +    }
> +    s->bitmap_directory_offset = new_offset;
> +    s->bitmap_directory_size = new_size;
> +    s->nb_bitmaps = new_nb_bitmaps;
> +
> +    ret = update_header_sync(bs);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    if (old_size) {
> +        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS);
> +    }
> +
> +    return 0;
> +
> +fail:
> +    if (new_offset > 0) {
> +        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS);
> +        s->bitmap_directory_offset = old_offset;
> +        s->bitmap_directory_size = old_size;
> +        s->nb_bitmaps = old_nb_bitmaps;
> +        s->autoclear_features = old_autocl;

Why are you restoring the autoclear features? From a quick glance I
can't see any code path that changes this field here, and if there is
one, it probably has a good reason to do so.

> +    }
> +
> +    return ret;
> +}
> +
> +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint8_t *dir, *new_dir, *new_pos;
> +    uint64_t dir_size;

I suggest initializing this variable here (to s->bitmap_directory_size)...

> +    Qcow2BitmapDirEntry *e;
> +    uint32_t new_nb_bitmaps = 0;
> +
> +    if (s->nb_bitmaps == 0) {
> +        /* No bitmaps - nothing to do */
> +        return 0;
> +    }
> +
> +    new_pos = new_dir = g_try_malloc(s->bitmap_directory_size);

...and then using it here...

> +    if (new_dir == NULL) {
> +        error_setg(errp, "Can't allocate space for bitmap directory.");
> +        return -ENOMEM;
> +    }
> +
> +    dir_size = s->bitmap_directory_size;
> +    dir = directory_read(bs, s->bitmap_directory_offset,
> +                         s->bitmap_directory_size, errp);

...and here.

Alternatively, of course, you can just drop the variable and use
s->bitmap_directory_size everywhere.

> +    if (dir == NULL) {
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    for_each_bitmap_dir_entry(e, dir, dir_size) {
> +        uint32_t fl = e->flags;
> +
> +        if (fl & BME_FLAG_AUTO) {
> +            BdrvDirtyBitmap *bitmap = load_bitmap(bs, e, errp);
> +            if (bitmap == NULL) {
> +                ret = -EINVAL;
> +                goto out;
> +            }
> +        } else {
> +            /* leave bitmap in the image */
> +            new_pos += copy_dir_entry(new_pos, e);
> +            new_nb_bitmaps++;
> +        }
> +    }
> +
> +    if (!can_write(bs)) {

You should be setting ret here (gcc complains about that).

> +        goto out;
> +    }
> +
> +    ret = directory_update(bs, new_dir, new_pos - new_dir, new_nb_bitmaps);
> +    if (ret < 0) {
> +        error_setg(errp, "Can't update bitmap directory.");

And once more, please no full stops at the end of error messages.

> +        goto out;
> +    }
> +
> +    /* to be consistent, free bitmap only after successfull directory update */

*successful

> +    for_each_bitmap_dir_entry(e, dir, dir_size) {
> +        uint32_t fl = e->flags;
> +
> +        if (fl & BME_FLAG_AUTO) {
> +            free_bitmap_clusters(bs, e);
> +        }
> +    }
> +
> +out:
> +    g_free(dir);
> +    g_free(new_dir);
> +
> +    return ret;
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 08c4ef9..02ec224 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -213,6 +213,11 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>              s->bitmap_directory_size =
>                      bitmaps_ext.bitmap_directory_size;
>  
> +            ret = qcow2_read_bitmaps(bs, errp);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +

I think I'd put this directly into qcow2_open(), just like
qcow2_read_snapshots(); but that's an optional suggestion.

Max

>  #ifdef DEBUG_EXT
>              printf("Qcow2: Got dirty bitmaps extension:"
>                     " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
> diff --git a/block/qcow2.h b/block/qcow2.h
> index c068b2c..482a29f 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -603,6 +603,9 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
>  void qcow2_free_snapshots(BlockDriverState *bs);
>  int qcow2_read_snapshots(BlockDriverState *bs);
>  
> +/* qcow2-bitmap.c functions */
> +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp);
> +
>  /* qcow2-cache.c functions */
>  Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
>  int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c);
> 



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