Re: [Qemu-block] [Qemu-devel] [QEMU-2.8] Source QEMU crashes with: "bdrv_co_pwritev: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed"

2017-04-25 Thread Hailiang Zhang 

On 2017/4/24 15:59, Kashyap Chamarthy wrote:

On Sat, Apr 22, 2017 at 05:23:49PM +0800, Hailiang Zhang wrote:

Hi,

Hi Hailiang,


I think the bellow patch can fix your problme.
[PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH
https://patchwork.kernel.org/patch/9591885/

Hmm, the above patch ("qmp-cont: invalidate on RUN_STATE_PRELAUNCH") is
not merged in Git, as it's stalled on design discussion between Kevin
Wolf and Vladimir.

And the below patch, from you, seems to be not submitted upstream (2.8
stable tree, perhaps).  Do you intend to do so?


Er, since this patch does the same thing with the above patch, I'm not sure if 
i should
send this patch ...


Actually, we encounter the same problem in our test, we fix it with the follow 
patch:

  From 0e4d6d706afd9909b5fd71536b45c58af60892f8 Mon Sep 17 00:00:00 2001
  From: zhanghailiang
  Date: Tue, 21 Mar 2017 09:44:36 +0800
  Subject: [PATCH] migration: Re-activate blocks whenever migration been
   cancelled

  In commit 1d2acc3162d9c7772510c973f446353fbdd1f9a8, we try to fix the bug
  'bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed'
  which occured in migration cancelling process.

  But it seems that we didn't cover all the cases, we caught such a case 
which
  slipped from the old fixup in our test: if libvirtd cancelled the 
migration
  process for a shutting down VM, it will send 'system_reset' command first,
  and then 'cont' command behind, after VM resumes to run, it will trigger 
the above
  error reports, because we didn't regain the control of blocks for VM.

  Signed-off-by: zhanghailiang
  Signed-off-by: Hongyang Yang
  ---
   block.c   | 12 +++-
   include/block/block.h |  1 +
   include/migration/migration.h |  3 ---
   migration/migration.c |  7 +--
   qmp.c |  4 +---
   5 files changed, 14 insertions(+), 13 deletions(-)

[...]

Re: [Qemu-block] [Qemu-devel] [QEMU-2.8] Source QEMU crashes with: "bdrv_co_pwritev: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed"

2017-04-24 Thread Kashyap Chamarthy 
On Sat, Apr 22, 2017 at 05:23:49PM +0800, Hailiang Zhang wrote:
> Hi,

Hi Hailiang,

> I think the bellow patch can fix your problme.
> [PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH
> https://patchwork.kernel.org/patch/9591885/

Hmm, the above patch ("qmp-cont: invalidate on RUN_STATE_PRELAUNCH") is
not merged in Git, as it's stalled on design discussion between Kevin
Wolf and Vladimir.

And the below patch, from you, seems to be not submitted upstream (2.8
stable tree, perhaps).  Do you intend to do so?

> Actually, we encounter the same problem in our test, we fix it with the 
> follow patch:
> 
>  From 0e4d6d706afd9909b5fd71536b45c58af60892f8 Mon Sep 17 00:00:00 2001
>  From: zhanghailiang
>  Date: Tue, 21 Mar 2017 09:44:36 +0800
>  Subject: [PATCH] migration: Re-activate blocks whenever migration been
>   cancelled
> 
>  In commit 1d2acc3162d9c7772510c973f446353fbdd1f9a8, we try to fix the bug
>  'bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed'
>  which occured in migration cancelling process.
> 
>  But it seems that we didn't cover all the cases, we caught such a case 
> which
>  slipped from the old fixup in our test: if libvirtd cancelled the 
> migration
>  process for a shutting down VM, it will send 'system_reset' command 
> first,
>  and then 'cont' command behind, after VM resumes to run, it will trigger 
> the above
>  error reports, because we didn't regain the control of blocks for VM.
> 
>  Signed-off-by: zhanghailiang
>  Signed-off-by: Hongyang Yang
>  ---
>   block.c   | 12 +++-
>   include/block/block.h |  1 +
>   include/migration/migration.h |  3 ---
>   migration/migration.c |  7 +--
>   qmp.c |  4 +---
>   5 files changed, 14 insertions(+), 13 deletions(-)

[...]

-- 
/kashyap

Re: [Qemu-block] [Qemu-devel] [QEMU-2.8] Source QEMU crashes with: "bdrv_co_pwritev: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed"

2017-04-22 Thread Hailiang Zhang 

Hi,

I think the bellow patch can fix your problme.
[PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH
https://patchwork.kernel.org/patch/9591885/

Actually, we encounter the same problem in our test, we fix it with the follow 
patch:

 From 0e4d6d706afd9909b5fd71536b45c58af60892f8 Mon Sep 17 00:00:00 2001
 From: zhanghailiang
 Date: Tue, 21 Mar 2017 09:44:36 +0800
 Subject: [PATCH] migration: Re-activate blocks whenever migration been
  cancelled

 In commit 1d2acc3162d9c7772510c973f446353fbdd1f9a8, we try to fix the bug
 'bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed'
 which occured in migration cancelling process.

 But it seems that we didn't cover all the cases, we caught such a case 
which
 slipped from the old fixup in our test: if libvirtd cancelled the migration
 process for a shutting down VM, it will send 'system_reset' command first,
 and then 'cont' command behind, after VM resumes to run, it will trigger 
the above
 error reports, because we didn't regain the control of blocks for VM.

 Signed-off-by: zhanghailiang
 Signed-off-by: Hongyang Yang
 ---
  block.c   | 12 +++-
  include/block/block.h |  1 +
  include/migration/migration.h |  3 ---
  migration/migration.c |  7 +--
  qmp.c |  4 +---
  5 files changed, 14 insertions(+), 13 deletions(-)

 diff --git a/block.c b/block.c
 index 6e906ec..c2555b0 100644
 --- a/block.c
 +++ b/block.c
 @@ -3875,6 +3875,13 @@ void bdrv_invalidate_cache(BlockDriverState *bs, 
Error **errp)
  }
  }

 +static bool is_inactivated;
 +
 +bool bdrv_is_inactivated(void)
 +{
 +return is_inactivated;
 +}
 +
  void bdrv_invalidate_cache_all(Error **errp)
  {
  BlockDriverState *bs;
 @@ -3892,6 +3899,7 @@ void bdrv_invalidate_cache_all(Error **errp)
  return;
  }
  }
 +is_inactivated = false;
  }

  static int bdrv_inactivate_recurse(BlockDriverState *bs,
 @@ -3948,7 +3956,9 @@ out:
  for (bs = bdrv_first(); bs; bs = bdrv_next()) {
  aio_context_release(bdrv_get_aio_context(bs));
  }
 -
 +if (!ret) {
 +is_inactivated = true;
 +}
  return ret;
  }

 diff --git a/include/block/block.h b/include/block/block.h
 index 5149260..f77b57f 100644
 --- a/include/block/block.h
 +++ b/include/block/block.h
 @@ -365,6 +365,7 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void 
*buf);
  void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
  void bdrv_invalidate_cache_all(Error **errp);
  int bdrv_inactivate_all(void);
 +bool bdrv_is_inactivated(void);

  /* Ensure contents are flushed to disk.  */
  int bdrv_flush(BlockDriverState *bs);
 diff --git a/include/migration/migration.h b/include/migration/migration.h
 index 5720c88..a9a2071 100644
 --- a/include/migration/migration.h
 +++ b/include/migration/migration.h
 @@ -183,9 +183,6 @@ struct MigrationState
  /* Flag set once the migration thread is running (and needs joining) 
*/
  bool migration_thread_running;

 -/* Flag set once the migration thread called bdrv_inactivate_all */
 -bool block_inactive;
 -
  /* Queue of outstanding page requests from the destination */
  QemuMutex src_page_req_mutex;
  QSIMPLEQ_HEAD(src_page_requests, MigrationSrcPageRequest) 
src_page_requests;
 diff --git a/migration/migration.c b/migration/migration.c
 index 54060f7..7c3d593 100644
 --- a/migration/migration.c
 +++ b/migration/migration.c
 @@ -1015,14 +1015,12 @@ static void migrate_fd_cancel(MigrationState *s)
  if (s->state == MIGRATION_STATUS_CANCELLING && f) {
  qemu_file_shutdown(f);
  }
 -if (s->state == MIGRATION_STATUS_CANCELLING && s->block_inactive) {
 +if (bdrv_is_inactivated()) {
  Error *local_err = NULL;

  bdrv_invalidate_cache_all(_err);
  if (local_err) {
  error_report_err(local_err);
 -} else {
 -s->block_inactive = false;
  }
  }
  }
 @@ -1824,7 +1822,6 @@ static void migration_completion(MigrationState *s, 
int current_active_state,
  if (ret >= 0) {
  qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
  qemu_savevm_state_complete_precopy(s->to_dst_file, false);
 -s->block_inactive = true;
  }
  }
  qemu_mutex_unlock_iothread();
 @@ -1879,8 +1876,6 @@ fail_invalidate:
  bdrv_invalidate_cache_all(_err);