Re: [Qemu-block] [PATCH v5 07/23] block: Convert bdrv_get_block_status() to bytes

[Eric Blake]

On 10/10/2017 09:46 AM, Kevin Wolf wrote:
> Am 04.10.2017 um 04:00 hat Eric Blake geschrieben:
>> 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() to
>> bdrv_block_status() 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.
>>
>> Note that we have an inherent limitation in the BDRV_BLOCK_* return
>> values: BDRV_BLOCK_OFFSET_VALID can only return the start of a
>> sector, even if we later relax the interface to query for the status
>> starting at an intermediate byte; document the obvious interpretation
>> that valid offsets are always sector-relative.
>>
>> Therefore, 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_is_allocated(), gets a lot simpler because
>> it no longer has to mess with sectors.
>>
>> For ease of review, bdrv_get_block_status_above() will be tackled
>> separately.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>

>> +++ b/include/block/block.h
> 
> A bit above the first hunk:
> 
>  * Allocation status flags for bdrv_get_block_status() and friends.
> 
> This should be bdrv_block_status() now.

Good catch. It wasn't there on v1, but I forgot to re-grep when rebasing.

>> @@ -138,8 +138,10 @@ typedef struct HDGeometry {
>>   *
>>   * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK)
>>   * represent the offset in the returned BDS that is allocated for the
>> - * corresponding raw data; however, whether that offset actually contains
>> - * data also depends on BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO, as follows:
>> + * corresponding raw data.  Individual bytes are at the same sector-relative
>> + * locations (and thus, this bit cannot be set for mappings which are
>> + * not equivalent modulo 512).  However, whether that offset actually
>> + * contains data also depends on BDRV_BLOCK_DATA, as follows:
> 
> This suggests to me that it was a bad idea to embed the offset in the
> bitmask. In the long run, we should probably make flags and offset two
> separate things (e.g. making offset a new by-reference parameter).

As this (plus the next series) is all about changing the driver
interface, NOW is as good a time as any to split those into two separate
parameters.  In fact, returning an offset only makes sense when the
caller also is asking for what file is associated with the offset, and
patch 1 of this series already changed several callers to pass NULL when
they don't care about the file.

Unless anyone has a reason why I should NOT split the offset from the
bitmask, I can proceed with making that change for v6 (it will be a bit
more rebase churn as a result - but thinking about the long-term
interface is worth the short-term pain).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature