qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask an


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask and BlockCopyState types and functions
Date: Thu, 20 May 2021 18:00:32 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.2

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



--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]