From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56233) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGxjc-0001Os-HU for qemu-devel@nongnu.org; Thu, 29 Jan 2015 17:38:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGxjZ-0005Ne-3n for qemu-devel@nongnu.org; Thu, 29 Jan 2015 17:38:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53248) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGxjY-0005N8-Q7 for qemu-devel@nongnu.org; Thu, 29 Jan 2015 17:38:45 -0500 Message-ID: <54CAB672.8060301@redhat.com> Date: Thu, 29 Jan 2015 17:38:42 -0500 From: John Snow MIME-Version: 1.0 References: <1421080265-2228-1-git-send-email-jsnow@redhat.com> In-Reply-To: <1421080265-2228-1-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v11 00/13] block: Incremental backup series List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com Just a ping. There has been some feedback here, but I am hesitant to roll out a v12 until there is a little more consensus from Eric Blake and Markus Armbruster on the parameter naming issue, as well as a cursory overview by Stefan or Kevin of the "successor" methodology. --js On 01/12/2015 11:30 AM, 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(-) >