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

[Kevin Wolf]

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 
> >> Reviewed-by: John Snow 
> >>
> >> ---
> >> 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


pgp9xonQbnqjw.pgp
Description: PGP signature

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

[Eric Blake]

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 
>> Reviewed-by: John Snow 
>>
>> ---
>> 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

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

[Kevin Wolf]

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 
> Reviewed-by: John Snow 
> 
> ---
> 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.

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.

Kevin

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

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:42PM -0500, Eric Blake wrote:
> 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 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> 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;
> +int64_t offset = 0;
>  uint64_t delay_ns = 0;
>  int error = 0;
>  int ret = 0;
> -int n = 0;
> +int n = 0; /* sectors */
>  void *buf;
> 
>  if (!bs->backing) {
> @@ -126,7 +125,6 @@ static void coroutine_fn stream_run(void *opaque)
>  goto out;
>  }
> 
> -end = s->common.len >> BDRV_SECTOR_BITS;
>  buf = qemu_blockalign(bs, STREAM_BUFFER_SIZE);
> 
>  /* Turn on copy-on-read for the whole block device so that guest read
> @@ -138,7 +136,7 @@ static void coroutine_fn stream_run(void *opaque)
>  bdrv_enable_copy_on_read(bs);
>  }
> 
> -for (sector_num = 0; sector_num < end; sector_num += n) {
> +for ( ; offset < s->common.len; offset += n * BDRV_SECTOR_SIZE) {
>  bool copy;
> 
>  /* Note that even when no rate limit is applied we need to yield
> @@ -151,28 +149,26 @@ static void coroutine_fn stream_run(void *opaque)
> 
>  copy = false;
> 
> -ret = bdrv_is_allocated(bs, sector_num,
> +ret = bdrv_is_allocated(bs, offset / BDRV_SECTOR_SIZE,
>  STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, );
>  if (ret == 1) {
>  /* Allocated in the top, no need to copy.  */
>  } else if (ret >= 0) {
>  /* Copy if allocated in the intermediate images.  Limit to the
> - * known-unallocated area [sector_num, sector_num+n).  */
> + * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE).  
> */
>  ret = bdrv_is_allocated_above(backing_bs(bs), base,
> -  sector_num, n, );
> +  offset / BDRV_SECTOR_SIZE, n, );
> 
>  /* Finish early if end of backing file has been reached */
>  if (ret == 0 && n == 0) {
> -n = end - sector_num;
> +n = (s->common.len - offset) / BDRV_SECTOR_SIZE;
>  }
> 
>  copy = (ret == 1);
>  }
> -trace_stream_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
> -   n * BDRV_SECTOR_SIZE, ret);
> +trace_stream_one_iteration(s, offset, n * BDRV_SECTOR_SIZE, ret);
>  if (copy) {
> -ret = stream_populate(blk, sector_num * BDRV_SECTOR_SIZE,
> -  n * BDRV_SECTOR_SIZE, buf);
> +ret = stream_populate(blk, offset, n * BDRV_SECTOR_SIZE, buf);
>  }
>  if (ret < 0) {
>  BlockErrorAction action =
> @@ -211,7 +207,7 @@ out:
>  /* Modify backing chain and close BDSes in main loop */
>  data = g_malloc(sizeof(*data));
>  data->ret = ret;
> -data->reached_end = sector_num == end;
> +data->reached_end = offset == s->common.len;
>  block_job_defer_to_main_loop(>common, stream_complete, data);
>  }
> 
> -- 
> 2.9.4
> 

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

[Eric Blake]

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 
Reviewed-by: John Snow 

---
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;
+int64_t offset = 0;
 uint64_t delay_ns = 0;
 int error = 0;
 int ret = 0;
-int n = 0;
+int n = 0; /* sectors */
 void *buf;

 if (!bs->backing) {
@@ -126,7 +125,6 @@ static void coroutine_fn stream_run(void *opaque)
 goto out;
 }

-end = s->common.len >> BDRV_SECTOR_BITS;
 buf = qemu_blockalign(bs, STREAM_BUFFER_SIZE);

 /* Turn on copy-on-read for the whole block device so that guest read
@@ -138,7 +136,7 @@ static void coroutine_fn stream_run(void *opaque)
 bdrv_enable_copy_on_read(bs);
 }

-for (sector_num = 0; sector_num < end; sector_num += n) {
+for ( ; offset < s->common.len; offset += n * BDRV_SECTOR_SIZE) {
 bool copy;

 /* Note that even when no rate limit is applied we need to yield
@@ -151,28 +149,26 @@ static void coroutine_fn stream_run(void *opaque)

 copy = false;

-ret = bdrv_is_allocated(bs, sector_num,
+ret = bdrv_is_allocated(bs, offset / BDRV_SECTOR_SIZE,
 STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, );
 if (ret == 1) {
 /* Allocated in the top, no need to copy.  */
 } else if (ret >= 0) {
 /* Copy if allocated in the intermediate images.  Limit to the
- * known-unallocated area [sector_num, sector_num+n).  */
+ * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE).  */
 ret = bdrv_is_allocated_above(backing_bs(bs), base,
-  sector_num, n, );
+  offset / BDRV_SECTOR_SIZE, n, );

 /* Finish early if end of backing file has been reached */
 if (ret == 0 && n == 0) {
-n = end - sector_num;
+n = (s->common.len - offset) / BDRV_SECTOR_SIZE;
 }

 copy = (ret == 1);
 }
-trace_stream_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
-   n * BDRV_SECTOR_SIZE, ret);
+trace_stream_one_iteration(s, offset, n * BDRV_SECTOR_SIZE, ret);
 if (copy) {
-ret = stream_populate(blk, sector_num * BDRV_SECTOR_SIZE,
-  n * BDRV_SECTOR_SIZE, buf);
+ret = stream_populate(blk, offset, n * BDRV_SECTOR_SIZE, buf);
 }
 if (ret < 0) {
 BlockErrorAction action =
@@ -211,7 +207,7 @@ out:
 /* Modify backing chain and close BDSes in main loop */
 data = g_malloc(sizeof(*data));
 data->ret = ret;
-data->reached_end = sector_num == end;
+data->reached_end = offset == s->common.len;
 block_job_defer_to_main_loop(>common, stream_complete, data);
 }

-- 
2.9.4