Message ID | 1479835586-74394-8-git-send-email-vsementsov@virtuozzo.com |
---|---|
State | New |
Headers | show |
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. (But this can be added in a later patch.) Reviewed-by: Max Reitz <mreitz@redhat.com>
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? Actually, all data will be removed on check, as there will be no refcounts if this bit is not set. So, IMHO, only extension header should be deleted additionally in qemu-img check. > > (But this can be added in a later patch.) > > Reviewed-by: Max Reitz <mreitz@redhat.com> >
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
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 >
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; + } + + ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len); + if (ret < 0) { + error_setg_errno(errp, -ret, "bitmaps_ext: " + "Could not read ext header"); + return ret; + } + + if (bitmaps_ext.reserved32 != 0) { + error_setg_errno(errp, -ret, "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 > QCOW2_MAX_BITMAPS) { + error_setg(errp, + "bitmaps_ext: File %s has %" PRIu32 " bitmaps, " + "exceeding the QEMU supported maximum of %d", + bs->filename, bitmaps_ext.nb_bitmaps, + QCOW2_MAX_BITMAPS); + return -EINVAL; + } + + if (bitmaps_ext.nb_bitmaps == 0) { + error_setg(errp, "found bitmaps extension with zero bitmaps"); + return -EINVAL; + } + + if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) { + error_setg(errp, "bitmaps_ext: " + "invalid bitmap directory offset"); + return -EINVAL; + } + + if (bitmaps_ext.bitmap_directory_size > + QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { + error_setg(errp, "bitmaps_ext: " + "bitmap directory size (%" PRIu64 ") exceeds " + "the maximum supported size (%d)", + bitmaps_ext.bitmap_directory_size, + QCOW2_MAX_BITMAP_DIRECTORY_SIZE); + 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 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 +1221,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 +2022,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_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 +2610,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 1823414..861b501 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 QCOW2_MAX_BITMAPS 65535 +#define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_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_BITMAPS_BITNR = 0, + QCOW2_AUTOCLEAR_BITMAPS = + 1 << QCOW2_AUTOCLEAR_BITMAPS_BITNR, + + QCOW2_AUTOCLEAR_MASK = QCOW2_AUTOCLEAR_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;