21.05.2021 18:10, Paolo Bonzini wrote:
On 20/05/21 16:42, Vladimir Sementsov-Ogievskiy wrote:
18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:
From: Paolo Bonzini <pbonz...@redhat.com>
Put the logic to determine the copy size in a separate function, so
that there is a simple state machine for the possible methods of
copying data from one BlockDriverState to the other.
Honestly, for me 4-state state-maching + function to determine copy-size
doesn't seem better than two simple variables copy_size and use_copy_range.
There were six states before (2 for s->use_copy_range, three for s->copy_size),
of which two were unused. The heuristics for going between copy and read/write
were quite illegible.
What's the benefit of it?
Less duplication here, for example:
+ if (s->max_transfer < cluster_size) {
/*
* copy_range does not respect max_transfer. We don't want to bother
* with requests smaller than block-copy cluster size, so fallback to
* buffered copying (read and write respect max_transfer on their
* behalf).
*/
- s->use_copy_range = false;
- s->copy_size = cluster_size;
+ s->method = COPY_READ_WRITE_CLUSTER;
} else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
/* Compression supports only cluster-size writes and no copy-range.
*/
- s->use_copy_range = false;
- s->copy_size = cluster_size;
+ s->method = COPY_READ_WRITE_CLUSTER;
and here:
trace_block_copy_copy_range_fail(s, offset, ret);
- s->use_copy_range = false;
- s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
+ s->method = COPY_READ_WRITE;
...
/*
* We enable copy-range, but keep small copy_size, until first
* successful copy_range (look at block_copy_do_copy).
*/
- s->use_copy_range = use_copy_range;
- s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
+ s->method = use_copy_range ? COPY_RANGE_SMALL : COPY_READ_WRITE;
where it's not obvious that the two assignments to copy_size should be
the same (and they're suboptimal, too, since they don't obey max_transfer).
... plus...
While at it, store the common computation of block_copy_max_transfer
into a new field of BlockCopyState, and make sure that we always
obey max_transfer; that's more efficient even for the
COPY_RANGE_READ_WRITE case.
having a function makes it easier to spot slight differences that are
just bugs, such as this one.
Signed-off-by: Emanuele Giuseppe Esposito <eespo...@redhat.com>
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
block/block-copy.c | 59 ++++++++++++++++++++++++++++++----------------
stats agree with me, that its' not a simplification.
Stats don't say everything. Not having something like this:
s->copy_size =
MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
QEMU_ALIGN_DOWN(block_copy_max_transfer(s->source,
s->target),
s->cluster_size));
in the inner loop is already worth the extra lines for the
function declaration, for example.
After my "[PATCH v2 00/33] block: publish backup-top filter" copy_range path becomes
unused. I keep it thinking about further moving qemu-img convert to block-copy. But I don't even
have a plan when to start this work. So, if we want to do something around copy_range here to
prepare for thread-safety, let's just drop it for now as unused on top of "[PATCH v2 00/33]
block: publish backup-top filter" (you can take one patch that drop copy_range support in
backup to your series to not make a dependency). It's not difficult to reimplement it later.
--
Best regards,
Vladimir
