qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 20/25] qapi: backup: disable copy_range by default


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v3 20/25] qapi: backup: disable copy_range by default
Date: Tue, 12 Jan 2021 18:44:01 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

12.01.2021 17:05, Max Reitz wrote:
On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:
Further commit will add a benchmark
(scripts/simplebench/bench-backup.py), which will show that backup
works better with async parallel requests (previous commit) and
disabled copy_range. So, let's disable copy_range by default.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
  qapi/block-core.json | 2 +-
  blockdev.c           | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

Uh, well, er, then why did you add it as true in patch 2?

Do you mean this patch as the basis for a discussion whether it should be true or false by default?

Yes, I just kept the change in separate, to make it simpler to discuss.. Hmm, 
or seems I've tried to keep it close to the point when we are going to increase 
performance anyway (and making default false is needed for good final 
performance), when if we do the change earlier it would like a degradation 
during previous 20 commits.

I don’t have anything to contribute, though, ergo I don’t mind either way.

Do you have an idea why copy offloading isn’t better?


As I understand, it may be really fast for filesystem which will avoid data 
copying, like btrfs. Why it works slower for others when we enable async + 
block-status things, I don't know. It should depend on kernel implementation.

I have two additional thoughts:

1. for backup we usually want to do a copy on another node, or at least another 
hard drive, so we really going to copy the data, not just create some metadata 
links in fs.

2. even if fs can just create some links to existing clusters instead of 
copying data, IMHO it is not good for backup:

Main difference of backup from snapshot is that we don't touch active disk: it 
works in the same way like without backup job running. But if we do create 
fs-metadata links in backup image to original clusters, it means that on next 
guest write fs will have to do COW operation, so backup file will reference old 
cluster from active disk, and new cluster will be allocated for active disk. 
This means, that backup job influence on active vm disk, increasing its 
fragmentation.. Possibly I'm wrong, I'm not really good in how filesystems 
works.



diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5a21c24c1d..58eb2bcb86 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1376,7 +1376,7 @@
  # Optional parameters for backup. These parameters don't affect
  # functionality, but may significantly affect performance.
  #
-# @use-copy-range: Use copy offloading. Default true.
+# @use-copy-range: Use copy offloading. Default false.
  #
  # @max-workers: Maximum number of parallel requests for the sustained 
background
  #               copying process. Doesn't influence copy-before-write 
operations.
diff --git a/blockdev.c b/blockdev.c
index 0ed390abe0..1ac64d8ee2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2788,7 +2788,7 @@ static BlockJob *do_backup_common(BackupCommon *backup,
  {
      BlockJob *job = NULL;
      BdrvDirtyBitmap *bmap = NULL;
-    BackupPerf perf = { .use_copy_range = true, .max_workers = 64 };
+    BackupPerf perf = { .max_workers = 64 };
      int job_flags = JOB_DEFAULT;
      if (!backup->has_speed) {




--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]