On 03/19/2018 04:07 AM, Vladimir Sementsov-Ogievskiy wrote:
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
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?