Re: [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[Eric Blake]

On 3/1/19 1:48 PM, John Snow wrote:

>> I understand forbidding inconsistent sources (because if the source is
>> potentially missing bits, then the merge destination will also be
>> missing bits and thus be inconsistent), but why forbid busy?  If I've
>> associated a bitmap with an NBD server (making it busy), it is still
>> readable, and so I should still be able to merge its bits into another copy.
>>
> 
> True, do you rely on this, though?

Not in my current libvirt code (as I create a temporary bitmap to hand
to NBD, since it may be the merge of one or more disabled bitmaps in a
differential backup case), so being tighter for now and relaxing later
if we DO come up with a use is acceptable.

> 
> I was working from a space of "busy" meant "actively in-use by an
> operation, and COULD change" so I was forbidding it out of good hygiene.
> 
> Clearly the ones in-use by NBD are actually static and unchanging, so
> it's safer -- but that might not be true for push backups, where you
> might not actually be getting what you think you are, because of the
> bifurcated nature of those bitmaps.

Oh, good point, especially after you worked so hard to merge
locked/frozen into a single status - you WILL miss the bits from the
successor (unless we teach the merge algorithm to pull in the busy
bitmap's bits AND all the bits of its successors - but that feels like a
lot of work if we don't have a client needing it now).  Okay, with the
extra justification mentioned in the commit message,

> 
> If this causes a problem for you in the short-term I will simply roll
> this back, but it stands out to me.
> 
> (I can't stop myself from trying to protect the user from themselves.
> It's clearly a recurring theme in my design and reviews.)
> 
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 769668ccdc..8403c9981d 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, 
>>> const BdrvDirtyBitmap *src,
>>>          goto out;
>>>      }
>>>  
>>> +    if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) {
>>
>> Thus, I think this should be BDRV_BITMAP_INCONSISTENT.

then I retract my complaint, and the code is acceptable for now.

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org