Re: [Qemu-devel] [PATCH v10 05/20] dirty-bitmap: Avoid size query failure during truncate

[Eric Blake]

On 09/25/2017 10:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.09.2017 17:55, Eric Blake wrote:
>> We've previously fixed several places where we failed to account
>> for possible errors from bdrv_nb_sectors().  Fix another one by
>> making bdrv_dirty_bitmap_truncate() take the new size from the
>> caller instead of querying itself; then adjust the sole caller
>> bdrv_truncate() to pass the size just determined by a successful
>> resize, or to skip the bitmap resize on failure, thus avoiding
> 
> nothing is skipped in this version

So much for me squashing in a change.  If the maintainer is willing
(Kevin, is this going through your tree?), we can reword that to:

"...pass the size just determined by a successful resize, or to reuse
the size given to the original truncate operation when
refresh_total_sectors() was not able to confirm the actual size (the two
sizes can potentially differ according to rounding constraints), thus
avoiding..."

> 
>> sizing the bitmaps to -1.  This also fixes a bug where not all
>> failure paths in bdrv_truncate() would set errp.
>>
>> Note that bdrv_truncate() is still a bit awkward.  We may want
>> to revisit it later and clean up things to better guarantee that
>> a resize attempt either fails cleanly up front, or cannot fail
>> after guest-visible changes have been made (if temporary changes
>> are made, then they need to be cleanly rolled back).  But that
>> is a task for another day; for now, the goal is the bare minimum
>> fix to ensure that just bdrv_dirty_bitmap_truncate() cannot fail.
>>
>> Signed-off-by: Eric Blake <address@hidden>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> 
> 
>>
>> ---
>> v10: always resize bitmap as before [John], enhance commit message to
>> point out errp bugfix [Vladimir]
>> v9: skip only bdrv_dirty_bitmap_truncate on error [Fam]
>> v8: retitle and rework to avoid possibility of secondary failure [John]
>> v7: new patch [Kevin]
>> ---
>>   include/block/dirty-bitmap.h |  2 +-
>>   block.c                      | 16 +++++++++++-----
>>   block/dirty-bitmap.c         |  6 +++---
>>   3 files changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 8fd842eac9..7a27590047 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -83,7 +83,7 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter
>> *iter);
>>   void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
>>   int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
>>   int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
>> -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
>>   bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>> diff --git a/block.c b/block.c
>> index 528cda7b2c..ef5af81f66 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3545,12 +3545,18 @@ int bdrv_truncate(BdrvChild *child, int64_t
>> offset, PreallocMode prealloc,
>>       assert(!(bs->open_flags & BDRV_O_INACTIVE));
>>
>>       ret = drv->bdrv_truncate(bs, offset, prealloc, errp);
>> -    if (ret == 0) {
>> -        ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>> -        bdrv_dirty_bitmap_truncate(bs);
>> -        bdrv_parent_cb_resize(bs);
>> -        atomic_inc(&bs->write_gen);
>> +    if (ret < 0) {
>> +        return ret;
>>       }
>> +    ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Could not refresh total sector
>> count");
>> +    } else {
>> +        offset = bs->total_sectors * BDRV_SECTOR_SIZE;
>> +    }
>> +    bdrv_dirty_bitmap_truncate(bs, offset);
>> +    bdrv_parent_cb_resize(bs);
>> +    atomic_inc(&bs->write_gen);
>>       return ret;
>>   }
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 42a55e4a4b..ee164fb518 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -1,7 +1,7 @@
>>   /*
>>    * Block Dirty Bitmap
>>    *
>> - * Copyright (c) 2016 Red Hat. Inc
>> + * Copyright (c) 2016-2017 Red Hat. Inc
>>    *
>>    * Permission is hereby granted, free of charge, to any person
>> obtaining a copy
>>    * of this software and associated documentation files (the
>> "Software"), to deal
>> @@ -302,10 +302,10 @@ BdrvDirtyBitmap
>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>    * Truncates _all_ bitmaps attached to a BDS.
>>    * Called with BQL taken.
>>    */
>> -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
>>   {
>>       BdrvDirtyBitmap *bitmap;
>> -    uint64_t size = bdrv_nb_sectors(bs);
>> +    int64_t size = DIV_ROUND_UP(bytes, BDRV_SECTOR_SIZE);
>>
>>       bdrv_dirty_bitmaps_lock(bs);
>>       QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature