Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Vladimir Sementsov-Ogievskiy]

19.12.2016 18:34, Max Reitz wrote:
> On 19.12.2016 16:26, Vladimir Sementsov-Ogievskiy wrote:
> > 19.12.2016 18:14, Max Reitz wrote:
> > > On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
> > > > 09.12.2016 20:05, Max Reitz wrote:
> > > > > On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> > > > > > Realize block bitmap storing interface, to allow qcow2 images store
> > > > > > persistent bitmaps.
> > > > > >
> > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > > > > ---
> > > > > >    block/qcow2-bitmap.c | 451
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >    block/qcow2.c        |   1 +
> > > > [...]
> > > >
> > > > > > +
> > > > > > +/* store_bitmap_data()
> > > > > > + * Store bitmap to image, filling bitmap table accordingly.
> > > > > > + */
> > > > > > +static uint64_t *store_bitmap_data(BlockDriverState *bs,
> > > > > > +                                   BdrvDirtyBitmap *bitmap,
> > > > > > +                                   uint32_t *bitmap_table_size,
> > > > > > Error **errp)
> > > > > > +{
> > > > > > +    int ret;
> > > > > > +    BDRVQcow2State *s = bs->opaque;
> > > > > > +    int64_t sector;
> > > > > > +    uint64_t dsc;
> > > > > > +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> > > > > > +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
> > > > > > +    uint8_t *buf = NULL;
> > > > > > +    BdrvDirtyBitmapIter *dbi;
> > > > > > +    uint64_t *tb;
> > > > > > +    uint64_t tb_size =
> > > > > > +            size_to_clusters(s,
> > > > > > +                bdrv_dirty_bitmap_serialization_size(bitmap, 0,
> > > > > > bm_size));
> > > > > > +
> > > > > > +    if (tb_size > BME_MAX_TABLE_SIZE ||
> > > > > > +            tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) {
> > > > > Alignment to the opening parenthesis, please.
> > > > >
> > > > > > +        error_setg(errp, "Bitmap '%s' is too big", bm_name);
> > > > > > +        return NULL;
> > > > > > +    }
> > > > > > +
> > > > > > +    tb = g_try_new0(uint64_t, tb_size);
> > > > > > +    if (tb == NULL) {
> > > > > > +        error_setg(errp, "No memory");
> > > > > > +        return NULL;
> > > > > > +    }
> > > > > > +
> > > > > > +    dbi = bdrv_dirty_iter_new(bitmap, 0);
> > > > > > +    buf = g_malloc(s->cluster_size);
> > > > > > +    dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
> > > > > > +
> > > > > > +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
> > > > > > +        uint64_t cluster = sector / dsc;
> > > > > > +        uint64_t end, write_size;
> > > > > > +        int64_t off;
> > > > > > +
> > > > > > +        sector = cluster * dsc;
> > > > > > +        end = MIN(bm_size, sector + dsc);
> > > > > > +        write_size =
> > > > > > +            bdrv_dirty_bitmap_serialization_size(bitmap, sector,
> > > > > > end - sector);
> > > > > > +
> > > > > > +        off = qcow2_alloc_clusters(bs, s->cluster_size);
> > > > > > +        if (off < 0) {
> > > > > > +            error_setg_errno(errp, -off,
> > > > > > +                             "Failed to allocate clusters for
> > > > > > bitmap '%s'",
> > > > > > +                             bm_name);
> > > > > > +            goto fail;
> > > > > > +        }
> > > > > > +        tb[cluster] = off;
> > > > > Somehow I would feel better with either an assert(cluster < tb_size);
> > > > > here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the
> > > > > error
> > > > > handling for bdrv_nb_sectors()) above the loop.
> > > > assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
> > > > error handling. Right?
> > > Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
> > > necessarily a multiple of dsc. So that should be good. Alternatively, I
> > > think the following would be slightly easier to read:
> > >
> > > assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);
> > >
> > > > > > +
> > > > > > +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end
> > > > > > - sector);
> > > > > > +        if (write_size < s->cluster_size) {
> > > > > > +            memset(buf + write_size, 0, s->cluster_size -
> > > > > > write_size);
> > > > > > +        }
> > > > > Should we assert that write_size <= s->cluster_size?
> > > > Ok
> > > >
> > > > [...].
> > > >
> > > > > > +        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 (++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;
> > > > > > +        }
> > > > > You only need to increment new_nb_bitmaps and increase new_dir_size if
> > > > > the bitmap does not already exist in the image (i.e. if
> > > > > find_bitmap_by_name() below returns NULL).
> > > > Why? No, I need to check the whole sum and the whole size.
> > > If the bitmap already exists, you don't create a new directory entry but
> > > reuse the existing one. Therefore, the number of bitmaps in the image
> > > and the directory size will not grow then.
> > new_nb_bitmaps is not number of "newly created bitmaps", but just new
> > value of field nb_bitmaps, so, all bitmaps - old and new are calculated
> > into new_nb_bitmaps. Anyway, this misunderstanding shows that variable
> > name is bad..
> Yes. But when you store a bitmap of the same name as an existing one,
> you are replacing it. The number of bitmaps does not grow in that case.

Oh, I'm stupid)) I see now, you are right.

> Max


--
Best regards,
Vladimir