[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask an
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask and BlockCopyState types and functions |
Date: |
Thu, 20 May 2021 18:00:32 +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:
As done in BlockCopyCallState, categorize BlockCopyTask
and BlockCopyState in IN, State and OUT fields.
This is just to understand which field has to be protected with a lock.
BlockCopyTask .zeroes is a special case, because it is only initialized
and then read by the coroutine in block_copy_task_entry.
Also set block_copy_task_create as coroutine_fn because:
1) it is static and only invoked by coroutine functions
2) next patches will introduce and use a CoMutex lock there
this change is unrelated, why not to put it into commit, which adds use of
CoMutex in that function?
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/block-copy.c | 49 ++++++++++++++++++++++++++++++++--------------
1 file changed, 34 insertions(+), 15 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index 10ce51a244..d2d3839dec 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -67,13 +67,28 @@ typedef struct BlockCopyCallState {
typedef struct BlockCopyTask {
AioTask task;
+ /*
+ * IN parameters. Initialized in block_copy_task_create()
+ * and never changed.
+ */
BlockCopyState *s;
BlockCopyCallState *call_state;
int64_t offset;
- int64_t bytes;
+ int64_t bytes; /* only re-set in task_shrink, before running the task */
+
+ /*
+ * "local" parameter: used only to communicate between
+ * the caller (block_copy_dirty_clusters) and the aiotask
+ * coroutine running block_copy_task_entry
+ */
I a bit don't follow. bytes and offset are used for the same thing.. and bytes
modified the same way, before running task, as you write in a comment. Why
zeroes is in a separate group?
bool zeroes;
- QLIST_ENTRY(BlockCopyTask) list;
+
+ /* State */
CoQueue wait_queue; /* coroutines blocked on this task */
+
+ /* To reference all call states from BlockCopyState */
+ QLIST_ENTRY(BlockCopyTask) list;
+
extra new-line?
} BlockCopyTask;
static int64_t task_end(BlockCopyTask *task)
@@ -89,15 +104,25 @@ typedef struct BlockCopyState {
*/
BdrvChild *source;
BdrvChild *target;
- BdrvDirtyBitmap *copy_bitmap;
+
you add an empty line before group, it looks good
+ /* State */
int64_t in_flight_bytes;
- int64_t cluster_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;
but not here..
+ /* State fields that use a thread-safe API */
+ BdrvDirtyBitmap *copy_bitmap;
+ ProgressMeter *progress;
+ SharedResource *mem;
+ RateLimit rate_limit;
+ /*
+ * IN parameters. Initialized in block_copy_state_new()
+ * and never changed.
+ */
+ int64_t cluster_size;
+ int64_t max_transfer;
+ uint64_t len;
BdrvRequestFlags write_flags;
/*
@@ -115,12 +140,6 @@ typedef struct BlockCopyState {
* block_copy_reset_unallocated() every time it does.
*/
bool skip_unallocated;
-
- ProgressMeter *progress;
-
- SharedResource *mem;
-
- RateLimit rate_limit;
} BlockCopyState;
static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
@@ -176,9 +195,9 @@ static inline int64_t block_copy_chunk_size(BlockCopyState
*s)
* Search for the first dirty area in offset/bytes range and create task at
* the beginning of it.
*/
-static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
- BlockCopyCallState *call_state,
- int64_t offset, int64_t bytes)
+static coroutine_fn BlockCopyTask *block_copy_task_create(BlockCopyState *s,
+ BlockCopyCallState *call_state,
+ int64_t offset, int64_t bytes)
{
BlockCopyTask *task;
int64_t max_chunk = block_copy_chunk_size(s);
--
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
- Re: [PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask and BlockCopyState types and functions,
Vladimir Sementsov-Ogievskiy <=
[PATCH v2 3/7] block-copy: move progress_set_remaining in block_copy_task_end, Emanuele Giuseppe Esposito, 2021/05/18
[PATCH v2 4/7] block-copy: add a CoMutex to the BlockCopyTask list, Emanuele Giuseppe Esposito, 2021/05/18
Re: [PATCH v2 4/7] block-copy: add a CoMutex to the BlockCopyTask list, Stefan Hajnoczi, 2021/05/27