Re: [Qemu-devel] [PATCH 07/21] qcow2: add bitmaps extension

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

16.12.2016 17:25, Max Reitz wrote:
> On 14.12.2016 13:23, Vladimir Sementsov-Ogievskiy wrote:
>> 07.12.2016 21:25, Max Reitz wrote:
>>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add bitmap extension as specified in docs/specs/qcow2.txt.
>>>> For now, just mirror extension header into Qcow2 state and check
>>>> constraints.
>>>>
>>>> For now, disable image resize if it has bitmaps. It will be fixed later.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    block/qcow2.c | 107
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>    block/qcow2.h |  24 +++++++++++++
>>>>    2 files changed, 129 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 6d5689a..7908657 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -63,6 +63,7 @@ typedef struct {
>>>>    #define  QCOW2_EXT_MAGIC_END 0
>>>>    #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>>>    #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>>> +#define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
>>>>      static int qcow2_probe(const uint8_t *buf, int buf_size, const
>>>> char *filename)
>>>>    {
>>>> @@ -92,6 +93,7 @@ static int qcow2_read_extensions(BlockDriverState
>>>> *bs, uint64_t start_offset,
>>>>        QCowExtension ext;
>>>>        uint64_t offset;
>>>>        int ret;
>>>> +    Qcow2BitmapHeaderExt bitmaps_ext;
>>>>      #ifdef DEBUG_EXT
>>>>        printf("qcow2_read_extensions: start=%ld end=%ld\n",
>>>> start_offset, end_offset);
>>>> @@ -162,6 +164,81 @@ static int
>>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>                }
>>>>                break;
>>>>    +        case QCOW2_EXT_MAGIC_BITMAPS:
>>>> +            if (ext.len != sizeof(bitmaps_ext)) {
>>>> +                error_setg_errno(errp, -ret, "bitmaps_ext: "
>>>> +                                 "Invalid extension length");
>>>> +                return -EINVAL;
>>>> +            }
>>>> +
>>>> +            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) {
>>>> +                fprintf(stderr,
>>>> +                        "WARNING: a program lacking bitmap support
>>>> modified "
>>>> +                        "this file, so all bitmaps are now considered "
>>>> +                        "inconsistent");
>>>> +                break;
>>>> +            }
>>> I think It would be nice if qemu-img check could remove all bitmaps and
>>> related data if this bit is not set. Otherwise, we're practically
>>> leaking data.
>> Is it safe? Can old version somehow reuse these clusters?
> You're right, it can. It's not so easy then.
>
>>                                                            Actually, all
>> data will be removed on check, as there will be no refcounts if this bit
>> is not set.
> The refcounts will be there, but no references, right. So an old
> qemu-img check -r leaks will free the clusters.
>
> We could still remove this extension field, right? Then the clusters are
> at least really leaked so that even a new qemu-img check -r leaks will
> free them.
>
>> So, IMHO, only extension header should be deleted additionally in
>> qemu-img check.
> I think we could remove it here instead of only in qemu-img check, but
> either way should be fine. Doing it in qemu-img check is probably
> cleaner, actually.

Now I think it would be more deterministic to clear extension header 
here, as, actually, it will be cleared on any future qcow2_update_header().

>
> If you do it there, however, the warning should probably include a hint
> like "(Use 'qemu-img check -r leaks' to reclaim the space used for the
> bitmaps)".
>
> Max
>


-- 
Best regards,
Vladimir