From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34697) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6lUf-0002Kv-Qo for qemu-devel@nongnu.org; Tue, 15 Nov 2016 16:42:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6lUe-0003hI-AB for qemu-devel@nongnu.org; Tue, 15 Nov 2016 16:42:17 -0500 References: <1478715476-132280-1-git-send-email-vsementsov@virtuozzo.com> <1478715476-132280-8-git-send-email-vsementsov@virtuozzo.com> From: John Snow Message-ID: Date: Tue, 15 Nov 2016 16:42:08 -0500 MIME-Version: 1.0 In-Reply-To: <1478715476-132280-8-git-send-email-vsementsov@virtuozzo.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/21] qcow2: add dirty bitmaps extension List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, mreitz@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, den@openvz.org, Eric Blake 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 > --- > 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