qemu-block
[Top][All Lists]
Advanced

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

Re: [PATCH v3 4/5] block/block-copy: refactor task creation


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v3 4/5] block/block-copy: refactor task creation
Date: Wed, 29 Apr 2020 15:04:20 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

29.04.2020 14:56, Max Reitz wrote:
On 29.04.20 13:54, Vladimir Sementsov-Ogievskiy wrote:
29.04.2020 14:38, Max Reitz wrote:
On 29.04.20 08:10, Vladimir Sementsov-Ogievskiy wrote:
Instead of just relying on the comment "Called only on full-dirty
region" in block_copy_task_create() let's move initial dirty area
search directly to block_copy_task_create(). Let's also use effective
bdrv_dirty_bitmap_next_dirty_area instead of looping through all
non-dirty clusters.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
   block/block-copy.c | 78 ++++++++++++++++++++++++++--------------------
   1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 35ff9cc3ef..5cf032c4d8 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c

[...]

@@ -106,17 +111,27 @@ static bool coroutine_fn
block_copy_wait_one(BlockCopyState *s, int64_t offset,
       return true;
   }
   -/* Called only on full-dirty region */
+/*
+ * Search for the first dirty area in offset/bytes range and create
task at
+ * the beginning of it.

Oh, that’s even better.

+ */
   static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
                                                int64_t offset,
int64_t bytes)
   {
-    BlockCopyTask *task = g_new(BlockCopyTask, 1);
+    if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
+                                           offset, offset + bytes,
+                                           s->copy_size, &offset,
&bytes))
+    {
+        return NULL;
+    }
   +    /* region is dirty, so no existent tasks possible in it */
       assert(!find_conflicting_task(s, offset, bytes));
         bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
       s->in_flight_bytes += bytes;
   +    BlockCopyTask *task = g_new(BlockCopyTask, 1);

This should be declared at the top of the function.


I just thought, why not to try another style? Are you against?
Requirement to declare variables at start of block is obsolete, isn't it?

Oh, it absolutely is and personally I’m absolutely not against it, but
CODING_STYLE says:

Mixed declarations (interleaving statements and declarations within
blocks) are generally not allowed; declarations should be at the beginning
of blocks.


Oh, missed (or forget). Let's fix it? :) Not in these series, of course. OK, 
I'll fix the patch.



--
Best regards,
Vladimir



reply via email to

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