[07/21] qcow2: add dirty bitmaps extension
diff mbox

Message ID 1478715476-132280-8-git-send-email-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Nov. 9, 2016, 6:17 p.m. UTC
Add dirty 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>
---
 block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 block/qcow2.h |  24 ++++++++++++++
 2 files changed, 123 insertions(+), 2 deletions(-)

Comments

John Snow Nov. 15, 2016, 9:42 p.m. UTC | #1
On 11/09/2016 01:17 PM, Vladimir Sementsov-Ogievskiy wrote:
> Add dirty 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>
> ---
>  block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  block/qcow2.h |  24 ++++++++++++++
>  2 files changed, 123 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6d5689a..c6050de 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_DIRTY_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);

Unrelated, the comment at the top of qcow2_read_extensions is sorely 
lacking.

> @@ -162,6 +164,75 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>              }
>              break;
>
> +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
> +            if (ext.len != sizeof(bitmaps_ext)) {
> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
> +                                 "Invalid extension length");
> +                return -EINVAL;
> +            }
> +
> +            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS)) {
> +                fprintf(stderr,
> +                        "WARNING: bitmaps_ext: autoclear flag is not "
> +                        "set, all bitmaps will be considered as inconsistent");
> +                break;
> +            }
> +

I might drop the "as" and just say "WARNING: bitmaps_ext: [the] 
autoclear flag is not set. All bitmaps will be considered inconsistent."

This may be a good place for Eric to check our English.

> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
> +                                 "Could not read ext header");
> +                return ret;
> +            }
> +
> +            if (bitmaps_ext.reserved32 != 0) {
> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
> +                                 "Reserved field is not zero");
> +                return -EINVAL;
> +            }
> +
> +            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
> +
> +            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "too many dirty bitmaps");

I might opt for something more like "File %s has %d bitmaps, exceeding 
the QEMU supported maximum of %d" to be a little more informative than 
"too many." (How many is too many? How many do we have?)

> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.nb_bitmaps == 0) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "found bitmaps extension with zero bitmaps");
> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "invalid dirty bitmap directory offset");
> +                return -EINVAL;
> +            }
> +

Don't we have a helper for checking alignment? We've got 
"validate_table_offset." Can we use that?

> +            if (bitmaps_ext.bitmap_directory_size >
> +                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "too large dirty bitmap directory");
> +                return -EINVAL;
> +            }
> +

"Too large dirty bitmap" is an awkward phrasing, because it turns the 
entire message into a large compound noun.

I suggest working in a verb into the message, like "is" or "exceeds," 
here are some suggestions:

"[the] dirty bitmap directory size is too large" or "[the] dirty bitmap 
directory size (%zu) exceeds [the] maximum supported size (%zu)"

I can't decide if it's appropriate to include or exclude the article.

Luckily nobody else knows how English works either.

> +            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
> +            s->bitmap_directory_offset =
> +                    bitmaps_ext.bitmap_directory_offset;
> +            s->bitmap_directory_size =
> +                    bitmaps_ext.bitmap_directory_size;
> +
> +#ifdef DEBUG_EXT
> +            printf("Qcow2: Got dirty bitmaps extension:"
> +                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
> +                   s->bitmap_directory_offset, s->nb_bitmaps);
> +#endif
> +            break;
> +
>          default:
>              /* unknown magic - save it in case we need to rewrite the header */
>              {
> @@ -1144,8 +1215,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>
>      /* Clear unknown autoclear feature bits */
> -    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) && s->autoclear_features) {
> -        s->autoclear_features = 0;
> +    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) &&
> +        (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
> +        s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>          ret = qcow2_update_header(bs);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "Could not update qcow2 header");
> @@ -1944,6 +2016,24 @@ int qcow2_update_header(BlockDriverState *bs)
>          buflen -= ret;
>      }
>
> +    if (s->nb_bitmaps > 0) {
> +        Qcow2BitmapHeaderExt bitmaps_header = {
> +            .nb_bitmaps = cpu_to_be32(s->nb_bitmaps),
> +            .bitmap_directory_size =
> +                    cpu_to_be64(s->bitmap_directory_size),
> +            .bitmap_directory_offset =
> +                    cpu_to_be64(s->bitmap_directory_offset)
> +        };
> +        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DIRTY_BITMAPS,
> +                             &bitmaps_header, sizeof(bitmaps_header),
> +                             buflen);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        buf += ret;
> +        buflen -= ret;
> +    }
> +
>      /* Keep unknown header extensions */
>      QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
>          ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
> @@ -2514,6 +2604,13 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>          return -ENOTSUP;
>      }
>
> +    /* cannot proceed if image has bitmaps */
> +    if (s->nb_bitmaps) {
> +        /* TODO: resize bitmaps in the image */
> +        error_report("Can't resize an image which has bitmaps");
> +        return -ENOTSUP;
> +    }
> +

I guess this is an open problem and not one we address in this series.

>      /* shrinking is currently not supported */
>      if (offset < bs->total_sectors * 512) {
>          error_report("qcow2 doesn't support shrinking images yet");
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 92203a8..be9ba32 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -52,6 +52,10 @@
>   * space for snapshot names and IDs */
>  #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
>
> +/* Bitmap header extension constraints */
> +#define QCOW_MAX_DIRTY_BITMAPS 65535
> +#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS)
> +

64MiB for just the headers seems like a really generous limit that we 
will never, ever, ever exhaust.

(I look forward to future versions of QEMU where this feedback item is 
laughed at playfully.)

>  /* indicate that the refcount of the referenced cluster is exactly one. */
>  #define QCOW_OFLAG_COPIED     (1ULL << 63)
>  /* indicate that the cluster is compressed (they never have the copied flag) */
> @@ -195,6 +199,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,
> +};
> +

This seems like a strange construct to me -- the ranges for _BITNR and 
mask variants will begin colliding as soon as we add just one more 
constant to the list.

Enumerations do allow this, but I generally don't like using a single 
enumeration to describe more than one thing.

Do we really need the BITNR variants at all? Why not the following:

enum {
     QCOW2_AUTOCLEAR_DIRTY_BITMAPS = 1 << 0,
};

#define QCOW2_AUTOCLEAR_MASK QCOW2_AUTOCLEAR_DIRTY_BITMAPS


>  enum qcow2_discard_type {
>      QCOW2_DISCARD_NEVER = 0,
>      QCOW2_DISCARD_ALWAYS,
> @@ -222,6 +235,13 @@ typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array,
>  typedef void Qcow2SetRefcountFunc(void *refcount_array,
>                                    uint64_t index, uint64_t value);
>
> +typedef struct Qcow2BitmapHeaderExt {
> +    uint32_t nb_bitmaps;
> +    uint32_t reserved32;
> +    uint64_t bitmap_directory_size;
> +    uint64_t bitmap_directory_offset;
> +} QEMU_PACKED Qcow2BitmapHeaderExt;
> +
>  typedef struct BDRVQcow2State {
>      int cluster_bits;
>      int cluster_size;
> @@ -263,6 +283,10 @@ typedef struct BDRVQcow2State {
>      unsigned int nb_snapshots;
>      QCowSnapshot *snapshots;
>
> +    uint32_t nb_bitmaps;
> +    uint64_t bitmap_directory_size;
> +    uint64_t bitmap_directory_offset;
> +
>      int flags;
>      int qcow_version;
>      bool use_lazy_refcounts;
>

Only style issues from my POV...

Reviewed-by: John Snow <jsnow@redhat.com>
Eric Blake Nov. 15, 2016, 10:39 p.m. UTC | #2
On 11/15/2016 03:42 PM, John Snow wrote:
> 
> 
> On 11/09/2016 01:17 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Add dirty 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>
>> ---

>> +            if (!(s->autoclear_features &
>> QCOW2_AUTOCLEAR_DIRTY_BITMAPS)) {
>> +                fprintf(stderr,
>> +                        "WARNING: bitmaps_ext: autoclear flag is not "
>> +                        "set, all bitmaps will be considered as
>> inconsistent");
>> +                break;
>> +            }
>> +
> 
> I might drop the "as" and just say "WARNING: bitmaps_ext: [the]
> autoclear flag is not set. All bitmaps will be considered inconsistent."

Even the 'bitmaps_ext:' prefix seems a bit redundant, since the rest of
the message talks about bitmaps.

> 
> This may be a good place for Eric to check our English.
> 

Maybe take the message from a different angle:

WARNING: all bitmaps are considered inconsistent since the autoclear
flag was cleared

or

WARNING: the autoclear flag was cleared, so all bitmaps are considered
inconsistent

or even skip the technical details, and report it with a longer message
but while still sounding legible:

WARNING: a program lacking bitmap support modified this file, so all
bitmaps are now considered inconsistent


>> +
>> +            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too many dirty bitmaps");
> 
> I might opt for something more like "File %s has %d bitmaps, exceeding
> the QEMU supported maximum of %d" to be a little more informative than
> "too many." (How many is too many? How many do we have?)
> 

The use of ERROR: in error_setg() seems over the top.

John's proposed wording is nice, here.

>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.nb_bitmaps == 0) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "found bitmaps extension with zero
>> bitmaps");

So why is it an error to have a bitmaps extension but no bitmaps
allocated?  Is that too strict?

Again, the ERROR: prefix is a bit much in error_setg().


> 
>> +            if (bitmaps_ext.bitmap_directory_size >
>> +                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too large dirty bitmap directory");
>> +                return -EINVAL;
>> +            }
>> +
> 
> "Too large dirty bitmap" is an awkward phrasing, because it turns the
> entire message into a large compound noun.
> 
> I suggest working in a verb into the message, like "is" or "exceeds,"
> here are some suggestions:
> 
> "[the] dirty bitmap directory size is too large" or "[the] dirty bitmap
> directory size (%zu) exceeds [the] maximum supported size (%zu)"

The latter is the most informative.

> 
> I can't decide if it's appropriate to include or exclude the article.

Yep, choosing when to use articles is sometimes subjective.

the/blank sounds odd - it's the only combo I'd avoid
blank/blank seems reasonable, and has the benefit of being short
blank/the seems reasonable
the/the seems rather formal, but still works

> 
> Luckily nobody else knows how English works either.

What, there's rules to follow?  :)
John Snow Nov. 15, 2016, 10:45 p.m. UTC | #3
On 11/15/2016 05:39 PM, Eric Blake wrote:
> On 11/15/2016 03:42 PM, John Snow wrote:
>>
>>
>> On 11/09/2016 01:17 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Add dirty 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>
>>> ---
>
>>> +            if (!(s->autoclear_features &
>>> QCOW2_AUTOCLEAR_DIRTY_BITMAPS)) {
>>> +                fprintf(stderr,
>>> +                        "WARNING: bitmaps_ext: autoclear flag is not "
>>> +                        "set, all bitmaps will be considered as
>>> inconsistent");
>>> +                break;
>>> +            }
>>> +
>>
>> I might drop the "as" and just say "WARNING: bitmaps_ext: [the]
>> autoclear flag is not set. All bitmaps will be considered inconsistent."
>
> Even the 'bitmaps_ext:' prefix seems a bit redundant, since the rest of
> the message talks about bitmaps.
>
>>
>> This may be a good place for Eric to check our English.
>>
>
> Maybe take the message from a different angle:
>
> WARNING: all bitmaps are considered inconsistent since the autoclear
> flag was cleared
>
> or
>
> WARNING: the autoclear flag was cleared, so all bitmaps are considered
> inconsistent
>
> or even skip the technical details, and report it with a longer message
> but while still sounding legible:
>
> WARNING: a program lacking bitmap support modified this file, so all
> bitmaps are now considered inconsistent
>
>
>>> +
>>> +            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
>>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>>> +                                 "too many dirty bitmaps");
>>
>> I might opt for something more like "File %s has %d bitmaps, exceeding
>> the QEMU supported maximum of %d" to be a little more informative than
>> "too many." (How many is too many? How many do we have?)
>>
>
> The use of ERROR: in error_setg() seems over the top.
>
> John's proposed wording is nice, here.
>
>>> +                return -EINVAL;
>>> +            }
>>> +
>>> +            if (bitmaps_ext.nb_bitmaps == 0) {
>>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>>> +                                 "found bitmaps extension with zero
>>> bitmaps");
>
> So why is it an error to have a bitmaps extension but no bitmaps
> allocated?  Is that too strict?
>
> Again, the ERROR: prefix is a bit much in error_setg().
>

We wrote it that way in the spec:

````
The fields of the bitmaps extension are:

     Byte  0 -  3:  nb_bitmaps
                    The number of bitmaps contained in the image. Must be
                    greater than or equal to 1.
````

So if we have no bitmaps, we must have no header.

>
>>
>>> +            if (bitmaps_ext.bitmap_directory_size >
>>> +                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
>>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>>> +                                 "too large dirty bitmap directory");
>>> +                return -EINVAL;
>>> +            }
>>> +
>>
>> "Too large dirty bitmap" is an awkward phrasing, because it turns the
>> entire message into a large compound noun.
>>
>> I suggest working in a verb into the message, like "is" or "exceeds,"
>> here are some suggestions:
>>
>> "[the] dirty bitmap directory size is too large" or "[the] dirty bitmap
>> directory size (%zu) exceeds [the] maximum supported size (%zu)"
>
> The latter is the most informative.
>
>>
>> I can't decide if it's appropriate to include or exclude the article.
>
> Yep, choosing when to use articles is sometimes subjective.
>
> the/blank sounds odd - it's the only combo I'd avoid
> blank/blank seems reasonable, and has the benefit of being short
> blank/the seems reasonable
> the/the seems rather formal, but still works
>

'Dirty bitmap directory size (%zu) exceeds the maximum supported size 
(%zu)' is probably my favorite upon review.

>>
>> Luckily nobody else knows how English works either.
>
> What, there's rules to follow?  :)
>

Not that I am aware of. Thanks for the proofreading.
Vladimir Sementsov-Ogievskiy Nov. 22, 2016, 12:58 p.m. UTC | #4
16.11.2016 00:42, John Snow wrote:
>
>
> On 11/09/2016 01:17 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Add dirty 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>
>> ---
>>  block/qcow2.c | 101 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  block/qcow2.h |  24 ++++++++++++++
>>  2 files changed, 123 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 6d5689a..c6050de 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_DIRTY_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);
>
> Unrelated, the comment at the top of qcow2_read_extensions is sorely 
> lacking.
>
>> @@ -162,6 +164,75 @@ static int 
>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>              }
>>              break;
>>
>> +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
>> +            if (ext.len != sizeof(bitmaps_ext)) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Invalid extension length");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (!(s->autoclear_features & 
>> QCOW2_AUTOCLEAR_DIRTY_BITMAPS)) {
>> +                fprintf(stderr,
>> +                        "WARNING: bitmaps_ext: autoclear flag is not "
>> +                        "set, all bitmaps will be considered as 
>> inconsistent");
>> +                break;
>> +            }
>> +
>
> I might drop the "as" and just say "WARNING: bitmaps_ext: [the] 
> autoclear flag is not set. All bitmaps will be considered inconsistent."
>
> This may be a good place for Eric to check our English.
>
>> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
>> +            if (ret < 0) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Could not read ext header");
>> +                return ret;
>> +            }
>> +
>> +            if (bitmaps_ext.reserved32 != 0) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Reserved field is not zero");
>> +                return -EINVAL;
>> +            }
>> +
>> +            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
>> + be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
>> + be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
>> +
>> +            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too many dirty bitmaps");
>
> I might opt for something more like "File %s has %d bitmaps, exceeding 
> the QEMU supported maximum of %d" to be a little more informative than 
> "too many." (How many is too many? How many do we have?)
>
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.nb_bitmaps == 0) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "found bitmaps extension with zero 
>> bitmaps");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.bitmap_directory_offset & 
>> (s->cluster_size - 1)) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "invalid dirty bitmap directory 
>> offset");
>> +                return -EINVAL;
>> +            }
>> +
>
> Don't we have a helper for checking alignment? We've got 
> "validate_table_offset." Can we use that?
>
>> +            if (bitmaps_ext.bitmap_directory_size >
>> +                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too large dirty bitmap directory");
>> +                return -EINVAL;
>> +            }
>> +
>
> "Too large dirty bitmap" is an awkward phrasing, because it turns the 
> entire message into a large compound noun.
>
> I suggest working in a verb into the message, like "is" or "exceeds," 
> here are some suggestions:
>
> "[the] dirty bitmap directory size is too large" or "[the] dirty 
> bitmap directory size (%zu) exceeds [the] maximum supported size (%zu)"
>
> I can't decide if it's appropriate to include or exclude the article.
>
> Luckily nobody else knows how English works either.
>
>> +            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
>> +            s->bitmap_directory_offset =
>> +                    bitmaps_ext.bitmap_directory_offset;
>> +            s->bitmap_directory_size =
>> +                    bitmaps_ext.bitmap_directory_size;
>> +
>> +#ifdef DEBUG_EXT
>> +            printf("Qcow2: Got dirty bitmaps extension:"
>> +                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
>> +                   s->bitmap_directory_offset, s->nb_bitmaps);
>> +#endif
>> +            break;
>> +
>>          default:
>>              /* unknown magic - save it in case we need to rewrite 
>> the header */
>>              {
>> @@ -1144,8 +1215,9 @@ static int qcow2_open(BlockDriverState *bs, 
>> QDict *options, int flags,
>>      }
>>
>>      /* Clear unknown autoclear feature bits */
>> -    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) && 
>> s->autoclear_features) {
>> -        s->autoclear_features = 0;
>> +    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) &&
>> +        (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
>> +        s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>>          ret = qcow2_update_header(bs);
>>          if (ret < 0) {
>>              error_setg_errno(errp, -ret, "Could not update qcow2 
>> header");
>> @@ -1944,6 +2016,24 @@ int qcow2_update_header(BlockDriverState *bs)
>>          buflen -= ret;
>>      }
>>
>> +    if (s->nb_bitmaps > 0) {
>> +        Qcow2BitmapHeaderExt bitmaps_header = {
>> +            .nb_bitmaps = cpu_to_be32(s->nb_bitmaps),
>> +            .bitmap_directory_size =
>> +                    cpu_to_be64(s->bitmap_directory_size),
>> +            .bitmap_directory_offset =
>> +                    cpu_to_be64(s->bitmap_directory_offset)
>> +        };
>> +        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DIRTY_BITMAPS,
>> +                             &bitmaps_header, sizeof(bitmaps_header),
>> +                             buflen);
>> +        if (ret < 0) {
>> +            goto fail;
>> +        }
>> +        buf += ret;
>> +        buflen -= ret;
>> +    }
>> +
>>      /* Keep unknown header extensions */
>>      QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
>>          ret = header_ext_add(buf, uext->magic, uext->data, 
>> uext->len, buflen);
>> @@ -2514,6 +2604,13 @@ static int qcow2_truncate(BlockDriverState 
>> *bs, int64_t offset)
>>          return -ENOTSUP;
>>      }
>>
>> +    /* cannot proceed if image has bitmaps */
>> +    if (s->nb_bitmaps) {
>> +        /* TODO: resize bitmaps in the image */
>> +        error_report("Can't resize an image which has bitmaps");
>> +        return -ENOTSUP;
>> +    }
>> +
>
> I guess this is an open problem and not one we address in this series.
>
>>      /* shrinking is currently not supported */
>>      if (offset < bs->total_sectors * 512) {
>>          error_report("qcow2 doesn't support shrinking images yet");
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 92203a8..be9ba32 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -52,6 +52,10 @@
>>   * space for snapshot names and IDs */
>>  #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
>>
>> +/* Bitmap header extension constraints */
>> +#define QCOW_MAX_DIRTY_BITMAPS 65535
>> +#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * 
>> QCOW_MAX_DIRTY_BITMAPS)
>> +
>
> 64MiB for just the headers seems like a really generous limit that we 
> will never, ever, ever exhaust.
>
> (I look forward to future versions of QEMU where this feedback item is 
> laughed at playfully.)
>
>>  /* indicate that the refcount of the referenced cluster is exactly 
>> one. */
>>  #define QCOW_OFLAG_COPIED     (1ULL << 63)
>>  /* indicate that the cluster is compressed (they never have the 
>> copied flag) */
>> @@ -195,6 +199,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,
>> +};
>> +
>
> This seems like a strange construct to me -- the ranges for _BITNR and 
> mask variants will begin colliding as soon as we add just one more 
> constant to the list.
>
> Enumerations do allow this, but I generally don't like using a single 
> enumeration to describe more than one thing.
>
> Do we really need the BITNR variants at all? Why not the following:
>
> enum {
>     QCOW2_AUTOCLEAR_DIRTY_BITMAPS = 1 << 0,
> };
>
> #define QCOW2_AUTOCLEAR_MASK QCOW2_AUTOCLEAR_DIRTY_BITMAPS


I've just copied the same structure for 'Compatible feature bits'. And 
it was even already discussed). Anyway, this code should be refactored 
with separate patch, before or after these series.

>
>
>>  enum qcow2_discard_type {
>>      QCOW2_DISCARD_NEVER = 0,
>>      QCOW2_DISCARD_ALWAYS,
>> @@ -222,6 +235,13 @@ typedef uint64_t Qcow2GetRefcountFunc(const void 
>> *refcount_array,
>>  typedef void Qcow2SetRefcountFunc(void *refcount_array,
>>                                    uint64_t index, uint64_t value);
>>
>> +typedef struct Qcow2BitmapHeaderExt {
>> +    uint32_t nb_bitmaps;
>> +    uint32_t reserved32;
>> +    uint64_t bitmap_directory_size;
>> +    uint64_t bitmap_directory_offset;
>> +} QEMU_PACKED Qcow2BitmapHeaderExt;
>> +
>>  typedef struct BDRVQcow2State {
>>      int cluster_bits;
>>      int cluster_size;
>> @@ -263,6 +283,10 @@ typedef struct BDRVQcow2State {
>>      unsigned int nb_snapshots;
>>      QCowSnapshot *snapshots;
>>
>> +    uint32_t nb_bitmaps;
>> +    uint64_t bitmap_directory_size;
>> +    uint64_t bitmap_directory_offset;
>> +
>>      int flags;
>>      int qcow_version;
>>      bool use_lazy_refcounts;
>>
>
> Only style issues from my POV...
>
> Reviewed-by: John Snow <jsnow@redhat.com>
Vladimir Sementsov-Ogievskiy Nov. 22, 2016, 4:11 p.m. UTC | #5
16.11.2016 00:42, John Snow wrote:
>
>
> On 11/09/2016 01:17 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Add dirty 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>
>> ---
>>  block/qcow2.c | 101 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  block/qcow2.h |  24 ++++++++++++++
>>  2 files changed, 123 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 6d5689a..c6050de 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_DIRTY_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);
>
> Unrelated, the comment at the top of qcow2_read_extensions is sorely 
> lacking.
>
>> @@ -162,6 +164,75 @@ static int 
>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>              }
>>              break;
>>
>> +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
>> +            if (ext.len != sizeof(bitmaps_ext)) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Invalid extension length");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (!(s->autoclear_features & 
>> QCOW2_AUTOCLEAR_DIRTY_BITMAPS)) {
>> +                fprintf(stderr,
>> +                        "WARNING: bitmaps_ext: autoclear flag is not "
>> +                        "set, all bitmaps will be considered as 
>> inconsistent");
>> +                break;
>> +            }
>> +
>
> I might drop the "as" and just say "WARNING: bitmaps_ext: [the] 
> autoclear flag is not set. All bitmaps will be considered inconsistent."
>
> This may be a good place for Eric to check our English.
>
>> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
>> +            if (ret < 0) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Could not read ext header");
>> +                return ret;
>> +            }
>> +
>> +            if (bitmaps_ext.reserved32 != 0) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Reserved field is not zero");
>> +                return -EINVAL;
>> +            }
>> +
>> +            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
>> + be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
>> + be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
>> +
>> +            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too many dirty bitmaps");
>
> I might opt for something more like "File %s has %d bitmaps, exceeding 
> the QEMU supported maximum of %d" to be a little more informative than 
> "too many." (How many is too many? How many do we have?)
>
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.nb_bitmaps == 0) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "found bitmaps extension with zero 
>> bitmaps");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.bitmap_directory_offset & 
>> (s->cluster_size - 1)) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "invalid dirty bitmap directory 
>> offset");
>> +                return -EINVAL;
>> +            }
>> +
>
> Don't we have a helper for checking alignment? We've got 
> "validate_table_offset." Can we use that?

function validate_table_offset() is strange, it actually validates not 
offset, but offset, size and entry-size.. It should be probably renamed 
to vailidate_table()..  And I have no "entry size" for bitmap directory, 
as it's entries are of different sizes.

>
>> +            if (bitmaps_ext.bitmap_directory_size >
>> +                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too large dirty bitmap directory");
>> +                return -EINVAL;
>> +            }
>> +
>
> "Too large dirty bitmap" is an awkward phrasing, because it turns the 
> entire message into a large compound noun.
>
> I suggest working in a verb into the message, like "is" or "exceeds," 
> here are some suggestions:
>
> "[the] dirty bitmap directory size is too large" or "[the] dirty 
> bitmap directory size (%zu) exceeds [the] maximum supported size (%zu)"
>
> I can't decide if it's appropriate to include or exclude the article.
>
> Luckily nobody else knows how English works either.
>
>> +            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
>> +            s->bitmap_directory_offset =
>> +                    bitmaps_ext.bitmap_directory_offset;
>> +            s->bitmap_directory_size =
>> +                    bitmaps_ext.bitmap_directory_size;
>> +
>> +#ifdef DEBUG_EXT
>> +            printf("Qcow2: Got dirty bitmaps extension:"
>> +                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
>> +                   s->bitmap_directory_offset, s->nb_bitmaps);
>> +#endif
>> +            break;
>> +
>>          default:
>>              /* unknown magic - save it in case we need to rewrite 
>> the header */
>>              {
>> @@ -1144,8 +1215,9 @@ static int qcow2_open(BlockDriverState *bs, 
>> QDict *options, int flags,
>>      }
>>
>>      /* Clear unknown autoclear feature bits */
>> -    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) && 
>> s->autoclear_features) {
>> -        s->autoclear_features = 0;
>> +    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) &&
>> +        (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
>> +        s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>>          ret = qcow2_update_header(bs);
>>          if (ret < 0) {
>>              error_setg_errno(errp, -ret, "Could not update qcow2 
>> header");
>> @@ -1944,6 +2016,24 @@ int qcow2_update_header(BlockDriverState *bs)
>>          buflen -= ret;
>>      }
>>
>> +    if (s->nb_bitmaps > 0) {
>> +        Qcow2BitmapHeaderExt bitmaps_header = {
>> +            .nb_bitmaps = cpu_to_be32(s->nb_bitmaps),
>> +            .bitmap_directory_size =
>> +                    cpu_to_be64(s->bitmap_directory_size),
>> +            .bitmap_directory_offset =
>> +                    cpu_to_be64(s->bitmap_directory_offset)
>> +        };
>> +        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DIRTY_BITMAPS,
>> +                             &bitmaps_header, sizeof(bitmaps_header),
>> +                             buflen);
>> +        if (ret < 0) {
>> +            goto fail;
>> +        }
>> +        buf += ret;
>> +        buflen -= ret;
>> +    }
>> +
>>      /* Keep unknown header extensions */
>>      QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
>>          ret = header_ext_add(buf, uext->magic, uext->data, 
>> uext->len, buflen);
>> @@ -2514,6 +2604,13 @@ static int qcow2_truncate(BlockDriverState 
>> *bs, int64_t offset)
>>          return -ENOTSUP;
>>      }
>>
>> +    /* cannot proceed if image has bitmaps */
>> +    if (s->nb_bitmaps) {
>> +        /* TODO: resize bitmaps in the image */
>> +        error_report("Can't resize an image which has bitmaps");
>> +        return -ENOTSUP;
>> +    }
>> +
>
> I guess this is an open problem and not one we address in this series.
>
>>      /* shrinking is currently not supported */
>>      if (offset < bs->total_sectors * 512) {
>>          error_report("qcow2 doesn't support shrinking images yet");
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 92203a8..be9ba32 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -52,6 +52,10 @@
>>   * space for snapshot names and IDs */
>>  #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
>>
>> +/* Bitmap header extension constraints */
>> +#define QCOW_MAX_DIRTY_BITMAPS 65535
>> +#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * 
>> QCOW_MAX_DIRTY_BITMAPS)
>> +
>
> 64MiB for just the headers seems like a really generous limit that we 
> will never, ever, ever exhaust.
>
> (I look forward to future versions of QEMU where this feedback item is 
> laughed at playfully.)
>
>>  /* indicate that the refcount of the referenced cluster is exactly 
>> one. */
>>  #define QCOW_OFLAG_COPIED     (1ULL << 63)
>>  /* indicate that the cluster is compressed (they never have the 
>> copied flag) */
>> @@ -195,6 +199,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,
>> +};
>> +
>
> This seems like a strange construct to me -- the ranges for _BITNR and 
> mask variants will begin colliding as soon as we add just one more 
> constant to the list.
>
> Enumerations do allow this, but I generally don't like using a single 
> enumeration to describe more than one thing.
>
> Do we really need the BITNR variants at all? Why not the following:
>
> enum {
>     QCOW2_AUTOCLEAR_DIRTY_BITMAPS = 1 << 0,
> };
>
> #define QCOW2_AUTOCLEAR_MASK QCOW2_AUTOCLEAR_DIRTY_BITMAPS
>
>
>>  enum qcow2_discard_type {
>>      QCOW2_DISCARD_NEVER = 0,
>>      QCOW2_DISCARD_ALWAYS,
>> @@ -222,6 +235,13 @@ typedef uint64_t Qcow2GetRefcountFunc(const void 
>> *refcount_array,
>>  typedef void Qcow2SetRefcountFunc(void *refcount_array,
>>                                    uint64_t index, uint64_t value);
>>
>> +typedef struct Qcow2BitmapHeaderExt {
>> +    uint32_t nb_bitmaps;
>> +    uint32_t reserved32;
>> +    uint64_t bitmap_directory_size;
>> +    uint64_t bitmap_directory_offset;
>> +} QEMU_PACKED Qcow2BitmapHeaderExt;
>> +
>>  typedef struct BDRVQcow2State {
>>      int cluster_bits;
>>      int cluster_size;
>> @@ -263,6 +283,10 @@ typedef struct BDRVQcow2State {
>>      unsigned int nb_snapshots;
>>      QCowSnapshot *snapshots;
>>
>> +    uint32_t nb_bitmaps;
>> +    uint64_t bitmap_directory_size;
>> +    uint64_t bitmap_directory_offset;
>> +
>>      int flags;
>>      int qcow_version;
>>      bool use_lazy_refcounts;
>>
>
> Only style issues from my POV...
>
> Reviewed-by: John Snow <jsnow@redhat.com>

Patch
diff mbox

diff --git a/block/qcow2.c b/block/qcow2.c
index 6d5689a..c6050de 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_DIRTY_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,75 @@  static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             break;
 
+        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
+            if (ext.len != sizeof(bitmaps_ext)) {
+                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+                                 "Invalid extension length");
+                return -EINVAL;
+            }
+
+            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS)) {
+                fprintf(stderr,
+                        "WARNING: bitmaps_ext: autoclear flag is not "
+                        "set, all bitmaps will be considered as inconsistent");
+                break;
+            }
+
+            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+                                 "Could not read ext header");
+                return ret;
+            }
+
+            if (bitmaps_ext.reserved32 != 0) {
+                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+                                 "Reserved field is not zero");
+                return -EINVAL;
+            }
+
+            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
+            be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
+            be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
+
+            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "too many dirty bitmaps");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.nb_bitmaps == 0) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "found bitmaps extension with zero bitmaps");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "invalid dirty bitmap directory offset");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.bitmap_directory_size >
+                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "too large dirty bitmap directory");
+                return -EINVAL;
+            }
+
+            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
+            s->bitmap_directory_offset =
+                    bitmaps_ext.bitmap_directory_offset;
+            s->bitmap_directory_size =
+                    bitmaps_ext.bitmap_directory_size;
+
+#ifdef DEBUG_EXT
+            printf("Qcow2: Got dirty bitmaps extension:"
+                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
+                   s->bitmap_directory_offset, s->nb_bitmaps);
+#endif
+            break;
+
         default:
             /* unknown magic - save it in case we need to rewrite the header */
             {
@@ -1144,8 +1215,9 @@  static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Clear unknown autoclear feature bits */
-    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) && s->autoclear_features) {
-        s->autoclear_features = 0;
+    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) &&
+        (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
+        s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
         ret = qcow2_update_header(bs);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not update qcow2 header");
@@ -1944,6 +2016,24 @@  int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    if (s->nb_bitmaps > 0) {
+        Qcow2BitmapHeaderExt bitmaps_header = {
+            .nb_bitmaps = cpu_to_be32(s->nb_bitmaps),
+            .bitmap_directory_size =
+                    cpu_to_be64(s->bitmap_directory_size),
+            .bitmap_directory_offset =
+                    cpu_to_be64(s->bitmap_directory_offset)
+        };
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DIRTY_BITMAPS,
+                             &bitmaps_header, sizeof(bitmaps_header),
+                             buflen);
+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+    }
+
     /* Keep unknown header extensions */
     QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
         ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
@@ -2514,6 +2604,13 @@  static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     }
 
+    /* cannot proceed if image has bitmaps */
+    if (s->nb_bitmaps) {
+        /* TODO: resize bitmaps in the image */
+        error_report("Can't resize an image which has bitmaps");
+        return -ENOTSUP;
+    }
+
     /* shrinking is currently not supported */
     if (offset < bs->total_sectors * 512) {
         error_report("qcow2 doesn't support shrinking images yet");
diff --git a/block/qcow2.h b/block/qcow2.h
index 92203a8..be9ba32 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -52,6 +52,10 @@ 
  * space for snapshot names and IDs */
 #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
 
+/* Bitmap header extension constraints */
+#define QCOW_MAX_DIRTY_BITMAPS 65535
+#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS)
+
 /* indicate that the refcount of the referenced cluster is exactly one. */
 #define QCOW_OFLAG_COPIED     (1ULL << 63)
 /* indicate that the cluster is compressed (they never have the copied flag) */
@@ -195,6 +199,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,
+};
+
 enum qcow2_discard_type {
     QCOW2_DISCARD_NEVER = 0,
     QCOW2_DISCARD_ALWAYS,
@@ -222,6 +235,13 @@  typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array,
 typedef void Qcow2SetRefcountFunc(void *refcount_array,
                                   uint64_t index, uint64_t value);
 
+typedef struct Qcow2BitmapHeaderExt {
+    uint32_t nb_bitmaps;
+    uint32_t reserved32;
+    uint64_t bitmap_directory_size;
+    uint64_t bitmap_directory_offset;
+} QEMU_PACKED Qcow2BitmapHeaderExt;
+
 typedef struct BDRVQcow2State {
     int cluster_bits;
     int cluster_size;
@@ -263,6 +283,10 @@  typedef struct BDRVQcow2State {
     unsigned int nb_snapshots;
     QCowSnapshot *snapshots;
 
+    uint32_t nb_bitmaps;
+    uint64_t bitmap_directory_size;
+    uint64_t bitmap_directory_offset;
+
     int flags;
     int qcow_version;
     bool use_lazy_refcounts;