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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3783 bytes --]

On 09/23/2017 07:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2017 23:18, 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
>> sizing the bitmaps to -1.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> 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                      | 15 ++++++++++-----
>>   block/dirty-bitmap.c         |  6 +++---
>>   3 files changed, 14 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);
> 
> why not uint64_t as in following patches?

Because off_t is signed, so you can never have more than 2^63 (and NOT
2^64) bytes for your size anyways.  The following patches use int64_t,
rather than uint64_t, both because of off_t, and because it leaves room
for returning negative values on error.

> 
>>   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 ee6a48976e..89261a7a53 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3450,12 +3450,17 @@ 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");
> 
> looks like a separate bug - we didn't set errp with <0 return value

Yes, it was a pre-existing bug.  If I have to respin, I can update the
commit message to mention it.

> 
> Looks like this all needs more work to make it really correct and safe
> (reading last John's comment)..
> And this patch don't really relate to the series, so if it will be the
> only obstacle for merging, can we
> merge all other patches first? I'll then rebase dirty bitmap migration
> series on master..

But it does relate, because I have to do something to avoid calling a
failing bdrv_nb_sectors/bdrv_getlength.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]