|
From: | Emanuele Giuseppe Esposito |
Subject: | Re: [RFC PATCH 1/3] block-copy: improve documentation for BlockCopyTask type and functions |
Date: | Tue, 20 Apr 2021 14:51:03 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 |
On 20/04/2021 14:03, Vladimir Sementsov-Ogievskiy wrote:
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..
It is set but as far as I understood it contains the result of the operation (thus OUT), correct?
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.
Not sure I understand what you mean here.
AioTask task; - BlockCopyState *s; BlockCopyCallState *call_state; + + /* State */ int64_t offset;I think, offset is never changed after block_copy_task_create()..
right, will revert that for offset
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
right, agree, thanks
In my opinion, block_copy_task_create is a static function and it's called only by another coroutine_fn, block_copy_dirty_clusters, so it matches what you just wrote above.+ 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.
Thank you, Emanuele
[Prev in Thread] | Current Thread | [Next in Thread] |