From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58424) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVkvk-0003m1-Ue for qemu-devel@nongnu.org; Tue, 16 Feb 2016 14:05:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVkvf-0000z1-Rp for qemu-devel@nongnu.org; Tue, 16 Feb 2016 14:05:00 -0500 Received: from mx2.parallels.com ([199.115.105.18]:46488) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVkvf-0000yw-JQ for qemu-devel@nongnu.org; Tue, 16 Feb 2016 14:04:55 -0500 Message-ID: <56C372CD.7010502@virtuozzo.com> Date: Tue, 16 Feb 2016 22:04:45 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1441471439-6157-1-git-send-email-vsementsov@virtuozzo.com> <1441471439-6157-7-git-send-email-vsementsov@virtuozzo.com> <561452E5.1090005@redhat.com> In-Reply-To: <561452E5.1090005@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 06/17] qcow2-dirty-bitmap: add qcow2_dirty_bitmap_load() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, stefanha@redhat.com, den@openvz.org On 07.10.2015 02:01, John Snow wrote: > > On 09/05/2015 12:43 PM, Vladimir Sementsov-Ogievskiy wrote: >> This function loads block dirty bitmap from qcow2. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> block/qcow2-dirty-bitmap.c | 155 +++++++++++++++++++++++++++++++++++++++++++++ >> block/qcow2.c | 2 + >> block/qcow2.h | 5 ++ >> include/block/block_int.h | 5 ++ >> 4 files changed, 167 insertions(+) >> >> diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c >> index 1260d1d..ea50137 100644 >> --- a/block/qcow2-dirty-bitmap.c >> +++ b/block/qcow2-dirty-bitmap.c >> @@ -99,6 +99,13 @@ static int check_constraints(int cluster_size, >> return fail ? -EINVAL : 0; >> } >> >> +static QCowDirtyBitmapHeader *bitmap_header(BDRVQcowState *s, >> + QCowDirtyBitmap *bitmap) >> +{ > BDRVQcow2State here and everywhere below, again. > >> + return (QCowDirtyBitmapHeader *) >> + (s->dirty_bitmap_directory + bitmap->offset); >> +} >> + >> static int directory_read(BlockDriverState *bs) >> { >> int ret; >> @@ -195,3 +202,151 @@ out: >> } >> return ret; >> } >> + >> +static QCowDirtyBitmap *find_dirty_bitmap_by_name(BlockDriverState *bs, >> + const char *name) >> +{ >> + BDRVQcowState *s = bs->opaque; >> + QCowDirtyBitmap *bm, *end = s->dirty_bitmaps + s->nb_dirty_bitmaps; >> + >> + for (bm = s->dirty_bitmaps; bm < end; ++bm) { >> + if (strcmp(bm->name, name) == 0) { >> + return bm; >> + } >> + } >> + >> + return NULL; >> +} >> + > Whoops. This says to me we really need to prohibit bitmaps with the same > name from being stored in the same file, and mention this in the spec, > and test for it on load. > > Perhaps we can create a hash-table and fail verification on open if > there's a collision. We can then use that hash-table here for > find_dirty_bitmap_by_name to speed up lookup since we already went > through the trouble of loading it. > > Might help for large cases where we're approaching 64K bitmaps, will not > be too big of a performance hit for casual use. So, it (hash table approach) may be implemented later > >> +/* dirty sectors in cluster is a number of sectors in the image, corresponding >> + * to one cluster of bitmap data */ >> +static uint64_t dirty_sectors_in_cluster(const BDRVQcowState *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); >> +} >> + >> +/* load_bitmap() >> + * load dirty bitmap from Dirty Bitmap Table >> + * Dirty Bitmap Table entries are assumed to be in big endian format */ >> +static int load_bitmap(BlockDriverState *bs, >> + const uint64_t *dirty_bitmap_table, >> + uint32_t dirty_bitmap_table_size, >> + BdrvDirtyBitmap *bitmap) >> +{ >> + int ret = 0; >> + BDRVQcowState *s = bs->opaque; >> + uint64_t sector, dsc; >> + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); > I found some of this hard to unwind, bear with me: > > AKA, the number of sectors that bitmap tracks ... > >> + int cl_size = s->cluster_size; >> + uint8_t *buf = NULL; >> + uint32_t i, tab_size = >> + size_to_clusters(s, bdrv_dirty_bitmap_data_size(bitmap, bm_size)); >> + > bdrv_dirty_bitmap_data_size(bitmap, COUNT) calculates for us how much > actual real size the lowest level of the hbitmap actually takes. > > Then size_to_clusters tells us how many clusters we need to store that, > and therefore should map back to be the same as the predicted value, > dirty_bitmap_table_size. > >> + if (tab_size > dirty_bitmap_table_size) { >> + return -EINVAL; >> + } >> + > I assume this is not == because the real table size might have padding > or other such things, but if the calculated tab size is bigger than the > actual then we have a problem. > > But I think that you've passed in "birty_ditmap_table_size" as the total > byte count of the table, but "tab_size" is computed here as the number > of entries. I think you should multiply tab_size by uint64_t and test if > they're equal. > >> + bdrv_clear_dirty_bitmap(bitmap); >> + > Clear takes the aio_context for the associated BDS and then releases it... > >> + buf = g_malloc0(cl_size); >> + dsc = dirty_sectors_in_cluster(s, bitmap); >> + for (i = 0, sector = 0; i < tab_size; ++i, sector += dsc) { >> + uint64_t end = MIN(bm_size, sector + dsc); >> + uint64_t offset = be64_to_cpu(dirty_bitmap_table[i]); >> + >> + if (offset & DBM_TABLE_ENTRY_RESERVED_MASK) { >> + ret = -EINVAL; >> + goto finish; >> + } >> + >> + /* zero offset means cluster unallocated */ >> + if (offset) { >> + ret = bdrv_pread(bs->file, offset, buf, cl_size); >> + if (ret < 0) { >> + goto finish; >> + } >> + bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, end); > ...but at this point, I believe we're editing this bitmap without its > associated lock, which might be a problem when we go to add QMP commands > later. in the next version load_bitmap is changed a lot, and now not using part-serialization. >> + } >> + } >> + ret = 0; >> + >> + bdrv_dirty_bitmap_deserialize_finish(bitmap); >> + >> +finish: >> + g_free(buf); >> + >> + return ret; >> +} >> + >> +BdrvDirtyBitmap * qcow2_dirty_bitmap_load(BlockDriverState *bs_for, >> + BlockDriverState *bs_file, >> + const char *name, >> + Error **errp) >> +{ >> + BDRVQcowState *s = bs_file->opaque; >> + int ret; >> + QCowDirtyBitmap *bm; >> + QCowDirtyBitmapHeader *bmh; >> + uint64_t *dirty_bitmap_table = NULL; >> + uint32_t granularity; >> + uint64_t size = bdrv_nb_sectors(bs_for); >> + BdrvDirtyBitmap *bitmap = NULL; >> + >> + bm = find_dirty_bitmap_by_name(bs_file, name); >> + if (bm == NULL) { >> + error_setg(errp, "Could not find bitmap '%s' in the node '%s'", name, >> + bdrv_get_device_or_node_name(bs_file)); >> + return NULL; >> + } >> + bmh = bitmap_header(s, bm); >> + >> + if (size != bmh->nb_virtual_bits) { >> + error_setg(errp, >> + "Bitmap '%s' in the node '%s' has size = %" PRIu64 >> + "when requested size (for node %s) = %" PRIu64, >> + name, bdrv_get_device_or_node_name(bs_file), >> + bmh->nb_virtual_bits, >> + bdrv_get_device_or_node_name(bs_for), size); >> + return NULL; >> + } >> + >> + >> + dirty_bitmap_table = g_try_malloc(bmh->dirty_bitmap_table_size * sizeof(uint64_t)); >> + if (dirty_bitmap_table == NULL) { >> + error_setg_errno(errp, -ENOMEM, "Could not allocate Dirty Bitmap Table"); >> + return NULL; >> + } >> + >> + ret = bdrv_pread(bs_file->file, bmh->dirty_bitmap_table_offset, dirty_bitmap_table, >> + bmh->dirty_bitmap_table_size * sizeof(uint64_t)); >> + if (ret < 0) { >> + error_setg_errno(errp, -ret, "Could not read dirty_bitmap_table table from image"); >> + goto finish; >> + } >> + >> + granularity = BDRV_SECTOR_SIZE << bmh->granularity_bits; >> + bitmap = bdrv_create_dirty_bitmap(bs_for, granularity, name, errp); >> + if (bitmap == NULL) { >> + error_setg_errno(errp, -ENOMEM, "Could not create dirty bitmap"); > why -ENOMEM? create can fail for a number of reasons ... since we've > been given an errp parameter in this function, and we can trust > bdrv_create_dirty_bitmap to have set it, we can just return NULL here > and the caller can check errp to see what went wrong. > >> + goto finish; >> + } >> + > Do we need to mark this bitmap as temporarily unusable until we complete > the load? I guess not in the context of bdrv_open at boot time ... Also, keep in mind that now we have only bitmaps, related to the same bds which contains it. > >> + ret = load_bitmap(bs_file, dirty_bitmap_table, bmh->dirty_bitmap_table_size, bitmap); >> + if (ret < 0) { >> + error_setg_errno(errp, -ret, "Could not read bitmap from image"); >> + goto finish; >> + } >> + >> +finish: >> + if (*errp != NULL) { >> + bdrv_release_dirty_bitmap(bs_for, bitmap); >> + bitmap = NULL; >> + } >> + g_free(dirty_bitmap_table); > > I think we're not supposed to be reaching into errp to check its > implementation detail like this ... the usual paradigm I see is just > "goto fail" or similar statements instead of checking for > error-or-success in a shared return block. > > finish: > g_free(dirty_bitmap_table); > return bitmap; > fail: > g_free(dirty_bitmap_table); > bdrv_release_dirty_bitmap(bs_for, bitmap); > return NULL; ok > >> + >> + return bitmap; >> +} >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 76c331b..58ebdd3 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2965,6 +2965,8 @@ BlockDriver bdrv_qcow2 = { >> .bdrv_get_info = qcow2_get_info, >> .bdrv_get_specific_info = qcow2_get_specific_info, >> >> + .bdrv_dirty_bitmap_load = qcow2_dirty_bitmap_load, >> + >> .bdrv_save_vmstate = qcow2_save_vmstate, >> .bdrv_load_vmstate = qcow2_load_vmstate, >> >> diff --git a/block/qcow2.h b/block/qcow2.h >> index 5016fa1..51d1907 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -608,6 +608,11 @@ int qcow2_read_snapshots(BlockDriverState *bs); >> void qcow2_free_dirty_bitmaps(BlockDriverState *bs); >> int qcow2_read_dirty_bitmaps(BlockDriverState *bs); >> >> +BdrvDirtyBitmap *qcow2_dirty_bitmap_load(BlockDriverState *bs_for, >> + BlockDriverState *bs_file, >> + const char *name, >> + Error **errp); >> + >> /* qcow2-cache.c functions */ >> Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables); >> int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c); >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index 14ad4c3..f982adc 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -204,6 +204,11 @@ struct BlockDriver { >> int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi); >> ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs); >> >> + BdrvDirtyBitmap *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs_for, >> + BlockDriverState *bs_file, >> + const char *name, >> + Error **errp); >> + >> int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov, >> int64_t pos); >> int (*bdrv_load_vmstate)(BlockDriverState *bs, uint8_t *buf, >> > Looking good, thanks! > --js -- Best regards, Vladimir