Re: [Qemu-devel] [PATCH v6 00/10] block: incremental backup transactions

[John Snow <jsnow@redhat.com>]

On 06/05/2015 05:52 PM, John Snow wrote:
> This series adds incremental backup related commands to the QMP
> transaction subsystem in order to accomplish some slightly more
> sophisticated bitmap and backup management tasks that require
> atomic actions.
> 
> Patch 1 adds basic support for add and clear transactions.
> Patch 2 tests this basic support.
> Patches 3-4 refactor transactions a little bit, to add clarity.
> Patch 5 adds the framework for error scenarios where only
>     some jobs that were launched by a transaction complete successfully,
>     and we need to perform context-sensitive cleanup after the transaction
>     itself has already completed.
> Patches 6 adds necessary bookkeeping information to bitmap
>     data structures to take advantage of this new feature.
> Patch 7 modifies qmp_drive_backup to support the new feature.
> Patch 8 implements the new feature for drive_backup transaction actions.
> Patch 9 tests the new feature.
> Patch 10 updates documentation.
> 
> ===
> v6:
> ===
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/10:[----] [--] 'qapi: Add transaction support to block-dirty-bitmap operations'
> 002/10:[----] [--] 'iotests: add transactional incremental backup test'
> 003/10:[----] [--] 'block: rename BlkTransactionState and BdrvActionOps'
> 004/10:[----] [--] 'block: re-add BlkTransactionState'
> 005/10:[0016] [FC] 'block: add transactional callbacks feature'
> 006/10:[0010] [FC] 'block: add delayed bitmap successor cleanup'
> 007/10:[----] [--] 'qmp: Add an implementation wrapper for qmp_drive_backup'
> 008/10:[0002] [FC] 'block: drive_backup transaction callback support'
> 009/10:[----] [--] 'iotests: 124 - transactional failure test'
> 010/10:[----] [--] 'qmp-commands.hx: Update the supported 'transaction' operations'
> 
> 05: Replaced CallbackFn typedef by existing BlockCompletionFunc
> 06: Assert that hbitmap_merge will work, instead of testing it
>     Add some (successor_refcnt == 0) assertions while we're at it
> 08: Fallout from 05.
> 
> ===
> v5:
> ===
> 
> 04: Renamed "jobs" to "refcnt" in BlkTransactionState
> 05: Fallout from above changes.
> 06: Removed a lot of errp parameters from the BdrvDirtyBitmap helpers
>     Renamed "bdrv_dirty_bitmap_incref" to "bdrv_frozen_bitmap_incref"
> xx: Deleted job refcount patch.
> 07: Removed separate cb/opaque nullchecks in favor of a single check
> 08: Broadly, the job reference counting was removed.
>     drive_backup_prepare should now read much more cleanly.
>     Removed (state->job == bs->job) assertion
>     Moved two different bs accesses under aio_context_acquire()
> 10: Included Kashyap's new documentation patch.
> 
> ===
> v4:
> ===
> 
> 01: New approach for clear transaction.
>     bdrv_clear_dirty_bitmap (and undo_clear) moved to block_int.h
>     Removed more outdated documentation from bitmaps.md
> 03: Fallout from adding a _clear_abort() method.
> 11: New documentation patch from Kashyap.
> 
> ===
> v3:
> ===
> 
> 01: Removed "(Not yet implemented)" line from bitmaps.md.
>     Kept R-Bs, like a selfish person would.
> 02: Rebased on latest transactionless series.
>     Added some transaction helpers.
> 05: Removed superfluous deletion loop in put_blk_action_state.
> 08: Rebased without the preceding code motion patch.
>     Re-added forward declaration for do_drive_backup.
> 09: Fixed commit message whitespace.
>     Re-added block_job_cb forward declaration to avoid code motion patch.
> 10: Rebased on latest transactionless series;
>     Added "wait_qmp_backup" function to complement "do_qmp_backup."
>     General bike-shedding.
> 
> ===
> v2:
> ===
> 
>  01: Fixed indentation.
>      Fixed QMP commands to behave with new bitmap_lookup from
>        transactionless-v4.
>      2.3 --> 2.4.
>  02: Folded in improvements to qemu-iotest 124 from transactional-v1.
>  03: NEW
>  04: NEW
>  05: A lot:
>      Don't delete the comma in the transaction actions config
>      use g_new0 instead of g_malloc0
>      Phrasing: "retcode" --> "Return code"
>      Use GCC attributes to mark functions as unused until future patches.
>      Added some data structure documentation.
>      Many structure and function renames, hopefully to improve readability.
>      Use just one list for all Actions instead of two separate lists.
>      Remove ActionState from the list upon deletion/decref
>      And many other small tweaks.
>  06: Comment phrasing.
>  07: Removed errp parameter from all functions introduced by this commit.
>      bdrv_dirty_bitmap_decref --> bdrv_frozen_bitmap_decref
>  08: NEW
>  09: _drive_backup --> do_drive_backup()
>      Forward declarations removed.
>  10: Rebased on top of drastically modified #05.
>      Phrasing: "BackupBlockJob" instead of "BackupJob" in comments.
>  11: Removed extra parameters to wait_incremental() in
>        test_transaction_failure()
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch incremental-transactions
> https://github.com/jnsnow/qemu/tree/incremental-transactions
> 
> This version is tagged incremental-transactions-v6:
> https://github.com/jnsnow/qemu/releases/tag/incremental-transactions-v6
> 
> John Snow (9):
>   qapi: Add transaction support to block-dirty-bitmap operations
>   iotests: add transactional incremental backup test
>   block: rename BlkTransactionState and BdrvActionOps
>   block: re-add BlkTransactionState
>   block: add transactional callbacks feature
>   block: add delayed bitmap successor cleanup
>   qmp: Add an implementation wrapper for qmp_drive_backup
>   block: drive_backup transaction callback support
>   iotests: 124 - transactional failure test
> 
> Kashyap Chamarthy (1):
>   qmp-commands.hx: Update the supported 'transaction' operations
> 
>  block.c                    | 103 +++++++--
>  block/backup.c             |  40 ++--
>  blockdev.c                 | 553 +++++++++++++++++++++++++++++++++++++++------
>  docs/bitmaps.md            |   6 +-
>  include/block/block.h      |  11 +-
>  include/block/block_int.h  |  19 ++
>  qapi-schema.json           |   6 +-
>  qmp-commands.hx            |  21 +-
>  tests/qemu-iotests/124     | 174 +++++++++++++-
>  tests/qemu-iotests/124.out |   4 +-
>  10 files changed, 812 insertions(+), 125 deletions(-)
> 

NACK

Stefan has proposed a new series that uses mutexes (mutices?) in the
BlockJob to accomplish a similar transactional cleanup barrier with a
bit less code and is likely the correct way forward.

--js