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

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 19/06/2021 20:53, Vladimir Sementsov-Ogievskiy wrote:
> 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.

In patch 5 I added a comment above @ret and @error_is_read:
/* Fields protected by lock in BlockCopyState */

I can add your explanation too.

> 
> 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.
> 
Yes but I think especially the IN vs State division helps a lot to 
understand what needs a lock and what doesn't.

Emanuele