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

[John Snow <jsnow@redhat.com>]

On 01/30/2015 05:24 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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

Fair enough, it is inconsistent. I was mostly following Fam's lead for 
his prototypes, so I could go either way with it.

Before I cull the unused parameters, though:

Fam, do we have a strong reason for why we want to maintain the unused 
BDS parameter for some of these functions? (Like making sure a caller 
HAS a BDS before they call these functions?)

Thanks,
--js

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

-- 
—js