[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/7] block-copy: streamline choice of copy_range vs. read/
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH v2 1/7] block-copy: streamline choice of copy_range vs. read/write |
Date: |
Thu, 20 May 2021 17:42:35 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.2 |
18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:
From: Paolo Bonzini <pbonzini@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.
What's the benefit of it?
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.
hmm, maybe. It could be a separate patch.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/block-copy.c | 59 ++++++++++++++++++++++++++++++----------------
stats agree with me, that its' not a simplification.
1 file changed, 39 insertions(+), 20 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index 37c8e8504b..10ce51a244 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -28,6 +28,13 @@
#define BLOCK_COPY_MAX_WORKERS 64
#define BLOCK_COPY_SLICE_TIME 100000000ULL /* ns */
+typedef enum {
+ COPY_READ_WRITE_CLUSTER,
+ COPY_READ_WRITE,
+ COPY_RANGE_SMALL,
+ COPY_RANGE_FULL
+} BlockCopyMethod;
+
static coroutine_fn int block_copy_task_entry(AioTask *task);
typedef struct BlockCopyCallState {
@@ -85,8 +92,8 @@ typedef struct BlockCopyState {
BdrvDirtyBitmap *copy_bitmap;
int64_t in_flight_bytes;
int64_t cluster_size;
- bool use_copy_range;
- int64_t copy_size;
+ BlockCopyMethod method;
+ int64_t max_transfer;
uint64_t len;
QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls
*/
QLIST_HEAD(, BlockCopyCallState) calls;
@@ -148,6 +155,23 @@ static bool coroutine_fn
block_copy_wait_one(BlockCopyState *s, int64_t offset,
return true;
}
+static inline int64_t block_copy_chunk_size(BlockCopyState *s)
"inline" word does nothing in static definitions in c files. Compiler make a
decision independently of it.
+{
+ switch (s->method) {
+ case COPY_READ_WRITE_CLUSTER:
+ return s->cluster_size;
+ case COPY_READ_WRITE:
+ case COPY_RANGE_SMALL:
+ return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER),
+ s->max_transfer);
+ case COPY_RANGE_FULL:
+ return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
+ s->max_transfer);
+ default:
+ abort();
+ }
+}
+
/*
* Search for the first dirty area in offset/bytes range and create task at
* the beginning of it.
@@ -157,8 +181,9 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState
*s,
int64_t offset, int64_t bytes)
{
BlockCopyTask *task;
- int64_t max_chunk = MIN_NON_ZERO(s->copy_size, call_state->max_chunk);
+ int64_t max_chunk = block_copy_chunk_size(s);
+ max_chunk = MIN_NON_ZERO(max_chunk, call_state->max_chunk);
if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
offset, offset + bytes,
max_chunk, &offset, &bytes))
@@ -265,28 +290,27 @@ BlockCopyState *block_copy_state_new(BdrvChild *source,
BdrvChild *target,
.len = bdrv_dirty_bitmap_size(copy_bitmap),
.write_flags = write_flags,
.mem = shres_create(BLOCK_COPY_MAX_MEM),
+ .max_transfer = QEMU_ALIGN_DOWN(block_copy_max_transfer(source, target)
+ , cluster_size),
};
- if (block_copy_max_transfer(source, target) < cluster_size) {
+ 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;
} else {
/*
* 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;
}
ratelimit_init(&s->rate_limit);
@@ -369,30 +393,25 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState
*s,
return ret;
}
- if (s->use_copy_range) {
+ if (s->method >= COPY_RANGE_SMALL) {
I don't like such condition:
1. it forces me to go to enum definition to understand the logic
2. it's error prone: it's very possibly to forget to update it, when updating
the enum, and logic will be broken.
No, I don't like moving to state machine for this simple thing.
ret = bdrv_co_copy_range(s->source, offset, s->target, offset, nbytes,
0, s->write_flags);
if (ret < 0) {
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;
/* Fallback to read+write with allocated buffer */
} else {
- if (s->use_copy_range) {
+ if (s->method == COPY_RANGE_SMALL) {
/*
* Successful copy-range. Now increase copy_size. copy_range
* does not respect max_transfer (it's a TODO), so we factor
* that in here.
*
- * Note: we double-check s->use_copy_range for the case when
+ * Note: we double-check s->method for the case when
* parallel block-copy request unsets it during previous
* bdrv_co_copy_range call.
*/
- 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));
+ s->method = COPY_RANGE_FULL;
}
goto out;
}
--
Best regards,
Vladimir
Re: [PATCH v2 1/7] block-copy: streamline choice of copy_range vs. read/write, Stefan Hajnoczi, 2021/05/27
[PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask and BlockCopyState types and functions, Emanuele Giuseppe Esposito, 2021/05/18