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

[John Snow <jsnow@redhat.com>]

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.

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