[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 3/6] bloc/qcow2: drop dirty_bitmaps
From: |
John Snow |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 3/6] bloc/qcow2: drop dirty_bitmaps_loaded state variable |
Date: |
Tue, 17 Jul 2018 15:10:40 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
On 07/10/2018 03:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> 10.07.2018 02:25, John Snow wrote:
>>
>> On 06/26/2018 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> This variable doesn't work as it should, because it is actually cleared
>>> in qcow2_co_invalidate_cache() by memset(). Drop it, as the following
>>> patch will introduce new behavior.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>> block/qcow2.h | 1 -
>>> block/qcow2.c | 19 ++-----------------
>>> 2 files changed, 2 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index 01b5250415..c9df19b6e6 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -294,7 +294,6 @@ typedef struct BDRVQcow2State {
>>> uint32_t nb_bitmaps;
>>> uint64_t bitmap_directory_size;
>>> uint64_t bitmap_directory_offset;
>>> - bool dirty_bitmaps_loaded;
>>> int flags;
>>> int qcow_version;
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 46194a33ca..0044ff58e7 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1149,7 +1149,6 @@ static int coroutine_fn
>>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>> uint64_t ext_end;
>>> uint64_t l1_vm_state_index;
>>> bool update_header = false;
>>> - bool header_updated = false;
>>> ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
>>> if (ret < 0) {
>>> @@ -1488,23 +1487,9 @@ static int coroutine_fn
>>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>> s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>>> }
>>> - if (s->dirty_bitmaps_loaded) {
>>> - /* It's some kind of reopen. There are no known cases where
>>> we need to
>>> - * reload bitmaps in such a situation, so it's safer to skip
>>> them.
>>> - *
>>> - * Moreover, if we have some readonly bitmaps and we are
>>> reopening for
>>> - * rw we should reopen bitmaps correspondingly.
>>> - */
>>> - if (bdrv_has_readonly_bitmaps(bs) &&
>>> - !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) &
>>> BDRV_O_INACTIVE))
>>> - {
>>> - qcow2_reopen_bitmaps_rw_hint(bs, &header_updated,
>>> &local_err);
>>> - }
>>> - } else {
>>> - header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
>>> - s->dirty_bitmaps_loaded = true;
>>> + if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
>>> + update_header = false;
>>> }
>>> - update_header = update_header && !header_updated;
>>> if (local_err != NULL) {
>>> error_propagate(errp, local_err);
>>> ret = -EINVAL;
>>>
>> Usually I'd like to see the new behavior introduced before removing the
>> old, imperfect solution... (it doesn't make any tests fail!? ...)
>>
>> At this point in the series I just have to trust that you will fix it to
>> be better :)
>
> As I understand, this field is actually always false, so it's safe to
> drop it.
Ah, you know, I think I was remembering a patch (?) where you explicitly
cache this value and restore it -- but that must have been a previous
incarnation of this patchset.
I realize now that as the code actually exists in origin/master that
it's always going to be false, yeah.
--js