Re: [Qemu-block] [PATCH] block: simplify code around releasing bitmaps

[Paolo Bonzini]

On 26/03/2018 12:24, Vladimir Sementsov-Ogievskiy wrote:
> 23.03.2018 19:42, Paolo Bonzini wrote:
>> QLIST_REMOVE does not require walking the list, and once the "bitmap"
>> argument is removed from bdrv_do_release_matching_dirty_bitmap_locked
>> the code simplifies a lot and it is worth inlining everything in the
>> callers of bdrv_do_release_matching_dirty_bitmap.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>   block/dirty-bitmap.c | 81
>> +++++++++++++++++++---------------------------------
>>   1 file changed, 30 insertions(+), 51 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 13bb5066a4..c0fb49bf7b 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -250,48 +250,15 @@ void
>> bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
>>   }
>>     /* Called within bdrv_dirty_bitmap_lock..unlock */
> 
> Worth adding "Called with BQL taken" to the comment?

Yes, good idea.

>> +static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)

> worth adding "Called with BQL taken." to the comment?
> bdrv_release_named_dirty_bitmaps functions has it.
> 
>>   void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs)

This is pre-existing, but it's also a good idea.

Paolo

>> -    bdrv_do_release_matching_dirty_bitmap(bs, NULL,
>> -                                         
>> bdrv_dirty_bitmap_get_persistance);
>> +    BdrvDirtyBitmap *bm, *next;
>> +
>> +    bdrv_dirty_bitmaps_lock(bs);
>> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>> +        if (bdrv_dirty_bitmap_get_persistance(bm)) {
>> +            bdrv_release_dirty_bitmap_locked(bm);
>> +        }
>> +    }
>> +    bdrv_dirty_bitmaps_unlock(bs);
>>   }
>>     /**
> 
> anyway,
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>