* [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap @ 2019-10-14 11:51 Vladimir Sementsov-Ogievskiy 2019-10-14 11:51 ` [PATCH v2 1/2] qcow2-bitmaps: " Vladimir Sementsov-Ogievskiy ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-10-14 11:51 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, den, jsnow Hi all! Here is a fix for persistent bitmaps managing: we must check existent but not yet stored bitmaps for qcow2-related constraints, like maximum number of bitmaps in qcow2 image. v2: 01: change assertion to error-return at function start Be free to add Reported-by: aihua liang <aliang@redhat.com> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636 if it's appropriate 02: new test Ohh, it takes about 4 minutes. Be free to drop it, as I doubt that it worth to have. The case is simple, we may live without a test. Vladimir Sementsov-Ogievskiy (2): qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap iotests: add 269 to check maximum of bitmaps in qcow2 block/qcow2-bitmap.c | 41 +++++++++++++++------------------ tests/qemu-iotests/269 | 47 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/269.out | 3 +++ tests/qemu-iotests/group | 1 + 4 files changed, 69 insertions(+), 23 deletions(-) create mode 100755 tests/qemu-iotests/269 create mode 100644 tests/qemu-iotests/269.out -- 2.21.0 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/2] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap 2019-10-14 11:51 [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap Vladimir Sementsov-Ogievskiy @ 2019-10-14 11:51 ` Vladimir Sementsov-Ogievskiy 2019-10-25 12:50 ` Max Reitz 2019-12-09 16:32 ` Max Reitz 2019-10-14 11:51 ` [PATCH v2 2/2] iotests: add 269 to check maximum of bitmaps in qcow2 Vladimir Sementsov-Ogievskiy ` (2 subsequent siblings) 3 siblings, 2 replies; 18+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-10-14 11:51 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, den, jsnow qcow2_can_store_new_dirty_bitmap works wrong, as it considers only bitmaps already stored in the qcow2 image and ignores persistent BdrvDirtyBitmap objects. So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 bitmaps on open, so there should not be any bitmap in the image for which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of corruption, and no reason to check for corruptions here (open() and close() are better places for it). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2-bitmap.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 98294a7696..d5131181a3 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1671,8 +1671,14 @@ bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, Error **errp) { BDRVQcow2State *s = bs->opaque; - bool found; - Qcow2BitmapList *bm_list; + BdrvDirtyBitmap *bitmap; + uint64_t bitmap_directory_size = 0; + uint32_t nb_bitmaps = 0; + + if (bdrv_find_dirty_bitmap(bs, name)) { + error_setg(errp, "Bitmap already exists: %s", name); + return false; + } if (s->qcow_version < 3) { /* Without autoclear_features, we would always have to assume @@ -1688,38 +1694,27 @@ bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, goto fail; } - if (s->nb_bitmaps == 0) { - return true; + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { + nb_bitmaps++; + bitmap_directory_size += + calc_dir_entry_size(strlen(bdrv_dirty_bitmap_name(bitmap)), 0); + } } + nb_bitmaps++; + bitmap_directory_size += calc_dir_entry_size(strlen(name), 0); - if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { + if (nb_bitmaps > QCOW2_MAX_BITMAPS) { error_setg(errp, "Maximum number of persistent bitmaps is already reached"); goto fail; } - if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) > - QCOW2_MAX_BITMAP_DIRECTORY_SIZE) - { + if (bitmap_directory_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { error_setg(errp, "Not enough space in the bitmap directory"); goto fail; } - qemu_co_mutex_lock(&s->lock); - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, - s->bitmap_directory_size, errp); - qemu_co_mutex_unlock(&s->lock); - if (bm_list == NULL) { - goto fail; - } - - found = find_bitmap_by_name(bm_list, name); - bitmap_list_free(bm_list); - if (found) { - error_setg(errp, "Bitmap with the same name is already stored"); - goto fail; - } - return true; fail: -- 2.21.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap 2019-10-14 11:51 ` [PATCH v2 1/2] qcow2-bitmaps: " Vladimir Sementsov-Ogievskiy @ 2019-10-25 12:50 ` Max Reitz 2019-10-25 12:56 ` Vladimir Sementsov-Ogievskiy 2019-12-09 16:32 ` Max Reitz 1 sibling, 1 reply; 18+ messages in thread From: Max Reitz @ 2019-10-25 12:50 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, jsnow, qemu-devel, den [-- Attachment #1.1: Type: text/plain, Size: 981 bytes --] On 14.10.19 13:51, Vladimir Sementsov-Ogievskiy wrote: > qcow2_can_store_new_dirty_bitmap works wrong, as it considers only > bitmaps already stored in the qcow2 image and ignores persistent > BdrvDirtyBitmap objects. > > So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 > bitmaps on open, so there should not be any bitmap in the image for > which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of > corruption, and no reason to check for corruptions here (open() and > close() are better places for it). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2-bitmap.c | 41 ++++++++++++++++++----------------------- > 1 file changed, 18 insertions(+), 23 deletions(-) Am I right in interpreting qcow2_load_dirty_bitmaps() in such a way that every persistent dirty bitmap will cause a runtime dirty bitmap to be created? If so: Reviewed-by: Max Reitz <mreitz@redhat.com> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap 2019-10-25 12:50 ` Max Reitz @ 2019-10-25 12:56 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 18+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-10-25 12:56 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: kwolf, jsnow, qemu-devel, Denis Lunev 25.10.2019 15:50, Max Reitz wrote: > On 14.10.19 13:51, Vladimir Sementsov-Ogievskiy wrote: >> qcow2_can_store_new_dirty_bitmap works wrong, as it considers only >> bitmaps already stored in the qcow2 image and ignores persistent >> BdrvDirtyBitmap objects. >> >> So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 >> bitmaps on open, so there should not be any bitmap in the image for >> which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of >> corruption, and no reason to check for corruptions here (open() and >> close() are better places for it). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2-bitmap.c | 41 ++++++++++++++++++----------------------- >> 1 file changed, 18 insertions(+), 23 deletions(-) > > Am I right in interpreting qcow2_load_dirty_bitmaps() in such a way that > every persistent dirty bitmap will cause a runtime dirty bitmap to be > created? Yes, we load all the bitmaps. Every bitmap stored in qcow2 image is loaded and corresponding BdrvDirtyBitmap is created. If bitmap has IN_USE flag in the image, created BdrvDirtyBitmap is marked inconsistent, but it is still there. If bitmap doesn't have AUTO flag, it becomes disabled bitmap. > > If so: > > Reviewed-by: Max Reitz <mreitz@redhat.com> > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap 2019-10-14 11:51 ` [PATCH v2 1/2] qcow2-bitmaps: " Vladimir Sementsov-Ogievskiy 2019-10-25 12:50 ` Max Reitz @ 2019-12-09 16:32 ` Max Reitz 1 sibling, 0 replies; 18+ messages in thread From: Max Reitz @ 2019-12-09 16:32 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, jsnow, qemu-devel, den [-- Attachment #1.1: Type: text/plain, Size: 885 bytes --] On 14.10.19 13:51, Vladimir Sementsov-Ogievskiy wrote: > qcow2_can_store_new_dirty_bitmap works wrong, as it considers only > bitmaps already stored in the qcow2 image and ignores persistent > BdrvDirtyBitmap objects. > > So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 > bitmaps on open, so there should not be any bitmap in the image for > which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of > corruption, and no reason to check for corruptions here (open() and > close() are better places for it). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2-bitmap.c | 41 ++++++++++++++++++----------------------- > 1 file changed, 18 insertions(+), 23 deletions(-) Thanks, applied to my block-next branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] iotests: add 269 to check maximum of bitmaps in qcow2 2019-10-14 11:51 [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap Vladimir Sementsov-Ogievskiy 2019-10-14 11:51 ` [PATCH v2 1/2] qcow2-bitmaps: " Vladimir Sementsov-Ogievskiy @ 2019-10-14 11:51 ` Vladimir Sementsov-Ogievskiy 2019-10-25 13:12 ` Max Reitz 2019-10-14 11:52 ` [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap Vladimir Sementsov-Ogievskiy 2019-10-25 9:57 ` [bugfix ping] " Vladimir Sementsov-Ogievskiy 3 siblings, 1 reply; 18+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-10-14 11:51 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, den, jsnow Check that it's impossible to create more persistent bitmaps than qcow2 supports. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- tests/qemu-iotests/269 | 47 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/269.out | 3 +++ tests/qemu-iotests/group | 1 + 3 files changed, 51 insertions(+) create mode 100755 tests/qemu-iotests/269 create mode 100644 tests/qemu-iotests/269.out diff --git a/tests/qemu-iotests/269 b/tests/qemu-iotests/269 new file mode 100755 index 0000000000..cf14d519ee --- /dev/null +++ b/tests/qemu-iotests/269 @@ -0,0 +1,47 @@ +#!/usr/bin/env python +# +# Test exceeding dirty bitmaps maximum amount in qcow2 image +# +# Copyright (c) 2019 Virtuozzo International GmbH. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +import iotests +from iotests import qemu_img_create, file_path, log, filter_qmp_event + +iotests.verify_image_format(supported_fmts=['qcow2']) + +img = file_path('img') +size = 64 * 1024 + +qemu_img_create('-f', iotests.imgfmt, img, str(size)) +vm = iotests.VM().add_drive(img) +vm.launch() + +# Look at block/qcow2.h +QCOW2_MAX_BITMAPS = 65535 + +for i in range(QCOW2_MAX_BITMAPS): + result = vm.qmp('block-dirty-bitmap-add', node='drive0', + name='bitmap{}'.format(i), persistent=True) + assert result['return'] == {} + +log("{} persistent bitmap already created, " \ + "let's try to create one more".format(QCOW2_MAX_BITMAPS)) + +vm.qmp_log('block-dirty-bitmap-add', node='drive0', + name='bitmap{}'.format(QCOW2_MAX_BITMAPS), persistent=True) + +vm.shutdown() diff --git a/tests/qemu-iotests/269.out b/tests/qemu-iotests/269.out new file mode 100644 index 0000000000..bcfa616a2b --- /dev/null +++ b/tests/qemu-iotests/269.out @@ -0,0 +1,3 @@ +65535 persistent bitmap already created, let's try to create one more +{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap65535", "node": "drive0", "persistent": true}} +{"error": {"class": "GenericError", "desc": "Can't make bitmap 'bitmap65535' persistent in 'drive0': Maximum number of persistent bitmaps is already reached"}} diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 0c1e5ef414..fe8274a204 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -279,3 +279,4 @@ 265 rw auto quick 266 rw quick 267 rw auto quick snapshot +269 -- 2.21.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] iotests: add 269 to check maximum of bitmaps in qcow2 2019-10-14 11:51 ` [PATCH v2 2/2] iotests: add 269 to check maximum of bitmaps in qcow2 Vladimir Sementsov-Ogievskiy @ 2019-10-25 13:12 ` Max Reitz 2019-11-16 9:41 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 18+ messages in thread From: Max Reitz @ 2019-10-25 13:12 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, jsnow, qemu-devel, den [-- Attachment #1.1: Type: text/plain, Size: 972 bytes --] On 14.10.19 13:51, Vladimir Sementsov-Ogievskiy wrote: > Check that it's impossible to create more persistent bitmaps than qcow2 > supports. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > tests/qemu-iotests/269 | 47 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/269.out | 3 +++ > tests/qemu-iotests/group | 1 + > 3 files changed, 51 insertions(+) > create mode 100755 tests/qemu-iotests/269 > create mode 100644 tests/qemu-iotests/269.out Is there no way to make this test any faster, e.g. by creating like 65534 bitmaps with dd and a binary blob? (Similarly to what I do in “iotests: Test qcow2's snapshot table handling”) This is such an edge case, but running the test took 3:46 min before patch 1 (which I already find much too long), and 8:13 min afterwards (on my machine). (To be honest, if we take this test as-is, I’m probably just never going to run it.) Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] iotests: add 269 to check maximum of bitmaps in qcow2 2019-10-25 13:12 ` Max Reitz @ 2019-11-16 9:41 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 18+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-11-16 9:41 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: kwolf, jsnow, qemu-devel, Denis Lunev 25.10.2019 16:12, Max Reitz wrote: > On 14.10.19 13:51, Vladimir Sementsov-Ogievskiy wrote: >> Check that it's impossible to create more persistent bitmaps than qcow2 >> supports. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> tests/qemu-iotests/269 | 47 ++++++++++++++++++++++++++++++++++++++ >> tests/qemu-iotests/269.out | 3 +++ >> tests/qemu-iotests/group | 1 + >> 3 files changed, 51 insertions(+) >> create mode 100755 tests/qemu-iotests/269 >> create mode 100644 tests/qemu-iotests/269.out > > Is there no way to make this test any faster, e.g. by creating like > 65534 bitmaps with dd and a binary blob? (Similarly to what I do in > “iotests: Test qcow2's snapshot table handling”) Seems, that's not simple.. Each bitmap should have personal name and bitmap table.. Let's merge only patch 01 and forget about this one. > > This is such an edge case, but running the test took 3:46 min before > patch 1 (which I already find much too long), and 8:13 min afterwards > (on my machine). > > (To be honest, if we take this test as-is, I’m probably just never going > to run it.) > > Max > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-10-14 11:51 [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap Vladimir Sementsov-Ogievskiy 2019-10-14 11:51 ` [PATCH v2 1/2] qcow2-bitmaps: " Vladimir Sementsov-Ogievskiy 2019-10-14 11:51 ` [PATCH v2 2/2] iotests: add 269 to check maximum of bitmaps in qcow2 Vladimir Sementsov-Ogievskiy @ 2019-10-14 11:52 ` Vladimir Sementsov-Ogievskiy 2019-10-25 9:57 ` [bugfix ping] " Vladimir Sementsov-Ogievskiy 3 siblings, 0 replies; 18+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-10-14 11:52 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, Denis Lunev, qemu-devel, mreitz, jsnow 14.10.2019 14:51, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > Here is a fix for persistent bitmaps managing: we must check existent > but not yet stored bitmaps for qcow2-related constraints, like maximum > number of bitmaps in qcow2 image. > > v2: main thing based on https://github.com/jnsnow/qemu bitmaps Based-on: <20191011212550.27269-1-jsnow@redhat.com> > > 01: change assertion to error-return at function start > Be free to add > Reported-by: aihua liang <aliang@redhat.com> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636 > if it's appropriate > 02: new test > Ohh, it takes about 4 minutes. Be free to drop it, as I doubt that > it worth to have. The case is simple, we may live without a > test. > > Vladimir Sementsov-Ogievskiy (2): > qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap > iotests: add 269 to check maximum of bitmaps in qcow2 > > block/qcow2-bitmap.c | 41 +++++++++++++++------------------ > tests/qemu-iotests/269 | 47 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/269.out | 3 +++ > tests/qemu-iotests/group | 1 + > 4 files changed, 69 insertions(+), 23 deletions(-) > create mode 100755 tests/qemu-iotests/269 > create mode 100644 tests/qemu-iotests/269.out > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 18+ messages in thread
* [bugfix ping] Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-10-14 11:51 [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap Vladimir Sementsov-Ogievskiy ` (2 preceding siblings ...) 2019-10-14 11:52 ` [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap Vladimir Sementsov-Ogievskiy @ 2019-10-25 9:57 ` Vladimir Sementsov-Ogievskiy 2019-12-02 14:09 ` [bugfix ping2] " Vladimir Sementsov-Ogievskiy 3 siblings, 1 reply; 18+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-10-25 9:57 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, Denis Lunev, qemu-devel, mreitz, jsnow Hi! Don't we forget it? Here is a bug-fix, I think we want it for 4.2. 14.10.2019 14:51, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > Here is a fix for persistent bitmaps managing: we must check existent > but not yet stored bitmaps for qcow2-related constraints, like maximum > number of bitmaps in qcow2 image. > > v2: > > 01: change assertion to error-return at function start > Be free to add > Reported-by: aihua liang <aliang@redhat.com> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636 > if it's appropriate > 02: new test > Ohh, it takes about 4 minutes. Be free to drop it, as I doubt that > it worth to have. The case is simple, we may live without a > test. > > Vladimir Sementsov-Ogievskiy (2): > qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap > iotests: add 269 to check maximum of bitmaps in qcow2 > > block/qcow2-bitmap.c | 41 +++++++++++++++------------------ > tests/qemu-iotests/269 | 47 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/269.out | 3 +++ > tests/qemu-iotests/group | 1 + > 4 files changed, 69 insertions(+), 23 deletions(-) > create mode 100755 tests/qemu-iotests/269 > create mode 100644 tests/qemu-iotests/269.out > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [bugfix ping2] Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-10-25 9:57 ` [bugfix ping] " Vladimir Sementsov-Ogievskiy @ 2019-12-02 14:09 ` Vladimir Sementsov-Ogievskiy 2019-12-09 16:30 ` Max Reitz 0 siblings, 1 reply; 18+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-12-02 14:09 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, Denis Lunev, qemu-devel, mreitz, jsnow Hi again! Still forgotten bug-fix :( Is it too late for 4.2? I can't imagine better test, and it tests exactly what written in https://bugzilla.redhat.com/show_bug.cgi?id=1712636 (Hmm, actually, I doubt that it is real use-case, more probably it's a bug in management layer) So, take this with test or without test, to 4.2 or 5.0. 25.10.2019 12:57, Vladimir Sementsov-Ogievskiy wrote: > Hi! > > Don't we forget it? > > Here is a bug-fix, I think we want it for 4.2. > > 14.10.2019 14:51, Vladimir Sementsov-Ogievskiy wrote: >> Hi all! >> >> Here is a fix for persistent bitmaps managing: we must check existent >> but not yet stored bitmaps for qcow2-related constraints, like maximum >> number of bitmaps in qcow2 image. >> >> v2: >> >> 01: change assertion to error-return at function start >> Be free to add >> Reported-by: aihua liang <aliang@redhat.com> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >> if it's appropriate >> 02: new test >> Ohh, it takes about 4 minutes. Be free to drop it, as I doubt that >> it worth to have. The case is simple, we may live without a >> test. >> >> Vladimir Sementsov-Ogievskiy (2): >> qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap >> iotests: add 269 to check maximum of bitmaps in qcow2 >> >> block/qcow2-bitmap.c | 41 +++++++++++++++------------------ >> tests/qemu-iotests/269 | 47 ++++++++++++++++++++++++++++++++++++++ >> tests/qemu-iotests/269.out | 3 +++ >> tests/qemu-iotests/group | 1 + >> 4 files changed, 69 insertions(+), 23 deletions(-) >> create mode 100755 tests/qemu-iotests/269 >> create mode 100644 tests/qemu-iotests/269.out >> > > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [bugfix ping2] Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-12-02 14:09 ` [bugfix ping2] " Vladimir Sementsov-Ogievskiy @ 2019-12-09 16:30 ` Max Reitz 2019-12-09 17:58 ` Max Reitz 0 siblings, 1 reply; 18+ messages in thread From: Max Reitz @ 2019-12-09 16:30 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block Cc: kwolf, jsnow, qemu-devel, Denis Lunev [-- Attachment #1.1: Type: text/plain, Size: 694 bytes --] On 02.12.19 15:09, Vladimir Sementsov-Ogievskiy wrote: > Hi again! > > Still forgotten bug-fix :( > > Is it too late for 4.2? Sorry. :-/ Yes, I think I just forgot it. I don’t think it’s too important for 4.2, so, well, it isn’t too bad, but... Sorry. > I can't imagine better test, and it tests exactly what written in > https://bugzilla.redhat.com/show_bug.cgi?id=1712636 > > (Hmm, actually, I doubt that it is real use-case, more probably it's a bug in management layer) > > So, take this with test or without test, to 4.2 or 5.0. I was thinking of seeing whether I could write a quicker test, but of course we should take the patch either way. Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [bugfix ping2] Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-12-09 16:30 ` Max Reitz @ 2019-12-09 17:58 ` Max Reitz 2019-12-09 22:03 ` Eric Blake 0 siblings, 1 reply; 18+ messages in thread From: Max Reitz @ 2019-12-09 17:58 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block Cc: kwolf, jsnow, qemu-devel, Denis Lunev [-- Attachment #1.1: Type: text/plain, Size: 1032 bytes --] On 09.12.19 17:30, Max Reitz wrote: > On 02.12.19 15:09, Vladimir Sementsov-Ogievskiy wrote: >> Hi again! >> >> Still forgotten bug-fix :( >> >> Is it too late for 4.2? > > Sorry. :-/ > > Yes, I think I just forgot it. I don’t think it’s too important for > 4.2, so, well, it isn’t too bad, but... Sorry. > >> I can't imagine better test, and it tests exactly what written in >> https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >> >> (Hmm, actually, I doubt that it is real use-case, more probably it's a bug in management layer) >> >> So, take this with test or without test, to 4.2 or 5.0. > > I was thinking of seeing whether I could write a quicker test, but of > course we should take the patch either way. OK, I give up. It’s very much possible to create an image with 65535 bitmaps very quickly (like, under a second) outside of qemu, but just opening it takes 2:30 min (because of the quadratic complexity of checking whether a bitmap of the same name already exists). Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [bugfix ping2] Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-12-09 17:58 ` Max Reitz @ 2019-12-09 22:03 ` Eric Blake 2019-12-10 8:11 ` Max Reitz 0 siblings, 1 reply; 18+ messages in thread From: Eric Blake @ 2019-12-09 22:03 UTC (permalink / raw) To: Max Reitz, Vladimir Sementsov-Ogievskiy, qemu-block Cc: kwolf, jsnow, qemu-devel, Denis Lunev On 12/9/19 11:58 AM, Max Reitz wrote: > On 09.12.19 17:30, Max Reitz wrote: >> On 02.12.19 15:09, Vladimir Sementsov-Ogievskiy wrote: >>> Hi again! >>> >>> Still forgotten bug-fix :( >>> >>> Is it too late for 4.2? >> >> Sorry. :-/ >> >> Yes, I think I just forgot it. I don’t think it’s too important for >> 4.2, so, well, it isn’t too bad, but... Sorry. >> >>> I can't imagine better test, and it tests exactly what written in >>> https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >>> >>> (Hmm, actually, I doubt that it is real use-case, more probably it's a bug in management layer) >>> >>> So, take this with test or without test, to 4.2 or 5.0. >> >> I was thinking of seeing whether I could write a quicker test, but of >> course we should take the patch either way. > > OK, I give up. It’s very much possible to create an image with 65535 > bitmaps very quickly (like, under a second) outside of qemu, but just > opening it takes 2:30 min (because of the quadratic complexity of > checking whether a bitmap of the same name already exists). Can we fix that to use a hash table for amortized O(1) lookup rather than the current O(n) lookup? But such a fix is 5.0 material. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [bugfix ping2] Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-12-09 22:03 ` Eric Blake @ 2019-12-10 8:11 ` Max Reitz 2019-12-10 13:24 ` Max Reitz 0 siblings, 1 reply; 18+ messages in thread From: Max Reitz @ 2019-12-10 8:11 UTC (permalink / raw) To: Eric Blake, Vladimir Sementsov-Ogievskiy, qemu-block Cc: kwolf, jsnow, qemu-devel, Denis Lunev [-- Attachment #1.1: Type: text/plain, Size: 1707 bytes --] On 09.12.19 23:03, Eric Blake wrote: > On 12/9/19 11:58 AM, Max Reitz wrote: >> On 09.12.19 17:30, Max Reitz wrote: >>> On 02.12.19 15:09, Vladimir Sementsov-Ogievskiy wrote: >>>> Hi again! >>>> >>>> Still forgotten bug-fix :( >>>> >>>> Is it too late for 4.2? >>> >>> Sorry. :-/ >>> >>> Yes, I think I just forgot it. I don’t think it’s too important for >>> 4.2, so, well, it isn’t too bad, but... Sorry. >>> >>>> I can't imagine better test, and it tests exactly what written in >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >>>> >>>> (Hmm, actually, I doubt that it is real use-case, more probably it's >>>> a bug in management layer) >>>> >>>> So, take this with test or without test, to 4.2 or 5.0. >>> >>> I was thinking of seeing whether I could write a quicker test, but of >>> course we should take the patch either way. >> >> OK, I give up. It’s very much possible to create an image with 65535 >> bitmaps very quickly (like, under a second) outside of qemu, but just >> opening it takes 2:30 min (because of the quadratic complexity of >> checking whether a bitmap of the same name already exists). > > Can we fix that to use a hash table for amortized O(1) lookup rather > than the current O(n) lookup? Not unreasonable, considering that this is probably what we would’ve done from the start in any language where hash tables are built in. But OTOH when you have 66k bitmaps, you probably have other problems. Like, writes being incredibly slow, because all those bitmaps have to be updated. (Well, you can technically have 99 % of them disabled, but who’d do such a thing?) ((Maybe I’ll look into it.)) Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [bugfix ping2] Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-12-10 8:11 ` Max Reitz @ 2019-12-10 13:24 ` Max Reitz 2019-12-10 20:27 ` John Snow 0 siblings, 1 reply; 18+ messages in thread From: Max Reitz @ 2019-12-10 13:24 UTC (permalink / raw) To: Eric Blake, Vladimir Sementsov-Ogievskiy, qemu-block Cc: kwolf, jsnow, qemu-devel, Denis Lunev [-- Attachment #1.1: Type: text/plain, Size: 1906 bytes --] On 10.12.19 09:11, Max Reitz wrote: > On 09.12.19 23:03, Eric Blake wrote: >> On 12/9/19 11:58 AM, Max Reitz wrote: >>> On 09.12.19 17:30, Max Reitz wrote: >>>> On 02.12.19 15:09, Vladimir Sementsov-Ogievskiy wrote: >>>>> Hi again! >>>>> >>>>> Still forgotten bug-fix :( >>>>> >>>>> Is it too late for 4.2? >>>> >>>> Sorry. :-/ >>>> >>>> Yes, I think I just forgot it. I don’t think it’s too important for >>>> 4.2, so, well, it isn’t too bad, but... Sorry. >>>> >>>>> I can't imagine better test, and it tests exactly what written in >>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >>>>> >>>>> (Hmm, actually, I doubt that it is real use-case, more probably it's >>>>> a bug in management layer) >>>>> >>>>> So, take this with test or without test, to 4.2 or 5.0. >>>> >>>> I was thinking of seeing whether I could write a quicker test, but of >>>> course we should take the patch either way. >>> >>> OK, I give up. It’s very much possible to create an image with 65535 >>> bitmaps very quickly (like, under a second) outside of qemu, but just >>> opening it takes 2:30 min (because of the quadratic complexity of >>> checking whether a bitmap of the same name already exists). >> >> Can we fix that to use a hash table for amortized O(1) lookup rather >> than the current O(n) lookup? > > Not unreasonable, considering that this is probably what we would’ve > done from the start in any language where hash tables are built in. > > But OTOH when you have 66k bitmaps, you probably have other problems. > Like, writes being incredibly slow, because all those bitmaps have to be > updated. > > (Well, you can technically have 99 % of them disabled, but who’d do such > a thing?) > > ((Maybe I’ll look into it.)) Hmm, now I did. This gets the test down to 24 s. Still not sure whether it’s worth it, though... Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [bugfix ping2] Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-12-10 13:24 ` Max Reitz @ 2019-12-10 20:27 ` John Snow 2019-12-11 8:10 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 18+ messages in thread From: John Snow @ 2019-12-10 20:27 UTC (permalink / raw) To: Max Reitz, Eric Blake, Vladimir Sementsov-Ogievskiy, qemu-block Cc: kwolf, qemu-devel, Denis Lunev On 12/10/19 8:24 AM, Max Reitz wrote: > On 10.12.19 09:11, Max Reitz wrote: >> On 09.12.19 23:03, Eric Blake wrote: >>> On 12/9/19 11:58 AM, Max Reitz wrote: >>>> On 09.12.19 17:30, Max Reitz wrote: >>>>> On 02.12.19 15:09, Vladimir Sementsov-Ogievskiy wrote: >>>>>> Hi again! >>>>>> >>>>>> Still forgotten bug-fix :( >>>>>> >>>>>> Is it too late for 4.2? >>>>> >>>>> Sorry. :-/ >>>>> >>>>> Yes, I think I just forgot it. I don’t think it’s too important for >>>>> 4.2, so, well, it isn’t too bad, but... Sorry. >>>>> >>>>>> I can't imagine better test, and it tests exactly what written in >>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >>>>>> >>>>>> (Hmm, actually, I doubt that it is real use-case, more probably it's >>>>>> a bug in management layer) >>>>>> >>>>>> So, take this with test or without test, to 4.2 or 5.0. >>>>> >>>>> I was thinking of seeing whether I could write a quicker test, but of >>>>> course we should take the patch either way. >>>> >>>> OK, I give up. It’s very much possible to create an image with 65535 >>>> bitmaps very quickly (like, under a second) outside of qemu, but just >>>> opening it takes 2:30 min (because of the quadratic complexity of >>>> checking whether a bitmap of the same name already exists). >>> >>> Can we fix that to use a hash table for amortized O(1) lookup rather >>> than the current O(n) lookup? >> >> Not unreasonable, considering that this is probably what we would’ve >> done from the start in any language where hash tables are built in. >> >> But OTOH when you have 66k bitmaps, you probably have other problems. >> Like, writes being incredibly slow, because all those bitmaps have to be >> updated. >> >> (Well, you can technically have 99 % of them disabled, but who’d do such >> a thing?) >> >> ((Maybe I’ll look into it.)) > > Hmm, now I did. This gets the test down to 24 s. Still not sure > whether it’s worth it, though... > > Max > I agree we very likely have other problems once we reach resource usage of this level. Still, if we want to make this blazing fast for the love of doing so: (1) Read in the directory *once*, and cache it. We have avoided doing this largely to feel more confident that the code is correct and is never working on an "outdated" version of the directory. [On cache invalidation, we can write the directory back out to the bitmap, and delete our cache. The next time we need the list, we can reload it. This should alleviate consistency concerns.] (2) Store the entries in an rbtree! 65536 entries is only ~16 lookups maximum in the worst case. I took a look at the linux rbtree implementation and did a very quick back-of-the-envelope benchmarking for inserting strings (len=32) into a tree: name generation 53151 usec insert [0-10] 5 usec insert [10-100] 14 usec insert [100-1000] 195 usec insert [1000-10000] 2919 usec insert [10000-65536] 41485 usec This seems fast enough that we're likely going to be eclipsed just by other string handling concerns. --js ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [bugfix ping2] Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap 2019-12-10 20:27 ` John Snow @ 2019-12-11 8:10 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 18+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-12-11 8:10 UTC (permalink / raw) To: John Snow, Max Reitz, Eric Blake, qemu-block Cc: kwolf, qemu-devel, Denis Lunev 10.12.2019 23:27, John Snow wrote: > > > On 12/10/19 8:24 AM, Max Reitz wrote: >> On 10.12.19 09:11, Max Reitz wrote: >>> On 09.12.19 23:03, Eric Blake wrote: >>>> On 12/9/19 11:58 AM, Max Reitz wrote: >>>>> On 09.12.19 17:30, Max Reitz wrote: >>>>>> On 02.12.19 15:09, Vladimir Sementsov-Ogievskiy wrote: >>>>>>> Hi again! >>>>>>> >>>>>>> Still forgotten bug-fix :( >>>>>>> >>>>>>> Is it too late for 4.2? >>>>>> >>>>>> Sorry. :-/ >>>>>> >>>>>> Yes, I think I just forgot it. I don’t think it’s too important for >>>>>> 4.2, so, well, it isn’t too bad, but... Sorry. >>>>>> >>>>>>> I can't imagine better test, and it tests exactly what written in >>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >>>>>>> >>>>>>> (Hmm, actually, I doubt that it is real use-case, more probably it's >>>>>>> a bug in management layer) >>>>>>> >>>>>>> So, take this with test or without test, to 4.2 or 5.0. >>>>>> >>>>>> I was thinking of seeing whether I could write a quicker test, but of >>>>>> course we should take the patch either way. >>>>> >>>>> OK, I give up. It’s very much possible to create an image with 65535 >>>>> bitmaps very quickly (like, under a second) outside of qemu, but just >>>>> opening it takes 2:30 min (because of the quadratic complexity of >>>>> checking whether a bitmap of the same name already exists). >>>> >>>> Can we fix that to use a hash table for amortized O(1) lookup rather >>>> than the current O(n) lookup? >>> >>> Not unreasonable, considering that this is probably what we would’ve >>> done from the start in any language where hash tables are built in. >>> >>> But OTOH when you have 66k bitmaps, you probably have other problems. >>> Like, writes being incredibly slow, because all those bitmaps have to be >>> updated. >>> >>> (Well, you can technically have 99 % of them disabled, but who’d do such >>> a thing?) >>> >>> ((Maybe I’ll look into it.)) >> >> Hmm, now I did. This gets the test down to 24 s. Still not sure >> whether it’s worth it, though... >> >> Max >> > > I agree we very likely have other problems once we reach resource usage > of this level. > > > Still, if we want to make this blazing fast for the love of doing so: > > (1) Read in the directory *once*, and cache it. We have avoided doing > this largely to feel more confident that the code is correct and is > never working on an "outdated" version of the directory. > > [On cache invalidation, we can write the directory back out to the > bitmap, and delete our cache. The next time we need the list, we can > reload it. This should alleviate consistency concerns.] Note, that in this case, if we want to modify cached directory, bitmaps compatible bit must be cleared (so, if we fail to flush directory at some moment, we just lose bitmaps, not the consistency) Note2, interesting, could existing qcow2 metadata caching infrastructure be reused? > > > (2) Store the entries in an rbtree! 65536 entries is only ~16 lookups > maximum in the worst case. I took a look at the linux rbtree > implementation and did a very quick back-of-the-envelope benchmarking > for inserting strings (len=32) into a tree: > > name generation 53151 usec > insert [0-10] 5 usec > insert [10-100] 14 usec > insert [100-1000] 195 usec > insert [1000-10000] 2919 usec > insert [10000-65536] 41485 usec > > This seems fast enough that we're likely going to be eclipsed just by > other string handling concerns. > Is rbtree the best thing to use? May be, better to construct prefix tree? Still, I think g_hash_table should be enough and most simple to use.. Still, I really doubt that it worth it, as we never have too many bitmaps.. Actually there is no reason to have more than one active bitmap. So, we may have a lot of disabled bitmaps, marking some history of the drive. I think, in this case, the better optimization would be to teach Qemu not to load disabled bitmap data, only headers. And then, load them on demand (for example, when user wants to enable it or use somehow). And, if we have a lot of active bitmaps (for example to cover time regions [t0, t_current], [t1, t_current], [t2, t_current] ..., the optimization is to use instead disabled bitmaps, and merge them when we need: [t0, t1], [t1, t2], [t2, t_current] -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-12-11 8:11 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-14 11:51 [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap Vladimir Sementsov-Ogievskiy 2019-10-14 11:51 ` [PATCH v2 1/2] qcow2-bitmaps: " Vladimir Sementsov-Ogievskiy 2019-10-25 12:50 ` Max Reitz 2019-10-25 12:56 ` Vladimir Sementsov-Ogievskiy 2019-12-09 16:32 ` Max Reitz 2019-10-14 11:51 ` [PATCH v2 2/2] iotests: add 269 to check maximum of bitmaps in qcow2 Vladimir Sementsov-Ogievskiy 2019-10-25 13:12 ` Max Reitz 2019-11-16 9:41 ` Vladimir Sementsov-Ogievskiy 2019-10-14 11:52 ` [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap Vladimir Sementsov-Ogievskiy 2019-10-25 9:57 ` [bugfix ping] " Vladimir Sementsov-Ogievskiy 2019-12-02 14:09 ` [bugfix ping2] " Vladimir Sementsov-Ogievskiy 2019-12-09 16:30 ` Max Reitz 2019-12-09 17:58 ` Max Reitz 2019-12-09 22:03 ` Eric Blake 2019-12-10 8:11 ` Max Reitz 2019-12-10 13:24 ` Max Reitz 2019-12-10 20:27 ` John Snow 2019-12-11 8:10 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.