On 02/14/2017 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2017 03:38, John Snow wrote:
>>
>> On 02/03/2017 04:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Realize block bitmap storing interface, to allow qcow2 images store
>>> persistent bitmaps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>>> Reviewed-by: Max Reitz <mre...@redhat.com>
>>> ---
>>> block/qcow2-bitmap.c | 489
>>> +++++++++++++++++++++++++++++++++++++++++++++++++--
>>> block/qcow2.c | 1 +
>>> block/qcow2.h | 1 +
>>> 3 files changed, 476 insertions(+), 15 deletions(-)
>>>
>
> [...]
>
>>> +
>>> +static void clear_bitmap_table(BlockDriverState *bs, uint64_t
>>> *bitmap_table,
>>> + uint32_t bitmap_table_size)
>>> +{
>>> + BDRVQcow2State *s = bs->opaque;
>>> + int i;
>>> +
>>> + for (i = 0; i < bitmap_table_size; ++i) {
>>> + uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK;
>>> + if (!addr) {
>>> + continue;
>>> + }
>>> +
>>> + qcow2_free_clusters(bs, addr, s->cluster_size,
>>> QCOW2_DISCARD_OTHER);
>>> + bitmap_table[i] = 0;
>>> + }
>>> +}
>>> +
>>> +static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapTable
>>> *tb,
>>> uint64_t **bitmap_table)
>> It would have been nicer to have factored out the size and offset from
>> the very beginning to avoid churn within this series, but... oh well.
>> Next time you write a 24 patch series, OK? :)
>
> Hmm... What do you mean?
>
Sorry, I left the comment in a weird place. I meant the refactoring
mid-series to create the Qcow2BitmapTable; it could have been done earlier.
It's not a problem, just a comment.
>>
>>> {
>>> int ret;
>>> @@ -152,20 +218,20 @@ static int bitmap_table_load(BlockDriverState
>>> *bs, Qcow2Bitmap *bm,
>>> uint32_t i;
>>> uint64_t *table;
>>> - assert(bm->table_size != 0);
>>> - table = g_try_new(uint64_t, bm->table_size);
>>> + assert(tb->size != 0);
>>> + table = g_try_new(uint64_t, tb->size);
>>> if (table == NULL) {
>>> return -ENOMEM;
>>> }
>>> - assert(bm->table_size <= BME_MAX_TABLE_SIZE);
>>> - ret = bdrv_pread(bs->file, bm->table_offset,
>>> - table, bm->table_size * sizeof(uint64_t));
>>> + assert(tb->size <= BME_MAX_TABLE_SIZE);
>>> + ret = bdrv_pread(bs->file, tb->offset,
>>> + table, tb->size * sizeof(uint64_t));
>>> if (ret < 0) {
>>> goto fail;
>>> }
>>> - for (i = 0; i < bm->table_size; ++i) {
>>> + for (i = 0; i < tb->size; ++i) {
>>> be64_to_cpus(&table[i]);
>>> ret = check_table_entry(table[i], s->cluster_size);
>>> if (ret < 0) {
>>> @@ -182,6 +248,28 @@ fail:
>>> return ret;
>>> }
>>>
>
> [...]
>
>>> +
>>> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>> Error **errp)
>>> +{
>>> + BdrvDirtyBitmap *bitmap;
>>> + BDRVQcow2State *s = bs->opaque;
>>> + uint32_t new_nb_bitmaps = s->nb_bitmaps;
>>> + uint64_t new_dir_size = s->bitmap_directory_size;
>>> + int ret;
>>> + Qcow2BitmapList *bm_list;
>>> + Qcow2Bitmap *bm;
>>> + Qcow2BitmapTableList drop_tables;
>>> + Qcow2BitmapTable *tb, *tb_next;
>>> +
>>> + QSIMPLEQ_INIT(&drop_tables);
>>> +
>>> + if (!can_write(bs)) {
>>> + error_setg(errp, "No write access");
>>> + return;
>>> + }
>>> +
>>> + if (!bdrv_has_persistent_bitmaps(bs)) {
>>> + /* nothing to do */
>>> + return;
>>> + }
>>> +
>>> + if (s->nb_bitmaps == 0) {
>>> + bm_list = bitmap_list_new();
>>> + } else {
>>> + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>>> + s->bitmap_directory_size, errp);
>> Oh, this isn't cached from the autoload mechanism? We have to re-create
>> this list every time we save?
>>
>> I suppose it's safest that way, but it's something we can likely
>> improve on.
>
> There is a patch fixing in v11 series - [PATCH 24/24] qcow2-bitmap:
> cache bitmap list in BDRVQcow2State
> We decided to apply it later.
>
OK, gotcha.
>
>>
>>> + if (bm_list == NULL) {
>>> + /* errp is already set */
>> Usually implicit when checking the return code from a function that
>> takes an errp parameter.
>>
>>> + return;
>>> + }
>>> + }
>>> +
>>> + /* check constraints and names */
>>> + for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
>>> + bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>>> + {
>> It occurs to me at this point that the framework you have established is
>> very dirty-bitmap heavy, even though we support other types of bitmaps.
>>
>> Not really a big problem, as when we go to support OTHER types of
>> bitmaps, we can just change the names of things as we need to.
>>
>> Just a comment.
>>
>>> + const char *name = bdrv_dirty_bitmap_name(bitmap);
>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>>> + Qcow2Bitmap *bm;
>>> +
>>> + if (!bdrv_dirty_bitmap_get_persistance(bitmap)) {
>>> + continue;
>>> + }
>>> +
>>> + if (check_constraints_on_bitmap(bs, name, granularity) < 0) {
>>> + error_setg(errp, "Bitmap '%s' doesn't satisfy the
>>> constraints",
>>> + name);
>>> + goto fail;
>>> + }
>> At this point I really do begin to become concerned that it will be very
>> easy for people to accidentally back themselves into a case where they
>> cannot save their bitmaps to disk, but will have no idea why that is
>> true because the error message is vague.
>>
>> I am not fully clear on how easy it would be to create a bitmap that
>> QEMU will accept but refuse to flush to disk for size reasons, but I
>> think it's fairly easy to create a name that will overcome the string
>> limit we've imposed.
>
> Actually unsupported bitmap should be caught on qmp-bitmap-add if
> persistent=true. So, user can't create it. The other source of bitmaps -
> autoloading bitmaps, but they should be ok and they are checked on load.
> Considered check is a paranoic one. But I think it should not be
> replaced with an assert.
>
Ah, okay, didn't get that far. Good enough!
>>
>> Can we make the error message here more descriptive for starters? (We
>> should make sure we can change the names of bitmaps too, so we can allow
>> people to fix their bitmaps.
>
> Ok, I'll add errp parameter to check_constraints_on_bitmap as a new patch.
>
Well, not strictly necessary if the QMP interface is guarding against it!
>>
>> (Can we begin imposing warnings or errors for people who make bitmaps
>> with names that are too big? 1023 should be enough for all current uses
>> of this feature, I'd hope.)
>>
>> CC eblake, QMP lawyer ...
>>
>>> +
>>> + bm = find_bitmap_by_name(bm_list, name);
>>> + if (bm == NULL) {
>>> + if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
>>> + error_setg(errp, "Too many persistent bitmaps");
>>> + goto fail;
>>> + }
>>> +
>>> + new_dir_size += calc_dir_entry_size(strlen(name), 0);
>>> + if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
>>> + error_setg(errp, "Too large bitmap directory");
>>> + goto fail;
>>> + }
>>> +
>> Suggest "Bitmap directory is too large" instead.
>>
>>> + bm = g_new0(Qcow2Bitmap, 1);
>>> + bm->name = g_strdup(name);
>>> + QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
>>> + } else {
>>> + if (!(bm->flags & BME_FLAG_IN_USE)) {
>>> + error_setg(errp, "Bitmap '%s' already exists in the
>>> image",
>>> + name);
>>> + goto fail;
>>> + }
>>> + tb = g_memdup(&bm->table, sizeof(bm->table));
>>> + bm->table.offset = 0;
>>> + bm->table.size = 0;
>> I guess this is so that updating the tables is 'safe'.
>>
>>> + QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry);
>>> + }
>>> + bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ?
>>> BME_FLAG_AUTO : 0;
>>> + bm->granularity_bits =
>>> ctz32(bdrv_dirty_bitmap_granularity(bitmap));
>>> + bm->dirty_bitmap = bitmap;
>>> + }
>>> +
>>> + /* allocate clusters and store bitmaps */
>>> + QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>> + if (bm->dirty_bitmap == NULL) {
>>> + continue;
>>> + }
>>> +
>>> + ret = store_bitmap(bs, bm, errp);
>>> + if (ret < 0) {
>>> + goto fail;
>>> + }
>>> + }
>>> +
>>> + ret = update_ext_header_and_dir(bs, bm_list);
>>> + if (ret < 0) {
>>> + error_setg_errno(errp, -ret, "Failed to update bitmap
>>> extension");
>>> + goto fail;
>>> + }
>>> +
>>> + /* Bitmap directory was successfully updated, so, old data can
>>> be dropped.
>>> + * TODO it is better to reuse these clusters */
>>> + QSIMPLEQ_FOREACH_SAFE(tb, &drop_tables, entry, tb_next) {
>>> + free_bitmap_clusters(bs, tb);
>>> + g_free(tb);
>>> + }
>>> +
>>> + bitmap_list_free(bm_list);
>>> + return;
>>> +
>>> +fail:
>>> + QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>> + if (bm->dirty_bitmap == NULL || bm->table.offset == 0) {
>>> + continue;
>>> + }
>>> +
>>> + free_bitmap_clusters(bs, &bm->table);
>>> + }
>>> +
>>> + QSIMPLEQ_FOREACH_SAFE(tb, &drop_tables, entry, tb_next) {
>>> + g_free(tb);
>>> + }
>>> +
>>> + bitmap_list_free(bm_list);
>>> +}
>> Looks good otherwise, but some clarification on the error messages and
>> that errant free explained will garner you the R-B.
>>
>> Thanks,
>> --js
>
>
Thanks, I'll continue reviewing from v14.
