Re: [Qemu-block] [PATCH v22 19/30] qcow2: add persistent dirty bitmaps support

[Max Reitz]

On 2017-06-30 19:47, Eric Blake wrote:
> On 06/29/2017 09:23 PM, Max Reitz wrote:
>> On 2017-06-30 04:18, Eric Blake wrote:
>>> On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Store persistent dirty bitmaps in qcow2 image.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> Reviewed-by: Max Reitz <address@hidden>
>>>> ---
> 
>>>
>>> This grabs the size (currently in sectors, although I plan to fix it to
>>> be in bytes)...
>>>
>>>> +    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));
>>>
>>> ...then finds out how many bytes are required to serialize the entire
>>> image, where bm_size should be the same as before...
>>>
>>>> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
>>>> +        uint64_t cluster = sector / sbc;
>>>> +        uint64_t end, write_size;
>>>> +        int64_t off;
>>>> +
>>>> +        sector = cluster * sbc;
>>>> +        end = MIN(bm_size, sector + sbc);
>>>> +        write_size =
>>>> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - 
>>>> sector);
>>>
>>> But here, rather than tackling the entire image at once, you are
>>> subdividing your queries along arbitrary lines. But nowhere do I see a
>>> call to bdrv_dirty_bitmap_serialization_align() to make sure your
>>> end-sector value is properly aligned; if it is not aligned, you will
>>> trigger an assertion failure here...
>>
>> It's 4:21 am here, so I cannot claim to be right, but if I remember
>> correctly, it will automatically aligned because sbc is the number of
>> bits (and thus sectors) a bitmap cluster covers.
> 
> Okay, I re-read the spec.  First thing I noticed: we have a potential
> conflict if an image is allowed to be resized:

It isn't, though. (At least currently qemu won't allow it.)

> "All stored bitmaps are related to the virtual disk stored in the same
> image, so each bitmap size is equal to the virtual disk size."
> 
> If you resize an image, does the bitmap size have to be adjusted as
> well?

Sure.

>        What if you create one bitmap, then take an internal snapshot,
> then resize?

v3 images store the virtual disk size for each snapshot. So resizing an
image leaves snapshots unaffected.

Now bitmaps are not (yet) tied to snapshots. The spec thus says that the
bitmaps always correspond to the active state (it lists "snapshot
switching" as something that should dirty dirty bitmaps).

Therefore, if you then resize (which currently is doubly forbidden,
because qemu won't allow you to resize images with bitmaps or
snapshots), you would have to resize the bitmap, too.

In the future, we might want to allow binding bitmaps to snapshots. Then
the bitmap would not be resized but you couldn't see it anymore unless
you go back to the snapshot (which would bring the image to its original
size).

>               Or do we declare that (at least for now) the presence of a
> bitmap is incompatible with the use of an internal snapshot?

Well, that's not what the spec says. It clearly lists "snapshot
switching" as something that dirty bitmaps track.

> Conversely, we state that:
> 
> 
> "Structure of a bitmap directory entry:
> ...
>          8 - 11:    bitmap_table_size
>                     Number of entries in the bitmap table of the bitmap."
> 
> Since a bitmap therefore tracks its own size, I think the earlier
> statement that all bitmap sizes are equal to the virtual disk size is
> too strict (there seems to be no technical reason why a bitmap can't
> have a different size that the image).

But a logical. qcow2 is not a container format, it's an image format. I
for one am very opposed to storing bitmaps in a qcow2 file which have no
immediate connection to that virtual disk.

A reason I can see myself supporting would be that you could tie bitmaps
to snapshots and then they can have a size that differs from the one of
the active layer.

Max

> But, having read that, you are correct that we are subdividing our
> bitmaps according to what fits in a qcow2 cluster, and the smallest
> qcow2 cluster we can come up with is 512-bytes (or 4k bits of the
> bitmap).  Checking hbitmap.c, we are merely asserting that our alignment
> is always a multiple of 64 << hb->granularity.  But since we are
> partitioning a cluster at a time, our minimum alignment will be 512 <<
> hb->granularity, which is always aligned.
> 
> So all the more I need to do is add an assertion.
> 
>> I'm for fixing it later. I would have announced the series "applied" an
>> hour ago if I hadn't had to bisect iotest 055 breakage...
> 
> I'm working it into my v4 posting of dirty-bitmap cleanups.
>

signature.asc
Description: OpenPGP digital signature