Re: [Qemu-devel] [PATCH v22 00/30] qcow2: persistent dirty bitmaps

[John Snow]

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
>