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 <anton.nefe...@virtuozzo.com>
>>> Reviewed-by: Alberto Garcia <be...@igalia.com>
>>> ---
>>> 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
