From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51454) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHGah-0007OZ-18 for qemu-devel@nongnu.org; Fri, 30 Jan 2015 13:46:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YHGaa-0001wT-6R for qemu-devel@nongnu.org; Fri, 30 Jan 2015 13:46:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50150) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHGaZ-0001wJ-Sf for qemu-devel@nongnu.org; Fri, 30 Jan 2015 13:46:44 -0500 Message-ID: <54CBD191.1030909@redhat.com> Date: Fri, 30 Jan 2015 13:46:41 -0500 From: John Snow MIME-Version: 1.0 References: <1421080265-2228-1-git-send-email-jsnow@redhat.com> <54CB5BC3.8070301@parallels.com> In-Reply-To: <54CB5BC3.8070301@parallels.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: famz@redhat.com Cc: kwolf@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com, Vladimir Sementsov-Ogievskiy , stefanha@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=20 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=20 BDS parameter for some of these functions? (Like making sure a caller=20 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 permanen= t >> 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 >> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> >> 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 >> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> >> 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 patche= d. >> - 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 "und= o" >> 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. >> >> =3D=3D >> >> 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=3Ddirty-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(-) >> > --=20 =E2=80=94js