[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 6/8] qcow2: add autoclear bit for dirty bitmaps
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH 6/8] qcow2: add autoclear bit for dirty bitmaps |
Date: |
Thu, 11 Jun 2015 12:36:01 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
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 <address@hidden>
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>> 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,
>>>
>
>
- Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification, (continued)
[Qemu-devel] [PATCH 3/8] block: store persistent dirty bitmaps, Vladimir Sementsov-Ogievskiy, 2015/06/08
[Qemu-devel] [PATCH 6/8] qcow2: add autoclear bit for dirty bitmaps, Vladimir Sementsov-Ogievskiy, 2015/06/08
[Qemu-devel] [PATCH 7/8] qemu: command line option for dirty bitmaps, Vladimir Sementsov-Ogievskiy, 2015/06/08
[Qemu-devel] [PATCH 2/8] qcow2: add dirty-bitmaps feature, Vladimir Sementsov-Ogievskiy, 2015/06/08