[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
>
- [Qemu-block] [PATCH 14/25] block: add bdrv_can_store_new_dirty_bitmap, (continued)
- [Qemu-block] [PATCH 14/25] block: add bdrv_can_store_new_dirty_bitmap, Vladimir Sementsov-Ogievskiy, 2017/02/14
- [Qemu-block] [PATCH 10/25] block/dirty-bitmap: add autoload field to BdrvDirtyBitmap, Vladimir Sementsov-Ogievskiy, 2017/02/14
- [Qemu-block] [PATCH 15/25] qcow2: add .bdrv_can_store_new_dirty_bitmap, Vladimir Sementsov-Ogievskiy, 2017/02/14
- [Qemu-block] [PATCH 11/25] block: introduce persistent dirty bitmaps, Vladimir Sementsov-Ogievskiy, 2017/02/14
- [Qemu-block] [PATCH 03/25] hbitmap: improve dirty iter, Vladimir Sementsov-Ogievskiy, 2017/02/14
- [Qemu-block] [PATCH 13/25] qcow2: add .bdrv_store_persistent_dirty_bitmaps(), Vladimir Sementsov-Ogievskiy, 2017/02/14
- [Qemu-block] [PATCH 09/25] qcow2: add .bdrv_load_autoloading_dirty_bitmaps, Vladimir Sementsov-Ogievskiy, 2017/02/14
- Re: [Qemu-block] [Qemu-devel] [PATCH v14 00/24] qcow2: persistent dirty bitmaps,
John Snow <=