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

[Kevin Wolf]

Am 06.07.2017 um 19:03 hat Eric Blake geschrieben:
> 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.

Oh, that might be it. It just looked weird because it was directly above
the code that reduces n, so I thought it was related to that. (It looks
even more like that in the original commit c8c3080f.)

Maybe add "so we have to look at the next backing file" or something to
make the context clearer?

Of course, the exact values still aren't correct. [sector_num,
psectors_inter] is the unallocated part in top, and the allocated area
in the intermediate image can start anywhere, including at sector_num,
and have any length <= nb_sectors - x.

> 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.

Callers will generally loop anyway, so it's probably useless overhead to
check the first non-matching block twice.

Kevin

pgphilcl32gdX.pgp
Description: PGP signature