[PATCH hci-8.0 9/9] block/block-copy: coalesce write-zeroes tasks #VSTOR-134085

["Denis V. Lunev" <den@openvz.org>]

From: Denis V. Lunev <den@openvz.org>

Task size was capped to block_copy_chunk_size() regardless of method,
which sizes a task by the copy buffer. A write-zeroes task carries no
buffer, so that cap only splits one long known-zero run into a crowd of
small COPY_WRITE_ZEROES tasks, each with its own request against the
target.

block_copy_task_create() already decides from zero_bitmap how far a task
may run, so add block_copy_widen_zero_area() to that decision: for a
write-zeroes task, re-search the dirty area under BDRV_REQUEST_MAX_BYTES
instead of the buffer chunk size, and let the existing clamp cut it back
to where the zero run ends. That bound is INT_MAX rounded down to a
sector, so nothing has aligned it to cluster_size and it is aligned down
at the use site. Widening is safe: an overlapping caller waits on the
task's BlockReq via reqlist_wait_one() rather than observing it
mid-flight.

A request that large is only cheap where the target zeroes by metadata.
BDRV_REQ_NO_FALLBACK in supported_zero_flags rules out the targets which
cannot, qcow2 v2 and iscsi among them, but it is optimistic for the rest:
file-posix advertises it at open and only learns from a failing
fallocate. So a write-zeroes request asks for it while the target is
believed to oblige, which costs nothing when it does. A refusal ends the
widening, and the range is written in buffer-sized chunks, as is any
task widened before the refusal came. -ENOTSUP says the fast path is
unavailable, not that the target wrote nothing, so the range is
rewritten whole: zeroes over zeroes change nothing.

hci-8.0: 6b1a4754713 made the 056 copy-before-write source read as
zeroes. One widened write-zeroes task now covers all of it, and the
rate limit only charges a task after it ran, so speed=1 no longer
keeps the job alive until the test cancels it. Cap the task with
x-perf max-chunk there.

Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 block/block-copy.c     | 94 ++++++++++++++++++++++++++++++++++++++----
 tests/qemu-iotests/056 |  3 +-
 2 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index a35fba45183..082257da7be 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -161,6 +161,8 @@ typedef struct BlockCopyState {
     /* State fields that use a thread-safe API */
     BdrvDirtyBitmap *copy_bitmap;
     bool top_mode;
+    /* Whether a write-zeroes task may widen; see block_copy_write_zeroes(). */
+    bool zero_widen; /* atomic */
     /* Clusters reading as zero; allocated on demand, frozen once valid. */
     HBitmap *zero_bitmap;
     /* Published only once the scan is done. */
@@ -189,6 +191,34 @@ static int64_t block_copy_chunk_size(BlockCopyState *s)
     }
 }
 
+/*
+ * A write-zeroes task carries no buffer, so it may cover far more than
+ * block_copy_chunk_size(). Return how far it may run; the caller clamps it
+ * to where the zero run ends.
+ */
+static int64_t block_copy_widen_zero_area(BlockCopyState *s,
+                                          BlockCopyCallState *call_state,
+                                          int64_t offset, int64_t search_end,
+                                          int64_t bytes)
+{
+    int64_t aligned = QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
+                                      s->cluster_size);
+    int64_t zero_chunk = MIN_NON_ZERO(MAX(aligned, s->cluster_size),
+                                      call_state->max_chunk);
+    int64_t wide_offset, wide_bytes;
+
+    if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap, offset, search_end,
+                                           zero_chunk, &wide_offset,
+                                           &wide_bytes)) {
+        return bytes;
+    }
+
+    /* @offset is dirty, so the search cannot have moved past it. */
+    assert(wide_offset == offset);
+
+    return wide_bytes;
+}
+
 /*
  * Search for the first dirty area in offset/bytes range and create task at
  * the beginning of it.
@@ -200,6 +230,7 @@ block_copy_task_create(BlockCopyState *s, BlockCopyCallState *call_state,
     BlockCopyTask *task;
     BlockCopyMethod method;
     int64_t max_chunk;
+    int64_t search_end = offset + bytes;
 
     QEMU_LOCK_GUARD(&s->lock);
     max_chunk = MIN_NON_ZERO(block_copy_chunk_size(s), call_state->max_chunk);
@@ -221,6 +252,10 @@ block_copy_task_create(BlockCopyState *s, BlockCopyCallState *call_state,
 
         if (hbitmap_get(s->zero_bitmap, offset)) {
             method = COPY_WRITE_ZEROES;
+            if (qatomic_read(&s->zero_widen)) {
+                bytes = block_copy_widen_zero_area(s, call_state, offset,
+                                                   search_end, bytes);
+            }
             boundary = hbitmap_next_zero(s->zero_bitmap, offset, bytes);
         } else {
             boundary = hbitmap_next_dirty(s->zero_bitmap, offset, bytes);
@@ -481,6 +516,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
         .max_transfer = QEMU_ALIGN_DOWN(
                                     block_copy_max_transfer(source, target),
                                     cluster_size),
+        .zero_widen = target->bs->supported_zero_flags & BDRV_REQ_NO_FALLBACK,
     };
 
     s->discard_source = discard_source;
@@ -537,6 +573,56 @@ static coroutine_fn int block_copy_task_run(AioTaskPool *pool,
     return 0;
 }
 
+/*
+ * Widening a write-zeroes request only pays off where the target zeroes by
+ * metadata, so while the target is believed to oblige, a request asks to fail
+ * instead of falling back to writing the zeroes out. That costs nothing when
+ * it does oblige, and a refusal ends the widening for the rest of the run.
+ */
+static int coroutine_fn GRAPH_RDLOCK
+block_copy_write_zeroes(BlockCopyState *s, int64_t offset, int64_t bytes,
+                        bool *error_is_read)
+{
+    BdrvRequestFlags flags = s->write_flags & ~BDRV_REQ_WRITE_COMPRESSED;
+    int64_t chunk;
+    int ret = 0;
+
+    if (qatomic_read(&s->zero_widen)) {
+        ret = bdrv_co_pwrite_zeroes(s->target, offset, bytes,
+                                    flags | BDRV_REQ_NO_FALLBACK);
+        if (ret != -ENOTSUP) {
+            goto out;
+        }
+
+        /* Redoing the range is safe: zeroes over zeroes change nothing. */
+        qatomic_set(&s->zero_widen, false);
+    }
+
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+        chunk = block_copy_chunk_size(s);
+    }
+
+    while (bytes) {
+        int64_t n = MIN(bytes, chunk);
+
+        ret = bdrv_co_pwrite_zeroes(s->target, offset, n, flags);
+        if (ret < 0) {
+            break;
+        }
+
+        offset += n;
+        bytes -= n;
+    }
+
+out:
+    if (ret < 0) {
+        trace_block_copy_write_zeroes_fail(s, offset, ret);
+        *error_is_read = false;
+    }
+
+    return ret;
+}
+
 /*
  * block_copy_do_copy
  *
@@ -568,13 +654,7 @@ block_copy_do_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
 
     switch (*method) {
     case COPY_WRITE_ZEROES:
-        ret = bdrv_co_pwrite_zeroes(s->target, offset, nbytes, s->write_flags &
-                                    ~BDRV_REQ_WRITE_COMPRESSED);
-        if (ret < 0) {
-            trace_block_copy_write_zeroes_fail(s, offset, ret);
-            *error_is_read = false;
-        }
-        return ret;
+        return block_copy_write_zeroes(s, offset, nbytes, error_is_read);
 
     case COPY_RANGE_SMALL:
     case COPY_RANGE_FULL:
diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index e006d1296d9..72ea28365ea 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -117,7 +117,8 @@ class TestCopyBeforeWrite(iotests.QMPTestCase):
         self.vm.pause_drive("drive0")
         self.vm.cmd('drive-backup', device='drive0',
                     sync='full', target=target_img,
-                    format="file", speed=1)
+                    format="file", speed=1,
+                    x_perf={'max-chunk': 65536})
         self.vm.cmd('block-job-pause', device="drive0")
         # Speed is low enough that this must be an uncopied range, which will
         # trigger the before write notifier
-- 
2.53.0