Re: [Qemu-block] [Qemu-devel] [PATCH] dirty-bitmaps: allow merging to disabled bitmaps

[John Snow]

On 09/19/2018 04:58 PM, Eric Blake wrote:
> On 9/19/18 2:58 PM, John Snow wrote:
>> We wish to prohibit merging to read-only bitmaps and frozen bitmaps,
>> but "disabled" bitmaps only preclude their recording of live, new
>> information. It does not prohibit them from manual writes at the behest
>> of the user, as is the case for merge operations.
> 
> In particular, if I have a linear sequence of bitmaps,
> bitmap1 (disabled) tracking sectors touched during times T1-T2
> bitmap2 (disabled) tracking sectors touched during T2-T3
> bitmap3 (enabled) tracking sectors touched during T3-present
> 
> and later decide that I no longer care about time T2, but may still want
> to create a differential backup against time T1, then the logical action
> is to merge bitmap1 and bitmap2 into a single bitmap tracking T1-T3.
> Whether I keep the name bitmap1 (b1 as dst, b2 as src, delete b2 on
> completion), bitmap2 (b1 as src, b2 as dst, delete b1 on completion)
> or pick yet another name (create new disabled map b12, merge b1 into
> b12, merge b2 into b12, delete b1 and b2) is less important - the point
> remains that I am trying to merge into a disabled bitmap.  If a bitmap
> has to be enabled to perform the merge, then any guest I/O during the
> window in time where it is temporarily enabled will perhaps spuriously
> set bits corresponding to sectors not actually touched during T1-T3.
> 
> Perhaps it can be worked around with a transaction that does
> bitmap-enable, bitmap-merge, bitmap-disable - but that seems like a lot
> of overhead (and does it actually prevent guest I/O from happening
> during the transaction?), compared to just allowing the merge.
> 
>>
>> Reported-by: Eric Blake <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   block/dirty-bitmap.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index c9b8a6fd52..fa7e75e0af 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -798,7 +798,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap
>> *dest, const BdrvDirtyBitmap *src,
>>         qemu_mutex_lock(dest->mutex);
>>   -    assert(bdrv_dirty_bitmap_enabled(dest));
>> +    assert(!bdrv_dirty_bitmap_frozen(dest));
>>       assert(!bdrv_dirty_bitmap_readonly(dest));
> 
> At any rate, the fact that the deleted assertion was triggerable by QMP
> actions is a good reason to change it.  Here's how I triggered it, if
> you want to beef up the commit message:
> 

If you stage it, please feel free to amend it to add relevant details. I
was relying maybe too heavily on inference.

> $ qemu-img create -f qcow2 file 64k
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio
> {'execute':'qmp_capabilities'}
> {'execute':'blockdev-add', 'arguments':{'driver':'qcow2',
>  'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}}
> {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp',
>  'name':'b0'}}
> {'execute':'transaction', 'arguments':{'actions':[
>  {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp',
>   'name':'b0'}},
>  {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp',
>   'name':'b1'}}]}}
> {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp',
>  'name':'tmp'}}
> {'execute':'x-block-dirty-bitmap-disable', 'arguments':{'node':'tmp',
>  'name':'tmp'}}
> {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp',
>  'src_name':'b0', 'dst_name':'tmp'}}
> qemu-system-x86_64: block/dirty-bitmap.c:801: bdrv_merge_dirty_bitmap:
> Assertion `bdrv_dirty_bitmap_enabled(dest)' failed.
> 
> However, are we sure that the remaining assertions are properly flagged
> by callers, and that we aren't leaving any other lingering QMP crashers?

There's only one caller of merge, we should be OK.
(Am I misunderstanding your question?)

>  Let's see if I can modify my example
> 
> $ qemu-img create -f qcow2 -b file -F qcow2 file2
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio
> {'execute':'qmp_capabilities'}
> {'execute':'blockdev-add', 'arguments':{'driver':'qcow2',
>  'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}}
> {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp',
>  'name':'b0'}}
> {'execute':'nbd-server-start', 'arguments':{'addr':{'type':'inet',
>  'data':{'host':'localhost', 'port':'10809'}}}}
> {'execute':'blockdev-add', 'arguments':{'driver':'qcow2',
>  'node-name':'fleece', 'file':{'driver':'file', 'filename':'file2'},
>   'backing':'tmp'}}
> {'execute':'transaction', 'arguments':{'actions':[
>  {'type':'blockdev-backup', 'data':{'job-id':'backup', 'device':'tmp',
>   'target':'fleece', 'sync':'none'}},
>  {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp', 'name':'b1'}},
>  {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp',
>   'name':'b0'}}
>  ]}}
> {'execute':'nbd-server-add', 'arguments':{'device':'fleece'}}
> {'execute':'x-nbd-server-add-bitmap', 'arguments':{'name':'fleece',
>  'bitmap':'b0'}}
> {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp',
>  'src_name':'b1', 'dst_name':'b0'}}
> 
> Prior to your patch, that asserts; but after your patch, it silently
> succeeds. Shouldn't a bitmap be marked frozen while it is advertised
> over an NBD export? We already require such a bitmap to be disabled
> before you can attach it, and you REALLY don't want a read-only export
> to be seeing changes to block status information due to merges. And then
> we need to make sure that we give a proper error message rather than an
> assertion when using QMP to attempt to merge into a frozen bitmap. But
> that's probably an independent patch, so:
> 
> Reviewed-by: Eric Blake <address@hidden>
> 

So, it's not other callers we're worried about but new contexts that
were previously forbidden.

"Freezing" a bitmap has traditionally had somewhat of a specific
connotation in the code, which is that:

1. Users are prohibited from interacting with it at all through QMP,
generally. There might be exceptions, for things like query.

2. That bitmap is invisibly split into two bitmaps for the purposes of
transactional guarantees -- this means it acts as both an "enabled"
bitmap and a "disabled" bitmap simultaneously (it actually has one of
each under the hood!)

3. The bitmap is in use by a block job operation.

I'm not sure if we need that invisible-split mechanism for NBD pulls
(which are generally happening on fleeced nodes?) but it would certainly
be good to prohibit users from touching it in nearly any way until the
operation ends.


.... hmm, in this case it looks as if perhaps that merge ought to
respect that "QMP Locked" behavior.

We can do a V2 here, I think.

--js