[Qemu-devel] [PATCH 0/2] block: Two copy offloading corrections

[Qemu-devel] [PATCH 0/2] block: Two copy offloading corrections

[Fam Zheng]

Fam Zheng (2):
  qcow2: Remove dead check on !ret
  block: Move request tracking to children in copy offloading

 block/io.c    | 59 ++++++++++++++++++++++++---------------------------
 block/qcow2.c |  2 +-
 2 files changed, 29 insertions(+), 32 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] qcow2: Remove dead check on !ret

[Fam Zheng]

In the beginning of the function, we initialize the local variable to 0,
and in the body of the function, we check the assigned values and exit
the loop immediately. So here it can never be non-zero.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index a3a3aa2a97..ff23063616 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1772,7 +1772,7 @@ static coroutine_fn int qcow2_handle_l2meta(BlockDriverState *bs,
     while (l2meta != NULL) {
         QCowL2Meta *next;
 
-        if (!ret && link_l2) {
+        if (link_l2) {
             ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
             if (ret) {
                 goto out;
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] block: Move request tracking to children in copy offloading

[Fam Zheng]

in_flight and tracked requests need to be tracked in every layer during
recursion. For now the only user is qemu-img convert where overlapping
requests and IOThreads don't exist, therefore this change doesn't make
much difference form user point of view, but it is incorrect as part of
the API. Fix it.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 59 ++++++++++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 31 deletions(-)

diff --git a/block/io.c b/block/io.c
index ef4fedd364..585008a6fb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2932,6 +2932,9 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
                                                     BdrvRequestFlags flags,
                                                     bool recurse_src)
 {
+    BdrvTrackedRequest src_req, dst_req;
+    BlockDriverState *src_bs = src->bs;
+    BlockDriverState *dst_bs = dst->bs;
     int ret;
 
     if (!src || !dst || !src->bs || !dst->bs) {
@@ -2955,17 +2958,31 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
         || src->bs->encrypted || dst->bs->encrypted) {
         return -ENOTSUP;
     }
+    bdrv_inc_in_flight(src_bs);
+    bdrv_inc_in_flight(dst_bs);
+    tracked_request_begin(&src_req, src_bs, src_offset,
+                          bytes, BDRV_TRACKED_READ);
+    tracked_request_begin(&dst_req, dst_bs, dst_offset,
+                          bytes, BDRV_TRACKED_WRITE);
+
+    wait_serialising_requests(&src_req);
+    wait_serialising_requests(&dst_req);
     if (recurse_src) {
-        return src->bs->drv->bdrv_co_copy_range_from(src->bs,
-                                                     src, src_offset,
-                                                     dst, dst_offset,
-                                                     bytes, flags);
+        ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,
+                                                    src, src_offset,
+                                                    dst, dst_offset,
+                                                    bytes, flags);
     } else {
-        return dst->bs->drv->bdrv_co_copy_range_to(dst->bs,
-                                                   src, src_offset,
-                                                   dst, dst_offset,
-                                                   bytes, flags);
+        ret = dst->bs->drv->bdrv_co_copy_range_to(dst->bs,
+                                                  src, src_offset,
+                                                  dst, dst_offset,
+                                                  bytes, flags);
     }
+    tracked_request_end(&src_req);
+    tracked_request_end(&dst_req);
+    bdrv_dec_in_flight(src_bs);
+    bdrv_dec_in_flight(dst_bs);
+    return ret;
 }
 
 /* Copy range from @src to @dst.
@@ -2996,27 +3013,7 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
                                     BdrvChild *dst, uint64_t dst_offset,
                                     uint64_t bytes, BdrvRequestFlags flags)
 {
-    BdrvTrackedRequest src_req, dst_req;
-    BlockDriverState *src_bs = src->bs;
-    BlockDriverState *dst_bs = dst->bs;
-    int ret;
-
-    bdrv_inc_in_flight(src_bs);
-    bdrv_inc_in_flight(dst_bs);
-    tracked_request_begin(&src_req, src_bs, src_offset,
-                          bytes, BDRV_TRACKED_READ);
-    tracked_request_begin(&dst_req, dst_bs, dst_offset,
-                          bytes, BDRV_TRACKED_WRITE);
-
-    wait_serialising_requests(&src_req);
-    wait_serialising_requests(&dst_req);
-    ret = bdrv_co_copy_range_from(src, src_offset,
-                                  dst, dst_offset,
-                                  bytes, flags);
-
-    tracked_request_end(&src_req);
-    tracked_request_end(&dst_req);
-    bdrv_dec_in_flight(src_bs);
-    bdrv_dec_in_flight(dst_bs);
-    return ret;
+    return bdrv_co_copy_range_from(src, src_offset,
+                                   dst, dst_offset,
+                                   bytes, flags);
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH 0/2] block: Two copy offloading corrections

[Stefan Hajnoczi]

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

On Wed, Jun 27, 2018 at 11:57:50AM +0800, Fam Zheng wrote:
> 
> 
> Fam Zheng (2):
>   qcow2: Remove dead check on !ret
>   block: Move request tracking to children in copy offloading
> 
>  block/io.c    | 59 ++++++++++++++++++++++++---------------------------
>  block/qcow2.c |  2 +-
>  2 files changed, 29 insertions(+), 32 deletions(-)
> 
> -- 
> 2.17.1
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/2] block: Two copy offloading corrections

[Kevin Wolf]

Am 27.06.2018 um 05:57 hat Fam Zheng geschrieben:
> Fam Zheng (2):
>   qcow2: Remove dead check on !ret
>   block: Move request tracking to children in copy offloading

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH 1/2] qcow2: Remove dead check on !ret

[Philippe Mathieu-Daudé]

On 06/27/2018 12:57 AM, Fam Zheng wrote:
> In the beginning of the function, we initialize the local variable to 0,
> and in the body of the function, we check the assigned values and exit
> the loop immediately. So here it can never be non-zero.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/qcow2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index a3a3aa2a97..ff23063616 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1772,7 +1772,7 @@ static coroutine_fn int qcow2_handle_l2meta(BlockDriverState *bs,
>      while (l2meta != NULL) {
>          QCowL2Meta *next;
>  
> -        if (!ret && link_l2) {
> +        if (link_l2) {
>              ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
>              if (ret) {
>                  goto out;
>