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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

09.07.2018 04:15, Fam Zheng wrote:
> 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;
>   }
>   

A matter of taste, I think. I decided, that such way only stresses that 
these functions have more different than similar content and went 
another one.

-- 
Best regards,
Vladimir