On 12.01.21 16:44, Vladimir Sementsov-Ogievskiy wrote:
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 <vsement...@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.
Hm, yes, before this series, copy_range is effectively the default, right.
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.
Both sound like good points to me. So as far as I’m concerned,
disabling copy offloading sounds good to me.
So now this separate patch here makes sense to me:
Reviewed-by: Max Reitz <mre...@redhat.com>
Perhaps it deserves some explanation in a commit message, though. I.e.,
that originally, this defaulted to true, as it was before, but now that
async copying is implemented, we should let it default to false.
Max
