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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3528 bytes --]

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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]