Re: [RFC PATCH 1/3] block-copy: improve documentation for BlockCopyTask type and functions

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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