Re: [PATCH v4 0/5] fix & merge block_status_above and is_allocated_above

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

28.05.2020 20:09, Stefan Hajnoczi wrote:
> On Thu, May 28, 2020 at 01:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Thanks to Eric, the whole series is reviewed now!
>>
>> v4:
>> 01: fix grammar in comment, add Eric's r-b
>> 02-05: add Eric's r-b
>>
>> Hi all!
>>
>> These series are here to address the following problem:
>> block-status-above functions may consider space after EOF of
>> intermediate backing files as unallocated, which is wrong, as these
>> backing files are the reason of producing zeroes, we never go further by
>> backing chain after a short backing file. So, if such short-backing file
>> is _inside_ requested sub-chain of the backing chain, we should never
>> report space after its EOF as unallocated.
>>
>> See patches 01,04,05 for details.
>>
>> Note, that this series leaves for another day the general problem
>> around block-status: misuse of BDRV_BLOCK_ALLOCATED as is-fs-allocated
>> vs go-to-backing.
>> Audit for this problem is done here:
>> "backing chain & block status & filters"
>> https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg04706.html
>> And I'm going to prepare series to address this problem.
>>
>> Also, get_block_status func have same disease, but remains unfixed here:
>> I want to make separate series for it, as it need some more refactoring,
>> which should be based on series
>> "[PATCH v5 0/7] coroutines: generate wrapper code"
>>
>> Vladimir Sementsov-Ogievskiy (5):
>>    block/io: fix bdrv_co_block_status_above
>>    block/io: bdrv_common_block_status_above: support include_base
>>    block/io: bdrv_common_block_status_above: support bs == base
>>    block/io: fix bdrv_is_allocated_above
>>    iotests: add commit top->base cases to 274
>>
>>   block/io.c                 | 105 +++++++++++++++++++------------------
>>   block/qcow2.c              |  16 +++++-
>>   tests/qemu-iotests/274     |  20 +++++++
>>   tests/qemu-iotests/274.out |  65 +++++++++++++++++++++++
>>   4 files changed, 152 insertions(+), 54 deletions(-)
> 
> Hi Vladimir,
> Which series is this based on? It does not apply. Is there a dependency
> on the coroutine wrappers series?
> 
> Aside from the issue applying the patches this series looks good to me.
> 

It's on current master. Yes it conflicts with coroutine wrappers.

Still, actually, "[PATCH v3] block: Factor out bdrv_run_co()" was substituted
by "[PATCH v5 0/7] coroutines: generate wrapper code", which is partly reviewed.
Sorry, I should have answered on "Factor out bdrv_run_co()" about that. Actually,
it was mentioned in "[PATCH v2] block: Factor out bdrv_run_co()", I just posted
v3 too early.

So, I'd prefer to merge these series now about block-status, instead of
"[PATCH v3] block: Factor out bdrv_run_co()", and then I'll rebase
"[PATCH v5 0/7] coroutines: generate wrapper code"


-- 
Best regards,
Vladimir