On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <vsement...@parallels.com>
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> block/qcow2-dirty-bitmap.c | 5 +++++
> block/qcow2.c | 13 +++++++++++--
> block/qcow2.h | 9 +++++++++
> 3 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c
> index db83112..686a121 100644
> --- a/block/qcow2-dirty-bitmap.c
> +++ b/block/qcow2-dirty-bitmap.c
> @@ -188,6 +188,11 @@ static int qcow2_write_dirty_bitmaps(BlockDriverState
> *bs)
>
> s->dirty_bitmaps_offset = dirty_bitmaps_offset;
> s->dirty_bitmaps_size = dirty_bitmaps_size;
> + if (s->nb_dirty_bitmaps > 0) {
> + s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS;
> + } else {
> + s->autoclear_features &= ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS;
> + }
> ret = qcow2_update_header(bs);
> if (ret < 0) {
> fprintf(stderr, "Could not update qcow2 header\n");
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 406e55d..f85a55a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState *bs,
> uint64_t start_offset,
> return ret;
> }
>
> + if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) &&
> + s->nb_dirty_bitmaps > 0) {
> + ret = qcow2_delete_all_dirty_bitmaps(bs, errp);
> + if (ret < 0) {
> + return ret;
> + }
> + }
> +
> #ifdef DEBUG_EXT
> printf("Qcow2: Got dirty bitmaps extension:"
> " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
> @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict
> *options, int flags,
> }
>
> /* Clear unknown autoclear feature bits */
> - if (!bs->read_only && !(flags & BDRV_O_INCOMING) &&
> s->autoclear_features) {
> - s->autoclear_features = 0;
> + if (!bs->read_only && !(flags & BDRV_O_INCOMING) &&
> + (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
> + s->autoclear_features |= QCOW2_AUTOCLEAR_MASK;
Like Stefan already mentioned, fixing this |= to &= will fix iotest 036,
which is otherwise broken by this patch.
> ret = qcow2_update_header(bs);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not update qcow2 header");
> diff --git a/block/qcow2.h b/block/qcow2.h
> index b5e576c..14bd6f9 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -215,6 +215,15 @@ enum {
> QCOW2_COMPAT_FEAT_MASK = QCOW2_COMPAT_LAZY_REFCOUNTS,
> };
>
> +/* Autoclear feature bits */
> +enum {
> + QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR = 0,
> + QCOW2_AUTOCLEAR_DIRTY_BITMAPS =
> + 1 << QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR,
> +
> + QCOW2_AUTOCLEAR_MASK = QCOW2_AUTOCLEAR_DIRTY_BITMAPS,
> +};
> +
I find it a little awkward to have an enum with three different kinds of
data in it, unless I am reading this incorrectly. (bit position, bit
masks, and accumulated bit mask.)
Just enumerating the indices is probably sufficient:
enum {
QCOW2_AUTOCLEAR_BEGIN = 0,
QCOW2_AUTOCLEAR_DIRTY_BITMAPS = QCOW2_AUTOCLEAR_BEGIN,
...,
QCOW2_AUTOCLEAR_END
}
and then the QCOW2_AUTOCLEAR_MASK can either be programmatically defined
via a function, or just pre-computed as a #define.
If you still want the mask definitions, you could do something cheeky
like this:
#define AUTOCLEAR_MASK(X) (1 << QCOW2_AUTOCLEAR_ ## X)
and then you can use things like AUTOCLEAR_MASK(DIRTY_BITMAPS) without
having to create and maintain two separate tables if you want both forms
easily available.
> enum qcow2_discard_type {
> QCOW2_DISCARD_NEVER = 0,
> QCOW2_DISCARD_ALWAYS,
>
