[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method
From: |
John Snow |
Subject: |
Re: [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method |
Date: |
Mon, 9 Dec 2019 10:22:49 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 |
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 <address@hidden>
>> Reported-by: Coverity 😅
>
> What was the coverity number (I don't believe that it was smile:)?
>
Reported-by: Peter Maydell <address@hidden>
Reported-by: Coverity 😅
Reported-by: aihua liang <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Signed-off-by: John Snow <address@hidden>
Message-id: address@hidden
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490
Fixes: Coverity CID 1390625
CC: Stefan Hajnoczi <address@hidden>
Signed-off-by: John Snow <address@hidden>
> 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 <address@hidden>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490
>> Fixes: Coverity CID 1390625
>> CC: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>> 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 *
>>
>
>