Re: [Qemu-devel] [PATCH v11 00/13] block: Incremental backup series

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

About added functions for BdrvDirtyBitmap:

some functions has needless BlockDriverState* parameter, and others - 
doesn't:

with needless *bs:
bdrv_dirty_bitmap_make_anon
bdrv_dirty_bitmap_granularity
bdrv_clear_dirty_bitmap

without *bs:
bdrv_dirty_bitmap_enabled
bdrv_disable_dirty_bitmap
bdrv_enable_dirty_bitmap

I think, to be consistent, onlly one of two ways should be chosen:
1) all bdrv_dirty_bitmap_* functions has "BlockDriverState* bs" 
parameter, maybe needless
2) only bdrv_dirty_bitmap_* function that need "BlockDriverState* bs" 
parameter has it

Personally, I prefer the second one.

Best regards,
Vladimir

On 12.01.2015 19:30, John Snow wrote:
> Welcome to version 11. I hope you are enjoying our regular newsletter.
>
> This patchset enables the in-memory part of the incremental backup
> feature. A patchset by Vladimir Sementsov-Ogievskiy enables the
> migration of in-memory dirty bitmaps, and a future patchset will
> enable the storage and retrieval of dirty bitmaps to and from permanent
> storage.
>
> Enough changes have been made that most Reviewed-By lines from
> previous iterations have been removed. (Sorry!)
>
> This series was originally authored by Fam Zheng;
> his cover letter is included below.
>
> ~John Snow
>
> =================================================================
>
> This is the in memory part of the incremental backup feature.
>
> With the added commands, we can create a bitmap on a block
> backend, from which point of time all the writes are tracked by
> the bitmap, marking sectors as dirty. Later, we call drive-backup
> and pass the bitmap to it, to do an incremental backup.
>
> See the last patch which adds some tests for this use case.
>
> Fam
>
> =================================================================
>
> For convenience, this patchset is available on github:
>                  https://github.com/jnsnow/qemu/commits/dbm-backup
>
> v11:
>
>   - Instead of copying BdrvDirtyBitmaps and keeping a pointer to the
>     object we were copied from, we instead "freeze" a bitmap in-place
>     without copying it. On success, we thaw and delete the bitmap.
>     On failure, we merge the bitmap with a "successor," which is an
>     anonymous bitmap attached as a child that records writes for us
>     for the duration of the backup operation.
>
>     This means that incremental backups can NEVER BE RETRIED in a
>     deterministic fashion. If an incremental backup fails on a set
>     of dirty sectors {x}, and a new set of dirty sectors {y} are
>     introduced during the backup, then any possible retry action
>     on an incremental backup can only operate on {x,y}. There is no
>     way to get an incremental backup "as it would have been."
>
>     So, the failure mode for incremental backup is to try again,
>     and the resulting image will simply be a differential from the
>     last successful dirty bitmap backup.
>
>   - Removed hbitmap_copy and bdrv_dirty_bitmap_copy.
>
>   - Added a small fixup patch:
>     - Update all granularity fields to be uint64_t.
>     - Update documentation around BdrvDirtyBitmap structure.
>
>   - Modified transactions to obey frozen attribute of BdrvDirtyBitmaps.
>
>   - Added frozen attribute to the info query.
>
> v10 (Overview):
>
>   - I've included one of Vladimir's bitmap fixes as patch #1.
>
>   - QMP commands and transactions are now protected via
>     aio_context functions.
>
>   - QMP commands use "node-ref" as a parameter name now. Reasoning
>     is thus: Either a "device name" or a "node name" can be used to
>     reference a BDS, which is the key item we are actually acting
>     on with these bitmap commands. Thus, I refer to this unified
>     parameter as a "Node Reference," or "node-ref."
>
>     We could also argue for "backend-ref" or "device-ref" for the
>     reverse semantics: where we accept a unified parameter, but we
>     intend to resolve it to the BlockBackend instead of resolving
>     the parameter given to the BlockDriverState.
>
>     Or, we could use "reference" for both cases, and to match the
>     existing BlockdevRef command.
>
>   - All QMP commands added are now per-node via a unified parameter
>     name. Though backup only operates on "devices," you are free to
>     create bitmaps for any arbitrary node you wish. It is obviously
>     only useful for the root node, currently.
>
>   - Bitmap Sync modes (CONSUME, RESET) are removed. See below
>     (changelog, RFC questions) for more details.
>
>   - Code to recover the bitmap after a failure has been added,
>     but I have some major questions about drive_backup guarantees.
>     See RFC questions.
>
> v10 (Detailed Changelog):
>
>     (1/13) New Patch:
>   - Included Vladimir Sementsov-Ogievskiy's patch that clarifies
>     the semantics of the bitmap primitives.
>
>     (3/13):
>   - Edited function names for consistency (Stefanha)
>   - Removed compile-time constants (Stefanha)
>   - Acquire aio_context for bitmap add/remove (Stefanha)
>   - Documented optional granularity for bitmap-add in
>     qmp-commands.hx file (Eric Blake)
>   - block_dirty_bitmap_lookup moved forward to this patch to allow
>     more re-use. (Stefanha)
>   - Fixed a problem where the block_dirty_bitmap_lookup didn't
>     always set an error if it returned NULL.
>   - Added an optional return BDS lookup parameter to
>     bdrv_dirty_bitmap_lookup.
>   - Renamed "device" to "node-ref" for block_dirty_bitmap_lookup,
>     adjusted calls to bdrv_lookup_bs() to reflect unified
>     parameter usage.
>   - qmp_block_dirty_bitmap_{add,remove} now reference arbitrary
>     node names via @node-ref.
>
>     (4/13):
>   - Default granularity and granularity getters are both
>     now uint64_t (Stefanha)
>
>     (5/13):
>   - Added documentation to warn about the necessity of updating
>     the hbitmap deep copy. (Stefanha)
>
>     (6/13)
>   - Renamed bdrv_reset_dirty_bitmap to bdrv_clear_dirty_bitmap
>     to be consistent with Vladimir's patches.
>   - Removed const qualifier for bdrv_copy_dirty_bitmap,
>     to accommodate patch 8.
>
>     (7/13) New Patch:
>   - Added an hbitmap_merge operation to join two bitmaps together.
>
>     (8/13) New Patch:
>   - Added bdrv_reclaim_dirty_bitmap() to allow us to "roll back" a
>     bitmap into the bitmap that it was spawned from. This will let
>     us maintain an accurate account of dirty sectors even after a
>     failure.
>   - This adds an "originator" pointer to the BdrvDirtyBitmap and
>     is why "const" was removed for copy.
>
>     (9/13):
>   - QMP semantics changes as outlined for Patch #3.
>   - Enable/Disable now protected by aio_context (Stefanha)
>
>     (10/13):
>   - Add coroutine_fn annotation to block backup helper. (Stefanha)
>   - Removed sync_bitmap_gran from BackupBlockJob, just use the
>     getter on the BdrvDirtyBitmap instead. (Stefanha)
>   - bdrv_dirty_iter_set was renamed to bdrv_set_dirty_iter.
>   - Calls to bdrv_reset_dirty_bitmap are modified to
>     bdrv_clear_dirty_bitmap to reflect the rename in patch #6.
>   - Bitmap usage modes (RESET vs. CONSUME) has been deleted, for
>     the reason of targeting a simpler core usage first before
>     targeting optimizations. CONSUME is particularly problematic
>     in the case of errors; so this mode is omitted for now.
>   - Adjusted error message to use MirrorSyncMode enum, not
>     (incorrectly) the BitmapSyncMode enum.
>   - In the event of a failure, the sync_bitmap is now merged back
>     into the original bitmap so that we do not lose any dirty
>     bitmap information needlessly.
>
>     (11/13):
>   - Changed block_dirty_bitmap_add_abort to use
>     qmp_block_dirty_bitmap_remove:
>      - Old code used bdrv_lookup_bs to acquire bitmap and bs
>        anyway, and ignored the failure case.
>      - New code relies on the same information, and with a NULL
>        errp pointer we will similarly ignore the failure case.
>        prepare() should have properly vetted this information
>        before this point anyway.
>        This new code is also now protected via aio_context through
>        the use of the QMP commands.
>   - More code re-use of block_dirty_bitmap_lookup to get both the
>     bs and bitmap pointers.
>   - aio_context protection and cleanup in new abort methods.
>
>     (13/13):
>   - Modified test for new parameter names.
>
> v9:
>   - Edited commit message, for English embetterment (02/10)
>   - Rebased on top of stefanha/block-next (06,08/10)
>   - Adjusted error message and line length (07/10)
>
> v8:
>   - Changed int64_t return for bdrv_dbm_calc_def_granularity to uint64_t (2/10)
>   - Updated commit message (2/10)
>   - Removed redundant check for null in device parameter (2/10)
>   - Removed comment cruft. (2/10)
>   - Removed redundant local_err propagation (several)
>   - Updated commit message (3/10)
>   - Fix HBitmap copy loop index (4/10)
>   - Remove redundant ternary (5/10)
>   - Shift up the block_dirty_bitmap_lookup function (6/10)
>   - Error messages cleanup (7/10)
>   - Add an assertion to explain the re-use of .prepare() for two transactions.
>     (8/10)
>   - Removed BDS argument from bitmap enable/disable helper; it was unused. (8/10)
>
> v7: (highlights)
>   - First version being authored by jsnow
>   - Addressed most list feedback from V6, many small changes.
>     All feedback was either addressed on-list (as a wontfix) or patched.
>   - Replaced all error_set with error_setg
>   - Replaced all bdrv_find with bdrv_lookup_bs()
>   - Adjusted the max granularity to share a common function with
>     backup/mirror that attempts to "guess" a reasonable default.
>     It clamps between [4K,64K] currently.
>   - The BdrvDirtyBitmap object now counts granularity exclusively in
>     bytes to match its interface.
>     It leaves the sector granularity concerns to HBitmap.
>   - Reworked the backup loop to utilize the hbitmap iterator.
>     There are some extra concerns to handle arrhythmic cases where the
>     granularity of the bitmap does not match the backup cluster size.
>     This iteration works best when it does match, but it's not a
>     deal-breaker if it doesn't -- it just gets less efficient.
>   - Reworked the transactional functions so that abort() wouldn't "undo"
>     a redundant command. They now have been split into a prepare and a
>     commit function (with state) and do not provide an abort command.
>   - Added a block_dirty_bitmap_lookup(device, name, errp) function to
>     shorten a few of the commands added in this series, particularly
>     qmp_enable, qmp_disable, and the transaction preparations.
>
> v6: Re-send of v5.
>
> v5: Rebase to master.
>
> v4: Last version tailored by Fam Zheng.
>
> ==
>
> Fam Zheng (7):
>    qapi: Add optional field "name" to block dirty bitmap
>    qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
>    block: Introduce bdrv_dirty_bitmap_granularity()
>    qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable
>    qapi: Add transaction support to block-dirty-bitmap-{add, enable,
>      disable}
>    qmp: Add dirty bitmap status fields in query-block
>    qemu-iotests: Add tests for drive-backup sync=dirty-bitmap
>
> John Snow (5):
>    block: Add bdrv_clear_dirty_bitmap
>    hbitmap: add hbitmap_merge
>    block: Add bitmap successors
>    qmp: Add support of "dirty-bitmap" sync mode for drive-backup
>    block: BdrvDirtyBitmap miscellaneous fixup
>
> Vladimir Sementsov-Ogievskiy (1):
>    block: fix spoiling all dirty bitmaps by mirror and migration
>
>   block.c                       | 254 +++++++++++++++++++++++++++++++++++++--
>   block/backup.c                | 120 +++++++++++++++----
>   block/mirror.c                |  27 ++---
>   blockdev.c                    | 273 +++++++++++++++++++++++++++++++++++++++++-
>   hmp.c                         |   3 +-
>   include/block/block.h         |  31 ++++-
>   include/block/block_int.h     |   2 +
>   include/qemu/hbitmap.h        |  11 ++
>   migration/block.c             |   7 +-
>   qapi-schema.json              |   5 +-
>   qapi/block-core.json          | 103 +++++++++++++++-
>   qmp-commands.hx               |  68 ++++++++++-
>   tests/qemu-iotests/056        |  33 ++++-
>   tests/qemu-iotests/056.out    |   4 +-
>   tests/qemu-iotests/iotests.py |   8 ++
>   util/hbitmap.c                |  28 +++++
>   16 files changed, 910 insertions(+), 67 deletions(-)
>