* [Qemu-devel] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method @ 2019-05-14 20:19 John Snow 2019-05-16 10:12 ` Vladimir Sementsov-Ogievskiy 2019-12-06 22:31 ` Vladimir Sementsov-Ogievskiy 0 siblings, 2 replies; 13+ messages in thread From: John Snow @ 2019-05-14 20:19 UTC (permalink / raw) To: qemu-devel, qemu-block Cc: Fam Zheng, Peter Maydell, vsementsov, aihua liang, Juan Quintela, Dr. David Alan Gilbert, Stefan Hajnoczi, John Snow Shift from looking at every root BDS to *every* BDS. This will migrate bitmaps that are attached to blockdev created nodes instead of just ones attached to emulated storage devices. Note that this will not migrate anonymous or internal-use bitmaps, as those are defined as having no name. This will also fix the Coverity issues Peter Maydell has been asking about for the past several releases, as well as fixing a real bug. Reported-by: Peter Maydell <peter.maydell@linaro.org> Reported-by: Coverity 😅 Reported-by: aihua liang <aliang@redhat.com> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 Fixes: Coverity CID 1390625 CC: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: John Snow <jsnow@redhat.com> --- migration/block-dirty-bitmap.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index d1bb863cb6..4a896a09eb 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -273,7 +273,6 @@ static int init_dirty_bitmap_migration(void) BlockDriverState *bs; BdrvDirtyBitmap *bitmap; DirtyBitmapMigBitmapState *dbms; - BdrvNextIterator it; Error *local_err = NULL; dirty_bitmap_mig_state.bulk_completed = false; @@ -281,13 +280,8 @@ static int init_dirty_bitmap_migration(void) dirty_bitmap_mig_state.prev_bitmap = NULL; dirty_bitmap_mig_state.no_bitmaps = false; - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { - const char *drive_name = bdrv_get_device_or_node_name(bs); - - /* skip automatically inserted nodes */ - while (bs && bs->drv && bs->implicit) { - bs = backing_bs(bs); - } + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { + const char *name = bdrv_get_device_or_node_name(bs); for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) @@ -296,7 +290,7 @@ static int init_dirty_bitmap_migration(void) continue; } - if (drive_name == NULL) { + if (!name || strcmp(name, "") == 0) { error_report("Found bitmap '%s' in unnamed node %p. It can't " "be migrated", bdrv_dirty_bitmap_name(bitmap), bs); goto fail; @@ -313,7 +307,7 @@ static int init_dirty_bitmap_migration(void) dbms = g_new0(DirtyBitmapMigBitmapState, 1); dbms->bs = bs; - dbms->node_name = drive_name; + dbms->node_name = name; dbms->bitmap = bitmap; dbms->total_sectors = bdrv_nb_sectors(bs); dbms->sectors_per_chunk = CHUNK_SIZE * 8 * -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-05-14 20:19 [Qemu-devel] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method John Snow @ 2019-05-16 10:12 ` Vladimir Sementsov-Ogievskiy 2019-05-16 19:03 ` John Snow 2019-12-06 22:31 ` Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 13+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-05-16 10:12 UTC (permalink / raw) To: John Snow, qemu-devel, qemu-block Cc: Fam Zheng, Peter Maydell, aihua liang, Juan Quintela, Dr. David Alan Gilbert, Stefan Hajnoczi 14.05.2019 23:19, John Snow wrote: > Shift from looking at every root BDS to *every* BDS. This will migrate > bitmaps that are attached to blockdev created nodes instead of just ones > attached to emulated storage devices. > > Note that this will not migrate anonymous or internal-use bitmaps, as > those are defined as having no name. > > This will also fix the Coverity issues Peter Maydell has been asking > about for the past several releases, as well as fixing a real bug. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Reported-by: Coverity 😅 > Reported-by: aihua liang <aliang@redhat.com> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 > Fixes: Coverity CID 1390625 > CC: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > migration/block-dirty-bitmap.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c > index d1bb863cb6..4a896a09eb 100644 > --- a/migration/block-dirty-bitmap.c > +++ b/migration/block-dirty-bitmap.c > @@ -273,7 +273,6 @@ static int init_dirty_bitmap_migration(void) > BlockDriverState *bs; > BdrvDirtyBitmap *bitmap; > DirtyBitmapMigBitmapState *dbms; > - BdrvNextIterator it; > Error *local_err = NULL; > > dirty_bitmap_mig_state.bulk_completed = false; > @@ -281,13 +280,8 @@ static int init_dirty_bitmap_migration(void) > dirty_bitmap_mig_state.prev_bitmap = NULL; > dirty_bitmap_mig_state.no_bitmaps = false; > > - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { > - const char *drive_name = bdrv_get_device_or_node_name(bs); > - > - /* skip automatically inserted nodes */ > - while (bs && bs->drv && bs->implicit) { > - bs = backing_bs(bs); > - } hm, so, after the patch, for implicitly-filtered nodes we'll have node_name instead of device name.. But, on the other, hand, if we have implicitly-filtered node on target, we were doing wrong thing anyway, as dirty_bitmap_load_header don't skip implicit nodes. > + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { As I understand, difference with bdrv_next_node is that we don't skip unnamed nodes [...] > + const char *name = bdrv_get_device_or_node_name(bs); > > for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; > bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) > @@ -296,7 +290,7 @@ static int init_dirty_bitmap_migration(void) > continue; > } > > - if (drive_name == NULL) { > + if (!name || strcmp(name, "") == 0) { [...] to do this (may be paranoiac, but why not?) check > error_report("Found bitmap '%s' in unnamed node %p. It can't " > "be migrated", bdrv_dirty_bitmap_name(bitmap), bs); > goto fail; > @@ -313,7 +307,7 @@ static int init_dirty_bitmap_migration(void) > > dbms = g_new0(DirtyBitmapMigBitmapState, 1); > dbms->bs = bs; > - dbms->node_name = drive_name; > + dbms->node_name = name; > dbms->bitmap = bitmap; > dbms->total_sectors = bdrv_nb_sectors(bs); > dbms->sectors_per_chunk = CHUNK_SIZE * 8 * > There is still some mess about device name vs node name, and I don't know, could we somehow solve it, but patch looks OK for me anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-05-16 10:12 ` Vladimir Sementsov-Ogievskiy @ 2019-05-16 19:03 ` John Snow 2019-05-17 10:22 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 13+ messages in thread From: John Snow @ 2019-05-16 19:03 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block Cc: Fam Zheng, Peter Maydell, aihua liang, Juan Quintela, Dr. David Alan Gilbert, Stefan Hajnoczi On 5/16/19 6:12 AM, Vladimir Sementsov-Ogievskiy wrote: > 14.05.2019 23:19, John Snow wrote: >> Shift from looking at every root BDS to *every* BDS. This will migrate >> bitmaps that are attached to blockdev created nodes instead of just ones >> attached to emulated storage devices. >> >> Note that this will not migrate anonymous or internal-use bitmaps, as >> those are defined as having no name. >> >> This will also fix the Coverity issues Peter Maydell has been asking >> about for the past several releases, as well as fixing a real bug. >> >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Reported-by: Coverity 😅 >> Reported-by: aihua liang <aliang@redhat.com> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 >> Fixes: Coverity CID 1390625 >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> migration/block-dirty-bitmap.c | 14 ++++---------- >> 1 file changed, 4 insertions(+), 10 deletions(-) >> >> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c >> index d1bb863cb6..4a896a09eb 100644 >> --- a/migration/block-dirty-bitmap.c >> +++ b/migration/block-dirty-bitmap.c >> @@ -273,7 +273,6 @@ static int init_dirty_bitmap_migration(void) >> BlockDriverState *bs; >> BdrvDirtyBitmap *bitmap; >> DirtyBitmapMigBitmapState *dbms; >> - BdrvNextIterator it; >> Error *local_err = NULL; >> >> dirty_bitmap_mig_state.bulk_completed = false; >> @@ -281,13 +280,8 @@ static int init_dirty_bitmap_migration(void) >> dirty_bitmap_mig_state.prev_bitmap = NULL; >> dirty_bitmap_mig_state.no_bitmaps = false; >> >> - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { >> - const char *drive_name = bdrv_get_device_or_node_name(bs); >> - >> - /* skip automatically inserted nodes */ >> - while (bs && bs->drv && bs->implicit) { >> - bs = backing_bs(bs); >> - } > > hm, so, after the patch, for implicitly-filtered nodes we'll have node_name instead of device name.. > Oh, I see -- this does change what we send over the wire for interior/leaf nodes; that was unintentional on my part. After my patch, this requires that if you have a manually constructed tree such that you have attached a bitmap to an interior or leaf node, you *need* to name that node so that it can be consistently reconstructed at the target. I think that's a reasonable requirement and is actually superior to re-attaching all bitmaps to the root on migration (which would have happened before.) Codewise, what we have currently (both before and after this patch) is: if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) { qemu_put_counted_string(f, dbms->node_name); } So we named the constant "DEVICE_NAME", but the field was already named node_name, so this seems fine on the sending end. In practice, pre-patch we sent a device_name for any node that was the root attached to a device. Post-patch, that doesn't change because I am using the same API call to retrieve the name. For interior/leaf nodes, we now send the node-name specifically instead of the name of the device root. This requires identically constructed (or at least compatibly named) graphs on the source and destination, which is a reasonable requirement for migration. On the receiving end, we have this code: if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) { if (!qemu_get_counted_string(f, s->node_name)) { error_report("Unable to read node name string"); return -EINVAL; } s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err); which looks like a correct mapping. I think this is a safe change, even though I made it somewhat unintentionally. > But, on the other, hand, if we have implicitly-filtered node on target, we were doing wrong thing anyway, > as dirty_bitmap_load_header don't skip implicit nodes. > >> + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { > > As I understand, difference with bdrv_next_node is that we don't skip unnamed nodes [...] > The difference is that we iterate over states that aren't roots of trees; so not just unnamed nodes but rather intermediate nodes as well as nodes not attached to a storage device. bdrv_next says: `Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by the monitor or attached to a BlockBackend` So this loop is going to iterate over *everything*, not just top-level nodes. This lets me skip the tree-crawling loop that didn't work quite right. >> + const char *name = bdrv_get_device_or_node_name(bs); >> >> for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; >> bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) >> @@ -296,7 +290,7 @@ static int init_dirty_bitmap_migration(void) >> continue; >> } >> >> - if (drive_name == NULL) { >> + if (!name || strcmp(name, "") == 0) { > > [...] to do this (may be paranoiac, but why not?) check > Because bdrv_get_device_or_node_name technically has the capability to return an empty string, though I believe in contemporary block layer that we always generate a node-name. So it might be paranoid, but it matches the API I'm calling as documented. >> error_report("Found bitmap '%s' in unnamed node %p. It can't " >> "be migrated", bdrv_dirty_bitmap_name(bitmap), bs); >> goto fail; >> @@ -313,7 +307,7 @@ static int init_dirty_bitmap_migration(void) >> >> dbms = g_new0(DirtyBitmapMigBitmapState, 1); >> dbms->bs = bs; >> - dbms->node_name = drive_name; >> + dbms->node_name = name; >> dbms->bitmap = bitmap; >> dbms->total_sectors = bdrv_nb_sectors(bs); >> dbms->sectors_per_chunk = CHUNK_SIZE * 8 * >> > > There is still some mess about device name vs node name, and I don't know, could we somehow > solve it, but patch looks OK for me anyway: > Yes, there's more mess, but I think this is Less Wrong™️. I will try to combat some of this confusion soon; but I'll look at your cross-node merge patch first. > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Does this review still stand after my clarifications? --js ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-05-16 19:03 ` John Snow @ 2019-05-17 10:22 ` Vladimir Sementsov-Ogievskiy 2019-05-20 9:27 ` [Qemu-devel] [Qemu-block] " Kevin Wolf 0 siblings, 1 reply; 13+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-05-17 10:22 UTC (permalink / raw) To: John Snow, qemu-devel, qemu-block Cc: Fam Zheng, Peter Maydell, aihua liang, Juan Quintela, Dr. David Alan Gilbert, Stefan Hajnoczi 16.05.2019 22:03, John Snow wrote: > > > On 5/16/19 6:12 AM, Vladimir Sementsov-Ogievskiy wrote: >> 14.05.2019 23:19, John Snow wrote: >>> Shift from looking at every root BDS to *every* BDS. This will migrate >>> bitmaps that are attached to blockdev created nodes instead of just ones >>> attached to emulated storage devices. >>> >>> Note that this will not migrate anonymous or internal-use bitmaps, as >>> those are defined as having no name. >>> >>> This will also fix the Coverity issues Peter Maydell has been asking >>> about for the past several releases, as well as fixing a real bug. >>> >>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>> Reported-by: Coverity 😅 >>> Reported-by: aihua liang <aliang@redhat.com> >>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 >>> Fixes: Coverity CID 1390625 >>> CC: Stefan Hajnoczi <stefanha@redhat.com> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> migration/block-dirty-bitmap.c | 14 ++++---------- >>> 1 file changed, 4 insertions(+), 10 deletions(-) >>> >>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c >>> index d1bb863cb6..4a896a09eb 100644 >>> --- a/migration/block-dirty-bitmap.c >>> +++ b/migration/block-dirty-bitmap.c >>> @@ -273,7 +273,6 @@ static int init_dirty_bitmap_migration(void) >>> BlockDriverState *bs; >>> BdrvDirtyBitmap *bitmap; >>> DirtyBitmapMigBitmapState *dbms; >>> - BdrvNextIterator it; >>> Error *local_err = NULL; >>> >>> dirty_bitmap_mig_state.bulk_completed = false; >>> @@ -281,13 +280,8 @@ static int init_dirty_bitmap_migration(void) >>> dirty_bitmap_mig_state.prev_bitmap = NULL; >>> dirty_bitmap_mig_state.no_bitmaps = false; >>> >>> - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { >>> - const char *drive_name = bdrv_get_device_or_node_name(bs); >>> - >>> - /* skip automatically inserted nodes */ >>> - while (bs && bs->drv && bs->implicit) { >>> - bs = backing_bs(bs); >>> - } >> >> hm, so, after the patch, for implicitly-filtered nodes we'll have node_name instead of device name.. >> > > Oh, I see -- this does change what we send over the wire for > interior/leaf nodes; that was unintentional on my part. > > After my patch, this requires that if you have a manually constructed > tree such that you have attached a bitmap to an interior or leaf node, > you *need* to name that node so that it can be consistently > reconstructed at the target. > > I think that's a reasonable requirement and is actually superior to > re-attaching all bitmaps to the root on migration (which would have > happened before.) > > Codewise, what we have currently (both before and after this patch) is: > > if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) { > qemu_put_counted_string(f, dbms->node_name); > } > > So we named the constant "DEVICE_NAME", but the field was already named > node_name, so this seems fine on the sending end. In practice, pre-patch > we sent a device_name for any node that was the root attached to a > device. Post-patch, that doesn't change because I am using the same API > call to retrieve the name. > > For interior/leaf nodes, we now send the node-name specifically instead > of the name of the device root. This requires identically constructed > (or at least compatibly named) graphs on the source and destination, > which is a reasonable requirement for migration. > > On the receiving end, we have this code: > > if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) { > if (!qemu_get_counted_string(f, s->node_name)) { > error_report("Unable to read node name string"); > return -EINVAL; > } > s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err); > > which looks like a correct mapping. I think this is a safe change, even > though I made it somewhat unintentionally. > >> But, on the other, hand, if we have implicitly-filtered node on target, we were doing wrong thing anyway, >> as dirty_bitmap_load_header don't skip implicit nodes. >> >>> + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { >> >> As I understand, difference with bdrv_next_node is that we don't skip unnamed nodes [...] >> > > The difference is that we iterate over states that aren't roots of > trees; so not just unnamed nodes but rather intermediate nodes as well > as nodes not attached to a storage device. > > bdrv_next says: `Iterates over all top-level BlockDriverStates, i.e. > BDSs that are owned by the monitor or attached to a BlockBackend` > > So this loop is going to iterate over *everything*, not just top-level > nodes. This lets me skip the tree-crawling loop that didn't work quite > right. I meant not bdrv_next but bdrv_next_node, which iterates through graph nodes.. What is real difference between graph_bdrv_states and all_bdrv_states ? Node is inserted to graph_bdrv_states in bdrv_assign_node_name(), and to all_bdrv_states in bdrv_new(). Three calls to bdrv_new: bdrv_new_open_driver, calls bdrv_new and then bdrv_open_driver, which calls bdrv_assign_node_name, and if it fails new created node is released. bdrv_open_inherit bdrv_new .. bdrv_open_common bdrv_open_driver bdrv_assign_node_name iscsi_co_create_opts bdrv_new ... hmm.. looks like it a kind of temporary unnamed bs So, now, I'm not sure. Possibly we'd better use bdrv_next_node(). Kevin introduced all_bdrv_states in 0f12264e7a4145 , to use in drain instead of bdrv_next... But I don't understand, why he didn't use graph_bdrv_states and corresponding bdrv_next_node(), which is only skips some temporary or under-constuction nodes.. > >>> + const char *name = bdrv_get_device_or_node_name(bs); >>> >>> for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; >>> bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) >>> @@ -296,7 +290,7 @@ static int init_dirty_bitmap_migration(void) >>> continue; >>> } >>> >>> - if (drive_name == NULL) { >>> + if (!name || strcmp(name, "") == 0) { >> >> [...] to do this (may be paranoiac, but why not?) check >> > > Because bdrv_get_device_or_node_name technically has the capability to > return an empty string, though I believe in contemporary block layer > that we always generate a node-name. So it might be paranoid, but it > matches the API I'm calling as documented. > >>> error_report("Found bitmap '%s' in unnamed node %p. It can't " >>> "be migrated", bdrv_dirty_bitmap_name(bitmap), bs); >>> goto fail; >>> @@ -313,7 +307,7 @@ static int init_dirty_bitmap_migration(void) >>> >>> dbms = g_new0(DirtyBitmapMigBitmapState, 1); >>> dbms->bs = bs; >>> - dbms->node_name = drive_name; >>> + dbms->node_name = name; >>> dbms->bitmap = bitmap; >>> dbms->total_sectors = bdrv_nb_sectors(bs); >>> dbms->sectors_per_chunk = CHUNK_SIZE * 8 * >>> >> >> There is still some mess about device name vs node name, and I don't know, could we somehow >> solve it, but patch looks OK for me anyway: >> > > Yes, there's more mess, but I think this is Less Wrong™️. I will try to > combat some of this confusion soon; but I'll look at your cross-node > merge patch first. > >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> > > Does this review still stand after my clarifications? > Let's figure out graph_bdrv_states vs all_bdrv_states first. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-05-17 10:22 ` Vladimir Sementsov-Ogievskiy @ 2019-05-20 9:27 ` Kevin Wolf 2019-05-20 10:37 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 13+ messages in thread From: Kevin Wolf @ 2019-05-20 9:27 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy Cc: Fam Zheng, Peter Maydell, aihua liang, qemu-block, Juan Quintela, qemu-devel, Dr. David Alan Gilbert, Stefan Hajnoczi, John Snow Am 17.05.2019 um 12:22 hat Vladimir Sementsov-Ogievskiy geschrieben: > 16.05.2019 22:03, John Snow wrote: > > On 5/16/19 6:12 AM, Vladimir Sementsov-Ogievskiy wrote: > >> But, on the other, hand, if we have implicitly-filtered node on target, we were doing wrong thing anyway, > >> as dirty_bitmap_load_header don't skip implicit nodes. > >> > >>> + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { > >> > >> As I understand, difference with bdrv_next_node is that we don't skip unnamed nodes [...] > >> > > > > The difference is that we iterate over states that aren't roots of > > trees; so not just unnamed nodes but rather intermediate nodes as well > > as nodes not attached to a storage device. > > > > bdrv_next says: `Iterates over all top-level BlockDriverStates, i.e. > > BDSs that are owned by the monitor or attached to a BlockBackend` > > > > So this loop is going to iterate over *everything*, not just top-level > > nodes. This lets me skip the tree-crawling loop that didn't work quite > > right. > > I meant not bdrv_next but bdrv_next_node, which iterates through graph nodes.. > What is real difference between graph_bdrv_states and all_bdrv_states ? I don't think there is any relevant difference any more since commit 15489c769b9 ('block: auto-generated node-names'). We can only see a difference if a BDS was created, but never opened. This means either that we are still in the process of opening an image or that we have a bug somewhere. Maybe we should remove graph_bdrv_states and replace all of its uses with the more obviously correct all_bdrv_states (if we are sure that all users aren't called between creating and opening a BDS). > Node is inserted to graph_bdrv_states in bdrv_assign_node_name(), and to > all_bdrv_states in bdrv_new(). > > Three calls to bdrv_new: > > bdrv_new_open_driver, calls bdrv_new and then bdrv_open_driver, which calls bdrv_assign_node_name, > and if it fails new created node is released. > > bdrv_open_inherit > bdrv_new > .. > bdrv_open_common > bdrv_open_driver > bdrv_assign_node_name > > > iscsi_co_create_opts > bdrv_new > > ... hmm.. looks like it a kind of temporary unnamed bs > > So, now, I'm not sure. Possibly we'd better use bdrv_next_node(). I wonder if the iscsi one can't be replaced with bdrv_new_open_driver(). Manually building a half-opened BDS like it does currently looks suspicious. > Kevin introduced all_bdrv_states in 0f12264e7a4145 , to use in drain instead of > bdrv_next... But I don't understand, why he didn't use graph_bdrv_states and > corresponding bdrv_next_node(), which is only skips some temporary or under-constuction > nodes.. I probably just didn't realise that graph_bdrv_states exists and is effectively the same. I knew that all_bdrv_states contains what I want, so I just wanted to access that. But if anonymous BDSes did actually still exist, drain would want to wait for those as well, so it's the more natural choice anyway. Kevin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-05-20 9:27 ` [Qemu-devel] [Qemu-block] " Kevin Wolf @ 2019-05-20 10:37 ` Vladimir Sementsov-Ogievskiy 2019-05-20 16:43 ` John Snow 0 siblings, 1 reply; 13+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-05-20 10:37 UTC (permalink / raw) To: Kevin Wolf Cc: Fam Zheng, Peter Maydell, aihua liang, qemu-block, Juan Quintela, qemu-devel, Dr. David Alan Gilbert, Stefan Hajnoczi, John Snow 20.05.2019 12:27, Kevin Wolf wrote: > Am 17.05.2019 um 12:22 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 16.05.2019 22:03, John Snow wrote: >>> On 5/16/19 6:12 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> But, on the other, hand, if we have implicitly-filtered node on target, we were doing wrong thing anyway, >>>> as dirty_bitmap_load_header don't skip implicit nodes. >>>> >>>>> + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { >>>> >>>> As I understand, difference with bdrv_next_node is that we don't skip unnamed nodes [...] >>>> >>> >>> The difference is that we iterate over states that aren't roots of >>> trees; so not just unnamed nodes but rather intermediate nodes as well >>> as nodes not attached to a storage device. >>> >>> bdrv_next says: `Iterates over all top-level BlockDriverStates, i.e. >>> BDSs that are owned by the monitor or attached to a BlockBackend` >>> >>> So this loop is going to iterate over *everything*, not just top-level >>> nodes. This lets me skip the tree-crawling loop that didn't work quite >>> right. >> >> I meant not bdrv_next but bdrv_next_node, which iterates through graph nodes.. >> What is real difference between graph_bdrv_states and all_bdrv_states ? > > I don't think there is any relevant difference any more since commit > 15489c769b9 ('block: auto-generated node-names'). We can only see a > difference if a BDS was created, but never opened. This means either > that we are still in the process of opening an image or that we have a > bug somewhere. > > Maybe we should remove graph_bdrv_states and replace all of its uses > with the more obviously correct all_bdrv_states (if we are sure that > all users aren't called between creating and opening a BDS). > >> Node is inserted to graph_bdrv_states in bdrv_assign_node_name(), and to >> all_bdrv_states in bdrv_new(). >> >> Three calls to bdrv_new: >> >> bdrv_new_open_driver, calls bdrv_new and then bdrv_open_driver, which calls bdrv_assign_node_name, >> and if it fails new created node is released. >> >> bdrv_open_inherit >> bdrv_new >> .. >> bdrv_open_common >> bdrv_open_driver >> bdrv_assign_node_name >> >> >> iscsi_co_create_opts >> bdrv_new >> >> ... hmm.. looks like it a kind of temporary unnamed bs >> >> So, now, I'm not sure. Possibly we'd better use bdrv_next_node(). > > I wonder if the iscsi one can't be replaced with bdrv_new_open_driver(). > Manually building a half-opened BDS like it does currently looks > suspicious. > >> Kevin introduced all_bdrv_states in 0f12264e7a4145 , to use in drain instead of >> bdrv_next... But I don't understand, why he didn't use graph_bdrv_states and >> corresponding bdrv_next_node(), which is only skips some temporary or under-constuction >> nodes.. > > I probably just didn't realise that graph_bdrv_states exists and is > effectively the same. I knew that all_bdrv_states contains what I want, > so I just wanted to access that. > > But if anonymous BDSes did actually still exist, drain would want to > wait for those as well, so it's the more natural choice anyway. > > Kevin > Thank you for clarification. Then, my r-b still stand here, and fixing/refacting graph_nodes vs all_states should be a separate thing. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-05-20 10:37 ` Vladimir Sementsov-Ogievskiy @ 2019-05-20 16:43 ` John Snow 0 siblings, 0 replies; 13+ messages in thread From: John Snow @ 2019-05-20 16:43 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, Kevin Wolf Cc: Fam Zheng, Peter Maydell, aihua liang, qemu-block, Juan Quintela, qemu-devel, Dr. David Alan Gilbert, Stefan Hajnoczi On 5/20/19 6:37 AM, Vladimir Sementsov-Ogievskiy wrote: > 20.05.2019 12:27, Kevin Wolf wrote: >> Am 17.05.2019 um 12:22 hat Vladimir Sementsov-Ogievskiy geschrieben: >>> 16.05.2019 22:03, John Snow wrote: >>>> On 5/16/19 6:12 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>> But, on the other, hand, if we have implicitly-filtered node on target, we were doing wrong thing anyway, >>>>> as dirty_bitmap_load_header don't skip implicit nodes. >>>>> >>>>>> + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { >>>>> >>>>> As I understand, difference with bdrv_next_node is that we don't skip unnamed nodes [...] >>>>> >>>> >>>> The difference is that we iterate over states that aren't roots of >>>> trees; so not just unnamed nodes but rather intermediate nodes as well >>>> as nodes not attached to a storage device. >>>> >>>> bdrv_next says: `Iterates over all top-level BlockDriverStates, i.e. >>>> BDSs that are owned by the monitor or attached to a BlockBackend` >>>> >>>> So this loop is going to iterate over *everything*, not just top-level >>>> nodes. This lets me skip the tree-crawling loop that didn't work quite >>>> right. >>> >>> I meant not bdrv_next but bdrv_next_node, which iterates through graph nodes.. >>> What is real difference between graph_bdrv_states and all_bdrv_states ? >> >> I don't think there is any relevant difference any more since commit >> 15489c769b9 ('block: auto-generated node-names'). We can only see a >> difference if a BDS was created, but never opened. This means either >> that we are still in the process of opening an image or that we have a >> bug somewhere. >> >> Maybe we should remove graph_bdrv_states and replace all of its uses >> with the more obviously correct all_bdrv_states (if we are sure that >> all users aren't called between creating and opening a BDS). >> >>> Node is inserted to graph_bdrv_states in bdrv_assign_node_name(), and to >>> all_bdrv_states in bdrv_new(). >>> >>> Three calls to bdrv_new: >>> >>> bdrv_new_open_driver, calls bdrv_new and then bdrv_open_driver, which calls bdrv_assign_node_name, >>> and if it fails new created node is released. >>> >>> bdrv_open_inherit >>> bdrv_new >>> .. >>> bdrv_open_common >>> bdrv_open_driver >>> bdrv_assign_node_name >>> >>> >>> iscsi_co_create_opts >>> bdrv_new >>> >>> ... hmm.. looks like it a kind of temporary unnamed bs >>> >>> So, now, I'm not sure. Possibly we'd better use bdrv_next_node(). >> >> I wonder if the iscsi one can't be replaced with bdrv_new_open_driver(). >> Manually building a half-opened BDS like it does currently looks >> suspicious. >> >>> Kevin introduced all_bdrv_states in 0f12264e7a4145 , to use in drain instead of >>> bdrv_next... But I don't understand, why he didn't use graph_bdrv_states and >>> corresponding bdrv_next_node(), which is only skips some temporary or under-constuction >>> nodes.. >> >> I probably just didn't realise that graph_bdrv_states exists and is >> effectively the same. I knew that all_bdrv_states contains what I want, >> so I just wanted to access that. >> >> But if anonymous BDSes did actually still exist, drain would want to >> wait for those as well, so it's the more natural choice anyway. >> >> Kevin >> > > Thank you for clarification. Then, my r-b still stand here, and fixing/refacting > graph_nodes vs all_states should be a separate thing. > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Great, thanks all :) I think I will stage this through my tree -- on the premise that David Gilbert won't want to stage a block patch, and that since it's not Kevin's tree, he won't mind either. --js ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-05-14 20:19 [Qemu-devel] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method John Snow 2019-05-16 10:12 ` Vladimir Sementsov-Ogievskiy @ 2019-12-06 22:31 ` Vladimir Sementsov-Ogievskiy 2019-12-09 15:22 ` John Snow 1 sibling, 1 reply; 13+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-12-06 22:31 UTC (permalink / raw) To: John Snow, qemu-devel, qemu-block Cc: Fam Zheng, Peter Maydell, aihua liang, Juan Quintela, Dr. David Alan Gilbert, Stefan Hajnoczi 14.05.2019 23:19, John Snow wrote: > Shift from looking at every root BDS to *every* BDS. This will migrate > bitmaps that are attached to blockdev created nodes instead of just ones > attached to emulated storage devices. > > Note that this will not migrate anonymous or internal-use bitmaps, as > those are defined as having no name. > > This will also fix the Coverity issues Peter Maydell has been asking > about for the past several releases, as well as fixing a real bug. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Reported-by: Coverity 😅 What was the coverity number (I don't believe that it was smile:)? Do someone know, that this patch fixes very-very-very terrible bug? Before this patch, here were bdrv_next-based loop, with exists from it, but not using bdrv_next_cleanup(). This leads to leaked (incremented) refcnt of bds on any failure during this loop! Now we faced this bug, in Rhel-based Qemu, so I strongly recommend to fix it in Rhel. > Reported-by: aihua liang <aliang@redhat.com> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 > Fixes: Coverity CID 1390625 > CC: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > migration/block-dirty-bitmap.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c > index d1bb863cb6..4a896a09eb 100644 > --- a/migration/block-dirty-bitmap.c > +++ b/migration/block-dirty-bitmap.c > @@ -273,7 +273,6 @@ static int init_dirty_bitmap_migration(void) > BlockDriverState *bs; > BdrvDirtyBitmap *bitmap; > DirtyBitmapMigBitmapState *dbms; > - BdrvNextIterator it; > Error *local_err = NULL; > > dirty_bitmap_mig_state.bulk_completed = false; > @@ -281,13 +280,8 @@ static int init_dirty_bitmap_migration(void) > dirty_bitmap_mig_state.prev_bitmap = NULL; > dirty_bitmap_mig_state.no_bitmaps = false; > > - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { > - const char *drive_name = bdrv_get_device_or_node_name(bs); > - > - /* skip automatically inserted nodes */ > - while (bs && bs->drv && bs->implicit) { > - bs = backing_bs(bs); > - } > + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { > + const char *name = bdrv_get_device_or_node_name(bs); > > for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; > bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) > @@ -296,7 +290,7 @@ static int init_dirty_bitmap_migration(void) > continue; > } > > - if (drive_name == NULL) { > + if (!name || strcmp(name, "") == 0) { > error_report("Found bitmap '%s' in unnamed node %p. It can't " > "be migrated", bdrv_dirty_bitmap_name(bitmap), bs); > goto fail; > @@ -313,7 +307,7 @@ static int init_dirty_bitmap_migration(void) > > dbms = g_new0(DirtyBitmapMigBitmapState, 1); > dbms->bs = bs; > - dbms->node_name = drive_name; > + dbms->node_name = name; > dbms->bitmap = bitmap; > dbms->total_sectors = bdrv_nb_sectors(bs); > dbms->sectors_per_chunk = CHUNK_SIZE * 8 * > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-12-06 22:31 ` Vladimir Sementsov-Ogievskiy @ 2019-12-09 15:22 ` John Snow 2019-12-09 15:26 ` John Snow 0 siblings, 1 reply; 13+ messages in thread From: John Snow @ 2019-12-09 15:22 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block Cc: Fam Zheng, Peter Maydell, aihua liang, Juan Quintela, Dr. David Alan Gilbert, Stefan Hajnoczi On 12/6/19 5:31 PM, Vladimir Sementsov-Ogievskiy wrote: > 14.05.2019 23:19, John Snow wrote: >> Shift from looking at every root BDS to *every* BDS. This will migrate >> bitmaps that are attached to blockdev created nodes instead of just ones >> attached to emulated storage devices. >> >> Note that this will not migrate anonymous or internal-use bitmaps, as >> those are defined as having no name. >> >> This will also fix the Coverity issues Peter Maydell has been asking >> about for the past several releases, as well as fixing a real bug. >> >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Reported-by: Coverity 😅 > > What was the coverity number (I don't believe that it was smile:)? > Reported-by: Peter Maydell <peter.maydell@linaro.org> Reported-by: Coverity 😅 Reported-by: aihua liang <aliang@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: John Snow <jsnow@redhat.com> Message-id: 20190514201926.10407-1-jsnow@redhat.com Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 Fixes: Coverity CID 1390625 CC: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: John Snow <jsnow@redhat.com> > Do someone know, that this patch fixes very-very-very terrible bug? > > Before this patch, here were bdrv_next-based loop, with exists from it, > but not using bdrv_next_cleanup(). This leads to leaked (incremented) refcnt of > bds on any failure during this loop! > > Now we faced this bug, in Rhel-based Qemu, so I strongly recommend to fix it in Rhel. OK, this was fixed for 4.1, and was introduced in b35ebdf076d for 2.12.0, so all versions between have the problem. > >> Reported-by: aihua liang <aliang@redhat.com> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 >> Fixes: Coverity CID 1390625 >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> migration/block-dirty-bitmap.c | 14 ++++---------- >> 1 file changed, 4 insertions(+), 10 deletions(-) >> >> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c >> index d1bb863cb6..4a896a09eb 100644 >> --- a/migration/block-dirty-bitmap.c >> +++ b/migration/block-dirty-bitmap.c >> @@ -273,7 +273,6 @@ static int init_dirty_bitmap_migration(void) >> BlockDriverState *bs; >> BdrvDirtyBitmap *bitmap; >> DirtyBitmapMigBitmapState *dbms; >> - BdrvNextIterator it; >> Error *local_err = NULL; >> >> dirty_bitmap_mig_state.bulk_completed = false; >> @@ -281,13 +280,8 @@ static int init_dirty_bitmap_migration(void) >> dirty_bitmap_mig_state.prev_bitmap = NULL; >> dirty_bitmap_mig_state.no_bitmaps = false; >> >> - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { >> - const char *drive_name = bdrv_get_device_or_node_name(bs); >> - >> - /* skip automatically inserted nodes */ >> - while (bs && bs->drv && bs->implicit) { >> - bs = backing_bs(bs); >> - } >> + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { >> + const char *name = bdrv_get_device_or_node_name(bs); >> >> for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; >> bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) >> @@ -296,7 +290,7 @@ static int init_dirty_bitmap_migration(void) >> continue; >> } >> >> - if (drive_name == NULL) { >> + if (!name || strcmp(name, "") == 0) { >> error_report("Found bitmap '%s' in unnamed node %p. It can't " >> "be migrated", bdrv_dirty_bitmap_name(bitmap), bs); >> goto fail; >> @@ -313,7 +307,7 @@ static int init_dirty_bitmap_migration(void) >> >> dbms = g_new0(DirtyBitmapMigBitmapState, 1); >> dbms->bs = bs; >> - dbms->node_name = drive_name; >> + dbms->node_name = name; >> dbms->bitmap = bitmap; >> dbms->total_sectors = bdrv_nb_sectors(bs); >> dbms->sectors_per_chunk = CHUNK_SIZE * 8 * >> > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-12-09 15:22 ` John Snow @ 2019-12-09 15:26 ` John Snow 2019-12-09 15:45 ` John Snow 2019-12-09 17:17 ` Vladimir Sementsov-Ogievskiy 0 siblings, 2 replies; 13+ messages in thread From: John Snow @ 2019-12-09 15:26 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block (off list) On 12/9/19 10:22 AM, John Snow wrote: > > > On 12/6/19 5:31 PM, Vladimir Sementsov-Ogievskiy wrote: >> 14.05.2019 23:19, John Snow wrote: >>> Shift from looking at every root BDS to *every* BDS. This will migrate >>> bitmaps that are attached to blockdev created nodes instead of just ones >>> attached to emulated storage devices. >>> >>> Note that this will not migrate anonymous or internal-use bitmaps, as >>> those are defined as having no name. >>> >>> This will also fix the Coverity issues Peter Maydell has been asking >>> about for the past several releases, as well as fixing a real bug. >>> >>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>> Reported-by: Coverity 😅 >> >> What was the coverity number (I don't believe that it was smile:)? >> > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Reported-by: Coverity 😅 > Reported-by: aihua liang <aliang@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: John Snow <jsnow@redhat.com> > Message-id: 20190514201926.10407-1-jsnow@redhat.com > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 > Fixes: Coverity CID 1390625 > CC: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > > >> Do someone know, that this patch fixes very-very-very terrible bug? >> >> Before this patch, here were bdrv_next-based loop, with exists from it, >> but not using bdrv_next_cleanup(). This leads to leaked (incremented) refcnt of >> bds on any failure during this loop! >> >> Now we faced this bug, in Rhel-based Qemu, so I strongly recommend to fix it in Rhel. > > OK, this was fixed for 4.1, and was introduced in b35ebdf076d for > 2.12.0, so all versions between have the problem. > As far as I know, we don't "support" incremental backup for RHEL based packages, because we only support what you can do directly through libvirt. And since RHEL libvirt doesn't have incremental backup, ... I can try to fix it anyway, though, if it makes your life easier especially. Which version(s) are you using? I'll try to target a fix for that version, but it will likely be a special fix that just fixes the leak without changing the enumeration method, to keep migration ABI consistent with what we expect from the different versions. --js ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-12-09 15:26 ` John Snow @ 2019-12-09 15:45 ` John Snow 2019-12-09 17:17 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 13+ messages in thread From: John Snow @ 2019-12-09 15:45 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block On 12/9/19 10:26 AM, John Snow wrote: > (off list) lol, or not. No big deal. all the code goes upstream anyway (: ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-12-09 15:26 ` John Snow 2019-12-09 15:45 ` John Snow @ 2019-12-09 17:17 ` Vladimir Sementsov-Ogievskiy 2019-12-09 18:15 ` John Snow 1 sibling, 1 reply; 13+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-12-09 17:17 UTC (permalink / raw) To: John Snow, qemu-devel, qemu-block 09.12.2019 18:26, John Snow wrote: > (off list) > > On 12/9/19 10:22 AM, John Snow wrote: >> >> >> On 12/6/19 5:31 PM, Vladimir Sementsov-Ogievskiy wrote: >>> 14.05.2019 23:19, John Snow wrote: >>>> Shift from looking at every root BDS to *every* BDS. This will migrate >>>> bitmaps that are attached to blockdev created nodes instead of just ones >>>> attached to emulated storage devices. >>>> >>>> Note that this will not migrate anonymous or internal-use bitmaps, as >>>> those are defined as having no name. >>>> >>>> This will also fix the Coverity issues Peter Maydell has been asking >>>> about for the past several releases, as well as fixing a real bug. >>>> >>>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>>> Reported-by: Coverity 😅 >>> >>> What was the coverity number (I don't believe that it was smile:)? >>> >> >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Reported-by: Coverity 😅 >> Reported-by: aihua liang <aliang@redhat.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> Message-id: 20190514201926.10407-1-jsnow@redhat.com >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 >> Fixes: Coverity CID 1390625 Ah, missed it, sorry. >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> >> >>> Do someone know, that this patch fixes very-very-very terrible bug? >>> >>> Before this patch, here were bdrv_next-based loop, with exists from it, >>> but not using bdrv_next_cleanup(). This leads to leaked (incremented) refcnt of >>> bds on any failure during this loop! >>> >>> Now we faced this bug, in Rhel-based Qemu, so I strongly recommend to fix it in Rhel. >> >> OK, this was fixed for 4.1, and was introduced in b35ebdf076d for >> 2.12.0, so all versions between have the problem. >> > > As far as I know, we don't "support" incremental backup for RHEL based > packages, because we only support what you can do directly through > libvirt. And since RHEL libvirt doesn't have incremental backup, ... > > I can try to fix it anyway, though, if it makes your life easier especially. No, actually, it's no real difference, now we fixed it in our branch. So, it's up to you. > > Which version(s) are you using? 2.12.0-33 > I'll try to target a fix for that > version, but it will likely be a special fix that just fixes the leak > without changing the enumeration method, to keep migration ABI > consistent with what we expect from the different versions. > > --js > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method 2019-12-09 17:17 ` Vladimir Sementsov-Ogievskiy @ 2019-12-09 18:15 ` John Snow 0 siblings, 0 replies; 13+ messages in thread From: John Snow @ 2019-12-09 18:15 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block On 12/9/19 12:17 PM, Vladimir Sementsov-Ogievskiy wrote: > 09.12.2019 18:26, John Snow wrote: >> (off list) >> Well, it isn't now, but oh well. :) >> On 12/9/19 10:22 AM, John Snow wrote: >>> >>> >>> On 12/6/19 5:31 PM, Vladimir Sementsov-Ogievskiy wrote: >>>> 14.05.2019 23:19, John Snow wrote: >>>>> Shift from looking at every root BDS to *every* BDS. This will migrate >>>>> bitmaps that are attached to blockdev created nodes instead of just ones >>>>> attached to emulated storage devices. >>>>> >>>>> Note that this will not migrate anonymous or internal-use bitmaps, as >>>>> those are defined as having no name. >>>>> >>>>> This will also fix the Coverity issues Peter Maydell has been asking >>>>> about for the past several releases, as well as fixing a real bug. >>>>> >>>>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>>>> Reported-by: Coverity 😅 >>>> >>>> What was the coverity number (I don't believe that it was smile:)? >>>> >>> >>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>> Reported-by: Coverity 😅 >>> Reported-by: aihua liang <aliang@redhat.com> >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> Message-id: 20190514201926.10407-1-jsnow@redhat.com >>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490 >>> Fixes: Coverity CID 1390625 > > Ah, missed it, sorry. > No problem -- it got added for the PR instead of the patch. >>> CC: Stefan Hajnoczi <stefanha@redhat.com> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> >>> >>>> Do someone know, that this patch fixes very-very-very terrible bug? >>>> >>>> Before this patch, here were bdrv_next-based loop, with exists from it, >>>> but not using bdrv_next_cleanup(). This leads to leaked (incremented) refcnt of >>>> bds on any failure during this loop! >>>> >>>> Now we faced this bug, in Rhel-based Qemu, so I strongly recommend to fix it in Rhel. >>> >>> OK, this was fixed for 4.1, and was introduced in b35ebdf076d for >>> 2.12.0, so all versions between have the problem. >>> >> >> As far as I know, we don't "support" incremental backup for RHEL based >> packages, because we only support what you can do directly through >> libvirt. And since RHEL libvirt doesn't have incremental backup, ... >> >> I can try to fix it anyway, though, if it makes your life easier especially. > > No, actually, it's no real difference, now we fixed it in our branch. > So, it's up to you. > OK, thanks for the report, then! --js ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-12-09 18:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-14 20:19 [Qemu-devel] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method John Snow 2019-05-16 10:12 ` Vladimir Sementsov-Ogievskiy 2019-05-16 19:03 ` John Snow 2019-05-17 10:22 ` Vladimir Sementsov-Ogievskiy 2019-05-20 9:27 ` [Qemu-devel] [Qemu-block] " Kevin Wolf 2019-05-20 10:37 ` Vladimir Sementsov-Ogievskiy 2019-05-20 16:43 ` John Snow 2019-12-06 22:31 ` Vladimir Sementsov-Ogievskiy 2019-12-09 15:22 ` John Snow 2019-12-09 15:26 ` John Snow 2019-12-09 15:45 ` John Snow 2019-12-09 17:17 ` Vladimir Sementsov-Ogievskiy 2019-12-09 18:15 ` John Snow
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.