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 <vsement...@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 <js...@redhat.com>