* [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory
@ 2018-03-19 8:07 Vladimir Sementsov-Ogievskiy
2018-04-19 20:57 ` John Snow
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-03-19 8:07 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, jsnow, eblake
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
If it appropriate for 2.12, let's push it. If not - then for 2.13.
v2: - squash 02 (indentation fix) to 01
- drop comment from qcow2_check_metadata_overlap()
- set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
bitmap_list_store. I don't think non-inplace case should be changed,
as it don't touch active bitmap directory.
block/qcow2.h | 45 ++++++++++++++++++++++++---------------------
block/qcow2-bitmap.c | 7 ++++++-
block/qcow2-refcount.c | 10 ++++++++++
block/qcow2.c | 22 ++++++++++++++--------
4 files changed, 54 insertions(+), 30 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 6f0ff15dd0..896ad08e5b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -98,6 +98,7 @@
#define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
#define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
#define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
+#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY "overlap-check.bitmap-directory"
#define QCOW2_OPT_CACHE_SIZE "cache-size"
#define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
#define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
@@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
} QCow2ClusterType;
typedef enum QCow2MetadataOverlap {
- QCOW2_OL_MAIN_HEADER_BITNR = 0,
- QCOW2_OL_ACTIVE_L1_BITNR = 1,
- QCOW2_OL_ACTIVE_L2_BITNR = 2,
- QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
- QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
- QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
- QCOW2_OL_INACTIVE_L1_BITNR = 6,
- QCOW2_OL_INACTIVE_L2_BITNR = 7,
-
- QCOW2_OL_MAX_BITNR = 8,
-
- QCOW2_OL_NONE = 0,
- QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
- QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
- QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
- QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
- QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
- QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
- QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
+ QCOW2_OL_MAIN_HEADER_BITNR = 0,
+ QCOW2_OL_ACTIVE_L1_BITNR = 1,
+ QCOW2_OL_ACTIVE_L2_BITNR = 2,
+ QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
+ QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
+ QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
+ QCOW2_OL_INACTIVE_L1_BITNR = 6,
+ QCOW2_OL_INACTIVE_L2_BITNR = 7,
+ QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
+
+ QCOW2_OL_MAX_BITNR = 9,
+
+ QCOW2_OL_NONE = 0,
+ QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
+ QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
+ QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
+ QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
+ QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
+ QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
+ QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
/* NOTE: Checking overlaps with inactive L2 tables will result in bdrv
* reads. */
- QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
+ QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
+ QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
} QCow2MetadataOverlap;
/* Perform all overlap checks which can be done in constant time */
#define QCOW2_OL_CONSTANT \
(QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
- QCOW2_OL_SNAPSHOT_TABLE)
+ QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
/* Perform all overlap checks which don't require disk access */
#define QCOW2_OL_CACHED \
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f45e46cfbd..fb750ba8d3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -776,7 +776,12 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
}
}
- ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, dir_size);
+ /* Actually, even in in-place case ignoring QCOW2_OL_BITMAP_DIRECTORY is not
+ * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when updating bitmap
+ * directory in-place (actually, turn-off the extension), which is checked
+ * in qcow2_check_metadata_overlap() */
+ ret = qcow2_pre_write_overlap_check(
+ bs, in_place ? QCOW2_OL_BITMAP_DIRECTORY : 0, dir_offset, dir_size);
if (ret < 0) {
goto fail;
}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3de1ab51ba..275a303cfa 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2585,6 +2585,16 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
}
}
+ if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
+ (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
+ {
+ if (overlaps_with(s->bitmap_directory_offset,
+ s->bitmap_directory_size))
+ {
+ return QCOW2_OL_BITMAP_DIRECTORY;
+ }
+ }
+
return 0;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 1914a940e5..23f36a67c7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = {
.help = "Check for unintended writes into an inactive L2 table",
},
{
+ .name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
+ .type = QEMU_OPT_BOOL,
+ .help = "Check for unintended writes into the bitmap directory",
+ },
+ {
.name = QCOW2_OPT_CACHE_SIZE,
.type = QEMU_OPT_SIZE,
.help = "Maximum combined metadata (L2 tables and refcount blocks) "
@@ -682,14 +687,15 @@ static QemuOptsList qcow2_runtime_opts = {
};
static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
- [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
- [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
- [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
- [QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
- [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
- [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
- [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
- [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
+ [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
+ [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
+ [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
+ [QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
+ [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
+ [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
+ [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
+ [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
+ [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
};
static void cache_clean_timer_cb(void *opaque)
--
2.11.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory
2018-03-19 8:07 [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory Vladimir Sementsov-Ogievskiy
@ 2018-04-19 20:57 ` John Snow
2018-04-20 12:12 ` Vladimir Sementsov-Ogievskiy
2018-06-21 13:02 ` [Qemu-devel] ping " Vladimir Sementsov-Ogievskiy
2018-06-25 13:47 ` [Qemu-devel] " Max Reitz
2 siblings, 1 reply; 7+ messages in thread
From: John Snow @ 2018-04-19 20:57 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block; +Cc: kwolf, mreitz
On 03/19/2018 04:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> If it appropriate for 2.12, let's push it. If not - then for 2.13.
>
I wonder if I can make the case that this should be in 2.12.1; arguably
it is important to prevent corruption no matter how unlikely it is to
ever happen.
Moving it into stable increases the likelihood it shows up in
downstreams, so maybe let's see what we can get away with.
>
> v2: - squash 02 (indentation fix) to 01
> - drop comment from qcow2_check_metadata_overlap()
> - set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
> bitmap_list_store. I don't think non-inplace case should be changed,
> as it don't touch active bitmap directory.
>
> block/qcow2.h | 45 ++++++++++++++++++++++++---------------------
> block/qcow2-bitmap.c | 7 ++++++-
> block/qcow2-refcount.c | 10 ++++++++++
> block/qcow2.c | 22 ++++++++++++++--------
> 4 files changed, 54 insertions(+), 30 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 6f0ff15dd0..896ad08e5b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -98,6 +98,7 @@
> #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
> #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
> #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY "overlap-check.bitmap-directory"
> #define QCOW2_OPT_CACHE_SIZE "cache-size"
> #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
> #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
> @@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
> } QCow2ClusterType;
>
> typedef enum QCow2MetadataOverlap {
> - QCOW2_OL_MAIN_HEADER_BITNR = 0,
> - QCOW2_OL_ACTIVE_L1_BITNR = 1,
> - QCOW2_OL_ACTIVE_L2_BITNR = 2,
> - QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
> - QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
> - QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
> - QCOW2_OL_INACTIVE_L1_BITNR = 6,
> - QCOW2_OL_INACTIVE_L2_BITNR = 7,
> -
> - QCOW2_OL_MAX_BITNR = 8,
> -
> - QCOW2_OL_NONE = 0,
> - QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> - QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
> - QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
> - QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
> - QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
> - QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
> - QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
> + QCOW2_OL_MAIN_HEADER_BITNR = 0,
> + QCOW2_OL_ACTIVE_L1_BITNR = 1,
> + QCOW2_OL_ACTIVE_L2_BITNR = 2,
> + QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
> + QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
> + QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
> + QCOW2_OL_INACTIVE_L1_BITNR = 6,
> + QCOW2_OL_INACTIVE_L2_BITNR = 7,
> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
> +
A bit hard to read due to the formatting, but you've added #8 here, and
> + QCOW2_OL_MAX_BITNR = 9,
> +> + QCOW2_OL_NONE = 0,
> + QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> + QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
> + QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
> + QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
> + QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
> + QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
> + QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
> /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv
> * reads. */
> - QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> + QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
and this one down here.
> } QCow2MetadataOverlap;
>
> /* Perform all overlap checks which can be done in constant time */
> #define QCOW2_OL_CONSTANT \
> (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
> - QCOW2_OL_SNAPSHOT_TABLE)
> + QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
>
> /* Perform all overlap checks which don't require disk access */
> #define QCOW2_OL_CACHED \
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index f45e46cfbd..fb750ba8d3 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -776,7 +776,12 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
> }
> }
>
> - ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, dir_size);
> + /* Actually, even in in-place case ignoring QCOW2_OL_BITMAP_DIRECTORY is not
> + * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when updating bitmap
> + * directory in-place (actually, turn-off the extension), which is checked
> + * in qcow2_check_metadata_overlap() */
I might need you to rephrase this for me.
I guess we're ignoring QCOW2_OL_BITMAP_DIRECTORY if in_place is true,
but only as an optimization. Why is it not required?
> + ret = qcow2_pre_write_overlap_check(
> + bs, in_place ? QCOW2_OL_BITMAP_DIRECTORY : 0, dir_offset, dir_size);
> if (ret < 0) {
> goto fail;
> }
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3de1ab51ba..275a303cfa 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -2585,6 +2585,16 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
> }
> }
>
> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
> + {
> + if (overlaps_with(s->bitmap_directory_offset,
> + s->bitmap_directory_size))
> + {
> + return QCOW2_OL_BITMAP_DIRECTORY;
> + }
> + }
> +
> return 0;
> }
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 1914a940e5..23f36a67c7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = {
> .help = "Check for unintended writes into an inactive L2 table",
> },
> {
> + .name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
> + .type = QEMU_OPT_BOOL,
> + .help = "Check for unintended writes into the bitmap directory",
> + },
> + {
> .name = QCOW2_OPT_CACHE_SIZE,
> .type = QEMU_OPT_SIZE,
> .help = "Maximum combined metadata (L2 tables and refcount blocks) "
> @@ -682,14 +687,15 @@ static QemuOptsList qcow2_runtime_opts = {
> };
>
> static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
> - [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
> - [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
> - [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
> - [QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
> - [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
> - [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
> - [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
> - [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
> + [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
> + [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
> + [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
> + [QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
> + [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
> + [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
> + [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
> + [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
Just churn, and
> + [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
this is the only new line.
> };
>
> static void cache_clean_timer_cb(void *opaque)
>
Looks okay to me overall, apart from needing a comment rephrase.
--js
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory
2018-04-19 20:57 ` John Snow
@ 2018-04-20 12:12 ` Vladimir Sementsov-Ogievskiy
2018-06-04 16:11 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-04-20 12:12 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block; +Cc: kwolf, mreitz
19.04.2018 23:57, John Snow wrote:
>
> On 03/19/2018 04:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> If it appropriate for 2.12, let's push it. If not - then for 2.13.
>>
> I wonder if I can make the case that this should be in 2.12.1; arguably
> it is important to prevent corruption no matter how unlikely it is to
> ever happen.
>
> Moving it into stable increases the likelihood it shows up in
> downstreams, so maybe let's see what we can get away with.
>
>> v2: - squash 02 (indentation fix) to 01
>> - drop comment from qcow2_check_metadata_overlap()
>> - set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
>> bitmap_list_store. I don't think non-inplace case should be changed,
>> as it don't touch active bitmap directory.
>>
>> block/qcow2.h | 45 ++++++++++++++++++++++++---------------------
>> block/qcow2-bitmap.c | 7 ++++++-
>> block/qcow2-refcount.c | 10 ++++++++++
>> block/qcow2.c | 22 ++++++++++++++--------
>> 4 files changed, 54 insertions(+), 30 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 6f0ff15dd0..896ad08e5b 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -98,6 +98,7 @@
>> #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
>> #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
>> #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
>> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY "overlap-check.bitmap-directory"
>> #define QCOW2_OPT_CACHE_SIZE "cache-size"
>> #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
>> #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>> @@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
>> } QCow2ClusterType;
>>
>> typedef enum QCow2MetadataOverlap {
>> - QCOW2_OL_MAIN_HEADER_BITNR = 0,
>> - QCOW2_OL_ACTIVE_L1_BITNR = 1,
>> - QCOW2_OL_ACTIVE_L2_BITNR = 2,
>> - QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
>> - QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
>> - QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
>> - QCOW2_OL_INACTIVE_L1_BITNR = 6,
>> - QCOW2_OL_INACTIVE_L2_BITNR = 7,
>> -
>> - QCOW2_OL_MAX_BITNR = 8,
>> -
>> - QCOW2_OL_NONE = 0,
>> - QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
>> - QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
>> - QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
>> - QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
>> - QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
>> - QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
>> - QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
>> + QCOW2_OL_MAIN_HEADER_BITNR = 0,
>> + QCOW2_OL_ACTIVE_L1_BITNR = 1,
>> + QCOW2_OL_ACTIVE_L2_BITNR = 2,
>> + QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
>> + QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
>> + QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
>> + QCOW2_OL_INACTIVE_L1_BITNR = 6,
>> + QCOW2_OL_INACTIVE_L2_BITNR = 7,
>> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
>> +
> A bit hard to read due to the formatting, but you've added #8 here, and
>
>> + QCOW2_OL_MAX_BITNR = 9,
>> +> + QCOW2_OL_NONE = 0,
>> + QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
>> + QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
>> + QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
>> + QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
>> + QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
>> + QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
>> + QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
>> /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv
>> * reads. */
>> - QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>> + QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>> + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
> and this one down here.
>
>> } QCow2MetadataOverlap;
>>
>> /* Perform all overlap checks which can be done in constant time */
>> #define QCOW2_OL_CONSTANT \
>> (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
>> - QCOW2_OL_SNAPSHOT_TABLE)
>> + QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
>>
>> /* Perform all overlap checks which don't require disk access */
>> #define QCOW2_OL_CACHED \
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index f45e46cfbd..fb750ba8d3 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -776,7 +776,12 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
>> }
>> }
>>
>> - ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, dir_size);
>> + /* Actually, even in in-place case ignoring QCOW2_OL_BITMAP_DIRECTORY is not
>> + * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when updating bitmap
>> + * directory in-place (actually, turn-off the extension), which is checked
>> + * in qcow2_check_metadata_overlap() */
> I might need you to rephrase this for me.
>
> I guess we're ignoring QCOW2_OL_BITMAP_DIRECTORY if in_place is true,
> but only as an optimization. Why is it not required?
because, actually, we don't have any dirty bitmaps extension at the
moment when we rewrite bitmap directory in-place (as we drop autoclear
bit), so nothing will be checked.
>
>> + ret = qcow2_pre_write_overlap_check(
>> + bs, in_place ? QCOW2_OL_BITMAP_DIRECTORY : 0, dir_offset, dir_size);
>> if (ret < 0) {
>> goto fail;
>> }
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 3de1ab51ba..275a303cfa 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -2585,6 +2585,16 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
>> }
>> }
>>
>> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
>> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
>> + {
>> + if (overlaps_with(s->bitmap_directory_offset,
>> + s->bitmap_directory_size))
>> + {
>> + return QCOW2_OL_BITMAP_DIRECTORY;
>> + }
>> + }
>> +
>> return 0;
>> }
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 1914a940e5..23f36a67c7 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = {
>> .help = "Check for unintended writes into an inactive L2 table",
>> },
>> {
>> + .name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
>> + .type = QEMU_OPT_BOOL,
>> + .help = "Check for unintended writes into the bitmap directory",
>> + },
>> + {
>> .name = QCOW2_OPT_CACHE_SIZE,
>> .type = QEMU_OPT_SIZE,
>> .help = "Maximum combined metadata (L2 tables and refcount blocks) "
>> @@ -682,14 +687,15 @@ static QemuOptsList qcow2_runtime_opts = {
>> };
>>
>> static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
>> - [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
>> - [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
>> - [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
>> - [QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
>> - [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
>> - [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
>> - [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
>> - [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
>> + [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
>> + [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
>> + [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
>> + [QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
>> + [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
>> + [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
>> + [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
>> + [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
> Just churn, and
>
>> + [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
> this is the only new line.
>
>> };
>>
>> static void cache_clean_timer_cb(void *opaque)
>>
> Looks okay to me overall, apart from needing a comment rephrase.
>
> --js
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory
2018-04-20 12:12 ` Vladimir Sementsov-Ogievskiy
@ 2018-06-04 16:11 ` Vladimir Sementsov-Ogievskiy
2018-06-04 17:20 ` John Snow
0 siblings, 1 reply; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-04 16:11 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block; +Cc: kwolf, mreitz
20.04.2018 15:12, Vladimir Sementsov-Ogievskiy wrote:
> 19.04.2018 23:57, John Snow wrote:
>>
>> On 03/19/2018 04:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>
>>> If it appropriate for 2.12, let's push it. If not - then for 2.13.
>>>
>> I wonder if I can make the case that this should be in 2.12.1; arguably
>> it is important to prevent corruption no matter how unlikely it is to
>> ever happen.
>>
>> Moving it into stable increases the likelihood it shows up in
>> downstreams, so maybe let's see what we can get away with.
>>
>>> v2: - squash 02 (indentation fix) to 01
>>> - drop comment from qcow2_check_metadata_overlap()
>>> - set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
>>> bitmap_list_store. I don't think non-inplace case should be
>>> changed,
>>> as it don't touch active bitmap directory.
>>>
>>> block/qcow2.h | 45
>>> ++++++++++++++++++++++++---------------------
>>> block/qcow2-bitmap.c | 7 ++++++-
>>> block/qcow2-refcount.c | 10 ++++++++++
>>> block/qcow2.c | 22 ++++++++++++++--------
>>> 4 files changed, 54 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index 6f0ff15dd0..896ad08e5b 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -98,6 +98,7 @@
>>> #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE
>>> "overlap-check.snapshot-table"
>>> #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
>>> #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
>>> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY
>>> "overlap-check.bitmap-directory"
>>> #define QCOW2_OPT_CACHE_SIZE "cache-size"
>>> #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
>>> #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>>> @@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
>>> } QCow2ClusterType;
>>> typedef enum QCow2MetadataOverlap {
>>> - QCOW2_OL_MAIN_HEADER_BITNR = 0,
>>> - QCOW2_OL_ACTIVE_L1_BITNR = 1,
>>> - QCOW2_OL_ACTIVE_L2_BITNR = 2,
>>> - QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
>>> - QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
>>> - QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
>>> - QCOW2_OL_INACTIVE_L1_BITNR = 6,
>>> - QCOW2_OL_INACTIVE_L2_BITNR = 7,
>>> -
>>> - QCOW2_OL_MAX_BITNR = 8,
>>> -
>>> - QCOW2_OL_NONE = 0,
>>> - QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
>>> - QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
>>> - QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
>>> - QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
>>> - QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
>>> - QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
>>> - QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
>>> + QCOW2_OL_MAIN_HEADER_BITNR = 0,
>>> + QCOW2_OL_ACTIVE_L1_BITNR = 1,
>>> + QCOW2_OL_ACTIVE_L2_BITNR = 2,
>>> + QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
>>> + QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
>>> + QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
>>> + QCOW2_OL_INACTIVE_L1_BITNR = 6,
>>> + QCOW2_OL_INACTIVE_L2_BITNR = 7,
>>> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
>>> +
>> A bit hard to read due to the formatting, but you've added #8 here, and
>>
>>> + QCOW2_OL_MAX_BITNR = 9,
>>> +> + QCOW2_OL_NONE = 0,
>>> + QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
>>> + QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
>>> + QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
>>> + QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
>>> + QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
>>> + QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
>>> + QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
>>> /* NOTE: Checking overlaps with inactive L2 tables will result
>>> in bdrv
>>> * reads. */
>>> - QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>>> + QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>>> + QCOW2_OL_BITMAP_DIRECTORY = (1 <<
>>> QCOW2_OL_BITMAP_DIRECTORY_BITNR),
>> and this one down here.
>>
>>> } QCow2MetadataOverlap;
>>> /* Perform all overlap checks which can be done in constant time */
>>> #define QCOW2_OL_CONSTANT \
>>> (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 |
>>> QCOW2_OL_REFCOUNT_TABLE | \
>>> - QCOW2_OL_SNAPSHOT_TABLE)
>>> + QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
>>> /* Perform all overlap checks which don't require disk access */
>>> #define QCOW2_OL_CACHED \
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index f45e46cfbd..fb750ba8d3 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -776,7 +776,12 @@ static int bitmap_list_store(BlockDriverState
>>> *bs, Qcow2BitmapList *bm_list,
>>> }
>>> }
>>> - ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset,
>>> dir_size);
>>> + /* Actually, even in in-place case ignoring
>>> QCOW2_OL_BITMAP_DIRECTORY is not
>>> + * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when
>>> updating bitmap
>>> + * directory in-place (actually, turn-off the extension), which
>>> is checked
>>> + * in qcow2_check_metadata_overlap() */
>> I might need you to rephrase this for me.
>>
>> I guess we're ignoring QCOW2_OL_BITMAP_DIRECTORY if in_place is true,
>> but only as an optimization. Why is it not required?
>
> because, actually, we don't have any dirty bitmaps extension at the
> moment when we rewrite bitmap directory in-place (as we drop autoclear
> bit), so nothing will be checked.
Is it clear, do you want to rephrase this somehow?
>
>>
>>> + ret = qcow2_pre_write_overlap_check(
>>> + bs, in_place ? QCOW2_OL_BITMAP_DIRECTORY : 0,
>>> dir_offset, dir_size);
>>> if (ret < 0) {
>>> goto fail;
>>> }
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 3de1ab51ba..275a303cfa 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -2585,6 +2585,16 @@ int
>>> qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t
>>> offset,
>>> }
>>> }
>>> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
>>> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
>>> + {
>>> + if (overlaps_with(s->bitmap_directory_offset,
>>> + s->bitmap_directory_size))
>>> + {
>>> + return QCOW2_OL_BITMAP_DIRECTORY;
>>> + }
>>> + }
>>> +
>>> return 0;
>>> }
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 1914a940e5..23f36a67c7 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = {
>>> .help = "Check for unintended writes into an inactive
>>> L2 table",
>>> },
>>> {
>>> + .name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
>>> + .type = QEMU_OPT_BOOL,
>>> + .help = "Check for unintended writes into the bitmap
>>> directory",
>>> + },
>>> + {
>>> .name = QCOW2_OPT_CACHE_SIZE,
>>> .type = QEMU_OPT_SIZE,
>>> .help = "Maximum combined metadata (L2 tables and
>>> refcount blocks) "
>>> @@ -682,14 +687,15 @@ static QemuOptsList qcow2_runtime_opts = {
>>> };
>>> static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR]
>>> = {
>>> - [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
>>> - [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
>>> - [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
>>> - [QCOW2_OL_REFCOUNT_TABLE_BITNR] =
>>> QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
>>> - [QCOW2_OL_REFCOUNT_BLOCK_BITNR] =
>>> QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
>>> - [QCOW2_OL_SNAPSHOT_TABLE_BITNR] =
>>> QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
>>> - [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
>>> - [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
>>> + [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
>>> + [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
>>> + [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
>>> + [QCOW2_OL_REFCOUNT_TABLE_BITNR] =
>>> QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
>>> + [QCOW2_OL_REFCOUNT_BLOCK_BITNR] =
>>> QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
>>> + [QCOW2_OL_SNAPSHOT_TABLE_BITNR] =
>>> QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
>>> + [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
>>> + [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
>> Just churn, and
>>
>>> + [QCOW2_OL_BITMAP_DIRECTORY_BITNR] =
>>> QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
>> this is the only new line.
>>
>>> };
>>> static void cache_clean_timer_cb(void *opaque)
>>>
>> Looks okay to me overall, apart from needing a comment rephrase.
>>
>> --js
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory
2018-06-04 16:11 ` Vladimir Sementsov-Ogievskiy
@ 2018-06-04 17:20 ` John Snow
0 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2018-06-04 17:20 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block; +Cc: kwolf, mreitz
On 06/04/2018 12:11 PM, Vladimir Sementsov-Ogievskiy wrote:
> 20.04.2018 15:12, Vladimir Sementsov-Ogievskiy wrote:
>> 19.04.2018 23:57, John Snow wrote:
>>>
>>> On 03/19/2018 04:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>
>>>> If it appropriate for 2.12, let's push it. If not - then for 2.13.
>>>>
>>> I wonder if I can make the case that this should be in 2.12.1; arguably
>>> it is important to prevent corruption no matter how unlikely it is to
>>> ever happen.
>>>
>>> Moving it into stable increases the likelihood it shows up in
>>> downstreams, so maybe let's see what we can get away with.
>>>
>>>> v2: - squash 02 (indentation fix) to 01
>>>> - drop comment from qcow2_check_metadata_overlap()
>>>> - set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
>>>> bitmap_list_store. I don't think non-inplace case should be
>>>> changed,
>>>> as it don't touch active bitmap directory.
>>>>
>>>> block/qcow2.h | 45
>>>> ++++++++++++++++++++++++---------------------
>>>> block/qcow2-bitmap.c | 7 ++++++-
>>>> block/qcow2-refcount.c | 10 ++++++++++
>>>> block/qcow2.c | 22 ++++++++++++++--------
>>>> 4 files changed, 54 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index 6f0ff15dd0..896ad08e5b 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -98,6 +98,7 @@
>>>> #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE
>>>> "overlap-check.snapshot-table"
>>>> #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
>>>> #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
>>>> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY
>>>> "overlap-check.bitmap-directory"
>>>> #define QCOW2_OPT_CACHE_SIZE "cache-size"
>>>> #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
>>>> #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>>>> @@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
>>>> } QCow2ClusterType;
>>>> typedef enum QCow2MetadataOverlap {
>>>> - QCOW2_OL_MAIN_HEADER_BITNR = 0,
>>>> - QCOW2_OL_ACTIVE_L1_BITNR = 1,
>>>> - QCOW2_OL_ACTIVE_L2_BITNR = 2,
>>>> - QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
>>>> - QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
>>>> - QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
>>>> - QCOW2_OL_INACTIVE_L1_BITNR = 6,
>>>> - QCOW2_OL_INACTIVE_L2_BITNR = 7,
>>>> -
>>>> - QCOW2_OL_MAX_BITNR = 8,
>>>> -
>>>> - QCOW2_OL_NONE = 0,
>>>> - QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
>>>> - QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
>>>> - QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
>>>> - QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
>>>> - QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
>>>> - QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
>>>> - QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
>>>> + QCOW2_OL_MAIN_HEADER_BITNR = 0,
>>>> + QCOW2_OL_ACTIVE_L1_BITNR = 1,
>>>> + QCOW2_OL_ACTIVE_L2_BITNR = 2,
>>>> + QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
>>>> + QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
>>>> + QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
>>>> + QCOW2_OL_INACTIVE_L1_BITNR = 6,
>>>> + QCOW2_OL_INACTIVE_L2_BITNR = 7,
>>>> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
>>>> +
>>> A bit hard to read due to the formatting, but you've added #8 here, and
>>>
>>>> + QCOW2_OL_MAX_BITNR = 9,
>>>> +> + QCOW2_OL_NONE = 0,
>>>> + QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
>>>> + QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
>>>> + QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
>>>> + QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
>>>> + QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
>>>> + QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
>>>> + QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
>>>> /* NOTE: Checking overlaps with inactive L2 tables will result
>>>> in bdrv
>>>> * reads. */
>>>> - QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>>>> + QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>>>> + QCOW2_OL_BITMAP_DIRECTORY = (1 <<
>>>> QCOW2_OL_BITMAP_DIRECTORY_BITNR),
>>> and this one down here.
>>>
>>>> } QCow2MetadataOverlap;
>>>> /* Perform all overlap checks which can be done in constant time */
>>>> #define QCOW2_OL_CONSTANT \
>>>> (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 |
>>>> QCOW2_OL_REFCOUNT_TABLE | \
>>>> - QCOW2_OL_SNAPSHOT_TABLE)
>>>> + QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
>>>> /* Perform all overlap checks which don't require disk access */
>>>> #define QCOW2_OL_CACHED \
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index f45e46cfbd..fb750ba8d3 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -776,7 +776,12 @@ static int bitmap_list_store(BlockDriverState
>>>> *bs, Qcow2BitmapList *bm_list,
>>>> }
>>>> }
>>>> - ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset,
>>>> dir_size);
>>>> + /* Actually, even in in-place case ignoring
>>>> QCOW2_OL_BITMAP_DIRECTORY is not
>>>> + * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when
>>>> updating bitmap
>>>> + * directory in-place (actually, turn-off the extension), which
>>>> is checked
>>>> + * in qcow2_check_metadata_overlap() */
>>> I might need you to rephrase this for me.
>>>
>>> I guess we're ignoring QCOW2_OL_BITMAP_DIRECTORY if in_place is true,
>>> but only as an optimization. Why is it not required?
>>
>> because, actually, we don't have any dirty bitmaps extension at the
>> moment when we rewrite bitmap directory in-place (as we drop autoclear
>> bit), so nothing will be checked.
>
> Is it clear, do you want to rephrase this somehow?
>
Oh, sorry, I didn't wind up having a better suggestion. Thanks for the ping.
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] ping Re: [PATCH v2] qcow2: add overlap check for bitmap directory
2018-03-19 8:07 [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory Vladimir Sementsov-Ogievskiy
2018-04-19 20:57 ` John Snow
@ 2018-06-21 13:02 ` Vladimir Sementsov-Ogievskiy
2018-06-25 13:47 ` [Qemu-devel] " Max Reitz
2 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-21 13:02 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: kwolf, jsnow, mreitz
ping
19.03.2018 11:07, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> If it appropriate for 2.12, let's push it. If not - then for 2.13.
>
>
> v2: - squash 02 (indentation fix) to 01
> - drop comment from qcow2_check_metadata_overlap()
> - set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
> bitmap_list_store. I don't think non-inplace case should be changed,
> as it don't touch active bitmap directory.
>
> block/qcow2.h | 45 ++++++++++++++++++++++++---------------------
> block/qcow2-bitmap.c | 7 ++++++-
> block/qcow2-refcount.c | 10 ++++++++++
> block/qcow2.c | 22 ++++++++++++++--------
> 4 files changed, 54 insertions(+), 30 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 6f0ff15dd0..896ad08e5b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -98,6 +98,7 @@
> #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
> #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
> #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY "overlap-check.bitmap-directory"
> #define QCOW2_OPT_CACHE_SIZE "cache-size"
> #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
> #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
> @@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
> } QCow2ClusterType;
>
> typedef enum QCow2MetadataOverlap {
> - QCOW2_OL_MAIN_HEADER_BITNR = 0,
> - QCOW2_OL_ACTIVE_L1_BITNR = 1,
> - QCOW2_OL_ACTIVE_L2_BITNR = 2,
> - QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
> - QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
> - QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
> - QCOW2_OL_INACTIVE_L1_BITNR = 6,
> - QCOW2_OL_INACTIVE_L2_BITNR = 7,
> -
> - QCOW2_OL_MAX_BITNR = 8,
> -
> - QCOW2_OL_NONE = 0,
> - QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> - QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
> - QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
> - QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
> - QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
> - QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
> - QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
> + QCOW2_OL_MAIN_HEADER_BITNR = 0,
> + QCOW2_OL_ACTIVE_L1_BITNR = 1,
> + QCOW2_OL_ACTIVE_L2_BITNR = 2,
> + QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
> + QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
> + QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
> + QCOW2_OL_INACTIVE_L1_BITNR = 6,
> + QCOW2_OL_INACTIVE_L2_BITNR = 7,
> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
> +
> + QCOW2_OL_MAX_BITNR = 9,
> +
> + QCOW2_OL_NONE = 0,
> + QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> + QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
> + QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
> + QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
> + QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
> + QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
> + QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
> /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv
> * reads. */
> - QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> + QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
> } QCow2MetadataOverlap;
>
> /* Perform all overlap checks which can be done in constant time */
> #define QCOW2_OL_CONSTANT \
> (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
> - QCOW2_OL_SNAPSHOT_TABLE)
> + QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
>
> /* Perform all overlap checks which don't require disk access */
> #define QCOW2_OL_CACHED \
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index f45e46cfbd..fb750ba8d3 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -776,7 +776,12 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
> }
> }
>
> - ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, dir_size);
> + /* Actually, even in in-place case ignoring QCOW2_OL_BITMAP_DIRECTORY is not
> + * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when updating bitmap
> + * directory in-place (actually, turn-off the extension), which is checked
> + * in qcow2_check_metadata_overlap() */
> + ret = qcow2_pre_write_overlap_check(
> + bs, in_place ? QCOW2_OL_BITMAP_DIRECTORY : 0, dir_offset, dir_size);
> if (ret < 0) {
> goto fail;
> }
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3de1ab51ba..275a303cfa 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -2585,6 +2585,16 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
> }
> }
>
> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
> + {
> + if (overlaps_with(s->bitmap_directory_offset,
> + s->bitmap_directory_size))
> + {
> + return QCOW2_OL_BITMAP_DIRECTORY;
> + }
> + }
> +
> return 0;
> }
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 1914a940e5..23f36a67c7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = {
> .help = "Check for unintended writes into an inactive L2 table",
> },
> {
> + .name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
> + .type = QEMU_OPT_BOOL,
> + .help = "Check for unintended writes into the bitmap directory",
> + },
> + {
> .name = QCOW2_OPT_CACHE_SIZE,
> .type = QEMU_OPT_SIZE,
> .help = "Maximum combined metadata (L2 tables and refcount blocks) "
> @@ -682,14 +687,15 @@ static QemuOptsList qcow2_runtime_opts = {
> };
>
> static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
> - [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
> - [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
> - [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
> - [QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
> - [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
> - [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
> - [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
> - [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
> + [QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
> + [QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
> + [QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
> + [QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
> + [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
> + [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
> + [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
> + [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
> + [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
> };
>
> static void cache_clean_timer_cb(void *opaque)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory
2018-03-19 8:07 [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory Vladimir Sementsov-Ogievskiy
2018-04-19 20:57 ` John Snow
2018-06-21 13:02 ` [Qemu-devel] ping " Vladimir Sementsov-Ogievskiy
@ 2018-06-25 13:47 ` Max Reitz
2 siblings, 0 replies; 7+ messages in thread
From: Max Reitz @ 2018-06-25 13:47 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block; +Cc: kwolf, jsnow, eblake
[-- Attachment #1: Type: text/plain, Size: 1564 bytes --]
On 2018-03-19 09:07, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> If it appropriate for 2.12, let's push it. If not - then for 2.13.
>
>
> v2: - squash 02 (indentation fix) to 01
> - drop comment from qcow2_check_metadata_overlap()
> - set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
> bitmap_list_store. I don't think non-inplace case should be changed,
> as it don't touch active bitmap directory.
>
> block/qcow2.h | 45 ++++++++++++++++++++++++---------------------
> block/qcow2-bitmap.c | 7 ++++++-
> block/qcow2-refcount.c | 10 ++++++++++
> block/qcow2.c | 22 ++++++++++++++--------
> 4 files changed, 54 insertions(+), 30 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 6f0ff15dd0..896ad08e5b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -98,6 +98,7 @@
> #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
> #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
> #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY "overlap-check.bitmap-directory"
In addition to this, Qcow2OverlapCheckFlags in qapi/block-core.json
needs to be adjusted.
Apart from that, the patch looks good to me.
Max
> #define QCOW2_OPT_CACHE_SIZE "cache-size"
> #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
> #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-06-25 13:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-19 8:07 [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory Vladimir Sementsov-Ogievskiy
2018-04-19 20:57 ` John Snow
2018-04-20 12:12 ` Vladimir Sementsov-Ogievskiy
2018-06-04 16:11 ` Vladimir Sementsov-Ogievskiy
2018-06-04 17:20 ` John Snow
2018-06-21 13:02 ` [Qemu-devel] ping " Vladimir Sementsov-Ogievskiy
2018-06-25 13:47 ` [Qemu-devel] " Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.