Re: [PATCH v4 11/21] parallels: Add dirty bitmaps saving

[Alexander Ivanov]

On 1/18/24 14:27, Denis V. Lunev wrote:
> On 12/28/23 11:12, Alexander Ivanov wrote:
> > Now dirty bitmaps can be loaded but there is no their saving. Add 
> > code for
> > dirty bitmap storage.
> >
> > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > ---
> >   block/parallels-ext.c | 168 ++++++++++++++++++++++++++++++++++++++++++
> >   block/parallels.c     |  16 +++-
> >   block/parallels.h     |   5 ++
> >   3 files changed, 187 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/parallels-ext.c b/block/parallels-ext.c
> > index b4e14c88f2..c83d1ea393 100644
> > --- a/block/parallels-ext.c
> > +++ b/block/parallels-ext.c
> > @@ -24,6 +24,7 @@
> >    */
> >     #include "qemu/osdep.h"
> > +#include "qemu/error-report.h"
> >   #include "qapi/error.h"
> >   #include "block/block-io.h"
> >   #include "block/block_int.h"
> > @@ -300,3 +301,170 @@ out:
> >         return ret;
> >   }
> > +
> > +static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs,
> > +                                               BdrvDirtyBitmap *bitmap,
> > +                                               uint8_t **buf, int 
> > *buf_size)
> Do we need a error?
I think no. We save bitmaps in a loop. If some bitmap has a problem it 
would be better
just to  print an error message and try to save other bitmaps.
> > +{
> > +    BDRVParallelsState *s = bs->opaque;
> > +    ParallelsFeatureHeader *fh;
> > +    ParallelsDirtyBitmapFeature *bh;
> > +    uint64_t *l1_table, l1_size, granularity, limit;
> I would say that 'limit' here means 'bits_in_cluster'
>
> We are writing the new code and I would prefer if we
> would have bits, bytes, clusters, sectors etc are
> clearly seen in variable names. It is quite complex
> to track various measurables.
OK
> > +    int64_t bm_size, ser_size, offset, buf_used;
> > +    int64_t alloc_size = 1;
> > +    const char *name;
> > +    uint8_t *bm_buf;
> > +    QemuUUID uuid;
> > +    int ret = 0;
> > +
> > +    if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
> > +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
> > +        return;
> > +    }
> > +
> > +    bm_size = bdrv_dirty_bitmap_size(bitmap);
> > +    granularity = bdrv_dirty_bitmap_granularity(bitmap);
> > +    limit = 
> > bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
> > +    ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, 
> > bm_size);
> > +    l1_size = DIV_ROUND_UP(ser_size, s->cluster_size);
> > +
> > +    buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh);
> As far as I can see, bdrv_dirty_bitmap_serialization_size() returns 
> bytes.
> That is correct. Thus multiplying it by 8 seems fatal mistake.
l1_size is amount of entries in l1_table. Every entry has 8 bytes size 
and corresponds
to one cluster containing a part of a bitmap.
> I am also quite unsure that we should roundup to the cluster, that
> will occupy more clusters than needed. Can you please take a look
> here
> https://src.openvz.org/users/ibazhitov/repos/ploop/browse/tools/ploop-cbt.c 
Why shouldn't we? We need to have l1_table which should be able to contain
all the bitmap data. Note: there is not a ROUND_UP(), but DIV_ROUND_UP().
> > +    /* Check if there is enough space for the final section */
> > +    if (*buf_size - buf_used < sizeof(*fh)) {
> > +        return;
> > +    }
> > +
> > +    name = bdrv_dirty_bitmap_name(bitmap);
> > +    ret = qemu_uuid_parse(name, &uuid);
> > +    if (ret < 0) {
> > +        error_report("Can't save dirty bitmap: ID parsing error: 
> > '%s'", name);
> > +        return;
> > +    }
> > +
> > +    fh = (ParallelsFeatureHeader *)*buf;
> > +    bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh));
> bh = fh + 1 ?
> > +    l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
> l1_table = bh + 1 ?
Yes, it's better.
> > +
> > +    fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
> > +    fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
> I am quite concerned here. Please compare with
>
> int save_dirty_bitmap(int devfd, struct delta *delta, off_t offset,
>         void *buf, __u32 *size, void *or_data, writer_fn wr,
>         void *data)
> {
>     int ret = 0;
>     struct ploop_pvd_header *vh;
>     size_t block_size;
>     __u64 bits, bytes, *p;
>     __u32 byte_granularity;
>     void *block;
>     struct ploop_pvd_dirty_bitmap_raw *raw = (struct 
> ploop_pvd_dirty_bitmap_raw *)buf;
>     char x[50];
>
>     vh = (struct ploop_pvd_header *)delta->hdr0;
>
>     /* granularity and uuid */
>     if ((ret = cbt_get_dirty_bitmap_metadata(devfd, raw->m_Id, 
> &raw->m_Granularity)))
>         return ret;
>     raw->m_Granularity /= SECTOR_SIZE;
>
>     block_size = vh->m_Sectors * SECTOR_SIZE;
>     if (p_memalign((void **)&block, 4096, block_size))
>         return SYSEXIT_MALLOC;
>
>     raw->m_Size = vh->m_SizeInSectors_v2;
>
>     byte_granularity = raw->m_Granularity * SECTOR_SIZE;
>     bits = ((raw->m_Size + raw->m_Granularity - 1) / raw->m_Granularity);
>     bytes = (bits + 7) >> 3;
>     raw->m_L1Size = (bytes + block_size - 1) / block_size;
>
> which means that the header is rotten. In that case can you pls
> take a look why this has not been caught by tests?
It looks the same if block_size is cluster size in bytes.
bytes - bitmap size in bytes
raw->m_L1Size - bitmap size in clusters

In my code fh->data_size is l1_table size (8 bytes entry for each bitmap 
cluster) plus
dirty bitmap feature header.
> > +
> > +    bh->l1_size = cpu_to_le32(l1_size);
> > +    bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS);
> > +    bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS);
> > +    memcpy(bh->id, &uuid, sizeof(uuid));
> > +
> > +    bm_buf = qemu_blockalign(bs, s->cluster_size);
> > +
> > +    offset = 0;
> > +    while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, 
> > bm_size)) >= 0) {
> > +        uint64_t idx = offset / limit;
> > +        int64_t cluster_off, end, write_size;
> > +
> > +        offset = QEMU_ALIGN_DOWN(offset, limit);
> > +        end = MIN(bm_size, offset + limit);
> > +        write_size = bdrv_dirty_bitmap_serialization_size(bitmap, 
> > offset,
> > +                                                          end - 
> > offset);
> > +        assert(write_size <= s->cluster_size);
> > +
> > +        bdrv_dirty_bitmap_serialize_part(bitmap, bm_buf, offset, end 
> > - offset);
> > +        if (write_size < s->cluster_size) {
> > +            memset(bm_buf + write_size, 0, s->cluster_size - 
> > write_size);
> > +        }
> > +
> > +        cluster_off = parallels_allocate_host_clusters(bs, 
> > &alloc_size);
> > +        if (cluster_off <= 0) {
> > +            goto end;
> > +        }
>
>
>
> > +
> > +        ret = bdrv_pwrite(bs->file, cluster_off, s->cluster_size, 
> > bm_buf, 0);
> > +        if (ret < 0) {
> > +            memset(&fh->magic, 0, sizeof(fh->magic));
> > +            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
> > +                                  cluster_off, 1);
> That is incomplete. You have to clean all clusters inside of the
> extension.
OK
> > +            goto end;
> > +        }
> > +
> > +        l1_table[idx] = cpu_to_le64(cluster_off >> BDRV_SECTOR_BITS);
> > +        offset = end;
> > +    }
> > +
> > +    *buf_size -= buf_used;
> > +    *buf += buf_used;
> > +
> > +end:
> > +    qemu_vfree(bm_buf);
> > +}
> > +
> > +void GRAPH_RDLOCK
> > +parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error 
> > **errp)
> > +{
> > +    BDRVParallelsState *s = bs->opaque;
> > +    BdrvDirtyBitmap *bitmap;
> > +    ParallelsFormatExtensionHeader *eh;
> > +    int remaining = s->cluster_size;
> > +    uint8_t *buf, *pos;
> > +    int64_t header_off, alloc_size = 1;
> > +    g_autofree uint8_t *hash = NULL;
> > +    size_t hash_len = 0;
> > +    int ret;
> > +
> > +    s->header->ext_off = 0;
> > +
> > +    if (!bdrv_has_named_bitmaps(bs)) {
> > +        return;
> > +    }
> > +
> > +    buf = qemu_blockalign0(bs, s->cluster_size);
> > +
> > +    eh = (ParallelsFormatExtensionHeader *)buf;
> > +    pos = buf + sizeof(*eh);
> > +
> > +    eh->magic = cpu_to_le64(PARALLELS_FORMAT_EXTENSION_MAGIC);
> > +
> > +    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
> > +        parallels_save_bitmap(bs, bitmap, &pos, &remaining);
> > +    }
> > +
> > +    header_off = parallels_allocate_host_clusters(bs, &alloc_size);
> > +    if (header_off < 0) {
> > +        error_report("Can't save dirty bitmap: cluster allocation 
> > error");
> > +        ret = header_off;
> > +        goto end;
> > +    }
> > +
> > +    ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_MD5,
> > +                             (const char *)(buf + sizeof(*eh)),
> > +                             s->cluster_size - sizeof(*eh),
> > +                             &hash, &hash_len, errp);
> > +    if (ret < 0 || hash_len != sizeof(eh->check_sum)) {
> > +        error_report("Can't save dirty bitmap: hash error");
> > +        ret = -EINVAL;
> > +        goto end;
> > +    }
> > +    memcpy(eh->check_sum, hash, hash_len);
> > +
> > +    ret = bdrv_pwrite(bs->file, header_off, s->cluster_size, buf, 0);
> > +    if (ret < 0) {
> > +        error_report("Can't save dirty bitmap: IO error");
> > +        parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
> > +                              header_off, 1);
> > +        goto end;
> > +    }
> > +
> > +    s->header->ext_off = cpu_to_le64(header_off / BDRV_SECTOR_SIZE);
> > +end:
> > +    qemu_vfree(buf);
> > +}
> > +
> > +bool coroutine_fn 
> > parallels_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
> > +                                                          const char 
> > *name,
> > + uint32_t granularity,
> > +                                                          Error **errp)
> > +{
> > +    if (bdrv_find_dirty_bitmap(bs, name)) {
> > +        error_setg(errp, "Bitmap already exists: %s", name);
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > diff --git a/block/parallels.c b/block/parallels.c
> > index f38dd2309c..a49922c6a7 100644
> > --- a/block/parallels.c
> > +++ b/block/parallels.c
> > @@ -1466,14 +1466,25 @@ fail:
> >   static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
> >   {
> >       BDRVParallelsState *s = bs->opaque;
> > +    Error *err = NULL;
> >       int ret;
> >   +    parallels_store_persistent_dirty_bitmaps(bs, &err);
> > +    if (err != NULL) {
> For me this looks quite odd, indirect check for error. Not good
Will add return value;
> > +        error_reportf_err(err, "Lost persistent bitmaps during "
> > +                          "inactivation of node '%s': ",
> > +                          bdrv_get_device_or_node_name(bs));
> > +    }
> > +
> >       s->header->inuse = 0;
> >       parallels_update_header(bs);
> >         /* errors are ignored, so we might as well pass exact=true */
> > -    ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, 
> > true,
> > -                        PREALLOC_MODE_OFF, 0, NULL);
> > +    ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS,
> > +                        true, PREALLOC_MODE_OFF, 0, NULL);
> > +    if (ret < 0) {
> > +        error_report("Failed to truncate image: %s", strerror(-ret));
> > +    }
> >         return ret;
> >   }
> > @@ -1525,6 +1536,7 @@ static BlockDriver bdrv_parallels = {
> >       .bdrv_co_pdiscard           = parallels_co_pdiscard,
> >       .bdrv_co_pwrite_zeroes      = parallels_co_pwrite_zeroes,
> >       .bdrv_inactivate            = parallels_inactivate,
> > +    .bdrv_co_can_store_new_dirty_bitmap = 
> > parallels_co_can_store_new_dirty_bitmap,
> >   };
> >     static void bdrv_parallels_init(void)
> > diff --git a/block/parallels.h b/block/parallels.h
> > index 493c89e976..9db4f5c908 100644
> > --- a/block/parallels.h
> > +++ b/block/parallels.h
> > @@ -101,5 +101,10 @@ int64_t GRAPH_RDLOCK 
> > parallels_allocate_host_clusters(BlockDriverState *bs,
> >   int GRAPH_RDLOCK
> >   parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
> >                                   Error **errp);
> > +void GRAPH_RDLOCK
> > +parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error 
> > **errp);
> > +bool coroutine_fn
> > +parallels_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const 
> > char *name,
> > +                                        uint32_t granularity, Error 
> > **errp);
> >     #endif

--
Best regards,
Alexander Ivanov