Re: [Qemu-block] [PATCH v4 13/13] block/mirror: Block device IO during mirror exit

2015-05-19 Thread Paolo Bonzini 
On 19/05/2015 13:49, Fam Zheng wrote:
 When block job mirror is finished, the source and target is synced. But
 we call bdrv_swap() later in main loop bh. If the guest write before
 that, target will not get the new data.

This is too late.  As a rule, the blocker must be established before
calling bdrv_drain, and removed on the next yield (in this case, before
the assignment to last_pause_ns).

Paolo

 Reported-by: Wen Congyang we...@cn.fujitsu.com
 Signed-off-by: Fam Zheng f...@redhat.com
 ---
  block/mirror.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)
 
 diff --git a/block/mirror.c b/block/mirror.c
 index 58f391a..d96403b 100644
 --- a/block/mirror.c
 +++ b/block/mirror.c
 @@ -320,6 +320,8 @@ static void mirror_drain(MirrorBlockJob *s)
  
  typedef struct {
  int ret;
 +/* Use to pause device IO during mirror exit */
 +Error *blocker;
  } MirrorExitData;
  
  static void mirror_exit(BlockJob *job, void *opaque)
 @@ -328,6 +330,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
  MirrorExitData *data = opaque;
  AioContext *replace_aio_context = NULL;
  
 +bdrv_op_unblock(s-common.bs, BLOCK_OP_TYPE_DEVICE_IO, data-blocker);
 +error_free(data-blocker);
  if (s-to_replace) {
  replace_aio_context = bdrv_get_aio_context(s-to_replace);
  aio_context_acquire(replace_aio_context);
 @@ -563,8 +567,10 @@ immediate_exit:
  bdrv_release_dirty_bitmap(bs, s-dirty_bitmap);
  bdrv_iostatus_disable(s-target);
  
 -data = g_malloc(sizeof(*data));
 +data = g_malloc0(sizeof(*data));
  data-ret = ret;
 +error_setg(data-blocker, mirror job exiting);
 +bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, data-blocker);
  block_job_defer_to_main_loop(s-common, mirror_exit, data);
  }

Re: [Qemu-block] [PATCH v4 13/13] block/mirror: Block device IO during mirror exit

2015-05-19 Thread Fam Zheng 
On Tue, 05/19 10:04, Paolo Bonzini wrote:
 On 19/05/2015 13:49, Fam Zheng wrote:
  When block job mirror is finished, the source and target is synced. But
  we call bdrv_swap() later in main loop bh. If the guest write before
  that, target will not get the new data.
 
 This is too late.  As a rule, the blocker must be established before
 calling bdrv_drain, and removed on the next yield (in this case, before
 the assignment to last_pause_ns).

I don't understand. If the blocker is removed before mirror_run returns,
wouldn't more device IO already hit source image by the time mirror_exit runs?

Fam

 
 Paolo
 
  Reported-by: Wen Congyang we...@cn.fujitsu.com
  Signed-off-by: Fam Zheng f...@redhat.com
  ---
   block/mirror.c | 8 +++-
   1 file changed, 7 insertions(+), 1 deletion(-)
  
  diff --git a/block/mirror.c b/block/mirror.c
  index 58f391a..d96403b 100644
  --- a/block/mirror.c
  +++ b/block/mirror.c
  @@ -320,6 +320,8 @@ static void mirror_drain(MirrorBlockJob *s)
   
   typedef struct {
   int ret;
  +/* Use to pause device IO during mirror exit */
  +Error *blocker;
   } MirrorExitData;
   
   static void mirror_exit(BlockJob *job, void *opaque)
  @@ -328,6 +330,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
   MirrorExitData *data = opaque;
   AioContext *replace_aio_context = NULL;
   
  +bdrv_op_unblock(s-common.bs, BLOCK_OP_TYPE_DEVICE_IO, data-blocker);
  +error_free(data-blocker);
   if (s-to_replace) {
   replace_aio_context = bdrv_get_aio_context(s-to_replace);
   aio_context_acquire(replace_aio_context);
  @@ -563,8 +567,10 @@ immediate_exit:
   bdrv_release_dirty_bitmap(bs, s-dirty_bitmap);
   bdrv_iostatus_disable(s-target);
   
  -data = g_malloc(sizeof(*data));
  +data = g_malloc0(sizeof(*data));
   data-ret = ret;
  +error_setg(data-blocker, mirror job exiting);
  +bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, data-blocker);
   block_job_defer_to_main_loop(s-common, mirror_exit, data);
   }

Re: [Qemu-block] [PATCH v4 13/13] block/mirror: Block device IO during mirror exit

2015-05-19 Thread Paolo Bonzini 


On 19/05/2015 18:48, Fam Zheng wrote:
  
  This is too late.  As a rule, the blocker must be established before
  calling bdrv_drain, and removed on the next yield (in this case, before
  the assignment to last_pause_ns).
 I don't understand. If the blocker is removed before mirror_run returns,
 wouldn't more device IO already hit source image by the time mirror_exit runs?

If you go to mirror_exit, you won't reach the assignment (so you have to
remove the blocker in mirror_exit too).

But if you don't go to mirror_exit because cnt != 0, you must remove the
blocker before the next I/O.

Paolo

[Qemu-block] [PATCH v4 13/13] block/mirror: Block device IO during mirror exit

2015-05-18 Thread Fam Zheng 
When block job mirror is finished, the source and target is synced. But
we call bdrv_swap() later in main loop bh. If the guest write before
that, target will not get the new data.

Reported-by: Wen Congyang we...@cn.fujitsu.com
Signed-off-by: Fam Zheng f...@redhat.com
---
 block/mirror.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 58f391a..d96403b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -320,6 +320,8 @@ static void mirror_drain(MirrorBlockJob *s)
 
 typedef struct {
 int ret;
+/* Use to pause device IO during mirror exit */
+Error *blocker;
 } MirrorExitData;
 
 static void mirror_exit(BlockJob *job, void *opaque)
@@ -328,6 +330,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
 MirrorExitData *data = opaque;
 AioContext *replace_aio_context = NULL;
 
+bdrv_op_unblock(s-common.bs, BLOCK_OP_TYPE_DEVICE_IO, data-blocker);
+error_free(data-blocker);
 if (s-to_replace) {
 replace_aio_context = bdrv_get_aio_context(s-to_replace);
 aio_context_acquire(replace_aio_context);
@@ -563,8 +567,10 @@ immediate_exit:
 bdrv_release_dirty_bitmap(bs, s-dirty_bitmap);
 bdrv_iostatus_disable(s-target);
 
-data = g_malloc(sizeof(*data));
+data = g_malloc0(sizeof(*data));
 data-ret = ret;
+error_setg(data-blocker, mirror job exiting);
+bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, data-blocker);
 block_job_defer_to_main_loop(s-common, mirror_exit, data);
 }
 
-- 
2.4.1