Re: [Qemu-block] [PATCH v10 8/9] qcow2: skip writing zero buffers to empty COW areas

[Vladimir Sementsov-Ogievskiy]

05.12.2018 19:59, Anton Nefedov wrote:
> On 5/12/2018 5:01 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 03.12.2018 13:14, Anton Nefedov wrote:
>>> If COW areas of the newly allocated clusters are zeroes on the backing 
>>> image,
>>> efficient bdrv_write_zeroes(flags=BDRV_REQ_ALLOCATE) can be used on the 
>>> whole
>>> cluster instead of writing explicit zero buffers later in perform_cow().
>>>
>>> iotest 060:
>>> write to the discarded cluster does not trigger COW anymore.
>>> Use a backing image instead.
>>>
>>> Signed-off-by: Anton Nefedov <address@hidden>
>>> Reviewed-by: Alberto Garcia <address@hidden>
>>> ---
>>>     qapi/block-core.json       |  4 +-
>>>     block/qcow2.h              |  6 +++
>>>     block/qcow2-cluster.c      |  2 +-
>>>     block/qcow2.c              | 80 +++++++++++++++++++++++++++++++++++++-
>>>     block/trace-events         |  1 +
>>>     tests/qemu-iotests/060     | 26 ++++++++-----
>>>     tests/qemu-iotests/060.out |  5 ++-
>>>     7 files changed, 109 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index d4fe710836..50598aa8fe 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -3004,6 +3004,8 @@
>>>     #
>>>     # @cor_write: a write due to copy-on-read (since 2.11)
>>>     #
>>> +# @cluster_alloc_space: an allocation of file space for a cluster (since 
>>> 4.0)
>>> +#
>>>     # Since: 2.9
>>>     ##
>>>     { 'enum': 'BlkdebugEvent', 'prefix': 'BLKDBG',
>>> @@ -3022,7 +3024,7 @@
>>>                 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
>>>                 'pwritev_zero', 'pwritev_done', 'empty_image_prepare',
>>>                 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
>>> -            'cor_write'] }
>>> +            'cor_write', 'cluster_alloc_space'] }
>>>     
>>>     ##
>>>     # @BlkdebugInjectErrorOptions:
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index 8662b68575..8a64077897 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -389,6 +389,12 @@ typedef struct QCowL2Meta
>>>          */
>>>         Qcow2COWRegion cow_end;
>>>     
>>> +    /**
>>> +     * Indicates that COW regions are already handled and do not require
>>> +     * any more processing.
>>> +     */
>>> +    bool skip_cow;
>>> +
>>>         /**
>>>          * The I/O vector with the data from the actual guest write request.
>>>          * If non-NULL, this is meant to be merged together with the data
>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>> index d37fe08b3d..3685c5f67e 100644
>>> --- a/block/qcow2-cluster.c
>>> +++ b/block/qcow2-cluster.c
>>> @@ -806,7 +806,7 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
>>> *m)
>>>         assert(start->offset + start->nb_bytes <= end->offset);
>>>         assert(!m->data_qiov || m->data_qiov->size == data_bytes);
>>>     
>>> -    if (start->nb_bytes == 0 && end->nb_bytes == 0) {
>>> +    if ((start->nb_bytes == 0 && end->nb_bytes == 0) || m->skip_cow) {
>>>             return 0;
>>>         }
>>>     
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 991d6ac91b..027188a1a3 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -2015,6 +2015,11 @@ static bool merge_cow(uint64_t offset, unsigned 
>>> bytes,
>>>                 continue;
>>>             }
>>>     
>>> +        /* If COW regions are handled already, skip this too */
>>> +        if (m->skip_cow) {
>>> +            continue;
>>> +        }
>>> +
>>>             /* The data (middle) region must be immediately after the
>>>              * start region */
>>>             if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
>>> @@ -2040,6 +2045,68 @@ static bool merge_cow(uint64_t offset, unsigned 
>>> bytes,
>>>         return false;
>>>     }
>>>     
>>> +static bool is_unallocated(BlockDriverState *bs, int64_t offset, int64_t 
>>> bytes)
>>> +{
>>> +    int64_t nr;
>>> +    return !bytes ||
>>> +        (!bdrv_is_allocated_above(bs, NULL, offset, bytes, &nr) && nr == 
>>> bytes);
>>
>> bdrv_is_allocated_above may return error < 0
>>
> 
> Probably I just took is_zero() as an example.
> But somewhere there's even a rationale (bdrv_co_do_copy_on_readv):
> 
>           ret = bdrv_is_allocated(bs, cluster_offset,
>                                   MIN(cluster_bytes, max_transfer), &pnum);
>           if (ret < 0) {
>               /* Safe to treat errors in querying allocation as if
>                * unallocated; we'll probably fail again soon on the
>                * read, but at least that will set a decent errno.
>                */
>               pnum = MIN(cluster_bytes, max_transfer);
>           }

aha, anyway, !bdrv_is_allocated_above is true when and only when the function
successfully returned "unallocated".

ahaha, this rationale has funny mistake: s/unallocated/allocated )


-- 
Best regards,
Vladimir