qemu-block
[Top][All Lists]
Advanced

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

Re: [PATCH v2 4/6] block/block-copy: move task size initial calculation


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 4/6] block/block-copy: move task size initial calculation to _task_create
Date: Tue, 28 Apr 2020 12:28:33 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

28.04.2020 11:52, Max Reitz wrote:
On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
Comment "Called only on full-dirty region" without corresponding
assertion is a very unsafe thing.

Not sure whether it’s that unsafe for a static function with a single
caller, but, well.

Adding assertion means call
bdrv_dirty_bitmap_next_zero twice. Instead, let's move
bdrv_dirty_bitmap_next_zero call to block_copy_task_create. It also
allows to drop cur_bytes variable which partly duplicate task->bytes.

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

diff --git a/block/block-copy.c b/block/block-copy.c
index 63d8468b27..dd406eb4bb 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -106,12 +106,23 @@ static bool coroutine_fn 
block_copy_wait_one(BlockCopyState *s, int64_t offset,
      return true;
  }
-/* Called only on full-dirty region */
  static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
                                               int64_t offset, int64_t bytes)

A bit of documentation on the new interface might be nice.  For one
thing, that @offset must be dirty, although there is an assertion that,
well, checks it.  (An assertion doesn’t really check anything, it rather
verifies a contract.  And violation is fatal.)

Still, good to document that.


For another, what the range [offset, offset + bytes) is; namely
basically the whole range of data that we might potentially copy, only
that the head must be dirty, but the tail may be clean.

Right


Which makes me think that the interface is maybe less than intuitive.
It would make more sense if we could just call this function on the
whole region and it would figure out whether @offset is dirty by itself
(and return NULL if it isn’t).

Hmm. Actually, I didn't touch the very inefficient "if 
(!bdrv_dirty_bitmap_get(s->copy_bitmap, offset)) { continue }" construct, because I 
was waiting for my series about refactoring bdrv_dirty_bitmap_next_dirty_area to merge in. 
But now it is merged, and I should refactor this thing. And may be you are right, that it 
may be done inside block_copy_task_create.


OTOH I suppose the interface how it is here is more useful for
task-ification.  But maybe that should be documented.

On the first glance, it should not really matter.

OK, I'll try to improve it somehow for v3

--
Best regards,
Vladimir



reply via email to

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