On 13/12/2018 3:02 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().
>>
>
> [...]
>
>> --- 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);
>
> hm, nr may be < bytes if it is up to file length. And we lose this case,
> when, it
> may be considered as unallocated too.
>
> Doesn't harm, however.
>
Thanks guys for your review.
This case I think is too rare to care about.
>> +}
>> +
>> +static bool is_zero_cow(BlockDriverState *bs, QCowL2Meta *m)
>> +{
>> + /* This check is designed for optimization shortcut so it must be
>> + * efficient.
>> + * Instead of is_zero(), use is_unallocated() as it is faster (but not
>> + * as accurate and can result in false negatives). */
>
> But, in case of allocated zeros, we'll read them anyway (as part of COW
> process),
> so, it may be handled in the same way too. May be not here, but after read we
> can
> check for zeroes, and again effectively write zeros to the whole cluster.
>
> Again it may be done separately, I don't sure it worth doing.
>
Detecting zeroes after we read them (and not here) might make sense;
performance gain should be about the same (minus some CPU to check the
read buffer for zeroes).
The question is how frequent it might hit:
- raw backing image with holes
- qcow2 backing image with sub-cluster zero areas
(e.g. smaller cluster size)
- backing image contains lots of space with explicit zeroes
(e.g. guest FS with 'shred' extensively used to delete files)
None of these probably occur that frequent.
But might be a next step and a separate series.
>> + return is_unallocated(bs, m->offset + m->cow_start.offset,
>> + m->cow_start.nb_bytes) &&
>> + is_unallocated(bs, m->offset + m->cow_end.offset,
>> + m->cow_end.nb_bytes);
>> +}
>> +
>> +static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta)
>> +{
>> + BDRVQcow2State *s = bs->opaque;
>> + QCowL2Meta *m;
>> +
>> + if (!(bs->file->bs->supported_zero_flags & BDRV_REQ_ALLOCATE)) {
>> + return 0;
>> + }
>> +
>> + if (bs->encrypted) {
>> + return 0;
>> + }
>> +
>> + for (m = l2meta; m != NULL; m = m->next) {
>> + int ret;
>> +
>> + if (!m->cow_start.nb_bytes && !m->cow_end.nb_bytes) {
>> + continue;
>> + }
>> +
>> + if (!is_zero_cow(bs, m)) {
>> + continue;
>> + }
>
> pre_write_overlap_check should be here
>
Existing pre_write_overlap_check in qcow2_co_pwritev() should cover it,
since the check aligns the range to cluster boundaries.
However I think I missed a comment about it here. For suspicious
readers :) and just in case someone starts moving this code around.
I propose:
diff --git a/block/qcow2.c b/block/qcow2.c
index 027188a1a3..b3b3124083 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2088,6 +2088,9 @@ static int handle_alloc_space(BlockDriverState
*bs, QCowL2Meta *l2meta)
continue;
}
+ /* Conventional place for qcow2_pre_write_overlap_check() but
in this
+ case it is already done for these clusters */
+
BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_SPACE);
/* instead of writing zero COW buffers,
efficiently zero out the whole clusters */
>> +
>> + BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_SPACE);
>> + /* instead of writing zero COW buffers,
>> + efficiently zero out the whole clusters */
>> + ret = bdrv_co_pwrite_zeroes(bs->file, m->alloc_offset,
>> + m->nb_clusters * s->cluster_size,
>> + BDRV_REQ_ALLOCATE);
>> + if (ret < 0) {
>> + if (ret != -ENOTSUP && ret != -EAGAIN) {
>> + return ret;
>> + }
>> + continue;
>> + }
>> +
>> + trace_qcow2_skip_cow(qemu_coroutine_self(), m->offset,
>> m->nb_clusters);
>> + m->skip_cow = true;
>> + }
>> + return 0;
>> +}
>> +
>
>
