Re: [PATCH for-4.2 1/4] Revert "qcow2: skip writing zero buffers to empty COW areas"
On 01.11.19 16:42, Kevin Wolf wrote: > Am 01.11.2019 um 15:01 hat Max Reitz geschrieben: >> On 01.11.19 13:40, Eric Blake wrote: >>> On 11/1/19 11:00 AM, Max Reitz wrote: This reverts commit c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a. This commit causes fundamental performance problems on XFS (because fallocate() stalls the AIO pipeline), and as such it is not clear that we should unconditionally enable this behavior. We expect subclusters to alleviate the performance penalty of small writes to newly allocated clusters, so when we get them, the originally intended performance gain may actually no longer be significant. If we want to reintroduce something similar to c8bb23cbdbe, it will require extensive benchmarking on various systems with subclusters enabled. Cc: qemu-sta...@nongnu.org Signed-off-by: Max Reitz --- >>> +++ b/qapi/block-core.json @@ -3304,8 +3304,6 @@ # # @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.1) -# # @none: triggers once at creation of the blkdebug node (since 4.1) >>> >>> Deleting released qapi is not backwards-compatible. >> >> Right. :-/ >> >> I’ll just not make changes to the QAPI schema. It doesn’t hurt to leave >> this in. > > Why would it be incompatible to drop an enum value that is only ever > used in output and that QEMU doesn't generate? This isn’t output at all, it’s input for blockdev-add for blkdebug nodes. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH for-4.2 1/4] Revert "qcow2: skip writing zero buffers to empty COW areas"
Am 01.11.2019 um 15:01 hat Max Reitz geschrieben: > On 01.11.19 13:40, Eric Blake wrote: > > On 11/1/19 11:00 AM, Max Reitz wrote: > >> This reverts commit c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a. > >> > >> This commit causes fundamental performance problems on XFS (because > >> fallocate() stalls the AIO pipeline), and as such it is not clear that > >> we should unconditionally enable this behavior. > >> > >> We expect subclusters to alleviate the performance penalty of small > >> writes to newly allocated clusters, so when we get them, the originally > >> intended performance gain may actually no longer be significant. > >> > >> If we want to reintroduce something similar to c8bb23cbdbe, it will > >> require extensive benchmarking on various systems with subclusters > >> enabled. > >> > >> Cc: qemu-sta...@nongnu.org > >> Signed-off-by: Max Reitz > >> --- > > > >> +++ b/qapi/block-core.json > >> @@ -3304,8 +3304,6 @@ > >> # > >> # @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.1) > >> -# > >> # @none: triggers once at creation of the blkdebug node (since 4.1) > > > > Deleting released qapi is not backwards-compatible. > > Right. :-/ > > I’ll just not make changes to the QAPI schema. It doesn’t hurt to leave > this in. Why would it be incompatible to drop an enum value that is only ever used in output and that QEMU doesn't generate? Kevin signature.asc Description: PGP signature
Re: [PATCH for-4.2 1/4] Revert "qcow2: skip writing zero buffers to empty COW areas"
On 01.11.19 13:40, Eric Blake wrote: > On 11/1/19 11:00 AM, Max Reitz wrote: >> This reverts commit c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a. >> >> This commit causes fundamental performance problems on XFS (because >> fallocate() stalls the AIO pipeline), and as such it is not clear that >> we should unconditionally enable this behavior. >> >> We expect subclusters to alleviate the performance penalty of small >> writes to newly allocated clusters, so when we get them, the originally >> intended performance gain may actually no longer be significant. >> >> If we want to reintroduce something similar to c8bb23cbdbe, it will >> require extensive benchmarking on various systems with subclusters >> enabled. >> >> Cc: qemu-sta...@nongnu.org >> Signed-off-by: Max Reitz >> --- > >> +++ b/qapi/block-core.json >> @@ -3304,8 +3304,6 @@ >> # >> # @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.1) >> -# >> # @none: triggers once at creation of the blkdebug node (since 4.1) > > Deleting released qapi is not backwards-compatible. Right. :-/ I’ll just not make changes to the QAPI schema. It doesn’t hurt to leave this in. Max > However, given that > the only known user of this interface is debug testing via iotests, I'm > not too concerned that we would be impacting any external users. signature.asc Description: OpenPGP digital signature
Re: [PATCH for-4.2 1/4] Revert "qcow2: skip writing zero buffers to empty COW areas"
On 11/1/19 11:00 AM, Max Reitz wrote: This reverts commit c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a. This commit causes fundamental performance problems on XFS (because fallocate() stalls the AIO pipeline), and as such it is not clear that we should unconditionally enable this behavior. We expect subclusters to alleviate the performance penalty of small writes to newly allocated clusters, so when we get them, the originally intended performance gain may actually no longer be significant. If we want to reintroduce something similar to c8bb23cbdbe, it will require extensive benchmarking on various systems with subclusters enabled. Cc: qemu-sta...@nongnu.org Signed-off-by: Max Reitz --- +++ b/qapi/block-core.json @@ -3304,8 +3304,6 @@ # # @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.1) -# # @none: triggers once at creation of the blkdebug node (since 4.1) Deleting released qapi is not backwards-compatible. However, given that the only known user of this interface is debug testing via iotests, I'm not too concerned that we would be impacting any external users. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH for-4.2 1/4] Revert "qcow2: skip writing zero buffers to empty COW areas"
01.11.2019 13:00, Max Reitz wrote:
> This reverts commit c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a.
>
> This commit causes fundamental performance problems on XFS (because
> fallocate() stalls the AIO pipeline), and as such it is not clear that
> we should unconditionally enable this behavior.
>
> We expect subclusters to alleviate the performance penalty of small
> writes to newly allocated clusters, so when we get them, the originally
> intended performance gain may actually no longer be significant.
>
> If we want to reintroduce something similar to c8bb23cbdbe, it will
> require extensive benchmarking on various systems with subclusters
> enabled.
>
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Max Reitz
It's sad, but OK:
Reviewed-by: Vladimir Sementsov-Ogievskiy
> ---
> qapi/block-core.json | 4 +-
> block/qcow2.h | 6 ---
> block/qcow2-cluster.c | 2 +-
> block/qcow2.c | 86 --
> block/trace-events | 1 -
> tests/qemu-iotests/060 | 7 +---
> tests/qemu-iotests/060.out | 5 +--
> 7 files changed, 4 insertions(+), 107 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index aa97ee2641..f053f15431 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3304,8 +3304,6 @@
> #
> # @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.1)
> -#
> # @none: triggers once at creation of the blkdebug node (since 4.1)
> #
> # Since: 2.9
> @@ -3326,7 +3324,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', 'cluster_alloc_space', 'none'] }
> +'cor_write', 'none'] }
>
> ##
> # @BlkdebugIOType:
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 601c2e4c82..8166f6e311 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -418,12 +418,6 @@ 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 8982b7b762..fbfea8c817 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -809,7 +809,7 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta
> *m)
> assert(start->nb_bytes + end->nb_bytes <= UINT_MAX - data_bytes);
> assert(start->offset + start->nb_bytes <= end->offset);
>
> -if ((start->nb_bytes == 0 && end->nb_bytes == 0) || m->skip_cow) {
> +if (start->nb_bytes == 0 && end->nb_bytes == 0) {
> return 0;
> }
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7c18721741..17555cb0a1 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2274,11 +2274,6 @@ 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) {
> @@ -2305,81 +2300,6 @@ 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, false, offset, bytes, ) &&
> - 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,
> - 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 (!(s->data_file->bs->supported_zero_flags & BDRV_REQ_NO_FALLBACK)) {
> -return 0;
> -}
> -
> -if (bs->encrypted) {
> -return 0;
> -}
> -
> -for (m = l2meta; m != NULL; m = m->next) {
> -int ret;
> -
> -if (!m->cow_start.nb_bytes &&
