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

Reply via email to