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 <ebl...@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
signature.asc
Description: OpenPGP digital signature
