Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
On 21.10.2016 13:59, Vladimir Sementsov-Ogievskiy wrote: > 07.10.2016 22:25, Max Reitz пишет: >> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are >>> loaded at image open and becomes BdrvDirtyBitmap's for corresponding >>> drive. These bitmaps are deleted from Qcow2 image after loading to avoid >>> conflicts. >>> >>> Extra data in bitmaps is not supported for now. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> --- >>> block/qcow2-bitmap.c | 518 >>> ++- >>> block/qcow2.c| 5 + >>> block/qcow2.h| 3 + >>> 3 files changed, 525 insertions(+), 1 deletion(-) >>> >>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>> index cd18b07..760a047 100644 >>> --- a/block/qcow2-bitmap.c >>> +++ b/block/qcow2-bitmap.c >> [...] >> >>> +static int directory_update(BlockDriverState *bs, uint8_t *new_dir, >>> +size_t new_size, uint32_t new_nb_bitmaps) >>> +{ >>> +BDRVQcow2State *s = bs->opaque; >>> +int ret; >>> +int64_t new_offset = 0; >>> +uint64_t old_offset = s->bitmap_directory_offset; >>> +uint64_t old_size = s->bitmap_directory_size; >>> +uint32_t old_nb_bitmaps = s->nb_bitmaps; >>> +uint64_t old_autocl = s->autoclear_features; >>> + >>> +if (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) { >>> +return -EINVAL; >>> +} >>> + >>> +if ((new_size == 0) != (new_nb_bitmaps == 0)) { >>> +return -EINVAL; >>> +} >>> + >>> +if (new_nb_bitmaps > 0) { >>> +new_offset = directory_write(bs, new_dir, new_size); >>> +if (new_offset < 0) { >>> +return new_offset; >>> +} >>> + >>> +ret = bdrv_flush(bs); >>> +if (ret < 0) { >>> +goto fail; >>> +} >>> +} >>> +s->bitmap_directory_offset = new_offset; >>> +s->bitmap_directory_size = new_size; >>> +s->nb_bitmaps = new_nb_bitmaps; >>> + >>> +ret = update_header_sync(bs); >>> +if (ret < 0) { >>> +goto fail; >>> +} >>> + >>> +if (old_size) { >>> +qcow2_free_clusters(bs, old_offset, old_size, >>> QCOW2_DISCARD_ALWAYS); >>> +} >>> + >>> +return 0; >>> + >>> +fail: >>> +if (new_offset > 0) { >>> +qcow2_free_clusters(bs, new_offset, new_size, >>> QCOW2_DISCARD_ALWAYS); >>> +s->bitmap_directory_offset = old_offset; >>> +s->bitmap_directory_size = old_size; >>> +s->nb_bitmaps = old_nb_bitmaps; >>> +s->autoclear_features = old_autocl; >> What if bdrv_flush() in update_header_sync() failed? Then you cannot be >> sure what bitmap directory the header is actually pointing to. In that >> case, it would be wrong to assume it's still pointing to the old one and >> freeing the new one. >> >> Max > > Hmm.. Good point. Any suggestions? > > I'm trying to achieve some kind of transaction - if function failed file > is not changed.. But now I understand that because of flush ability to > fail I can't achieve this.. > > Has write and flush any guaranties in case of fail? Would it be > old-or-new state, or some mixed state is possible? Anything is possible, I think. However, if flushing the data fails, it still means writing it was successful -- you just don't know whether it has been written to the disk successfully. The most correct way out would be to signal image corruption, but that isn't what we should do, I think. If the write operation was successful but flushing was not, I'd always assume the data hopefully gets flushed later and continue as if it had been written to disk successfully. Therefore, I think it would be best for update_header_sync() to just ignore bdrv_flush()'s return value. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
On 20.10.2016 14:22, Vladimir Sementsov-Ogievskiy wrote: > On 01.10.2016 19:26, Max Reitz wrote: >> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are > > [...] > >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 08c4ef9..02ec224 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -213,6 +213,11 @@ static int qcow2_read_extensions(BlockDriverState >> *bs, uint64_t start_offset, >> s->bitmap_directory_size = >> bitmaps_ext.bitmap_directory_size; >> +ret = qcow2_read_bitmaps(bs, errp); >> +if (ret < 0) { >> +return ret; >> +} >> + >> I think I'd put this directly into qcow2_open(), just like >> qcow2_read_snapshots(); but that's an optional suggestion. >> >> Max >> >> > > Snapshots are not header extension.. so it is not the case. Here > qcow2_read_bitmaps looks like part of header extension loading, and > header extension fields describe other parts of the extension.. I think > this is a good point, isn't it? I said it's optional, so it's optional. :-) I personally feel like a header extension is just the bit of data in the qcow2 header. The bitmaps itself are managed by that extension, but not part of the extension itself. Therefore, I wouldn't load it here but in the main function qcow2_open(). However, this is a personal opinion and thus it's an optional suggestion (as I said), so if you disagree (which you apparently do) then don't let it bother you. :-) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
07.10.2016 22:25, Max Reitz пишет: On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are loaded at image open and becomes BdrvDirtyBitmap's for corresponding drive. These bitmaps are deleted from Qcow2 image after loading to avoid conflicts. Extra data in bitmaps is not supported for now. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/qcow2-bitmap.c | 518 ++- block/qcow2.c| 5 + block/qcow2.h| 3 + 3 files changed, 525 insertions(+), 1 deletion(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index cd18b07..760a047 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c [...] +static int directory_update(BlockDriverState *bs, uint8_t *new_dir, +size_t new_size, uint32_t new_nb_bitmaps) +{ +BDRVQcow2State *s = bs->opaque; +int ret; +int64_t new_offset = 0; +uint64_t old_offset = s->bitmap_directory_offset; +uint64_t old_size = s->bitmap_directory_size; +uint32_t old_nb_bitmaps = s->nb_bitmaps; +uint64_t old_autocl = s->autoclear_features; + +if (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) { +return -EINVAL; +} + +if ((new_size == 0) != (new_nb_bitmaps == 0)) { +return -EINVAL; +} + +if (new_nb_bitmaps > 0) { +new_offset = directory_write(bs, new_dir, new_size); +if (new_offset < 0) { +return new_offset; +} + +ret = bdrv_flush(bs); +if (ret < 0) { +goto fail; +} +} +s->bitmap_directory_offset = new_offset; +s->bitmap_directory_size = new_size; +s->nb_bitmaps = new_nb_bitmaps; + +ret = update_header_sync(bs); +if (ret < 0) { +goto fail; +} + +if (old_size) { +qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS); +} + +return 0; + +fail: +if (new_offset > 0) { +qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS); +s->bitmap_directory_offset = old_offset; +s->bitmap_directory_size = old_size; +s->nb_bitmaps = old_nb_bitmaps; +s->autoclear_features = old_autocl; What if bdrv_flush() in update_header_sync() failed? Then you cannot be sure what bitmap directory the header is actually pointing to. In that case, it would be wrong to assume it's still pointing to the old one and freeing the new one. Max Hmm.. Good point. Any suggestions? I'm trying to achieve some kind of transaction - if function failed file is not changed.. But now I understand that because of flush ability to fail I can't achieve this.. Has write and flush any guaranties in case of fail? Would it be old-or-new state, or some mixed state is possible? +} + +return ret; +} -- Best regards, Vladimir
Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
On 01.10.2016 19:26, Max Reitz wrote: On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are [...] diff --git a/block/qcow2.c b/block/qcow2.c index 08c4ef9..02ec224 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -213,6 +213,11 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, s->bitmap_directory_size = bitmaps_ext.bitmap_directory_size; +ret = qcow2_read_bitmaps(bs, errp); +if (ret < 0) { +return ret; +} + I think I'd put this directly into qcow2_open(), just like qcow2_read_snapshots(); but that's an optional suggestion. Max Snapshots are not header extension.. so it is not the case. Here qcow2_read_bitmaps looks like part of header extension loading, and header extension fields describe other parts of the extension.. I think this is a good point, isn't it? -- Best regards, Vladimir
Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
On 15.10.2016 20:03, Max Reitz wrote: On 14.10.2016 20:44, Vladimir Sementsov-Ogievskiy wrote: On 01.10.2016 19:26, Max Reitz wrote: On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are ... +goto fail; +} + +/* loop is safe because next entry offset is calculated after conversion to Should it be s/safe/unsafe/? loop is safe. _unsafe is related to absence of assert in a loop, as it loops through BE data. Bad wording, I'll change it somehow.. Yes, I know that the loop is safe in the sense of the word "safe", but I meant that it's kind of confusing that the loop uses "for_each_bitmap_dir_entry_unsafe" and thus is "unsafe", too. Another idea would be to write "This is actually safe" instead of "loop is safe". Finally I've decided to introduce normal list of normal structures like in snapshots.. + * cpu format */ +for_each_bitmap_dir_entry_unsafe(e, dir, size) { +if ((uint8_t *)(e + 1) > dir_end) { +goto broken_dir; +} + +bitmap_dir_entry_to_cpu(e); + +if ((uint8_t *)next_dir_entry(e) > dir_end) { +goto broken_dir; +} + +if (e->extra_data_size != 0) { +error_setg(errp, "Can't load bitmap '%.*s' from '%s':" + "extra data not supported.", e->name_size, Full stop again. Also, I'm not quite sure why you give the device/node name here. You don't do that anywhere else and I think if we want to emit the information where something failed, it should be added at some previous point in the call chain. For example, how? As I understand, I can emit it only by error_setg, so actually it would be better to add node and bitmap name to all error_setg in the code.. or create helper function for it. error_prepend() would be the function. This code will be invoked by any code that is opening an image. There are of course a couple of places where that can be the case: For external tools such as qemu-img, it's normally pretty clear which image is meant (and it additionally uses error_reportf_err() to give you more information). For -drive, every error message will at least be prepended by the corresponding -drive parameter, so that will help a bit. blockdev-add, unfortunately, doesn't do anything like this. But the user can of course choose to construct the BDS graph node by node and thus always know which node an error originates from. Anyway, if you want to add this information to every error message, you should probably do so in bdrv_open_inherit(). The issue I'd take with this is that most users probably won't set the node name themselves (either they don't at all or it's some management tool that does), so reporting the node name doesn't actually help them at all (and management tools are not supposed to parse error messages, so it won't help in that case either). Max Thanks for explanations, and for the whole review, it's great! Sorry for my laziness and for spelling( O! I've discovered vim spell, so one problem less, I hope. -- Best regards, Vladimir
Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
On 14.10.2016 20:44, Vladimir Sementsov-Ogievskiy wrote: > On 01.10.2016 19:26, Max Reitz wrote: >> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are > > ... > >> +goto fail; >> +} >> + >> +/* loop is safe because next entry offset is calculated after >> conversion to >> Should it be s/safe/unsafe/? > > loop is safe. _unsafe is related to absence of assert in a loop, as it > loops through BE data. Bad wording, I'll change it somehow.. Yes, I know that the loop is safe in the sense of the word "safe", but I meant that it's kind of confusing that the loop uses "for_each_bitmap_dir_entry_unsafe" and thus is "unsafe", too. Another idea would be to write "This is actually safe" instead of "loop is safe". >>> + * cpu format */ >>> +for_each_bitmap_dir_entry_unsafe(e, dir, size) { >>> +if ((uint8_t *)(e + 1) > dir_end) { >>> +goto broken_dir; >>> +} >>> + >>> +bitmap_dir_entry_to_cpu(e); >>> + >>> +if ((uint8_t *)next_dir_entry(e) > dir_end) { >>> +goto broken_dir; >>> +} >>> + >>> +if (e->extra_data_size != 0) { >>> +error_setg(errp, "Can't load bitmap '%.*s' from '%s':" >>> + "extra data not supported.", e->name_size, >> Full stop again. >> >> Also, I'm not quite sure why you give the device/node name here. You >> don't do that anywhere else and I think if we want to emit the >> information where something failed, it should be added at some previous >> point in the call chain. > > For example, how? As I understand, I can emit it only by error_setg, so > actually it would be better to add node and bitmap name to all > error_setg in the code.. or create helper function for it. error_prepend() would be the function. This code will be invoked by any code that is opening an image. There are of course a couple of places where that can be the case: For external tools such as qemu-img, it's normally pretty clear which image is meant (and it additionally uses error_reportf_err() to give you more information). For -drive, every error message will at least be prepended by the corresponding -drive parameter, so that will help a bit. blockdev-add, unfortunately, doesn't do anything like this. But the user can of course choose to construct the BDS graph node by node and thus always know which node an error originates from. Anyway, if you want to add this information to every error message, you should probably do so in bdrv_open_inherit(). The issue I'd take with this is that most users probably won't set the node name themselves (either they don't at all or it's some management tool that does), so reporting the node name doesn't actually help them at all (and management tools are not supposed to parse error messages, so it won't help in that case either). Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
On 01.10.2016 19:26, Max Reitz wrote: On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are ... +goto fail; +} + +/* loop is safe because next entry offset is calculated after conversion to Should it be s/safe/unsafe/? loop is safe. _unsafe is related to absence of assert in a loop, as it loops through BE data. Bad wording, I'll change it somehow.. + * cpu format */ +for_each_bitmap_dir_entry_unsafe(e, dir, size) { +if ((uint8_t *)(e + 1) > dir_end) { +goto broken_dir; +} + +bitmap_dir_entry_to_cpu(e); + +if ((uint8_t *)next_dir_entry(e) > dir_end) { +goto broken_dir; +} + +if (e->extra_data_size != 0) { +error_setg(errp, "Can't load bitmap '%.*s' from '%s':" + "extra data not supported.", e->name_size, Full stop again. Also, I'm not quite sure why you give the device/node name here. You don't do that anywhere else and I think if we want to emit the information where something failed, it should be added at some previous point in the call chain. For example, how? As I understand, I can emit it only by error_setg, so actually it would be better to add node and bitmap name to all error_setg in the code.. or create helper function for it. + dir_entry_name_notcstr(e), + bdrv_get_device_or_node_name(bs)); +goto fail; +} +} ... +if (ret < 0) { +goto fail; +} +} +s->bitmap_directory_offset = new_offset; +s->bitmap_directory_size = new_size; +s->nb_bitmaps = new_nb_bitmaps; + +ret = update_header_sync(bs); +if (ret < 0) { +goto fail; +} + +if (old_size) { +qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS); +} + +return 0; + +fail: +if (new_offset > 0) { +qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS); +s->bitmap_directory_offset = old_offset; +s->bitmap_directory_size = old_size; +s->nb_bitmaps = old_nb_bitmaps; +s->autoclear_features = old_autocl; Why are you restoring the autoclear features? From a quick glance I can't see any code path that changes this field here, and if there is one, it probably has a good reason to do so. hmm.. it's an artefact from future). should be moved to later patch. -- Best regards, Vladimir
Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are > loaded at image open and becomes BdrvDirtyBitmap's for corresponding > drive. These bitmaps are deleted from Qcow2 image after loading to avoid > conflicts. > > Extra data in bitmaps is not supported for now. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-bitmap.c | 518 > ++- > block/qcow2.c| 5 + > block/qcow2.h| 3 + > 3 files changed, 525 insertions(+), 1 deletion(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index cd18b07..760a047 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c [...] > +static int directory_update(BlockDriverState *bs, uint8_t *new_dir, > +size_t new_size, uint32_t new_nb_bitmaps) > +{ > +BDRVQcow2State *s = bs->opaque; > +int ret; > +int64_t new_offset = 0; > +uint64_t old_offset = s->bitmap_directory_offset; > +uint64_t old_size = s->bitmap_directory_size; > +uint32_t old_nb_bitmaps = s->nb_bitmaps; > +uint64_t old_autocl = s->autoclear_features; > + > +if (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) { > +return -EINVAL; > +} > + > +if ((new_size == 0) != (new_nb_bitmaps == 0)) { > +return -EINVAL; > +} > + > +if (new_nb_bitmaps > 0) { > +new_offset = directory_write(bs, new_dir, new_size); > +if (new_offset < 0) { > +return new_offset; > +} > + > +ret = bdrv_flush(bs); > +if (ret < 0) { > +goto fail; > +} > +} > +s->bitmap_directory_offset = new_offset; > +s->bitmap_directory_size = new_size; > +s->nb_bitmaps = new_nb_bitmaps; > + > +ret = update_header_sync(bs); > +if (ret < 0) { > +goto fail; > +} > + > +if (old_size) { > +qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS); > +} > + > +return 0; > + > +fail: > +if (new_offset > 0) { > +qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS); > +s->bitmap_directory_offset = old_offset; > +s->bitmap_directory_size = old_size; > +s->nb_bitmaps = old_nb_bitmaps; > +s->autoclear_features = old_autocl; What if bdrv_flush() in update_header_sync() failed? Then you cannot be sure what bitmap directory the header is actually pointing to. In that case, it would be wrong to assume it's still pointing to the old one and freeing the new one. Max > +} > + > +return ret; > +} signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are *with the AUTO flag set > loaded at image open and becomes BdrvDirtyBitmap's for corresponding "loaded when the image is opened and become BdrvDirtyBitmaps for the corresponding drive." > drive. These bitmaps are deleted from Qcow2 image after loading to avoid *from the Qcow2 image > conflicts. > > Extra data in bitmaps is not supported for now. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-bitmap.c | 518 > ++- > block/qcow2.c| 5 + > block/qcow2.h| 3 + > 3 files changed, 525 insertions(+), 1 deletion(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index cd18b07..760a047 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -25,6 +25,12 @@ [...] > +static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table, > + uint32_t bitmap_table_size) > +{ > +BDRVQcow2State *s = bs->opaque; > +int cl_size = s->cluster_size; > +int i; > + > +for (i = 0; i < bitmap_table_size; ++i) { > +uint64_t addr = bitmap_table[i]; > +if (addr <= 1) { I'd rather add a BME_TABLE_ENTRY_OFFSET_MASK (0x00fffe00ull) and then use: uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK; if (!addr) { > +continue; > +} > + > +qcow2_free_clusters(bs, addr, cl_size, QCOW2_DISCARD_ALWAYS); I think this should rather be QCOW2_DISCARD_OTHER; or you introduce a new QCOW2_DISCARD_* flag. (The same applies to all the other QCOW2_DISCARD_ALWAYS users here.) Also, I don't really see the point of introducing cl_size just for this place; it doesn't even save you from a line wrap. > +bitmap_table[i] = 0; > +} > +} > + > +static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapDirEntry > *entry, > + uint64_t **table) > +{ > +int ret; > + > +*table = g_try_new(uint64_t, entry->bitmap_table_size); > +if (*table == NULL) { Not that g_try_new() will return NULL if you try to allocate a buffer with size 0. Normally, entry->bitmap_table_size shouldn't be 0, but I don't think you're catching that case anywhere. > +return -ENOMEM; > +} > + I wouldn't mind an assert(entry->bitmap_table_size <= UINT32_MAX / sizeof(uint64_t)); here because of the following multiplication (which is extended to 64 bit on 64 bit machines, but stays in 32 bit on 32 bit machines). (Of course, assert(entry->bitmap_table_size <= BME_MAX_TABLE_SIZE); would be fine, too.) > +ret = bdrv_pread(bs->file, entry->bitmap_table_offset, > + *table, entry->bitmap_table_size * sizeof(uint64_t)); > +if (ret < 0) { > +g_free(*table); > +*table = NULL; > +return ret; > +} > + > +bitmap_table_to_cpu(*table, entry->bitmap_table_size); > + > +return 0; > +} > + > +static void do_free_bitmap_clusters(BlockDriverState *bs, > +uint64_t table_offset, > +uint32_t table_size, > +uint64_t *bitmap_table) > +{ > +clear_bitmap_table(bs, bitmap_table, table_size); > + > +qcow2_free_clusters(bs, table_offset, table_size * sizeof(uint64_t), > +QCOW2_DISCARD_ALWAYS); > +} > + > +static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapDirEntry > *entry) > +{ > +int ret; > +uint64_t *bitmap_table; > + > +ret = bitmap_table_load(bs, entry, &bitmap_table); > +if (ret < 0 || bitmap_table == NULL) { That should be: if (ret < 0) { assert(bitmap_table == NULL); Or the other way around: if (bitmap_table == NULL) { assert(ret < 0); Otherwise, it looks like bitmap_table may be NULL with ret being 0, and the next statement would make this function return success even though it actually failed. > +return ret; > +} > + > +do_free_bitmap_clusters(bs, entry->bitmap_table_offset, > +entry->bitmap_table_size, bitmap_table); > + > +return 0; You're leaking bitmap_table here. > +} > + > +/* dirty sectors in cluster is a number of sectors in the image, > corresponding > + * to one cluster of bitmap data */ How about: This function returns the number of sectors covered by a single cluster of dirty bitmap data. > +static uint64_t dirty_sectors_in_cluster(const BDRVQcow2State *s, > + const BdrvDirtyBitmap *bitmap) > +{ > +uint32_t sector_granularity = > +bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS; > + > +return (uint64_t)sector_granularity * (s->cluster_size << 3); > +} > + > +static int load_bitmap_data(BlockDriverState *bs, > +const uint64_t *dirty_bitmap_table, > +
[Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are loaded at image open and becomes BdrvDirtyBitmap's for corresponding drive. These bitmaps are deleted from Qcow2 image after loading to avoid conflicts. Extra data in bitmaps is not supported for now. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/qcow2-bitmap.c | 518 ++- block/qcow2.c| 5 + block/qcow2.h| 3 + 3 files changed, 525 insertions(+), 1 deletion(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index cd18b07..760a047 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -25,6 +25,12 @@ * THE SOFTWARE. */ +#include "qemu/osdep.h" +#include "qapi/error.h" + +#include "block/block_int.h" +#include "block/qcow2.h" + /* NOTICE: BME here means Bitmaps Extension and used as a namespace for * _internal_ constants. Please do not use this _internal_ abbreviation for * other needs and/or outside of this file. */ @@ -37,11 +43,521 @@ #define BME_MAX_NAME_SIZE 1023 /* Bitmap directory entry flags */ -#define BME_RESERVED_FLAGS 0x +#define BME_RESERVED_FLAGS 0xfffd +#define BME_FLAG_AUTO (1U << 1) /* bits [1, 8] U [56, 63] are reserved */ #define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001fe +#define for_each_bitmap_dir_entry_unsafe(entry, dir, size) \ +for (entry = (Qcow2BitmapDirEntry *)(dir); \ + entry < (Qcow2BitmapDirEntry *)((uint8_t *)(dir) + size); \ + entry = next_dir_entry(entry)) + +#define for_each_bitmap_dir_entry(entry, dir, size) \ +for (entry = (Qcow2BitmapDirEntry *)(dir); \ + assert(check_dir_iter(entry, (uint8_t *)(dir) + size)), \ + entry < (Qcow2BitmapDirEntry *)((uint8_t *)(dir) + size); \ + entry = next_dir_entry(entry)) + typedef enum BitmapType { BT_DIRTY_TRACKING_BITMAP = 1 } BitmapType; + +static inline bool can_write(BlockDriverState *bs) +{ +return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE); +} + +static inline void bitmap_dir_entry_to_cpu(Qcow2BitmapDirEntry *entry) +{ +be64_to_cpus(&entry->bitmap_table_offset); +be32_to_cpus(&entry->bitmap_table_size); +be32_to_cpus(&entry->flags); +be16_to_cpus(&entry->name_size); +be32_to_cpus(&entry->extra_data_size); +} + +static inline void bitmap_dir_entry_to_be(Qcow2BitmapDirEntry *entry) +{ +cpu_to_be64s(&entry->bitmap_table_offset); +cpu_to_be32s(&entry->bitmap_table_size); +cpu_to_be32s(&entry->flags); +cpu_to_be16s(&entry->name_size); +cpu_to_be32s(&entry->extra_data_size); +} + +static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size) +{ +size_t i; + +for (i = 0; i < size; ++i) { +be64_to_cpus(&bitmap_table[i]); +} +} + +static inline int calc_dir_entry_size(size_t name_size, size_t extra_data_size) +{ +return align_offset(sizeof(Qcow2BitmapDirEntry) + +name_size + extra_data_size, 8); +} + +static inline int dir_entry_size(Qcow2BitmapDirEntry *entry) +{ +return calc_dir_entry_size(entry->name_size, entry->extra_data_size); +} + +static inline const char *dir_entry_name_notcstr(Qcow2BitmapDirEntry *entry) +{ +return (const char *)(entry + 1) + entry->extra_data_size; +} + +static inline int copy_dir_entry(void *dest, Qcow2BitmapDirEntry *entry) +{ +int sz = dir_entry_size(entry); +memcpy(dest, entry, sz); +return sz; +} + +static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry) +{ +return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry)); +} + +static inline bool check_dir_iter(Qcow2BitmapDirEntry *it, void *directory_end) +{ +return ((void *)it == directory_end) || + (((void *)(it + 1) <= directory_end) && +((void *)next_dir_entry(it) <= directory_end)); +} + +static inline void bitmap_directory_to_be(uint8_t *dir, size_t size) +{ +uint8_t *end = dir + size; +while (dir < end) { +Qcow2BitmapDirEntry *e = (Qcow2BitmapDirEntry *)dir; +dir += dir_entry_size(e); + +bitmap_dir_entry_to_be(e); +} +} + +static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table, + uint32_t bitmap_table_size) +{ +BDRVQcow2State *s = bs->opaque; +int cl_size = s->cluster_size; +int i; + +for (i = 0; i < bitmap_table_size; ++i) { +uint64_t addr = bitmap_table[i]; +if (addr <= 1) { +continue; +} + +qcow2_free_clusters(bs, addr, cl_size, QCOW2_DISCARD_ALWAYS); +bitmap_table[i] = 0; +} +} + +static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapDirEntry *entry, + uint64_t **table) +{ +int ret; + +*table = g_try_new(uint64_t, entry->bitmap_table_size); +if (*table == NULL) { +return -ENOMEM; +} + +ret = bdrv_pread(bs->file, entry->bitmap_table_offset, +