[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query failure
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query failure during truncate |
Date: |
Mon, 25 Sep 2017 08:45:50 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
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 <address@hidden>
>>
>> ---
>> 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
[Qemu-devel] [PATCH v9 02/20] hbitmap: Rename serialization_granularity to serialization_align, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 03/20] qcow2: Ensure bitmap serialization is aligned, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 06/20] dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 07/20] dirty-bitmap: Track bitmap size by bytes, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 08/20] dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to take bytes, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 09/20] qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 10/20] dirty-bitmap: Set iterator start by offset, not sector, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 13/20] dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 12/20] dirty-bitmap: Change bdrv_get_dirty_count() to report bytes, Eric Blake, 2017/09/19