qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v14 00/24] qcow2: persistent dirty


From: John Snow
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v14 00/24] qcow2: persistent dirty bitmaps
Date: Tue, 14 Feb 2017 14:02:05 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0


On 02/14/2017 11:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 

Hi! :)

> There is a new update of qcow2-bitmap series - v14.
> 

Having the cover letter be 00/24 but including 25 patches confuses the
patch scraping tool a good deal. Also, can you include the "v14" in the
patch emails themselves, too?

(Don't respin just for this, thanks!)

> web: 
> https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=qcow2-bitmap-v14
> git: https://src.openvz.org/scm/~vsementsov/qemu.git (tag qcow2-bitmap-v14)
> 
> 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 (25):
>   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: add bitmaps extension
>   block: introduce auto-loading bitmaps
>   qcow2: add .bdrv_load_autoloading_dirty_bitmaps
>   block/dirty-bitmap: add autoload field to BdrvDirtyBitmap
>   block: introduce persistent dirty bitmaps
>   block/dirty-bitmap: add bdrv_dirty_bitmap_next()
>   qcow2: add .bdrv_store_persistent_dirty_bitmaps()
>   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
>   qcow2-refcount: rename inc_refcounts() and make it public
>   qcow2-bitmap: refcounts
>   block/dirty-bitmap: add bdrv_remove_persistent_dirty_bitmap
>   qcow2: add .bdrv_remove_persistent_dirty_bitmap
>   qmp: block-dirty-bitmap-remove: remove persistent
>   qcow2-bitmap: improve check_constraints_on_bitmap
> 
>  block.c                      |   68 +++
>  block/Makefile.objs          |    2 +-
>  block/dirty-bitmap.c         |   80 ++-
>  block/qcow2-bitmap.c         | 1360 
> ++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2-refcount.c       |   59 +-
>  block/qcow2.c                |  128 +++-
>  block/qcow2.h                |   42 ++
>  blockdev.c                   |   71 ++-
>  docs/specs/qcow2.txt         |    8 +-
>  include/block/block.h        |    5 +
>  include/block/block_int.h    |   12 +
>  include/block/dirty-bitmap.h |   21 +-
>  include/qemu/hbitmap.h       |   49 +-
>  qapi/block-core.json         |   39 +-
>  tests/Makefile.include       |    2 +-
>  tests/qemu-iotests/165       |   89 +++
>  tests/qemu-iotests/165.out   |    5 +
>  tests/qemu-iotests/group     |    1 +
>  tests/test-hbitmap.c         |   19 +
>  util/hbitmap.c               |   51 +-
>  20 files changed, 2047 insertions(+), 64 deletions(-)
>  create mode 100644 block/qcow2-bitmap.c
>  create mode 100755 tests/qemu-iotests/165
>  create mode 100644 tests/qemu-iotests/165.out
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]