Re: [Qemu-devel] [PATCH v3 04/20] stream: Switch stream_run() to byte-based

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

Am 05.07.2017 um 14:13 hat Eric Blake geschrieben:
> 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 <eblake@redhat.com>
> >> Reviewed-by: John Snow <jsnow@redhat.com>
> >>
> >> ---
> >> 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?)

Effectively probably yes, but it involves another write+flush for
rewriting the qcow2 header (even though nothing is expected to change in
it).

But if we wanted to avoid this, I think we could just directly check
bs->backing in stream_complete(). So I wonder why data->reached_end even
exists in the first place.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]