On 06/28/2017 08:05 AM, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > There is a new update of qcow2-bitmap series - v22. > > web: > https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=qcow2-bitmap-v22 > git: https://src.openvz.org/scm/~vsementsov/qemu.git (tag qcow2-bitmap-v22) >
Minor rebase conflicts, I staged a branch here: https://github.com/qemu/qemu/compare/master...jnsnow:review-vlad-22 As for the conflicts, they are: 08: Conflict when qed-gencb was removed; trivial fix. https://github.com/jnsnow/qemu/commit/ffa326ca1f8ee972dbaf2fd5ec1bd38c0ab1b3fd#diff-ddc19015eeeab7a5c73e0a51a8933e8fL2 10: 'count' was changed to 'bytes' which disrupted patch context. https://github.com/jnsnow/qemu/commit/19a6311ed09823ff2fb2b2bde4932cf53707c1c9#diff-02e65af103cfc4966c51c5aefe38da85L2272 Thanks, --js > v22: > > Rebase on master, so changes, mostly related to new dirty bitmaps mutex: > > 10: - asserts now in bdrv_{re,}set_dirty_bitmap_locked functions. > - also add assert into bdrv_undo_clear_dirty_bitmap (the only change, not > related to rebase) > - add mutex lock into bdrv_dirty_bitmap_set_readonly (as it changes > bitmap list, > so the lock should be taken) > - return instead of go-to in qmp_block_dirty_bitmap_clear > - in dirty-bitmaps.h, move bdrv_dirty_bitmap_set_readonly before block > "Functions that require manual locking", move > bdrv_dirty_bitmap_readonly and bdrv_has_readonly_bitmaps into this block > 15: - add mutex lock/unlock into bdrv_dirty_bitmap_set_autoload > - in dirty-bitmaps.h, move bdrv_dirty_bitmap_set_autoload before block > "Functions that require manual locking", move > bdrv_dirty_bitmap_get_autoload into this block > 17: - add mutex lock/unlock into bdrv_dirty_bitmap_set_persistance > - in dirty-bitmaps.h, move bdrv_dirty_bitmap_set_persistance before block > "Functions that require manual locking", move > bdrv_dirty_bitmap_get_persistance and > bdrv_has_changed_persistent_bitmaps into this block > 18: in dirty-bitmaps.h, move bdrv_dirty_bitmap_next into block > "Functions that require manual locking". (do not remove r-b, as it is > just one empty line removed before function declaration) > 23: return instead of go-to in qmp_block_dirty_bitmap_add > 24: return instead of go-to in qmp_block_dirty_bitmap_add > 25: - return instead of go-to > - remove aio_context_acquire/release calls > - no aio_context parameter for block_dirty_bitmap_lookup > - in dirty-bitmaps.h, move bdrv_dirty_bitmap_sha256 into block > "Functions that require manual locking". > 29: - return instead of go-to in qmp_block_dirty_bitmap_remove > > r-b's are dropped from 10,15,17,25. > > v21: > > 09,10: improve comment, add r-b's by Max and John > 10: improve comment,k > 11,12: add r-b by John > 13: prepend local_err with additional info (Max), add r-b by John > 14: add r-b's by Max and John > 20,30: add r-b by Max > > > v20: > > handle reopening images ro and rw. > > On reopening ro: store bitmaps (storing sets 'IN_USE'=0 in the image) > and mark them readonly (set readonly flag in BlockDirtyBitmap) > > After reopening rw: mark bitmaps IN_USE in the image > and unset readonly flag in BlockDirtyBitmap > > 09: new > 10: improve comment > add parameter 'value' to bdrv_dirty_bitmap_set_readonly > 11: use new parameter of bdrv_dirty_bitmap_set_readonly > 12-14, 20: new > > v19: > > rebased on master > > 05: move 'sign-off' over 'reviewed-by's > 08: error_report -> error_setg in qcow2_truncate (because of rebase) > 09: return EPERM in bdrv_aligned_pwritev and bdrv_co_pdiscard if there > are readonly bitmaps. EPERM is chosen because it is already used for > readonly image in bdrv_co_pdiscard. > Also handle readonly bitmap in block_dirty_bitmap_clear_prepare and > qmp_block_dirty_bitmap_clear > Max's r-b is not added > 10: fix grammar in comment > add Max's r-b > 12, 13, 15, 21: add Max's r-b > 24: fix grammar in comment > 25: fix grammar and wording in comment > also, I see contextual changes in inactiavate mechanism. Hope, they do not > affect these series. > > v18: > > rebased on master (sorry for v17) > > 08: contextual: qcow2_do_open is changed instead of qcow2_open > rename s/need_update_header/update_header/ in qcow2_do_open, to not do it > in 10 > save r-b's by Max and John > 09: new patch > 10: load_bitmap_data: do not clear bitmap parameter - it should be already > cleared > (it actually created before single load_bitmap_data() call) > if some bitmaps are loaded, but we can't write the image (it is readonly > or inactive), so we can't mark bitmaps "in use" in the image, mark > corresponding BdrvDirtyBitmap read-only. > change error_setg to error_setg_errno for "Can't update bitmap directory" > no needs to rename s/need_update_header/update_header/ here, as it done > in 08 > 13: function bdrv_has_persistent_bitmaps becomes > bdrv_has_changed_persistent_bitmaps, > to handle readonly field. > 14: declaration moved to the bottom of .h, save r-b's > 15: firstly check bdrv_has_changed_persistent_bitmaps and only then fail on > !can_write, and then QSIMPLEQ_INIT(&drop_tables) > skip readonly bitmaps in saving loop > 18: remove '#optional', 2.9 -> 2.10, save r-b's > 19: remove '#optional', 2.9 -> 2.10, save r-b's > 20: 2.9 -> 2.10, save r-b's > 21: add check of read-only image open, drop r-b's > 24: add comment to qapi/block-core.json, that block-dirty-bitmap-add removes > bitmap > from storage. r-b's by Max and John saved > > > v17: > 08: add r-b's by Max and John > 09: clear unknown autoclear features from BDRVQcow2State before calling > qcow2_load_autoloading_dirty_bitmaps(), and also do not extra update > header if it is updated by qcow2_load_autoloading_dirty_bitmaps(). > 11: new patch, splitted out from 16 > 12: rewrite commit message > 14: changing bdrv_close moved to separate new patch 11 > s/1/1ULL/ ; s/%u/%llu/ for two errors > 16: s/No/Not/ > add Max's r-b > 24: new patch > > > v16: > > mostly by Kevin's comments: > 07: just moved here, as preparation for merging refcounts to 08 > 08: move "qcow2-bitmap: refcounts" staff to this patch, to not break > qcow2-img check > drop r-b's > move necessary supporting static functions to this patch too (to not > break compilation) > fprintf -> error_report > other small changes with error messages and comments > code style > for qcow2-bitmap.c: > 'include "exec/log.h"' was dropped > s/1/(1U << 0) for BME_FLAG_IN_USE > add BME_TABLE_ENTRY_FLAG_ALL_ONES to replace magic 1 > don't check 'cluster_size <= 0' in check_table_entry > old "[PATCH v15 08/25] block: introduce auto-loading bitmaps" was dropped > 09: was "[PATCH v15 09/25] qcow2: add .bdrv_load_autoloading_dirty_bitmaps" > drop r-b's > some staff was moved to 08 > update_header_sync - error on bdrv_flush fail > rename disk_sectors_in_bitmap_cluster to sectors_covered_by_bitmap_cluster > and adjust comment. > so, variable for storing this function result: s/dsc/sbc/ > s/1/BME_TABLE_ENTRY_FLAG_ALL_ONES/ > also, as Qcow2BitmapTable already introduced in 08, > s/table_offset/table.offset/ and s/table_size/table.size, etc.. > update_ext_header_and_dir_in_place: add comments, add additional > update_header_sync, to reduce indeterminacy in case of error. > call qcow2_load_autoloading_dirty_bitmaps directly from qcow2_open > no .bdrv_load_autoloading_dirty_bitmaps > 11: drop bdrv_store_persistent_dirty_bitmaps at all. > drop r-b's > 13: rename patch, rewrite commit msg > drop r-b's > move bdrv_release_named_dirty_bitmaps in bdrv_close() after > drv->bdrv_close() > Qcow2BitmapTable is already introduced, so no > s/table_offset/table.offset/ and s/table_size/table.size, etc.. here > old 25 with check_constraints_on_bitmap() improvements merged here (Eric) > like in 09, s/dsc/sbc/ and > s/disk_sectors_in_bitmap_cluster/sectors_covered_by_bitmap_cluster/ > no .bdrv_store_persistent_dirty_bitmaps > call qcow2_store_persistent_dirty_bitmaps directly from qcow2_inactivate > 15: corresponding part of 25 merged here. Add John's r-b, as 25 was also > reviewed by John. > 16: add John's r-b > > > v15: > 13,14: add John's r-b > 15: qcow2_can_store_new_dirty_bitmap: > - check max dirty bitmaps and bitmap directory overhead > - switch to error_prepend > rm Max's r-b > not add John's r-b > 17-24: add John's r-b > 25: changed because 15 changed, > not add John's r-b > > > v14: > > 07: use '|=' to update need_update_header > add John's r-b > add Max's r-b > 09: remove unused bitmap_table_to_cpu() > left Max's r-b, hope it's ok > add John's r-b > 10: remove extra new line > add John's r-b > 11: add John's r-b > 12: add John's r-b > 13: small fixes by John's review: > - remove weird g_free of NULL pointer from > if (tb == NULL) { > g_free(tb); > return -EINVAL; > } > - remove extra comment "/* errp is already set */" > - s/"Too large bitmap directory"/"Bitmap directory is too large"/ > left Max's r-b, hope you don't mind > 22: add Max's r-b > 23: add Max's r-b > 24: add Max's r-b > 25: new patch to improve error message on check_constraints_on_bitmap fail > > > v13: Just a fix for style checker. > 13: line over 80 > 14: line over 80 > 22: s/if () \n{/if () {/ > > v12: > 07: do not update header in qcow2_read_extensions, instead do it in the > end of qcow2_open, where it is updated also to clear unknown > autoclear features. > Wrong bdrv_is_root_node used instead of bdrv_is_read_only is fixed > automatically. > 08: add Max's r-b > 09: contextual: s/raw_bsd/raw-format/ > qcow2-bitmap.c: copyright s/2016/2017/ > fix compilation: move update_header_sync() to this patch > add Max's r-b > 13: update_header_sync() is moved to 09 > add Max's r-b > 15: add Max's r-b > 16: As qmp-commands.txt is removed from master, remove it here too. > Sentence "For now only Qcow2 disks support persistent bitmaps. > Default is false." moved to qapi/block-core.json. > Hope that is OK, Max's r-b is not dropped. > 17: qmp-commands.txt deleted, r-b is not dropped too. > 18: s/is failed/has failed/, add Max's r-b > 19: copyright: s/2016/2017/ > 21: drop "res->corruptions > 0 => ret = -EINVAL", add Max's r-b > 22: actually, patch is replaced with a new one, however some parts are the > same. > instead of changing bdrv_release_dirty_bitmap behaviour, create new > bdrv_remove_persistent_dirty_bitmap > 23: improve comment, about not-exists is not an error > 24: new patch > > Old 24 (qcow2-bitmap: cache bitmap list in BDRVQcow2State) will be sent > separately, to be applied after these series. > > Also: about bdrv_dirty_bitmap_set_persistance: I've decided not change its > behaviour > for now and just call bdrv_remove_persistent_dirty_bitmap from > qmp_block_dirty_bitmap_remove. Reasons: > 1. Do not change reviewed part. > 2. I'm not sure, that this complication of BdrvDirtyBitmap.persistent > semantics > is good (current semantics are just: .persistent means that bitmap should > be > saved on disk close). .persistent actually is not very related to "is > there > stored version of the bitmap in the image". > 3. It may be done later. > 4. It is not actually needed for now, as the only usage is dropping bitmap in > bdrv_remove_persistent_dirty_bitmap. May be in future it will be needed, > when we will have qmp interfaces for finer control of persistent dirty > bitmaps. > > > v11: > > Fix automatic build test fail. > > 18: wording - "ASCII representation of SHA256 ..." > 24: fix redeclaration error. (This is still RFC, may be not needed patch, > see description in v10 below). > > v10: > > 07: rm John's r-b > not add Max's r-b > fix subject s/dirty bitmaps/bitmaps > improve WARNING message > remove header ext if autoclear bit is unset, through > qcow2_updata_header() - r-b blocking change > 08: move bdrv_load_autoloading_dirty_bitmaps under asserts block (Max) > fix typo in commit message > move bdrv_load_autoloading_dirty_bitmaps after asserts > John's r-b > 09: rewrite check_dir_entry > remove extra feature cluster_size=0 from check_table_entry > which clears autoclear bit before trying to update bitmap directory > bitmap_list_store - do not free old clusters if in_place=true > changes in comments, fix indents > add functions > bitmap_list_count > update_ext_header_and_dir_in_place (unset autoclear bit before trying to > modify bitmap directory) > (for usage in qcow2_load_autoloading_dirty_bitmaps) > 11: add node name to error report > Max's r-b > 13: add type Qcow2BitmapTable > move from bm.table_* to bm.table.* > rewrite check_constraints_on_bitmap > flush(bs->file) instead of flush(bs) in update_ext_header_and_dir > assert(DIV_ROUND_UP(bm_size, dsc) == tb_size); > and assert(write_size <= s->cluster_size); in store_bitmap_data > fix: s/tb_size/tb_size * sizeof(tb[0]) in store_bitmap > in qcow2_store_persistent_dirty_bitmaps: > fail if !can_write > fix counting of new_nb_bitmaps and new_dir_size > free old clusters _after_ successful directory update (aggregate old > bitmap > tables into drop_tables) > 2 14: rename to can_store_new_dirty_bitmap > Max's r-b > 15: rename to can_store_new_dirty_bitmap > rm extra !! > 16: Max's r-b > rename to can_store_new_dirty_bitmap > indenting > Since s/2.8/2.9 > 17: Max's r-b > Since s/2.8/2.9 > 18: hashing can fail in comment for qapi > Since s/2.8/2.9 > remove dead comment > 19: indentation > Max's r-b > 20: Max's r-b > 21: fix error handling > return 0 if nb_bitmaps == 0 > new patches: > 22-23: remove bitmap from file on bdrv_release_dirty_bitmap > 24: experimental, RFC: cache bitmap list to bs, to not reload it from file. > I'm not sure that is needed now, but if you want I can merge it to earlier > patches. > > v9: > > rebase on master! > > 01-04,06,07: John's r-b > 03: rewording in comment ("The concurrent resetting ..."), John's r-b > 07: reword error messages > commit message: dirty bitmap -> bitmap > 09: fix uninitialized ret in bitmap_list_store() (fix compilation warning) > 18: fix compilation of tests/test-hbitmap > > also, change constants names for qcow2: DIRTY_BITMAP -> BITMAP (as it is not > dirty bitmaps extension, but just bitmaps extension), QCOW -> QCOW2 (bitmaps > are only for qcow2) > > v8: > > Patches 01-06 are mostly unchanged, except for spelling and wording. > 02 - add Eric's r-b > 03,04 - add Max's r-b > > 07-09 is refactored "bitmap-read" part of the feature. Main changes: > - introduce bitmap list - normal list, which represents bitmap directory. > Function bitmap_list_load loads bitmap directory, checks everything and > creates normal list. > - introduce .bdrv_load_autoloading_dirty_bitmaps : instead of loading bitmaps > in qcow2_open, lets load them only in bdrv_open, to avoid reloading in > bdrv_snapshot_goto > - do not delete bitmaps after read, but mark them IN_USE > > 10 has contextual changes and rewording of comment. I've added Max's r-b, > hope it's ok. > > 11: add error_report("Persistent bitmaps are lost") in case of failed bitmap > store > > 12: add Max's r-b > > 13 is refactored "bitmap-store" part of the feature. see 07-09 description > - for now I just free old clusters and allocate new. This will be improved > with a separate patch. > > patch about "delete bitmaps on truncate" is removed. This case will be > handled later. > IN_USE, autoclear, check-constraints things are merged into other patches. > > 14-15 are mew patches, to early check possibility of creating persistent > bitmap with > specified name and granularity in specified BDS > > 16-17: spelling, rewording, indenting, tiny code simplifying, check can_store > (by 14-15), > s/if (autoload && !persistent)/if (has_autoload && !persistent)/, > rebase (deleted qmp-commands.hx) > > 18: alternative to md5 in query-block: > - separated qmp command -x-debug-block-dirty-bitmap-sha256 > - as adviced by Daniel P. Berrange in my parallel thread: > - sha256 instead of md5 > - use qcrypto_hash_* instead of GChecksum > - fix bug =) (size was wrong in hbitmap_md5) > > 19: s/3999/3fff/, use x-debug-block-dirty-bitmap-sha256 > > 20: new patch to rename and publish inc_refcounts > > 21: some fixes and refactoring mostyly by Max's comments. > > Max, Eric, great tanks for your review! > I hope, I've covered most of your comments by this update. > > TODO: > - handle reopening image RO->RW and incoming migration, set IN_USE for > existing loaded bitmaps > in these cases. > - reuse old, already allocated data clusters for bitmaps storing > - truncate bitmaps in the image on truncate > > > v7: > > https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fqcow2-bitmap-v7 > based on block-next (https://github.com/XanClic/qemu/commits/block-next) > > - a lot of refactoring and reordering of patches. > - dead code removed (bdrv_dirty_bitmap_load, etc.) > - do not maintain extra data for now > - do not store dirty bitmap directory in memory > (as we use it seldom, we can just reread if needed) > > By Kevin's review: > 01 - commit message changed: fix->improvement (as it was not a bug) > 03 - r-b > 04 - r-b > 05 - add 21 patch to fix spec, also, removed all (I hope) mentions of > "Bitmap Header", switch to one unified name for it - "Bitmap > Directory Entry", to avoid misunderstanding with Qcow2 header. > (also, add patch 22, to fix it in spec) > v6.06 - improve for_each_dir_entry loop, reorder patches, other small fixes > v6.07 ~> v7.09 - dead code removed, I've moved to one function > .bdrv_store_persistent_bitmaps and have wrapper and callback in one > patch (with also some other staff. If it not ok, I can split them) > v6.08 - about keeping bitmap directory instead of bitmap list: no I don't keep > it at all. > v6.16 - old bdrv_ bitmap-storing related functions are removed. The new one is > bdrv_store_persistent_bitmaps. > > > v6: > https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fqcow2-bitmap-v6 > based on block-next (https://github.com/XanClic/qemu/commits/block-next) > > There are a lot of changes, reorderings and additions in comparement with v5. > One principal thing: now bitmaps are removed from image after loading instead > of marking them in_use. It is simpler and we do not need to store superfluous > data. > Also, we are no more interested in command line interface to dirty bitmaps. > So it is dropped. If someone needs it I can add it later. > > Vladimir Sementsov-Ogievskiy (30): > specs/qcow2: fix bitmap granularity qemu-specific note > specs/qcow2: do not use wording 'bitmap header' > hbitmap: improve dirty iter > tests: add hbitmap iter test > block: fix bdrv_dirty_bitmap_granularity signature > block/dirty-bitmap: add deserialize_ones func > qcow2-refcount: rename inc_refcounts() and make it public > qcow2: add bitmaps extension > block/dirty-bitmap: fix comment for BlockDirtyBitmap.disabled field > block/dirty-bitmap: add readonly field to BdrvDirtyBitmap > qcow2: autoloading dirty bitmaps > block: refactor bdrv_reopen_commit > block: new bdrv_reopen_bitmaps_rw interface > qcow2: support .bdrv_reopen_bitmaps_rw > block/dirty-bitmap: add autoload field to BdrvDirtyBitmap > block: bdrv_close: release bitmaps after drv->bdrv_close > block: introduce persistent dirty bitmaps > block/dirty-bitmap: add bdrv_dirty_bitmap_next() > qcow2: add persistent dirty bitmaps support > qcow2: store bitmaps on reopening image as read-only > block: add bdrv_can_store_new_dirty_bitmap > qcow2: add .bdrv_can_store_new_dirty_bitmap > qmp: add persistent flag to block-dirty-bitmap-add > qmp: add autoload parameter to block-dirty-bitmap-add > qmp: add x-debug-block-dirty-bitmap-sha256 > iotests: test qcow2 persistent dirty bitmap > block/dirty-bitmap: add bdrv_remove_persistent_dirty_bitmap > qcow2: add .bdrv_remove_persistent_dirty_bitmap > qmp: block-dirty-bitmap-remove: remove persistent > block: release persistent bitmaps on inactivate > > block.c | 65 +- > block/Makefile.objs | 2 +- > block/dirty-bitmap.c | 154 ++++- > block/io.c | 8 + > block/qcow2-bitmap.c | 1481 > ++++++++++++++++++++++++++++++++++++++++++ > block/qcow2-refcount.c | 59 +- > block/qcow2.c | 155 ++++- > block/qcow2.h | 43 ++ > blockdev.c | 73 ++- > docs/interop/qcow2.txt | 8 +- > include/block/block.h | 3 + > include/block/block_int.h | 14 + > include/block/dirty-bitmap.h | 22 +- > include/qemu/hbitmap.h | 49 +- > qapi/block-core.json | 42 +- > tests/Makefile.include | 2 +- > tests/qemu-iotests/165 | 105 +++ > tests/qemu-iotests/165.out | 5 + > tests/qemu-iotests/group | 1 + > tests/test-hbitmap.c | 19 + > util/hbitmap.c | 51 +- > 21 files changed, 2280 insertions(+), 81 deletions(-) > create mode 100644 block/qcow2-bitmap.c > create mode 100755 tests/qemu-iotests/165 > create mode 100644 tests/qemu-iotests/165.out >
