Re: [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

27.04.2021 21:24, John Snow wrote:
> On 4/27/21 7:11 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> It's a simpler alternative for
>> "[PATCH v4 0/5] block: add block-dirty-bitmap-populate job"
>>    <20200902181831.2570048-1-eblake@redhat.com>
>>    https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg00978.html
>>    https://patchew.org/QEMU/20200902181831.2570048-1-eblake@redhat.com/
>>
>> Since we have "coroutine: true" feature for qmp commands, I think,
>> maybe we can merge allocation status to bitmap without bothering with
>> new block-job?
>>
>> It's an RFC:
>>
>> 1. Main question: is it OK as a simple blocking command, even in a
>> coroutine mode. It's a lot simpler, and it can be simply used in a
>> transaction with other bitmap commands.
>>
> 
> Hm, possibly... I did not follow the discussion of coroutine QMP commands closely to know what the qualifying criteria to use them are.
> 
> (Any wisdom for me here, Markus?)
> 
>> 2. Transaction support is not here now. Will add in future version, if
>> general approach is OK.
>>
> 
> That should be alright, I think. It means that the operation needs to succeed before the transaction returns success, though.
> 
> Depending on what else is in the transaction, do we run the risk of a deadlock if we need to wait for a coroutine to finish?
> 
>> 3. I just do bdrv_co_enter() / bdrv_co_leave() like it is done in the
>> only coroutine qmp command - block_resize(). I'm not sure how much is it
>> correct.
>>
> 
> See above concern!
> 
>> 4. I don't do any "drain". I think it's not needed, as intended usage
>> is to merge block-status to _active_ bitmap. So all concurrent
>> operations will just increase dirtyness of the bitmap and it is OK.
>>
> 
> That sounds fine for individual usage, but I can't convince myself it's safe for transactions.

qmp_transaction do drain itself.. Still, it's a bit strange that it does just drain and not drained section around the whole logic.

> 
>> 5. Probably we still need to create some BdrvChild to avoid node resize
>> during the loop of block-status querying.
>>
> 
> I'm less sure that it's OK to cause temporary graph changes during the course of a blocking QMP function... but maybe that's OK?
> 
> Peter Krempa is the expert to consult on that one.
> 
>> 6. Test is mostly copied from parallels-read-bitmap, I'll refactor it in
>> next version to avoid copy-paste.
>>
>> 7. Probably patch 01 is better be split into 2-3 patches.
>>
>> Vladimir Sementsov-Ogievskiy (2):
>>    qapi: block-dirty-bitmap-merge: support allocation maps
>>    iotests: add allocation-map-to-bitmap
>>
>>   qapi/block-core.json                          | 31 ++++++++-
>>   include/block/block_int.h                     |  4 ++
>>   block/dirty-bitmap.c                          | 42 ++++++++++++
>>   block/monitor/bitmap-qmp-cmds.c               | 55 +++++++++++++---
>>   .../tests/allocation-map-to-bitmap            | 64 +++++++++++++++++++
>>   .../tests/allocation-map-to-bitmap.out        |  9 +++
>>   6 files changed, 195 insertions(+), 10 deletions(-)
>>   create mode 100755 tests/qemu-iotests/tests/allocation-map-to-bitmap
>>   create mode 100644 tests/qemu-iotests/tests/allocation-map-to-bitmap.out
>>
> 


-- 
Best regards,
Vladimir