[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 1/3] block-copy: improve documentation for BlockCopyTask
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [RFC PATCH 1/3] block-copy: improve documentation for BlockCopyTask type and functions |
Date: |
Tue, 20 Apr 2021 15:03:08 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 |
20.04.2021 13:04, Emanuele Giuseppe Esposito wrote:
As done in BlockCopyCallState, categorize BlockCopyTask
in IN, State and OUT fields. This is just to understand
which field has to be protected with a lock.
Also add coroutine_fn attribute to block_copy_task_create,
because it is only usedn block_copy_dirty_clusters, a coroutine
function itself.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/block-copy.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index 39ae481c8b..03df50a354 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -48,25 +48,32 @@ typedef struct BlockCopyCallState {
QLIST_ENTRY(BlockCopyCallState) list;
/* State */
- int ret;
bool finished;
QemuCoSleepState *sleep_state;
bool cancelled;
/* OUT parameters */
+ int ret;
Hmm. Somehow, ret may be considered is part of state too.. But I don't really
care. Here it looks not bad. Will see how and what you are going protect by new
lock.
Note, that ret is concurently set in block_copy_task_entry..
bool error_is_read;
} BlockCopyCallState;
typedef struct BlockCopyTask {
+ /* IN parameters. Initialized in block_copy_task_create()
+ * and never changed.
+ */
It's wrong about task field, as it has "ret" inside.
AioTask task;
-
BlockCopyState *s;
BlockCopyCallState *call_state;
+
+ /* State */
int64_t offset;
I think, offset is never changed after block_copy_task_create()..
int64_t bytes;
bool zeroes;
- QLIST_ENTRY(BlockCopyTask) list;
CoQueue wait_queue; /* coroutines blocked on this task */
+
+ /* To reference all call states from BlockCopyTask */
Amm.. Actually,
To reference all tasks from BlockCopyState
+ QLIST_ENTRY(BlockCopyTask) list;
+
} BlockCopyTask;
static int64_t task_end(BlockCopyTask *task)
@@ -153,7 +160,7 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState
*s, int64_t offset,
* 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,
+static BlockCopyTask *coroutine_fn block_copy_task_create(BlockCopyState *s,
BlockCopyCallState *call_state,
int64_t offset, int64_t bytes)
{
We mark by "coroutine_fn" functions that can be called _only_ from coroutine
context. block_copy_task_create() may be called from any context, both coroutine and
non-coroutine. So, it shouldn't have this mark.
--
Best regards,
Vladimir