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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
