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

[Emanuele Giuseppe Esposito]

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