[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 05/18] dirty-bitmap: Change bdrv_dirty_bitmap
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v6 05/18] dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes |
Date: |
Fri, 8 Sep 2017 09:04:15 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 09/08/2017 07:22 AM, Kevin Wolf wrote:
> Am 30.08.2017 um 23:05 hat Eric Blake geschrieben:
>> We are still using an internal hbitmap that tracks a size in sectors,
>> with the granularity scaled down accordingly, because it lets us
>> use a shortcut for our iterators which are currently sector-based.
>> But there's no reason we can't track the dirty bitmap size in bytes,
>> since it is (mostly) an internal-only variable (remember, the size
>> is how many bytes are covered by the bitmap, not how many bytes the
>> bitmap occupies). Furthermore, we're already reporting bytes for
>> bdrv_dirty_bitmap_granularity(); mixing bytes and sectors in our
>> return values is a recipe for confusion. A later cleanup will
>> convert dirty bitmap internals to be entirely byte-based,
>> eliminating the intermediate sector rounding added here; and
>> technically, since bdrv_getlength() already rounds up to sectors,
>> our use of DIV_ROUND_UP is more for theoretical completeness than
>> for any actual rounding.
>>
>> The only external caller in qcow2-bitmap.c is temporarily more verbose
>> (because it is still using sector-based math), but will later be
>> switched to track progress by bytes instead of sectors.
>>
>> Use is_power_of_2() while at it, instead of open-coding that, and
>> add an assertion where bdrv_getlength() should not fail.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> Reviewed-by: John Snow <address@hidden>
>
> I think I would have preferred to change the unit of
> BdrvDirtyBitmap.size in one patch and the unit of the return value of
> bdrv_dirty_bitmap_size() in another one to keep review a bit easier.
I can split on respin, if there's still enough reason for a respin.
>> @@ -305,13 +307,14 @@ BdrvDirtyBitmap
>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>> void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>> {
>> BdrvDirtyBitmap *bitmap;
>> - uint64_t size = bdrv_nb_sectors(bs);
>> + int64_t size = bdrv_getlength(bs);
>>
>> + assert(size >= 0);
>
> How can you assert that there will never be an error? Even if it's
> correct (I don't know whether you can have dirty bitmaps on devices that
> don't use the cached value), this needs at least a comment.
The old code wasn't checking for errors; if an error occurs, we have no
way to report it. So I indeed need to audit whether all callers have a
cached length at this point in time (it can't fail), or else change
bdrv_dirty_bitmap_truncate() to be able to fail (pass failure along) and
update all callers. This may indeed be reason for a respin, depending
on what I find.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature