| Message ID | 20210518100757.31243-3-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block-copy: protect block-copy internal structures | expand |
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? > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/block-copy.c | 49 ++++++++++++++++++++++++++++++++-------------- > 1 file changed, 34 insertions(+), 15 deletions(-) > > diff --git a/block/block-copy.c b/block/block-copy.c > index 10ce51a244..d2d3839dec 100644 > --- a/block/block-copy.c > +++ b/block/block-copy.c > @@ -67,13 +67,28 @@ typedef struct BlockCopyCallState { > typedef struct BlockCopyTask { > AioTask task; > > + /* > + * IN parameters. Initialized in block_copy_task_create() > + * and never changed. > + */ > BlockCopyState *s; > BlockCopyCallState *call_state; > int64_t offset; > - int64_t bytes; > + int64_t bytes; /* only re-set in task_shrink, before running the task */ > + > + /* > + * "local" parameter: used only to communicate between > + * the caller (block_copy_dirty_clusters) and the aiotask > + * coroutine running block_copy_task_entry > + */ I a bit don't follow. bytes and offset are used for the same thing.. and bytes modified the same way, before running task, as you write in a comment. Why zeroes is in a separate group? > bool zeroes; > - QLIST_ENTRY(BlockCopyTask) list; > + > + /* State */ > CoQueue wait_queue; /* coroutines blocked on this task */ > + > + /* To reference all call states from BlockCopyState */ > + QLIST_ENTRY(BlockCopyTask) list; > + extra new-line? > } BlockCopyTask; > > static int64_t task_end(BlockCopyTask *task) > @@ -89,15 +104,25 @@ typedef struct BlockCopyState { > */ > BdrvChild *source; > BdrvChild *target; > - BdrvDirtyBitmap *copy_bitmap; > + 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.. > + /* 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; > > /* > @@ -115,12 +140,6 @@ typedef struct BlockCopyState { > * block_copy_reset_unallocated() every time it does. > */ > bool skip_unallocated; > - > - ProgressMeter *progress; > - > - SharedResource *mem; > - > - RateLimit rate_limit; > } BlockCopyState; > > static BlockCopyTask *find_conflicting_task(BlockCopyState *s, > @@ -176,9 +195,9 @@ static inline int64_t block_copy_chunk_size(BlockCopyState *s) > * 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, > - BlockCopyCallState *call_state, > - int64_t offset, int64_t bytes) > +static coroutine_fn BlockCopyTask *block_copy_task_create(BlockCopyState *s, > + BlockCopyCallState *call_state, > + int64_t offset, int64_t bytes) > { > BlockCopyTask *task; > int64_t max_chunk = block_copy_chunk_size(s); >
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
diff --git a/block/block-copy.c b/block/block-copy.c index 10ce51a244..d2d3839dec 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -67,13 +67,28 @@ typedef struct BlockCopyCallState { typedef struct BlockCopyTask { AioTask task; + /* + * IN parameters. Initialized in block_copy_task_create() + * and never changed. + */ BlockCopyState *s; BlockCopyCallState *call_state; int64_t offset; - int64_t bytes; + int64_t bytes; /* only re-set in task_shrink, before running the task */ + + /* + * "local" parameter: used only to communicate between + * the caller (block_copy_dirty_clusters) and the aiotask + * coroutine running block_copy_task_entry + */ bool zeroes; - QLIST_ENTRY(BlockCopyTask) list; + + /* State */ CoQueue wait_queue; /* coroutines blocked on this task */ + + /* To reference all call states from BlockCopyState */ + QLIST_ENTRY(BlockCopyTask) list; + } BlockCopyTask; static int64_t task_end(BlockCopyTask *task) @@ -89,15 +104,25 @@ typedef struct BlockCopyState { */ BdrvChild *source; BdrvChild *target; - BdrvDirtyBitmap *copy_bitmap; + + /* 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; + /* 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; /* @@ -115,12 +140,6 @@ typedef struct BlockCopyState { * block_copy_reset_unallocated() every time it does. */ bool skip_unallocated; - - ProgressMeter *progress; - - SharedResource *mem; - - RateLimit rate_limit; } BlockCopyState; static BlockCopyTask *find_conflicting_task(BlockCopyState *s, @@ -176,9 +195,9 @@ static inline int64_t block_copy_chunk_size(BlockCopyState *s) * 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, - BlockCopyCallState *call_state, - int64_t offset, int64_t bytes) +static coroutine_fn BlockCopyTask *block_copy_task_create(BlockCopyState *s, + BlockCopyCallState *call_state, + int64_t offset, int64_t bytes) { BlockCopyTask *task; int64_t max_chunk = block_copy_chunk_size(s);
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 Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/block-copy.c | 49 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 15 deletions(-)