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

[Vladimir Sementsov-Ogievskiy]

20.04.2021 15:51, Emanuele Giuseppe Esposito wrote:
> On 20/04/2021 14:03, Vladimir Sementsov-Ogievskiy wrote:
> > 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..
>
> It is set but as far as I understood it contains the result of the operation 
> (thus OUT), correct?

Yes. I just mean, that ret should be protected too. If block_copy_task_entry() 
called concurently from different threads, we'll check-and-set ret concurently.

> > >       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.
> Not sure I understand what you mean here.

task.ret it not an "IN" parameter

> > >       AioTask task;
> > > -
> > >       BlockCopyState *s;
> > >       BlockCopyCallState *call_state;
> > > +
> > > +    /* State */
> > >       int64_t offset;
> >
> > I think, offset is never changed after block_copy_task_create()..
>
> right, will revert that for offset
> > >       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
>
> right, agree, thanks
> > > +    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. 
> In my opinion, block_copy_task_create is a static function and it's called only 
> by another coroutine_fn, block_copy_dirty_clusters, so it matches what you just 
> wrote above.

"coroutine_fn" is restriction. block_copy_task_create doesn't need this 
restriction. It may be safely called from non-coroutine context. So, no reason to add the 
restriction.

> > block_copy_task_create() may be called from any context, both coroutine and 
> > non-coroutine. So, it shouldn't have this mark.
>
> Thank you,
> Emanuele


--
Best regards,
Vladimir