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

[John Snow <jsnow@redhat.com>]

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>