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

[Eric Blake]

On 04/19/2017 03:32 PM, John Snow wrote:

>> @@ -279,9 +280,9 @@ static int mig_save_device_bulk(QEMUFile *f, 
>> BlkMigDevState *bmds)
>>          /* Skip unallocated sectors; intentionally treats failure as
>>           * an allocated sector */
>>          while (cur_sector < total_sectors &&
>> -               !bdrv_is_allocated(blk_bs(bb), cur_sector,
>> -                                  MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
>> -            cur_sector += nr_sectors;
>> +               !bdrv_is_allocated(blk_bs(bb), cur_sector * BDRV_SECTOR_SIZE,
>> +                                  MAX_IS_ALLOCATED_SEARCH, &count)) {
>> +            cur_sector += DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
>>          }
> 
> Let's try asking again with the right vocabulary:
> 
> OK, so I guess the idea is that we definitely shouldn't ever have a
> partially allocated sector; so I suppose this ROUND_UP is just a precaution?

For now, yes - but the part 3 series (bdrv_get_block_status) makes
returning a sub-sector value possible.  For most drivers, it will only
happen at end-of-file (as status doesn't change mid-sector unless your
file is not a multiple of a sector size) - in fact, I found that I had
to patch the block layer to round up and sub-sector return that
coincides with end-of-file back to a round sector amount in order to
avoid breaking other code that assumed everything is allocated in sectors.

> 
> What would it mean if this actually DID round up? Would we miss
> transmitting a fraction of a sector because we assumed it was unallocated?

In general, the only mid-sector transition I encountered in testing was
at EOF, where the first half is allocated and the second half (beyond
EOF) was an implicit hole.  But here, you are worried about the opposite
case - can we ever have a hole that ends mid-sector, followed by actual
data in the second half.  Probably not, but I can add an assertion to
the generic block layer that mid-sector returns for an unallocated
answer can only happen at end-of-file.

> 
> In other places, you scale down with X / BDRV_SECTOR_SIZE or an
> equivalent bitshift, why does this receive a round *up* treatment?

Really? I thought I was being careful in this patch about ALWAYS
preserving the same sector value - the only time I did a bitshift was if
I already asserted the answer was aligned to sector boundaries to begin
with, and when I wasn't sure, it should be a round up.

> 
> Are we considering a future in which this function might actually give
> us some unaligned answers? Do we need to re-audit all of the callers at
> that point to make sure they cope appropriately?

Yes, I've been trying to do that all along, but a second set of eyes
never hurts.  Or, as I said, we can play it safe by guaranteeing that
even when byte-based, the block layer enforces sector-aligned answers.

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

signature.asc
Description: OpenPGP digital signature