[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 04/20] stream: Switch stream_run() to byte-ba
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v3 04/20] stream: Switch stream_run() to byte-based |
Date: |
Wed, 5 Jul 2017 07:13:49 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 |
On 07/04/2017 10:00 AM, Kevin Wolf wrote:
> Am 27.06.2017 um 21:24 hat Eric Blake geschrieben:
>> We are gradually converting to byte-based interfaces, as they are
>> easier to reason about than sector-based. Change the internal
>> loop iteration of streaming to track by bytes instead of sectors
>> (although we are still guaranteed that we iterate by steps that
>> are sector-aligned).
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> Reviewed-by: John Snow <address@hidden>
>>
>> ---
>> v2: no change
>> ---
>> block/stream.c | 24 ++++++++++--------------
>> 1 file changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index 746d525..2f9618b 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -108,12 +108,11 @@ static void coroutine_fn stream_run(void *opaque)
>> BlockBackend *blk = s->common.blk;
>> BlockDriverState *bs = blk_bs(blk);
>> BlockDriverState *base = s->base;
>> - int64_t sector_num = 0;
>> - int64_t end = -1;
>
> Here, end was initialised to -1. This made a differnce for early 'goto
> out' paths because otherwise data->reached_end would incorrectly be true
> in stream_complete.
Oh, good call.
>
> Because we also check data->ret, I think the only case where it actually
> makes a difference is for the !bs->backing case: This used to result in
> data->reached_end == false, but now it ends up as true. This is because
> s->common.len hasn't been set yet, so it is still 0.
When !bs->backing, ret is 0; so that means my commit message is wrong
about there being no semantic change. But remember, when !bs->backing,
stream is a no-op (there was no backing file to stream into the current
layer, anyways) - so which do we want, declaring that the operation
never reached the end (odd, since we did nothing), or that it is
complete? In other words, is this something where I should fix the
semantics (perhaps as a separate bug-fix patch), or where I need to fix
this patch to preserve existing semantics?
The next code reached is stream_complete(). Before my patch, it skipped
the main body of the function (because !data->reached_end); with my
patch, we are now calling bdrv_change_backing_file() (presumably a no-op
when there is no backing?)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature