Re: [Qemu-devel] [PATCH 6/8] qcow2: add autoclear bit for dirty bitmaps

[John Snow <jsnow@redhat.com>]

On 06/11/2015 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> On 11.06.2015 02:42, John Snow wrote:
>>
>> On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
> 
> 
> This enum is made like enums for  QCOW2_INCOMPAT_* and QCOW2_COMPAT_*,
> which are already in the code... Then, may I make a patch for them too?
> I agree, it is strange solution to put things of different nature to one
> enum.
> 

Follow Kevin's lead, here -- It looked strange to me, but it _is_ best
to follow the existing style. I didn't look at the surrounding code too
carefully.

> 
>>
>>>   enum qcow2_discard_type {
>>>       QCOW2_DISCARD_NEVER = 0,
>>>       QCOW2_DISCARD_ALWAYS,
>>>
> 
>