Re: [Qemu-devel] [PATCH 06/17] qcow2-dirty-bitmap: add qcow2_dirty_bitmap_load()

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 07.10.2015 02:01, John Snow wrote:
>
> On 09/05/2015 12:43 PM, Vladimir Sementsov-Ogievskiy wrote:
>> This function loads block dirty bitmap from qcow2.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-dirty-bitmap.c | 155 +++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c              |   2 +
>>   block/qcow2.h              |   5 ++
>>   include/block/block_int.h  |   5 ++
>>   4 files changed, 167 insertions(+)
>>
>> diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c
>> index 1260d1d..ea50137 100644
>> --- a/block/qcow2-dirty-bitmap.c
>> +++ b/block/qcow2-dirty-bitmap.c
>> @@ -99,6 +99,13 @@ static int check_constraints(int cluster_size,
>>       return fail ? -EINVAL : 0;
>>   }
>>   
>> +static QCowDirtyBitmapHeader *bitmap_header(BDRVQcowState *s,
>> +                                            QCowDirtyBitmap *bitmap)
>> +{
> BDRVQcow2State here and everywhere below, again.
>
>> +    return (QCowDirtyBitmapHeader *)
>> +           (s->dirty_bitmap_directory + bitmap->offset);
>> +}
>> +
>>   static int directory_read(BlockDriverState *bs)
>>   {
>>       int ret;
>> @@ -195,3 +202,151 @@ out:
>>       }
>>       return ret;
>>   }
>> +
>> +static QCowDirtyBitmap *find_dirty_bitmap_by_name(BlockDriverState *bs,
>> +                                                  const char *name)
>> +{
>> +    BDRVQcowState *s = bs->opaque;
>> +    QCowDirtyBitmap *bm, *end = s->dirty_bitmaps + s->nb_dirty_bitmaps;
>> +
>> +    for (bm = s->dirty_bitmaps; bm < end; ++bm) {
>> +        if (strcmp(bm->name, name) == 0) {
>> +            return bm;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
> Whoops. This says to me we really need to prohibit bitmaps with the same
> name from being stored in the same file, and mention this in the spec,
> and test for it on load.
>
> Perhaps we can create a hash-table and fail verification on open if
> there's a collision. We can then use that hash-table here for
> find_dirty_bitmap_by_name to speed up lookup since we already went
> through the trouble of loading it.
>
> Might help for large cases where we're approaching 64K bitmaps, will not
> be too big of a performance hit for casual use.

So, it (hash table approach) may be implemented later

>
>> +/* dirty sectors in cluster is a number of sectors in the image, corresponding
>> + * to one cluster of bitmap data */
>> +static uint64_t dirty_sectors_in_cluster(const BDRVQcowState *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);
>> +}
>> +
>> +/* load_bitmap()
>> + * load dirty bitmap from Dirty Bitmap Table
>> + * Dirty Bitmap Table entries are assumed to be in big endian format */
>> +static int load_bitmap(BlockDriverState *bs,
>> +                       const uint64_t *dirty_bitmap_table,
>> +                       uint32_t dirty_bitmap_table_size,
>> +                       BdrvDirtyBitmap *bitmap)
>> +{
>> +    int ret = 0;
>> +    BDRVQcowState *s = bs->opaque;
>> +    uint64_t sector, dsc;
>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> I found some of this hard to unwind, bear with me:
>
> AKA, the number of sectors that bitmap tracks ...
>
>> +    int cl_size = s->cluster_size;
>> +    uint8_t *buf = NULL;
>> +    uint32_t i, tab_size =
>> +            size_to_clusters(s, bdrv_dirty_bitmap_data_size(bitmap, bm_size));
>> +
> bdrv_dirty_bitmap_data_size(bitmap, COUNT) calculates for us how much
> actual real size the lowest level of the hbitmap actually takes.
>
> Then size_to_clusters tells us how many clusters we need to store that,
> and therefore should map back to be the same as the predicted value,
> dirty_bitmap_table_size.
>
>> +    if (tab_size > dirty_bitmap_table_size) {
>> +        return -EINVAL;
>> +    }
>> +
> I assume this is not == because the real table size might have padding
> or other such things, but if the calculated tab size is bigger than the
> actual then we have a problem.
>
> But I think that you've passed in "birty_ditmap_table_size" as the total
> byte count of the table, but "tab_size" is computed here as the number
> of entries. I think you should multiply tab_size by uint64_t and test if
> they're equal.
>
>> +    bdrv_clear_dirty_bitmap(bitmap);
>> +
> Clear takes the aio_context for the associated BDS and then releases it...
>
>> +    buf = g_malloc0(cl_size);
>> +    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 = be64_to_cpu(dirty_bitmap_table[i]);
>> +
>> +        if (offset & DBM_TABLE_ENTRY_RESERVED_MASK) {
>> +            ret = -EINVAL;
>> +            goto finish;
>> +        }
>> +
>> +        /* zero offset means cluster unallocated */
>> +        if (offset) {
>> +            ret = bdrv_pread(bs->file, offset, buf, cl_size);
>> +            if (ret < 0) {
>> +                goto finish;
>> +            }
>> +            bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, end);
> ...but at this point, I believe we're editing this bitmap without its
> associated lock, which might be a problem when we go to add QMP commands
> later.


in the next version load_bitmap is changed a lot, and now not using 
part-serialization.


>> +        }
>> +    }
>> +    ret = 0;
>> +
>> +    bdrv_dirty_bitmap_deserialize_finish(bitmap);
>> +
>> +finish:
>> +    g_free(buf);
>> +
>> +    return ret;
>> +}
>> +
>> +BdrvDirtyBitmap * qcow2_dirty_bitmap_load(BlockDriverState *bs_for,
>> +                                          BlockDriverState *bs_file,
>> +                                          const char *name,
>> +                                          Error **errp)
>> +{
>> +    BDRVQcowState *s = bs_file->opaque;
>> +    int ret;
>> +    QCowDirtyBitmap *bm;
>> +    QCowDirtyBitmapHeader *bmh;
>> +    uint64_t *dirty_bitmap_table = NULL;
>> +    uint32_t granularity;
>> +    uint64_t size = bdrv_nb_sectors(bs_for);
>> +    BdrvDirtyBitmap *bitmap = NULL;
>> +
>> +    bm = find_dirty_bitmap_by_name(bs_file, name);
>> +    if (bm == NULL) {
>> +        error_setg(errp, "Could not find bitmap '%s' in the node '%s'", name,
>> +                   bdrv_get_device_or_node_name(bs_file));
>> +        return NULL;
>> +    }
>> +    bmh = bitmap_header(s, bm);
>> +
>> +    if (size != bmh->nb_virtual_bits) {
>> +        error_setg(errp,
>> +                   "Bitmap '%s' in the node '%s' has size = %" PRIu64
>> +                   "when requested size (for node %s) = %" PRIu64,
>> +                   name, bdrv_get_device_or_node_name(bs_file),
>> +                   bmh->nb_virtual_bits,
>> +                   bdrv_get_device_or_node_name(bs_for), size);
>> +        return NULL;
>> +    }
>> +
>> +
>> +    dirty_bitmap_table = g_try_malloc(bmh->dirty_bitmap_table_size * sizeof(uint64_t));
>> +    if (dirty_bitmap_table == NULL) {
>> +        error_setg_errno(errp, -ENOMEM, "Could not allocate Dirty Bitmap Table");
>> +        return NULL;
>> +    }
>> +
>> +    ret = bdrv_pread(bs_file->file, bmh->dirty_bitmap_table_offset, dirty_bitmap_table,
>> +                     bmh->dirty_bitmap_table_size * sizeof(uint64_t));
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Could not read dirty_bitmap_table table from image");
>> +        goto finish;
>> +    }
>> +
>> +    granularity = BDRV_SECTOR_SIZE << bmh->granularity_bits;
>> +    bitmap = bdrv_create_dirty_bitmap(bs_for, granularity, name, errp);
>> +    if (bitmap == NULL) {
>> +        error_setg_errno(errp, -ENOMEM, "Could not create dirty bitmap");
> why -ENOMEM? create can fail for a number of reasons ... since we've
> been given an errp parameter in this function, and we can trust
> bdrv_create_dirty_bitmap to have set it, we can just return NULL here
> and the caller can check errp to see what went wrong.
>
>> +        goto finish;
>> +    }
>> +
> Do we need to mark this bitmap as temporarily unusable until we complete
> the load? I guess not in the context of bdrv_open at boot time ...

Also, keep in mind that now we have only bitmaps, related to the same 
bds which contains it.

>
>> +    ret = load_bitmap(bs_file, dirty_bitmap_table, bmh->dirty_bitmap_table_size, bitmap);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Could not read bitmap from image");
>> +        goto finish;
>> +    }
>> +
>> +finish:
>> +    if (*errp != NULL) {
>> +        bdrv_release_dirty_bitmap(bs_for, bitmap);
>> +        bitmap = NULL;
>> +    }
>> +    g_free(dirty_bitmap_table);
>
> I think we're not supposed to be reaching into errp to check its
> implementation detail like this ... the usual paradigm I see is just
> "goto fail" or similar statements instead of checking for
> error-or-success in a shared return block.
>
> finish:
>    g_free(dirty_bitmap_table);
>    return bitmap;
> fail:
>    g_free(dirty_bitmap_table);
>    bdrv_release_dirty_bitmap(bs_for, bitmap);
>    return NULL;

ok

>
>> +
>> +    return bitmap;
>> +}
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 76c331b..58ebdd3 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2965,6 +2965,8 @@ 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_save_vmstate    = qcow2_save_vmstate,
>>       .bdrv_load_vmstate    = qcow2_load_vmstate,
>>   
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 5016fa1..51d1907 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -608,6 +608,11 @@ int qcow2_read_snapshots(BlockDriverState *bs);
>>   void qcow2_free_dirty_bitmaps(BlockDriverState *bs);
>>   int qcow2_read_dirty_bitmaps(BlockDriverState *bs);
>>   
>> +BdrvDirtyBitmap *qcow2_dirty_bitmap_load(BlockDriverState *bs_for,
>> +                                         BlockDriverState *bs_file,
>> +                                         const char *name,
>> +                                         Error **errp);
>> +
>>   /* 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 14ad4c3..f982adc 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -204,6 +204,11 @@ struct BlockDriver {
>>       int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
>>       ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
>>   
>> +    BdrvDirtyBitmap *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs_for,
>> +                                               BlockDriverState *bs_file,
>> +                                               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,
>>
> Looking good, thanks!
> --js


-- 
Best regards,
Vladimir