[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v4 20/21] block: Minimize raw use of bds->total_

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v4 20/21] block: Minimize raw use of bds->total_sectors
Date: Thu, 6 Jul 2017 12:03:42 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1

On 07/06/2017 11:48 AM, Kevin Wolf wrote:
> Am 05.07.2017 um 23:08 hat Eric Blake geschrieben:
>> bdrv_is_allocated_above() was relying on intermediate->total_sectors,
>> which is a field that can have stale contents depending on the value
>> of intermediate->has_variable_length.  An audit shows that we are safe
>> (we were first calling through bdrv_co_get_block_status() which in
>> turn calls bdrv_nb_sectors() and therefore just refreshed the current
>> length), but it's nicer to favor our accessor functions to avoid having
>> to repeat such an audit, even if it means refresh_total_sectors() is
>> called more frequently.
>> @@ -1969,13 +1970,14 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>>          /*
>>           * [sector_num, nb_sectors] is unallocated on top but intermediate
>> -         * might have
>> -         *
>> -         * [sector_num+x, nr_sectors] allocated.
>> +         * might have [sector_num+x, nb_sectors-x] allocated.
>>           */
> I tried to figure out what this comment wants to tell us, and neither
> the original version nor your changed one seemed to make a lot of sense
> to me.

The original one was definitely weird. I'm fine with bike-shedding on my
replacement (including deleting the comment altogether).

> The only case that I can see that actually needs the following
> block is a case like this:
>     (. = unallocated, # = allocated)
>     top             ....####
>     intermediate    ........
> Our initial request was for 8 sectors, but when going to the
> intermediate node, we need to reduce this to 4 sectors, otherwise we
> would return unallocated for sectors 5 to 8 even though they are
> allocated in top.
> That's kind of the opposite of what the comment says, though...

I think the comment was trying to worry about:

    top             ........
    intermediate    ....####

then our first query says we have 8 sectors unallocated, but we can't
return 8 unless we also check 8 sectors of the intermediate image
(because the intermediate image may have sectors 4-7 allocated).  So it
is explaining why the for loop is continuing down the backing chain.

If we ever get an answer of allocated, we can return immediately; we
only have to keep looking (on potentially smaller prefixes) as long as
we have an answer of unallocated and more backing chain to still check.

On the other hand, there's this situation:

top           ....####
intermediate  ####....

Right now (whether or not you apply my patch), the code only returns a
pnum of 4 sectors allocated (the first query on top sees 4 unallocated,
the second query on intermediate sees 4 allocated).  But in reality, we
COULD return a pnum of 8 (between the two images being queries, we can
account for an allocation of 8 sectors), although doing so requires
additional bookkeeping and calls into the drivers (get_status(top, 0, 8)
returns pnum of 4, get_status(intermediate, 0, 4) returns pnum of 4,
then get_status(top, 4, 4) returns pnum of 4, so that the overall
is_allocated(top, intermediate, 0, 8) sets pnum of 8).  Maybe it's worth
doing, but not in the same series that converts to byte-based.

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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]