Re: [PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask and BlockCopyState types and functions

[Emanuele Giuseppe Esposito]

On 20/05/2021 17:00, Vladimir Sementsov-Ogievskiy wrote:
> 18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:
> > As done in BlockCopyCallState, categorize BlockCopyTask
> > and BlockCopyState in IN, State and OUT fields.
> > This is just to understand which field has to be protected with a lock.
> >
> > BlockCopyTask .zeroes is a special case, because it is only initialized
> > and then read by the coroutine in block_copy_task_entry.
> >
> > Also set block_copy_task_create as coroutine_fn because:
> > 1) it is static and only invoked by coroutine functions
> > 2) next patches will introduce and use a CoMutex lock there
>
> this change is unrelated, why not to put it into commit, which adds use 
> of CoMutex in that function?

Ok I will move it in patch 4.

> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

[...]
> you add an empty line before group, it looks good
>
> > +    /* State */
> >       int64_t in_flight_bytes;
> > -    int64_t cluster_size;
> >       BlockCopyMethod method;
> > -    int64_t max_transfer;
> > -    uint64_t len;
> >       QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all 
> > block-copy calls */
> >       QLIST_HEAD(, BlockCopyCallState) calls;
>
> but not here..

Because these are still State fields, so in the same State group. It is 
a different sub-category.

> > +    /* State fields that use a thread-safe API */
> > +    BdrvDirtyBitmap *copy_bitmap;
> > +    ProgressMeter *progress;
> > +    SharedResource *mem;
> > +    RateLimit rate_limit;
> > +    /*
> > +     * IN parameters. Initialized in block_copy_state_new()
> > +     * and never changed.
> > +     */
> > +    int64_t cluster_size;
> > +    int64_t max_transfer;
> > +    uint64_t len;
> >       BdrvRequestFlags write_flags;
> >       /*

Thank you,
Emanuele