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

[John Snow]

On 3/1/19 2:57 PM, Eric Blake wrote:
> 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,
> 

(Though I am being a little fast and loose here: when we split a bitmap,
the top-level one that retains the name actually stays unchanging and
the child bitmap is the one that starts accruing writes from a blank
canvas, but that STILL may not be what you expect when you choose it as
a merge source, however.)

>>
>> 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>
> 

We could always split it back out later, but in basic terms for
permissions and user perspective, "in use" seems robust enough of a
resolution. (It might be safe to read, it might not be, who knows --
it's in use.)

If it really comes to a point, we can always re-add a new status bit to
let the end-user know if they're working with a bifurcated (I have to
use weird vocabulary words sometimes) bitmap but at the moment it seems
very safely an implementation detail.

You can also check that for "enabled" bitmap as reported back to user
via QAPI I check to see if the parent OR child is enabled and report
that cumulatively as "enabled", because together they are "effectively"
enabled.

--js