Re: [PATCH v4 3/6] block-copy: improve comments of BlockCopyTask and BlockCopyState types and functions

[Vladimir Sementsov-Ogievskiy]

14.06.2021 10:33, Emanuele Giuseppe Esposito wrote:
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -52,29 +52,35 @@ typedef struct BlockCopyCallState {
>       /* Coroutine where async block-copy is running */
>       Coroutine *co;
>   
> -    /* To reference all call states from BlockCopyState */
> -    QLIST_ENTRY(BlockCopyCallState) list;
> -
>       /* State */
> -    int ret;
>       bool finished;
> -    QemuCoSleep sleep;
> -    bool cancelled;
> +    QemuCoSleep sleep; /* TODO: protect API with a lock */
> +
> +    /* To reference all call states from BlockCopyState */
> +    QLIST_ENTRY(BlockCopyCallState) list;
>   
>       /* OUT parameters */
> +    bool cancelled;
>       bool error_is_read;
> +    int ret;

Hmm, about that. Is @ret an "OUT parameter"? Yes it is.

But someone may think, that out parameters doesn't need locking like "State" 
parameters (otherwise, what is the difference for the person who read these comments?). 
But that is wrong. And ret is modified under mutex for reason.

Actually, the full description of "ret" field usage may look as follows:

Set concurrently by tasks under mutex. Only set once by first failed task (and 
untouched if no task failed).
After finish (if call_state->finished is true) not modified anymore and may be 
read safely without mutex.

So, before finished, ret is a kind of "State" too: it is both read and written 
by tasks.

This shows to me that dividing fields into "IN", "State" and "OUT", doesn't 
really help here. In this series we use different policies of concurrent access to fields: some are accessed 
only under mutex, other has more complex usage scenario (like this @ret), some needs atomic access.

--
Best regards,
Vladimir