Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate

[Max Reitz <mreitz@redhat.com>]

On 2015-03-03 at 17:48, John Snow wrote:
>
>
> On 03/03/2015 04:27 PM, Max Reitz wrote:
>> On 2015-03-03 at 16:24, John Snow wrote:
>>>
>>>
>>> On 03/03/2015 11:02 AM, Max Reitz wrote:
>>>> On 2015-03-02 at 18:20, John Snow wrote:
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>>   block.c                | 22 ++++++++++++++++++++
>>>>>   include/block/block.h  |  1 +
>>>>>   include/qemu/hbitmap.h | 10 +++++++++
>>>>>   util/hbitmap.c         | 55
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   4 files changed, 88 insertions(+)
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index e6b2696..5eaa874 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -3543,6 +3543,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t
>>>>> offset)
>>>>>       ret = drv->bdrv_truncate(bs, offset);
>>>>>       if (ret == 0) {
>>>>>           ret = refresh_total_sectors(bs, offset >> 
>>>>> BDRV_SECTOR_BITS);
>>>>> +        bdrv_dirty_bitmap_truncate(bs);
>>>>>           if (bs->blk) {
>>>>>               blk_dev_resize_cb(bs->blk);
>>>>>           }
>>>>> @@ -5562,6 +5563,27 @@ BdrvDirtyBitmap
>>>>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>>>>       return parent;
>>>>>   }
>>>>> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t
>>>>> size)
>>>>> +{
>>>>> +    /* Should only be frozen during a block backup job, which should
>>>>> have
>>>>> +     * blocked any resize actions. */
>>>>> +    assert(!bdrv_dirty_bitmap_frozen(bitmap));
>>>>> +    hbitmap_truncate(bitmap->bitmap, size);
>>>>> +}
>>>>> +
>>>>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>>>>> +{
>>>>> +    BdrvDirtyBitmap *bitmap;
>>>>> +    uint64_t size = bdrv_nb_sectors(bs);
>>>>> +
>>>>> +    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>>>>> +        if (bdrv_dirty_bitmap_frozen(bitmap)) {
>>>>> +            continue;
>>>>> +        }
>>>>
>>>> Hm!
>>>>
>>>> The assert() above doesn't do anything because this condition will 
>>>> just
>>>> skip over frozen bitmaps. Maybe we should drop it?
>>>>
>>>> I guess my R-b stands since bdrv_dirty_bitmap_frozen(bitmap) will 
>>>> always
>>>> be false anyway (hence the assertion), but it would stand with this
>>>> block dropped, too.
>>>>
>>>> Max
>>>>
>>>
>>> It's there for parity with the other bdrv helpers that perform a
>>> similar check. I am guarding against, in the future, any potential
>>> users from deciding to call this function directly for whichever 
>>> reason.
>>>
>>> Requesting an action on a drive that contains bitmaps that are not
>>> ready for the action is OK. Requesting an action on a bitmap that is
>>> not ready for the action should never happen, hence the 'documentation
>>> assert.'
>>>
>>> My default action will be to leave it in.
>>
>> Technically that's fine (which is why I said my R-b stands), my only
>> problem with that is that if something really strange happens and we
>> actually end up with a frozen bitmap here, the action we're taking is
>> wrong. The bitmap will have the wrong size after the BDS was truncated,
>> and that's not right.
>>
>
> If something really strange happens I would rather prefer the person 
> making the really strange patch to know sooner rather than later that 
> they're violating some assumptions made for incremental backups.

Me, too. That's why I'd like an assertion. :-)

> The only way the bitmap can be frozen is currently during a backup, 
> which should prevent any user-facing resize actions from the outset.

Right. This is why I'm (grudgingly) fine with it.

> If that changes, it's important that we know about it. truncating a 
> frozen bitmap is not a circumstance I want to entertain being possible.

That's why I'd like the assertion to do something instead of being 
skipped silently. :-)

>> I'd find just calling dirty_bitmap_truncate() regardless of the frozen
>> state more correct, the idea being "We want to resize all bitmaps,
>> because all bitmaps attached to this BDS need to be resized", and then,
>> in dirty_bitmap_truncate(), we notice that we cannot resize a frozen
>> bitmap. But we need to. So the assertion fails.
>>
>
> This is a good point, though. By the above reasoning, it's better to 
> force this whole command to fail exquisitely. Since we already do a 
> soft user check that should suffice up at qmp_drive_resize, we should 
> already be avoiding the assert() under normal operating conditions.

Right, an assert() is for cases that cannot happen (some call it "for 
contracts").

> I'd rather not re-spin for this one item, though ... If it's fine by 
> everyone else I'll tidy this up in the transaction patch that's going 
> out shortly.

That's completely fine with me.

Max