[Top][All Lists]

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

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

From: Emanuele Giuseppe Esposito
Subject: Re: [RFC PATCH 1/3] block-copy: improve documentation for BlockCopyTask type and functions
Date: Tue, 20 Apr 2021 14:51:03 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

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,

reply via email to

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