| Message ID | 20210420100416.30713-2-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block-copy: lock tasks and calls list | expand |
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.. > 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. > AioTask task; > - > BlockCopyState *s; > BlockCopyCallState *call_state; > + > + /* State */ > int64_t offset; I think, offset is never changed after block_copy_task_create().. > 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 > + 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. block_copy_task_create() may be called from any context, both coroutine and non-coroutine. So, it shouldn't have this mark.
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? > >> 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. > >> 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. > 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
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 >
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; bool error_is_read; } BlockCopyCallState; typedef struct BlockCopyTask { + /* IN parameters. Initialized in block_copy_task_create() + * and never changed. + */ AioTask task; - BlockCopyState *s; BlockCopyCallState *call_state; + + /* State */ int64_t 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 */ + 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) {
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(-)