[Qemu-devel] [PATCH v2 0/7] backup improvements

[Qemu-devel] [PATCH v2 0/7] backup improvements

[Vladimir Sementsov-Ogievskiy]

Hi all!

There are some fixes and refactorings I need on my way to resend
my backup-top series. It's obvious now that I need to share copying
code between backup and backup-top, as backup copying code becomes
smarter and more complicated. So the goal of the series is to make copying
code more share-able.

It's based on John's bitmaps branch, rebased on master.

v2 (by Max's comments):

02: Add Max's r-b
03: - split out backup changes to 03
    - handle common max_transfer = 0 case
04: splat out from 02
06: fix allocation size
07: - rebase on 06 changes
    - add Max's r-b

two patches are dropped or postponed for the next step

Vladimir Sementsov-Ogievskiy (7):
  block/backup: deal with zero detection
  block/backup: refactor write_flags
  block/io: handle alignment and max_transfer for copy_range
  block/backup: drop handling of max_transfer for copy_range
  block/backup: fix backup_cow_with_offload for last cluster
  block/backup: teach backup_cow_with_bounce_buffer to copy more at once
  block/backup: merge duplicated logic into backup_do_cow

 block/backup.c | 125 +++++++++++++++++++------------------------------
 block/io.c     |  48 +++++++++++++++----
 blockdev.c     |   8 ++--
 3 files changed, 91 insertions(+), 90 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH v2 1/7] block/backup: deal with zero detection

[Vladimir Sementsov-Ogievskiy]

We have detect_zeroes option, so at least for blockdev-backup user
should define it if zero-detection is needed. For drive-backup leave
detection enabled by default but do it through existing option instead
of open-coding.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/backup.c | 15 ++++++---------
 blockdev.c     |  8 ++++----
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index adc4d44244..d815436455 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -113,7 +113,10 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
     BlockBackend *blk = job->common.blk;
     int nbytes;
     int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
-    int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
+    int write_flags =
+            (job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0) |
+            (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
+
 
     assert(QEMU_IS_ALIGNED(start, job->cluster_size));
     bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
@@ -131,14 +134,8 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
         goto fail;
     }
 
-    if (buffer_is_zero(*bounce_buffer, nbytes)) {
-        ret = blk_co_pwrite_zeroes(job->target, start,
-                                   nbytes, write_flags | BDRV_REQ_MAY_UNMAP);
-    } else {
-        ret = blk_co_pwrite(job->target, start,
-                            nbytes, *bounce_buffer, write_flags |
-                            (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0));
-    }
+    ret = blk_co_pwrite(job->target, start, nbytes, *bounce_buffer,
+                        write_flags);
     if (ret < 0) {
         trace_backup_do_cow_write_fail(job, start, ret);
         if (error_is_read) {
diff --git a/blockdev.c b/blockdev.c
index 29c6c6044a..2d7e7be538 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3613,7 +3613,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
     BlockDriverState *source = NULL;
     BlockJob *job = NULL;
     AioContext *aio_context;
-    QDict *options = NULL;
+    QDict *options;
     Error *local_err = NULL;
     int flags;
     int64_t size;
@@ -3686,10 +3686,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
         goto out;
     }
 
+    options = qdict_new();
+    qdict_put_str(options, "discard", "unmap");
+    qdict_put_str(options, "detect-zeroes", "unmap");
     if (backup->format) {
-        if (!options) {
-            options = qdict_new();
-        }
         qdict_put_str(options, "driver", backup->format);
     }
 
-- 
2.18.0

[Qemu-devel] [PATCH v2 2/7] block/backup: refactor write_flags

[Vladimir Sementsov-Ogievskiy]

write flags are constant, let's store it in BackupBlockJob instead of
recalculating. It also makes two boolean fields to be unused, so,
drop them.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/backup.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index d815436455..c6a3b2b7bb 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -50,14 +50,13 @@ typedef struct BackupBlockJob {
     uint64_t len;
     uint64_t bytes_read;
     int64_t cluster_size;
-    bool compress;
     NotifierWithReturn before_write;
     QLIST_HEAD(, CowRequest) inflight_reqs;
 
     bool use_copy_range;
     int64_t copy_range_size;
 
-    bool serialize_target_writes;
+    BdrvRequestFlags write_flags;
     bool initializing_bitmap;
 } BackupBlockJob;
 
@@ -113,10 +112,6 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
     BlockBackend *blk = job->common.blk;
     int nbytes;
     int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
-    int write_flags =
-            (job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0) |
-            (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
-
 
     assert(QEMU_IS_ALIGNED(start, job->cluster_size));
     bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
@@ -135,7 +130,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
     }
 
     ret = blk_co_pwrite(job->target, start, nbytes, *bounce_buffer,
-                        write_flags);
+                        job->write_flags);
     if (ret < 0) {
         trace_backup_do_cow_write_fail(job, start, ret);
         if (error_is_read) {
@@ -163,7 +158,6 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
     BlockBackend *blk = job->common.blk;
     int nbytes;
     int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
-    int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
     assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
     assert(QEMU_IS_ALIGNED(start, job->cluster_size));
@@ -172,7 +166,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
     bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
                             job->cluster_size * nr_clusters);
     ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
-                            read_flags, write_flags);
+                            read_flags, job->write_flags);
     if (ret < 0) {
         trace_backup_do_cow_copy_range_fail(job, start, ret);
         bdrv_set_dirty_bitmap(job->copy_bitmap, start,
@@ -748,10 +742,16 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     job->sync_mode = sync_mode;
     job->sync_bitmap = sync_bitmap;
     job->bitmap_mode = bitmap_mode;
-    job->compress = compress;
 
-    /* Detect image-fleecing (and similar) schemes */
-    job->serialize_target_writes = bdrv_chain_contains(target, bs);
+    /*
+     * Set write flags:
+     *  1. Detect image-fleecing (and similar) schemes
+     *  2. Handle compression
+     */
+    job->write_flags =
+            (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 0) |
+            (compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
+
     job->cluster_size = cluster_size;
     job->copy_bitmap = copy_bitmap;
     copy_bitmap = NULL;
-- 
2.18.0

[Qemu-devel] [PATCH v2 3/7] block/io: handle alignment and max_transfer for copy_range

[Vladimir Sementsov-Ogievskiy]

copy_range ignores these limitations, let's improve it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 48 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/block/io.c b/block/io.c
index 06305c6ea6..a5efb2200f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3005,11 +3005,24 @@ static int coroutine_fn bdrv_co_copy_range_internal(
 {
     BdrvTrackedRequest req;
     int ret;
+    uint32_t align = MAX(src->bs->bl.request_alignment,
+                         dst->bs->bl.request_alignment);
+    uint32_t max_transfer =
+            QEMU_ALIGN_DOWN(MIN_NON_ZERO(MIN_NON_ZERO(src->bs->bl.max_transfer,
+                                                      dst->bs->bl.max_transfer),
+                                         INT_MAX), align);
 
     /* TODO We can support BDRV_REQ_NO_FALLBACK here */
     assert(!(read_flags & BDRV_REQ_NO_FALLBACK));
     assert(!(write_flags & BDRV_REQ_NO_FALLBACK));
 
+    if (max_transfer == 0 && bytes > 0) {
+        /*
+         * For example, if source max_transfer is smaller than target alignment.
+         */
+        return -ENOTSUP;
+    }
+
     if (!dst || !dst->bs) {
         return -ENOMEDIUM;
     }
@@ -3031,7 +3044,10 @@ static int coroutine_fn bdrv_co_copy_range_internal(
 
     if (!src->bs->drv->bdrv_co_copy_range_from
         || !dst->bs->drv->bdrv_co_copy_range_to
-        || src->bs->encrypted || dst->bs->encrypted) {
+        || src->bs->encrypted || dst->bs->encrypted ||
+        !QEMU_IS_ALIGNED(src_offset, src->bs->bl.request_alignment) ||
+        !QEMU_IS_ALIGNED(dst_offset, dst->bs->bl.request_alignment) ||
+        !QEMU_IS_ALIGNED(bytes, align)) {
         return -ENOTSUP;
     }
 
@@ -3046,11 +3062,22 @@ static int coroutine_fn bdrv_co_copy_range_internal(
             wait_serialising_requests(&req);
         }
 
-        ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,
-                                                    src, src_offset,
-                                                    dst, dst_offset,
-                                                    bytes,
-                                                    read_flags, write_flags);
+        while (bytes) {
+            int num = MIN(bytes, max_transfer);
+
+            ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,
+                                                        src, src_offset,
+                                                        dst, dst_offset,
+                                                        num,
+                                                        read_flags,
+                                                        write_flags);
+            if (ret < 0) {
+                break;
+            }
+            bytes -= num;
+            src_offset += num;
+            dst_offset += num;
+        }
 
         tracked_request_end(&req);
         bdrv_dec_in_flight(src->bs);
@@ -3060,12 +3087,17 @@ static int coroutine_fn bdrv_co_copy_range_internal(
                               BDRV_TRACKED_WRITE);
         ret = bdrv_co_write_req_prepare(dst, dst_offset, bytes, &req,
                                         write_flags);
-        if (!ret) {
+        while (!ret && bytes) {
+            int num = MIN(bytes, max_transfer);
+
             ret = dst->bs->drv->bdrv_co_copy_range_to(dst->bs,
                                                       src, src_offset,
                                                       dst, dst_offset,
-                                                      bytes,
+                                                      num,
                                                       read_flags, write_flags);
+            bytes -= num;
+            src_offset += num;
+            dst_offset += num;
         }
         bdrv_co_write_req_finish(dst, dst_offset, bytes, &req, ret);
         tracked_request_end(&req);
-- 
2.18.0

Re: [Qemu-devel] [PATCH v2 3/7] block/io: handle alignment and max_transfer for copy_range

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 370 bytes --]

On 09.08.19 17:32, Vladimir Sementsov-Ogievskiy wrote:
> copy_range ignores these limitations, let's improve it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 48 ++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 40 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 4/7] block/backup: drop handling of max_transfer for copy_range

[Vladimir Sementsov-Ogievskiy]

Since previous commit, copy_range supports max_transfer, so we don't
need to handle it by hand.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index c6a3b2b7bb..228ba9423c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -54,7 +54,6 @@ typedef struct BackupBlockJob {
     QLIST_HEAD(, CowRequest) inflight_reqs;
 
     bool use_copy_range;
-    int64_t copy_range_size;
 
     BdrvRequestFlags write_flags;
     bool initializing_bitmap;
@@ -156,12 +155,11 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
     int ret;
     int nr_clusters;
     BlockBackend *blk = job->common.blk;
-    int nbytes;
+    int nbytes = end - start;
     int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
 
-    assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
+    assert(end - start < INT_MAX);
     assert(QEMU_IS_ALIGNED(start, job->cluster_size));
-    nbytes = MIN(job->copy_range_size, end - start);
     nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
     bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
                             job->cluster_size * nr_clusters);
@@ -756,11 +754,6 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     job->copy_bitmap = copy_bitmap;
     copy_bitmap = NULL;
     job->use_copy_range = !compress; /* compression isn't supported for it */
-    job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
-                                        blk_get_max_transfer(job->target));
-    job->copy_range_size = MAX(job->cluster_size,
-                               QEMU_ALIGN_UP(job->copy_range_size,
-                                             job->cluster_size));
 
     /* Required permissions are already taken with target's blk_new() */
     block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
-- 
2.18.0

Re: [Qemu-devel] [PATCH v2 4/7] block/backup: drop handling of max_transfer for copy_range

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 379 bytes --]

On 09.08.19 17:32, Vladimir Sementsov-Ogievskiy wrote:
> Since previous commit, copy_range supports max_transfer, so we don't
> need to handle it by hand.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/backup.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 5/7] block/backup: fix backup_cow_with_offload for last cluster

[Vladimir Sementsov-Ogievskiy]

We shouldn't try to copy bytes beyond EOF. Fix it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/backup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/backup.c b/block/backup.c
index 228ba9423c..d482d93458 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -155,7 +155,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
     int ret;
     int nr_clusters;
     BlockBackend *blk = job->common.blk;
-    int nbytes = end - start;
+    int nbytes = MIN(end - start, job->len - start);
     int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
 
     assert(end - start < INT_MAX);
-- 
2.18.0

[Qemu-devel] [PATCH v2 6/7] block/backup: teach backup_cow_with_bounce_buffer to copy more at once

[Vladimir Sementsov-Ogievskiy]

backup_cow_with_offload can transfer more than on cluster. Let
backup_cow_with_bounce_buffer behave similarly. It reduces number
of IO and there are no needs to copy cluster by cluster.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index d482d93458..155e21d0a3 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -104,22 +104,25 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
                                                       int64_t start,
                                                       int64_t end,
                                                       bool is_write_notifier,
-                                                      bool *error_is_read,
-                                                      void **bounce_buffer)
+                                                      bool *error_is_read)
 {
     int ret;
     BlockBackend *blk = job->common.blk;
     int nbytes;
     int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
+    void *bounce_buffer;
 
     assert(QEMU_IS_ALIGNED(start, job->cluster_size));
-    bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
-    nbytes = MIN(job->cluster_size, job->len - start);
-    if (!*bounce_buffer) {
-        *bounce_buffer = blk_blockalign(blk, job->cluster_size);
+
+    nbytes = MIN(end - start, job->len - start);
+    bounce_buffer = blk_try_blockalign(blk, nbytes);
+    if (!bounce_buffer) {
+        return -ENOMEM;
     }
 
-    ret = blk_co_pread(blk, start, nbytes, *bounce_buffer, read_flags);
+    bdrv_reset_dirty_bitmap(job->copy_bitmap, start, end - start);
+
+    ret = blk_co_pread(blk, start, nbytes, bounce_buffer, read_flags);
     if (ret < 0) {
         trace_backup_do_cow_read_fail(job, start, ret);
         if (error_is_read) {
@@ -128,7 +131,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
         goto fail;
     }
 
-    ret = blk_co_pwrite(job->target, start, nbytes, *bounce_buffer,
+    ret = blk_co_pwrite(job->target, start, nbytes, bounce_buffer,
                         job->write_flags);
     if (ret < 0) {
         trace_backup_do_cow_write_fail(job, start, ret);
@@ -138,9 +141,12 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
         goto fail;
     }
 
+    qemu_vfree(bounce_buffer);
     return nbytes;
+
 fail:
     bdrv_set_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
+    qemu_vfree(bounce_buffer);
     return ret;
 
 }
@@ -254,7 +260,6 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
     CowRequest cow_request;
     int ret = 0;
     int64_t start, end; /* bytes */
-    void *bounce_buffer = NULL;
     int64_t skip_bytes;
 
     qemu_co_rwlock_rdlock(&job->flush_rwlock);
@@ -303,7 +308,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
         if (!job->use_copy_range) {
             ret = backup_cow_with_bounce_buffer(job, start, dirty_end,
                                                 is_write_notifier,
-                                                error_is_read, &bounce_buffer);
+                                                error_is_read);
         }
         if (ret < 0) {
             break;
@@ -318,10 +323,6 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
         ret = 0;
     }
 
-    if (bounce_buffer) {
-        qemu_vfree(bounce_buffer);
-    }
-
     cow_request_end(&cow_request);
 
     trace_backup_do_cow_return(job, offset, bytes, ret);
-- 
2.18.0

Re: [Qemu-devel] [PATCH v2 6/7] block/backup: teach backup_cow_with_bounce_buffer to copy more at once

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2715 bytes --]

On 8/9/19 10:32 AM, Vladimir Sementsov-Ogievskiy wrote:
> backup_cow_with_offload can transfer more than on cluster. Let

s/on/one/

> backup_cow_with_bounce_buffer behave similarly. It reduces number
> of IO and there are no needs to copy cluster by cluster.

It reduces the number of IO requests, since there is no need to copy
cluster by cluster.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/backup.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index d482d93458..155e21d0a3 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -104,22 +104,25 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
>                                                        int64_t start,
>                                                        int64_t end,
>                                                        bool is_write_notifier,
> -                                                      bool *error_is_read,
> -                                                      void **bounce_buffer)
> +                                                      bool *error_is_read)

Why is this signature changing?

>  {
>      int ret;
>      BlockBackend *blk = job->common.blk;
>      int nbytes;
>      int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
> +    void *bounce_buffer;
>  
>      assert(QEMU_IS_ALIGNED(start, job->cluster_size));
> -    bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
> -    nbytes = MIN(job->cluster_size, job->len - start);
> -    if (!*bounce_buffer) {
> -        *bounce_buffer = blk_blockalign(blk, job->cluster_size);

Pre-patch, you allocate the bounce_buffer at most once (but limited to a
cluster size), post-patch, you are now allocating and freeing a bounce
buffer every iteration through.  There may be fewer calls because you
are allocating something bigger, but still, isn't it a good goal to try
and allocate the bounce buffer as few times as possible and reuse it,
rather than getting a fresh one each iteration?

> +
> +    nbytes = MIN(end - start, job->len - start);

What is the largest this will be? Will it exhaust memory on a 32-bit or
otherwise memory-constrained system, where you should have some maximum
cap for how large the bounce buffer will be while still getting better
performance than one cluster at a time and not slowing things down by
over-sizing malloc?  64M, maybe?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 6/7] block/backup: teach backup_cow_with_bounce_buffer to copy more at once

[Vladimir Sementsov-Ogievskiy]

09.08.2019 18:59, Eric Blake wrote:
> On 8/9/19 10:32 AM, Vladimir Sementsov-Ogievskiy wrote:
>> backup_cow_with_offload can transfer more than on cluster. Let
> 
> s/on/one/
> 
>> backup_cow_with_bounce_buffer behave similarly. It reduces number
>> of IO and there are no needs to copy cluster by cluster.
> 
> It reduces the number of IO requests, since there is no need to copy
> cluster by cluster.
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/backup.c | 29 +++++++++++++++--------------
>>   1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index d482d93458..155e21d0a3 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -104,22 +104,25 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
>>                                                         int64_t start,
>>                                                         int64_t end,
>>                                                         bool is_write_notifier,
>> -                                                      bool *error_is_read,
>> -                                                      void **bounce_buffer)
>> +                                                      bool *error_is_read)
> 
> Why is this signature changing?
> 
>>   {
>>       int ret;
>>       BlockBackend *blk = job->common.blk;
>>       int nbytes;
>>       int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
>> +    void *bounce_buffer;
>>   
>>       assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>> -    bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
>> -    nbytes = MIN(job->cluster_size, job->len - start);
>> -    if (!*bounce_buffer) {
>> -        *bounce_buffer = blk_blockalign(blk, job->cluster_size);
> 
> Pre-patch, you allocate the bounce_buffer at most once (but limited to a
> cluster size), post-patch, you are now allocating and freeing a bounce
> buffer every iteration through.  There may be fewer calls because you
> are allocating something bigger, but still, isn't it a good goal to try
> and allocate the bounce buffer as few times as possible and reuse it,
> rather than getting a fresh one each iteration?

Yes, it's a "degradation" of this patch, I was afraid of this question.
However, I doubt that it should be optimized:

1. I'm going to run several copy requests in coroutines to parallelize copying loop,
to improve performance (series for qcow2 are here
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg06654.html), so we'll need
several buffers for parallel copying requests and it's extremely easier to allocate
buffer when it's needed and free after it, than do some allocated memory sharing like
in mirror.

2. Actually it is a question about memory allocator: is it fast and optimized
enough to not care, or is it bad, and we should work-around it like in
mirror? And in my opinion (proved by a bit of benchmarking) memalign
is fast enough to don't care. I was answering similar question in more details
and with some numbers here:
https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00087.html

So, I'd prefere to not care and keep code simpler. But if you don't agree,
I can leave shared buffer here, at least until introduction of parallel requests.
Then, it will be something like qemu_try_blockalign(MIN(bytes, 64M))..

> 
>> +
>> +    nbytes = MIN(end - start, job->len - start);
> 
> What is the largest this will be? Will it exhaust memory on a 32-bit or
> otherwise memory-constrained system, where you should have some maximum
> cap for how large the bounce buffer will be while still getting better
> performance than one cluster at a time and not slowing things down by
> over-sizing malloc?  64M, maybe?
> 

Hmm, good point. I thought that it's safe to allocate buffer for a full request,
as if such buffer is already allocated for the guest request itself, it should
not be bad to allocate another one with same size. But I forgot about write-zeros
and discard operations which may be large without any allocation and here we need
to allocate anyway.

Hmm2, but we have parallel guest writes(discards) and therefore parallel
copy-before-write operations. So total allocation is not limited anyway and it
should be fixed somehow too..

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 6/7] block/backup: teach backup_cow_with_bounce_buffer to copy more at once

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2145 bytes --]

On 8/10/19 7:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 09.08.2019 18:59, Eric Blake wrote:
>> On 8/9/19 10:32 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> backup_cow_with_offload can transfer more than on cluster. Let
>>
>> s/on/one/
>>
>>> backup_cow_with_bounce_buffer behave similarly. It reduces number
>>> of IO and there are no needs to copy cluster by cluster.
>>
>> It reduces the number of IO requests, since there is no need to copy
>> cluster by cluster.

>>> -                                                      bool *error_is_read,
>>> -                                                      void **bounce_buffer)
>>> +                                                      bool *error_is_read)
>>
>> Why is this signature changing?
>>

> 
> 2. Actually it is a question about memory allocator: is it fast and optimized
> enough to not care, or is it bad, and we should work-around it like in
> mirror? And in my opinion (proved by a bit of benchmarking) memalign
> is fast enough to don't care. I was answering similar question in more details
> and with some numbers here:
> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00087.html
> 
> So, I'd prefere to not care and keep code simpler. But if you don't agree,
> I can leave shared buffer here, at least until introduction of parallel requests.
> Then, it will be something like qemu_try_blockalign(MIN(bytes, 64M))..

It may still be worth capping at 64M.  I'm not opposed to a change to
per-request allocation rather than trying to reuse a buffer, if it is
going to make parallelization efforts easier; but I am worried about the
maximum memory usage.  I'm more worried that you are trying to cram two
distinct changes into one patch, and didn't even mention the change
about a change from buffer reuse to per-request allocations, in the
commit message.  If you make that sort of change, it should be called
out in the commit message as intentional, or maybe even split to a
separate patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 6/7] block/backup: teach backup_cow_with_bounce_buffer to copy more at once

[Vladimir Sementsov-Ogievskiy]

10.08.2019 15:47, Eric Blake wrote:
> On 8/10/19 7:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 09.08.2019 18:59, Eric Blake wrote:
>>> On 8/9/19 10:32 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> backup_cow_with_offload can transfer more than on cluster. Let
>>>
>>> s/on/one/
>>>
>>>> backup_cow_with_bounce_buffer behave similarly. It reduces number
>>>> of IO and there are no needs to copy cluster by cluster.
>>>
>>> It reduces the number of IO requests, since there is no need to copy
>>> cluster by cluster.
> 
>>>> -                                                      bool *error_is_read,
>>>> -                                                      void **bounce_buffer)
>>>> +                                                      bool *error_is_read)
>>>
>>> Why is this signature changing?
>>>
> 
>>
>> 2. Actually it is a question about memory allocator: is it fast and optimized
>> enough to not care, or is it bad, and we should work-around it like in
>> mirror? And in my opinion (proved by a bit of benchmarking) memalign
>> is fast enough to don't care. I was answering similar question in more details
>> and with some numbers here:
>> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00087.html
>>
>> So, I'd prefere to not care and keep code simpler. But if you don't agree,
>> I can leave shared buffer here, at least until introduction of parallel requests.
>> Then, it will be something like qemu_try_blockalign(MIN(bytes, 64M))..
> 
> It may still be worth capping at 64M.  I'm not opposed to a change to
> per-request allocation rather than trying to reuse a buffer, if it is
> going to make parallelization efforts easier; but I am worried about the
> maximum memory usage.  I'm more worried that you are trying to cram two
> distinct changes into one patch, and didn't even mention the change
> about a change from buffer reuse to per-request allocations, in the
> commit message.  If you make that sort of change, it should be called
> out in the commit message as intentional, or maybe even split to a
> separate patch.
> 

OK, I failed to hide it :) Will split out and add 64M limit.

-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v2 7/7] block/backup: merge duplicated logic into backup_do_cow

[Vladimir Sementsov-Ogievskiy]

backup_cow_with_offload and backup_cow_with_bounce_buffer contains a
lot of duplicated logic. Move it into backup_do_cow.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/backup.c | 84 +++++++++++++++++++-------------------------------
 1 file changed, 31 insertions(+), 53 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 155e21d0a3..ae780e1260 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -100,85 +100,60 @@ static void cow_request_end(CowRequest *req)
 
 /* Copy range to target with a bounce buffer and return the bytes copied. If
  * error occurred, return a negative error number */
-static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
-                                                      int64_t start,
-                                                      int64_t end,
-                                                      bool is_write_notifier,
-                                                      bool *error_is_read)
+static int coroutine_fn backup_cow_with_bounce_buffer(
+        BackupBlockJob *job, int64_t offset, int64_t bytes,
+        BdrvRequestFlags read_flags, bool *error_is_read)
 {
-    int ret;
+    int ret = 0;
     BlockBackend *blk = job->common.blk;
-    int nbytes;
-    int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
-    void *bounce_buffer;
-
-    assert(QEMU_IS_ALIGNED(start, job->cluster_size));
+    void *bounce_buffer = blk_try_blockalign(blk, bytes);
 
-    nbytes = MIN(end - start, job->len - start);
-    bounce_buffer = blk_try_blockalign(blk, nbytes);
     if (!bounce_buffer) {
         return -ENOMEM;
     }
 
-    bdrv_reset_dirty_bitmap(job->copy_bitmap, start, end - start);
-
-    ret = blk_co_pread(blk, start, nbytes, bounce_buffer, read_flags);
+    ret = blk_co_pread(blk, offset, bytes, bounce_buffer, read_flags);
     if (ret < 0) {
-        trace_backup_do_cow_read_fail(job, start, ret);
+        trace_backup_do_cow_read_fail(job, offset, ret);
         if (error_is_read) {
             *error_is_read = true;
         }
-        goto fail;
+        goto out;
     }
 
-    ret = blk_co_pwrite(job->target, start, nbytes, bounce_buffer,
+    ret = blk_co_pwrite(job->target, offset, bytes, bounce_buffer,
                         job->write_flags);
     if (ret < 0) {
-        trace_backup_do_cow_write_fail(job, start, ret);
+        trace_backup_do_cow_write_fail(job, offset, ret);
         if (error_is_read) {
             *error_is_read = false;
         }
-        goto fail;
+        goto out;
     }
 
+out:
     qemu_vfree(bounce_buffer);
-    return nbytes;
 
-fail:
-    bdrv_set_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
-    qemu_vfree(bounce_buffer);
     return ret;
-
 }
 
 /* Copy range to target and return the bytes copied. If error occurred, return a
  * negative error number. */
 static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
-                                                int64_t start,
-                                                int64_t end,
-                                                bool is_write_notifier)
+                                                int64_t offset,
+                                                int64_t bytes,
+                                                BdrvRequestFlags read_flags)
 {
     int ret;
-    int nr_clusters;
     BlockBackend *blk = job->common.blk;
-    int nbytes = MIN(end - start, job->len - start);
-    int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
-
-    assert(end - start < INT_MAX);
-    assert(QEMU_IS_ALIGNED(start, job->cluster_size));
-    nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
-    bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
-                            job->cluster_size * nr_clusters);
-    ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
+
+    ret = blk_co_copy_range(blk, offset, job->target, offset, bytes,
                             read_flags, job->write_flags);
     if (ret < 0) {
-        trace_backup_do_cow_copy_range_fail(job, start, ret);
-        bdrv_set_dirty_bitmap(job->copy_bitmap, start,
-                              job->cluster_size * nr_clusters);
-        return ret;
+        trace_backup_do_cow_copy_range_fail(job, offset, ret);
     }
 
-    return nbytes;
+    return ret;
 }
 
 /*
@@ -261,6 +236,8 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
     int ret = 0;
     int64_t start, end; /* bytes */
     int64_t skip_bytes;
+    BdrvRequestFlags read_flags =
+            is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
 
     qemu_co_rwlock_rdlock(&job->flush_rwlock);
 
@@ -274,6 +251,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
 
     while (start < end) {
         int64_t dirty_end;
+        int64_t cur_bytes;
 
         if (!bdrv_dirty_bitmap_get(job->copy_bitmap, start)) {
             trace_backup_do_cow_skip(job, start);
@@ -297,30 +275,30 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
         }
 
         trace_backup_do_cow_process(job, start);
+        cur_bytes = MIN(dirty_end - start, job->len - start);
+        bdrv_reset_dirty_bitmap(job->copy_bitmap, start, dirty_end - start);
 
         if (job->use_copy_range) {
-            ret = backup_cow_with_offload(job, start, dirty_end,
-                                          is_write_notifier);
+            ret = backup_cow_with_offload(job, start, cur_bytes, read_flags);
             if (ret < 0) {
                 job->use_copy_range = false;
             }
         }
         if (!job->use_copy_range) {
-            ret = backup_cow_with_bounce_buffer(job, start, dirty_end,
-                                                is_write_notifier,
-                                                error_is_read);
+            ret = backup_cow_with_bounce_buffer(job, start, cur_bytes,
+                                                read_flags, error_is_read);
         }
         if (ret < 0) {
+            bdrv_set_dirty_bitmap(job->copy_bitmap, start, dirty_end - start);
             break;
         }
 
         /* Publish progress, guest I/O counts as progress too.  Note that the
          * offset field is an opaque progress value, it is not a disk offset.
          */
-        start += ret;
-        job->bytes_read += ret;
-        job_progress_update(&job->common.job, ret);
-        ret = 0;
+        start += cur_bytes;
+        job->bytes_read += cur_bytes;
+        job_progress_update(&job->common.job, cur_bytes);
     }
 
     cow_request_end(&cow_request);
-- 
2.18.0

Re: [Qemu-devel] [PATCH v2 0/7] backup improvements

[Vladimir Sementsov-Ogievskiy]

09.08.2019 18:32, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> There are some fixes and refactorings I need on my way to resend
> my backup-top series. It's obvious now that I need to share copying
> code between backup and backup-top, as backup copying code becomes
> smarter and more complicated. So the goal of the series is to make copying
> code more share-able.
> 
> It's based on John's bitmaps branch, rebased on master.

No, it's based directly on John's bitmaps branch:
Based-on: https://github.com/jnsnow/qemu bitmaps

> 
> v2 (by Max's comments):
> 
> 02: Add Max's r-b
> 03: - split out backup changes to 03
>      - handle common max_transfer = 0 case
> 04: splat out from 02
> 06: fix allocation size
> 07: - rebase on 06 changes
>      - add Max's r-b
> 
> two patches are dropped or postponed for the next step
> 
> Vladimir Sementsov-Ogievskiy (7):
>    block/backup: deal with zero detection
>    block/backup: refactor write_flags
>    block/io: handle alignment and max_transfer for copy_range
>    block/backup: drop handling of max_transfer for copy_range
>    block/backup: fix backup_cow_with_offload for last cluster
>    block/backup: teach backup_cow_with_bounce_buffer to copy more at once
>    block/backup: merge duplicated logic into backup_do_cow
> 
>   block/backup.c | 125 +++++++++++++++++++------------------------------
>   block/io.c     |  48 +++++++++++++++----
>   blockdev.c     |   8 ++--
>   3 files changed, 91 insertions(+), 90 deletions(-)
> 


-- 
Best regards,
Vladimir