24.12.2018 11:21, Anton Nefedov wrote: > On 21/12/2018 7:16 PM, Vladimir Sementsov-Ogievskiy wrote: >> 18.12.2018 10:57, 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> >>> --- >>> qapi/block-core.json | 4 +- >>> block/qcow2.h | 6 +++ >>> block/qcow2-cluster.c | 2 +- >>> block/qcow2.c | 89 +++++++++++++++++++++++++++++++++++++- >>> block/trace-events | 1 + >>> tests/qemu-iotests/060 | 7 ++- >>> tests/qemu-iotests/060.out | 5 ++- >>> 7 files changed, 108 insertions(+), 6 deletions(-) >>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 762000f31f..204528b3f6 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -3009,6 +3009,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', >>> @@ -3027,7 +3029,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 a98d24500b..32d2c04bfa 100644 >>> --- a/block/qcow2.h >>> +++ b/block/qcow2.h >>> @@ -386,6 +386,12 @@ typedef struct QCowL2Meta >>> */ >>> Qcow2COWRegion cow_end; >>> >>> + /* >>> + * Indicates that COW regions are already handled and do not require >>> + * any more processing. >>> + */ >>> + bool skip_cow; >>> + >>> /** >> >> hmm, around it, all comments starts from '/**', so, I think, yours should >> too. >> (this note doesn't touch your other comments) >> > > that triggers a warning in checkpatch.pl > > WARNING: Block comments use a leading /* on a separate line
I thing, it is the case, when we should ignore this warning. > >>> * 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 e2737429f5..23e0702027 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 4897abae5e..161b935962 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -2021,6 +2021,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) { >>> @@ -2046,6 +2051,77 @@ 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); >>> +} >>> + >>> +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). >>> + */ >>> + return is_unallocated(bs, m->offset + m->cow_start.offset, >> >> here you add m->cow_start.offset ...[1] >> >>> + 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; >>> + } >>> + >>> + /* >>> + * Conventional place for qcow2_pre_write_overlap_check() but in >>> this >>> + * case it is already done for these clusters >>> + */ >> >> to be honest, check is done for original request not, not for cow_regions, >> and, >> on the other hand, >> do_perform_cow_write _does_ the check. >> >> So, I see two consistent ways: >> 1. you just add check here, like in do_perform_cow_write >> 2. improve check in qcow2_co_pwritev to cover the whole area, and drop >> checks from >> perform_cow. >> >> I prefer 1, as it more helpful in case of bugs in the code (just check >> exactly what >> you are going to write in the next write). >> > > ok > > diff --git a/block/qcow2.c b/block/qcow2.c > index 161b935962..05a7cbebbd 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2096,16 +2096,18 @@ 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 > */ > + > + ret = qcow2_pre_write_overlap_check(bs, 0, m->alloc_offset, > + m->nb_clusters * > s->cluster_size); > + if (ret < 0) { > + return ret; > + } > + > + BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_SPACE); ok > ret = bdrv_co_pwrite_zeroes(bs->file, m->alloc_offset, > m->nb_clusters * s->cluster_size, > BDRV_REQ_ALLOCATE); > >>> + >>> + 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, >> >> [1]... and here - not. >> And perform_cow uses start too. >> hmm.. >> >> On the other hand, here we just want to zero all allocated clusters, it is >> not related >> to cow regions. So, ok. >> >>> + 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; >>> +} >>> + >> >> [...] >> >> >> with pre-write-check added: >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> >> >> -- Best regards, Vladimir