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 <js...@redhat.com> >> >>> For ease of review, bdrv_get_block_status() was tackled separately. >>> >>> Signed-off-by: Eric Blake <ebl...@redhat.com> >>> >> >> Looks mechanically correct, anyway. >> >> Reviewed-by: John Snow <js...@redhat.com> >> >
