Re: [PATCH v6 2/5] block/io: bdrv_common_block_status_above: support include_base

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

23.09.2020 19:18, Alberto Garcia wrote:
> On Wed 16 Sep 2020 02:20:05 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>> -    for (p = backing_bs(bs); p != base; p = backing_bs(p)) {
>> +    for (p = backing_bs(bs); include_base || p != base; p = backing_bs(p)) {
>>           ret = bdrv_co_block_status(p, want_zero, offset, bytes, pnum, map,
>>                                      file);
>>           if (ret < 0) {
>> @@ -2420,6 +2422,11 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
>>               break;
>>           }
>>   
>> +        if (p == base) {
>> +            assert(include_base);
>> +            break;
>> +        }
>> +
> 
> Another option is something like:
> 
>     BlockDriverState *last_bs = include_base ? base : backing_bs(base);

hmm, in case when include_base is false, last bs is not backing_bs(base) but the parent of base.

> 
> and you get a simpler 'for' loop.
> 
> But why do we need include_base at all? Can't the caller just pass
> backing_bs(base) instead? I'm talking also about the existing case of
> bdrv_is_allocated_above().
> 


include_base was introduced for the case when caller doesn't own backing_bs(base), and therefore shouldn't do operations that may yield (block_status can) dependent on backing_bs(base). In particular, in block stream, where link to base is not frozen.


-- 
Best regards,
Vladimir