[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 12/23] block: Convert bdrv_get_block_status_a
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH v4 12/23] block: Convert bdrv_get_block_status_above() to bytes |
Date: |
Wed, 27 Sep 2017 15:40:16 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 09/27/2017 02:57 PM, Eric Blake wrote:
> On 09/27/2017 01:41 PM, John Snow wrote:
>>
>>
>> On 09/13/2017 12:03 PM, Eric Blake wrote:
>>> We are gradually moving away from sector-based interfaces, towards
>>> byte-based. In the common case, allocation is unlikely to ever use
>>> values that are not naturally sector-aligned, but it is possible
>>> that byte-based values will let us be more precise about allocation
>>> at the end of an unaligned file that can do byte-based access.
>>>
>>> Changing the name of the function from bdrv_get_block_status_above()
>>> to bdrv_block_status_above() ensures that the compiler enforces that
>>> all callers are updated. For now, the io.c layer still assert()s
>>> that all callers are sector-aligned, but that can be relaxed when a
>>> later patch implements byte-based block status in the drivers.
>>>
>>> For the most part this patch is just the addition of scaling at the
>>> callers followed by inverse scaling at bdrv_block_status(). But some
>>> code, particularly bdrv_block_status(), gets a lot simpler because
>>> it no longer has to mess with sectors. Likewise, mirror code no
>>> longer computes s->granularity >> BDRV_SECTOR_BITS, and can therefore
>>> drop an assertion (fix a neighboring assertion to use is_power_of_2
>>> while there).
>>>
>>
>> Huh, I suppose so, yeah. Do you have a test that covers what happens in
>> this newly available use case?
>
> Not directly - the mirror code no longer requires sector alignment, but
> is still unlikely to use sub-sector requests unless a particular driver
> returns really small status information. I suppose we could tweak the
> blkdebug driver to force status requests to be fragmented at
> ridiculously small alignments, and then prove that mirroring still
> occurs correctly, once all the series are in, but it's probably more
> effort than it is worth to force sub-sector mirroring if we don't have a
> real use case that will rely on it.
>
Hmm, yeah, the code probably can't be exercised currently but I do
wonder if we're removing too many breadcrumbs for potential problem
spots if someone decides to return sub-sector information in the future.
Well, I suppose I haven't been too diligent about complaining about
their removal elsewhere, so for consistency:
Either with or without the assertion removed as you see fit:
Reviewed-by: John Snow <address@hidden>
>>
>>> For ease of review, bdrv_get_block_status() was tackled separately.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>>
>> Looks mechanically correct, anyway.
>>
>> Reviewed-by: John Snow <address@hidden>
>>
>
- [Qemu-devel] [PATCH v4 15/23] qemu-img: Add find_nonzero(), (continued)
- [Qemu-devel] [PATCH v4 17/23] qemu-img: Change check_empty_sectors() to byte-based, Eric Blake, 2017/09/13
- [Qemu-devel] [PATCH v4 18/23] qemu-img: Change compare_sectors() to be byte-based, Eric Blake, 2017/09/13
- [Qemu-devel] [PATCH v4 19/23] qemu-img: Change img_rebase() to be byte-based, Eric Blake, 2017/09/13
- [Qemu-devel] [PATCH v4 20/23] qemu-img: Change img_compare() to be byte-based, Eric Blake, 2017/09/13
- [Qemu-devel] [PATCH v4 21/23] block: Align block status requests, Eric Blake, 2017/09/13