Re: [Qemu-devel] [PATCH v4 1/4] block/io: fix copy_range

[Fam Zheng <famz@redhat.com>]

On Fri, 07/06 21:30, Vladimir Sementsov-Ogievskiy wrote:
> Here two things are fixed:
> 
> 1. Architecture
> 
> On each recursion step, we go to the child of src or dst, only for one
> of them. So, it's wrong to create tracked requests for both on each
> step. It leads to tracked requests duplication.
> 
> 2. Wait for serializing requests on write path independently of
>    BDRV_REQ_NO_SERIALISING
> 
> Before commit 9ded4a01149 "backup: Use copy offloading",
> BDRV_REQ_NO_SERIALISING was used for only one case: read in
> copy-on-write operation during backup. Also, the flag was handled only
> on read path (in bdrv_co_preadv and bdrv_aligned_preadv).
> 
> After 9ded4a01149, flag is used for not waiting serializing operations
> on backup target (in same case of copy-on-write operation). This
> behavior change is unsubstantiated and potentially dangerous, let's
> drop it and add additional asserts and documentation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block.h |  13 +++++++
>  block/io.c            | 103 +++++++++++++++++++++++++++++++-------------------
>  2 files changed, 78 insertions(+), 38 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index e5c7759a0c..a06a4d27de 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -50,6 +50,19 @@ typedef enum {
>       * opened with BDRV_O_UNMAP.
>       */
>      BDRV_REQ_MAY_UNMAP          = 0x4,
> +
> +    /* The BDRV_REQ_NO_SERIALISING means that we don't want to
> +     * wait_serialising_requests(), when reading.
> +     *
> +     * This flag is used for backup copy on write operation, when we need to
> +     * read old data before write (write notifier triggered). It is ok, due to
> +     * we already waited for serializing requests in initiative write (see
> +     * bdrv_aligned_pwritev), and it is necessary for the case when initiative
> +     * write is serializing itself (we'll dead lock waiting it).
> +     *
> +     * The described case is the only usage for the flag for now, so, it is
> +     * supported only for read operation and restricted for write.
> +     */
>      BDRV_REQ_NO_SERIALISING     = 0x8,
>      BDRV_REQ_FUA                = 0x10,
>      BDRV_REQ_WRITE_COMPRESSED   = 0x20,
> diff --git a/block/io.c b/block/io.c
> index 1a2272fad3..621b21c455 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1572,6 +1572,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
>      max_transfer = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_transfer, INT_MAX),
>                                     align);
>  
> +    /* BDRV_REQ_NO_SERIALISING is only for read operation */
> +    assert(!(flags & BDRV_REQ_NO_SERIALISING));
>      waited = wait_serialising_requests(req);
>      assert(!waited || !req->serialising);
>      assert(req->overlap_offset <= offset);
> @@ -2888,15 +2890,19 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host)
>      }
>  }
>  
> -static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
> -                                                    uint64_t src_offset,
> -                                                    BdrvChild *dst,
> -                                                    uint64_t dst_offset,
> -                                                    uint64_t bytes,
> -                                                    BdrvRequestFlags flags,
> -                                                    bool recurse_src)
> +/* Common part of bdrv_co_copy_range_from and bdrv_co_copy_range_to.
> + *
> + * Return -errno on failure,
> + *        0 if successfully handled by bdrv_co_pwrite_zeroes
> + *        1 to continue copy_range operation
> + */
> +static int coroutine_fn bdrv_co_copy_range_check(BdrvChild *src,
> +                                                 uint64_t src_offset,
> +                                                 BdrvChild *dst,
> +                                                 uint64_t dst_offset,
> +                                                 uint64_t bytes,
> +                                                 BdrvRequestFlags flags)
>  {
> -    BdrvTrackedRequest src_req, dst_req;
>      int ret;
>  
>      if (!dst || !dst->bs) {
> @@ -2923,33 +2929,8 @@ 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);
>  
> -    if (!(flags & BDRV_REQ_NO_SERIALISING)) {
> -        wait_serialising_requests(&src_req);
> -        wait_serialising_requests(&dst_req);
> -    }
> -    if (recurse_src) {
> -        ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,
> -                                                    src, src_offset,
> -                                                    dst, dst_offset,
> -                                                    bytes, flags);
> -    } else {
> -        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;
> +    return 1;
>  }
>  
>  /* Copy range from @src to @dst.
> @@ -2960,8 +2941,31 @@ int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, uint64_t src_offset,
>                                           BdrvChild *dst, uint64_t dst_offset,
>                                           uint64_t bytes, BdrvRequestFlags flags)
>  {
> -    return bdrv_co_copy_range_internal(src, src_offset, dst, dst_offset,
> -                                       bytes, flags, true);
> +    BdrvTrackedRequest req;
> +    int ret;
> +
> +    ret = bdrv_co_copy_range_check(src, src_offset, dst, dst_offset, bytes,
> +                                   flags);

I don't like a function called _check to already do I/O here. Instead, I think
this is cleaner:

---


diff --git a/block/io.c b/block/io.c
index 1a2272fad3..694a94dfae 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2923,32 +2923,34 @@ 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);
 
-    if (!(flags & BDRV_REQ_NO_SERIALISING)) {
-        wait_serialising_requests(&src_req);
-        wait_serialising_requests(&dst_req);
-    }
     if (recurse_src) {
+        bdrv_inc_in_flight(src->bs);
+        tracked_request_begin(&src_req, src->bs, src_offset,
+                              bytes, BDRV_TRACKED_READ);
+        if (!(flags & BDRV_REQ_NO_SERIALISING)) {
+            wait_serialising_requests(&src_req);
+        }
         ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,
                                                     src, src_offset,
                                                     dst, dst_offset,
                                                     bytes, flags);
+        tracked_request_end(&src_req);
+        bdrv_dec_in_flight(src->bs);
     } else {
+        bdrv_inc_in_flight(dst->bs);
+        tracked_request_begin(&dst_req, dst->bs, dst_offset,
+                              bytes, BDRV_TRACKED_WRITE);
+        /* BDRV_REQ_NO_SERIALISING is only for read operation, so we ignore it
+         * in flags. */
+        wait_serialising_requests(&dst_req);
         ret = dst->bs->drv->bdrv_co_copy_range_to(dst->bs,
                                                   src, src_offset,
                                                   dst, dst_offset,
                                                   bytes, flags);
+        tracked_request_end(&dst_req);
+        bdrv_dec_in_flight(dst->bs);
     }
-    tracked_request_end(&src_req);
-    tracked_request_end(&dst_req);
-    bdrv_dec_in_flight(src->bs);
-    bdrv_dec_in_flight(dst->bs);
     return ret;
 }