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

2021-05-20 Thread 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 


[...]


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

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

2021-05-20 Thread Vladimir Sementsov-Ogievskiy 

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 
---
  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);




--
Best regards,
Vladimir