Re: [Qemu-devel] [Qemu-block] [PATCH 17/17] block: Make bdrv_is_allocated_above() byte-based

[Eric Blake]

On 04/24/2017 06:06 PM, John Snow wrote:
> 
> 
> On 04/11/2017 06:29 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 signature of the function to use int64_t *pnum 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.  Therefore, for the most part this patch is just the
>> addition of scaling at the callers followed by inverse scaling at
>> bdrv_is_allocated().  But some code, particularly stream_run(),
>> gets a lot simpler because it no longer has to mess with sectors.
>>

>> +++ b/block/io.c
>> @@ -1930,52 +1930,46 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState 
>> *bs, int64_t offset,
>>  /*
>>   * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP]
>>   *
>> - * Return true if the given sector is allocated in any image between
>> + * Return true if the given offset is allocated in any image between
> 
> perhaps "range" instead of "offset"?
> 
>>   * BASE and TOP (inclusive).  BASE can be NULL to check if the given
>> - * sector is allocated in any image of the chain.  Return false otherwise,
>> + * offset is allocated in any image of the chain.  Return false otherwise,
>>   * or negative errno on failure.

Seems reasonable.


>>          /*
>> -         * [sector_num, nb_sectors] is unallocated on top but intermediate
>> -         * might have
>> -         *
>> -         * [sector_num+x, nr_sectors] allocated.
>> +         * [offset, bytes] is unallocated on top but intermediate
>> +         * might have [offset+x, bytes-x] allocated.
>>           */
>> -        if (n > psectors_inter &&
>> +        if (n > pnum_inter &&
>>              (intermediate == top ||
>> -             sector_num + psectors_inter < intermediate->total_sectors)) {
> 
> 
> 
>> -            n = psectors_inter;
>> +             offset + pnum_inter < (intermediate->total_sectors *
>> +                                    BDRV_SECTOR_SIZE))) {
> 
> Naive question: not worth using either bdrv_getlength for bytes, or the
> bdrv_nb_sectors helpers?

bdrv_getlength(intermediate) should indeed be the same as
intermediate->total_sectors * BDRV_SECTOR_SIZE (for now - ultimately it
would be nice to track a byte length rather than a sector length for
images). I can make that cleanup for v2.

> 
> Reviewed-by: John Snow <address@hidden>
> 

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

signature.asc
Description: OpenPGP digital signature