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

[John Snow]

On 3/1/19 2:44 PM, Eric Blake wrote:
> On 3/1/19 1:15 PM, John Snow wrote:
>> We didn't do any state checking on source bitmaps at all,
>> so this adds inconsistent and busy checks. readonly is
>> allowed, so you can still copy a readonly bitmap to a new
>> destination to use it for operations like drive-backup.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  block/dirty-bitmap.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
> 
> 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?

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.

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