[Qemu-devel] [PATCH for-2.10 0/4] block: Fix non-shared storage migration

[Qemu-devel] [PATCH for-2.10 0/4] block: Fix non-shared storage migration

[Fam Zheng]

This fixes the issue reported as https://bugs.launchpad.net/bugs/1711602

Fam Zheng (3):
  block-backend: Refactor inactivate check
  block-backend: Allow more "can inactivate" cases
  mirror: Mark target BB as "force allow inactivate"

Stefan Hajnoczi (1):
  block: Update open_flags after ->inactivate() callback

 block.c                        |  7 +++----
 block/block-backend.c          | 30 +++++++++++++++++++++++++-----
 block/mirror.c                 | 14 ++++++++++++--
 include/sysemu/block-backend.h |  1 +
 4 files changed, 41 insertions(+), 11 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH for-2.10 1/4] block-backend: Refactor inactivate check

[Fam Zheng]

The logic will be fixed (extended), move it to a separete function.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/block-backend.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index e9798e897d..a3984d2bec 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -192,6 +192,19 @@ static void blk_root_activate(BdrvChild *child, Error **errp)
     }
 }
 
+static bool blk_can_inactivate(BlockBackend *blk)
+{
+    /* Only inactivate BlockBackends for guest devices (which are inactive at
+     * this point because the VM is stopped) and unattached monitor-owned
+     * BlockBackends. If there is still any other user like a block job, then
+     * we simply can't inactivate the image. */
+    if (blk->dev || blk_name(blk)[0]) {
+        return true;
+    }
+
+    return false;
+}
+
 static int blk_root_inactivate(BdrvChild *child)
 {
     BlockBackend *blk = child->opaque;
@@ -200,11 +213,7 @@ static int blk_root_inactivate(BdrvChild *child)
         return 0;
     }
 
-    /* Only inactivate BlockBackends for guest devices (which are inactive at
-     * this point because the VM is stopped) and unattached monitor-owned
-     * BlockBackends. If there is still any other user like a block job, then
-     * we simply can't inactivate the image. */
-    if (!blk->dev && !blk_name(blk)[0]) {
+    if (!blk_can_inactivate(blk)) {
         return -EPERM;
     }
 
-- 
2.13.5

[Qemu-devel] [PATCH for-2.10 2/4] block-backend: Allow more "can inactivate" cases

[Fam Zheng]

These two conditions corresponds to mirror job's source and target,
which need to be allowed as they are part of the non-shared storage
migration workflow: failing to inactivate either will result in a
failure during migration completion.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/block-backend.c          | 21 ++++++++++++++++-----
 include/sysemu/block-backend.h |  1 +
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index a3984d2bec..48f5356657 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -70,6 +70,7 @@ struct BlockBackend {
 
     int quiesce_counter;
     VMChangeStateEntry *vmsh;
+    bool force_allow_inactivate;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -192,17 +193,27 @@ static void blk_root_activate(BdrvChild *child, Error **errp)
     }
 }
 
+void blk_set_force_allow_inactivate(BlockBackend *blk)
+{
+    blk->force_allow_inactivate = true;
+}
+
 static bool blk_can_inactivate(BlockBackend *blk)
 {
-    /* Only inactivate BlockBackends for guest devices (which are inactive at
-     * this point because the VM is stopped) and unattached monitor-owned
-     * BlockBackends. If there is still any other user like a block job, then
-     * we simply can't inactivate the image. */
+    /* If it is a guest device, inactivate is ok. */
     if (blk->dev || blk_name(blk)[0]) {
         return true;
     }
 
-    return false;
+    /* Inactivating means no more write to the image can be done, even if it's
+     * guest invisible change. For block job BBs that satisfy this, we can just
+     * allow it.  This is the case for mirror job source, which is required by
+     * libvirt non-shared block migration. */
+    if (!(blk->perm & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED))) {
+        return true;
+    }
+
+    return blk->force_allow_inactivate;
 }
 
 static int blk_root_inactivate(BdrvChild *child)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 4a3730596b..aadc733daf 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -241,5 +241,6 @@ void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg);
 void blk_io_limits_disable(BlockBackend *blk);
 void blk_io_limits_enable(BlockBackend *blk, const char *group);
 void blk_io_limits_update_group(BlockBackend *blk, const char *group);
+void blk_set_force_allow_inactivate(BlockBackend *blk);
 
 #endif
-- 
2.13.5

[Qemu-devel] [PATCH for-2.10 3/4] mirror: Mark target BB as "force allow inactivate"

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index c9a6a3ca86..429751b9fe 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1134,6 +1134,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
                              const BlockJobDriver *driver,
                              bool is_none_mode, BlockDriverState *base,
                              bool auto_complete, const char *filter_node_name,
+                             bool is_mirror,
                              Error **errp)
 {
     MirrorBlockJob *s;
@@ -1222,6 +1223,15 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     if (ret < 0) {
         goto fail;
     }
+    if (is_mirror) {
+        /* XXX: Mirror target could be a NBD server of target QEMU in the case
+         * of non-shared block migration. To allow migration completion, we
+         * have to allow "inactivate" of the target BB.  When that happens, we
+         * know the job is drained, and the vcpus are stopped, so no write
+         * operation will be performed. Block layer already has assertions to
+         * ensure that. */
+        blk_set_force_allow_inactivate(s->target);
+    }
 
     s->replaces = g_strdup(replaces);
     s->on_source_error = on_source_error;
@@ -1306,7 +1316,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
                      speed, granularity, buf_size, backing_mode,
                      on_source_error, on_target_error, unmap, NULL, NULL,
                      &mirror_job_driver, is_none_mode, base, false,
-                     filter_node_name, errp);
+                     filter_node_name, true, errp);
 }
 
 void commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -1329,7 +1339,7 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
                      MIRROR_LEAVE_BACKING_CHAIN,
                      on_error, on_error, true, cb, opaque,
                      &commit_active_job_driver, false, base, auto_complete,
-                     filter_node_name, &local_err);
+                     filter_node_name, false, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto error_restore_flags;
-- 
2.13.5

[Qemu-devel] [PATCH for-2.10 4/4] block: Update open_flags after ->inactivate() callback

[Fam Zheng]

From: Stefan Hajnoczi <stefanha@redhat.com>

In the ->inactivate() callbacks, permissions are updated, which
typically involves a recursive check of the whole graph. Setting
BDRV_O_INACTIVE right before doing that creates a state that
bdrv_is_writable() returns false, which causes permission update
failure.

Reorder them so the flag is updated after calling the function. Note
that this doesn't break the assert in bdrv_child_cb_inactivate() because
for any specific BDS, we still update its flags first before calling
->inactivate() on it one level deeper in the recursion.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 3615a6809e..3308814bba 100644
--- a/block.c
+++ b/block.c
@@ -4085,21 +4085,20 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
         }
     }
 
-    if (setting_flag) {
+    if (setting_flag && !(bs->open_flags & BDRV_O_INACTIVE)) {
         uint64_t perm, shared_perm;
 
-        bs->open_flags |= BDRV_O_INACTIVE;
-
         QLIST_FOREACH(parent, &bs->parents, next_parent) {
             if (parent->role->inactivate) {
                 ret = parent->role->inactivate(parent);
                 if (ret < 0) {
-                    bs->open_flags &= ~BDRV_O_INACTIVE;
                     return ret;
                 }
             }
         }
 
+        bs->open_flags |= BDRV_O_INACTIVE;
+
         /* Update permissions, they may differ for inactive nodes */
         bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
         bdrv_check_perm(bs, perm, shared_perm, NULL, &error_abort);
-- 
2.13.5

Re: [Qemu-devel] [PATCH for-2.10 4/4] block: Update open_flags after ->inactivate() callback

[Stefan Hajnoczi]

On Wed, Aug 23, 2017 at 09:42:42PM +0800, Fam Zheng wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> In the ->inactivate() callbacks, permissions are updated, which
> typically involves a recursive check of the whole graph. Setting
> BDRV_O_INACTIVE right before doing that creates a state that
> bdrv_is_writable() returns false, which causes permission update
> failure.
> 
> Reorder them so the flag is updated after calling the function. Note
> that this doesn't break the assert in bdrv_child_cb_inactivate() because
> for any specific BDS, we still update its flags first before calling
> ->inactivate() on it one level deeper in the recursion.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH for-2.10 0/4] block: Fix non-shared storage migration

[Stefan Hajnoczi]

On Wed, Aug 23, 2017 at 09:42:38PM +0800, Fam Zheng wrote:
> This fixes the issue reported as https://bugs.launchpad.net/bugs/1711602
> 
> Fam Zheng (3):
>   block-backend: Refactor inactivate check
>   block-backend: Allow more "can inactivate" cases
>   mirror: Mark target BB as "force allow inactivate"
> 
> Stefan Hajnoczi (1):
>   block: Update open_flags after ->inactivate() callback
> 
>  block.c                        |  7 +++----
>  block/block-backend.c          | 30 +++++++++++++++++++++++++-----
>  block/mirror.c                 | 14 ++++++++++++--
>  include/sysemu/block-backend.h |  1 +
>  4 files changed, 41 insertions(+), 11 deletions(-)
> 
> -- 
> 2.13.5
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH for-2.10 0/4] block: Fix non-shared storage migration

[Dr. David Alan Gilbert]

* Fam Zheng (famz@redhat.com) wrote:
> This fixes the issue reported as https://bugs.launchpad.net/bugs/1711602

Yep, that fixes it in my test setup (1 of 1 passes!)

Dave
> Fam Zheng (3):
>   block-backend: Refactor inactivate check
>   block-backend: Allow more "can inactivate" cases
>   mirror: Mark target BB as "force allow inactivate"
> 
> Stefan Hajnoczi (1):
>   block: Update open_flags after ->inactivate() callback
> 
>  block.c                        |  7 +++----
>  block/block-backend.c          | 30 +++++++++++++++++++++++++-----
>  block/mirror.c                 | 14 ++++++++++++--
>  include/sysemu/block-backend.h |  1 +
>  4 files changed, 41 insertions(+), 11 deletions(-)
> 
> -- 
> 2.13.5
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-2.10 1/4] block-backend: Refactor inactivate check

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On 08/23/2017 08:42 AM, Fam Zheng wrote:
> The logic will be fixed (extended), move it to a separete function.

s/separete/separate/

[can fix on pull request]

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/block-backend.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 2/4] block-backend: Allow more "can inactivate" cases

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

On 08/23/2017 08:42 AM, Fam Zheng wrote:
> These two conditions corresponds to mirror job's source and target,

s/corresponds to/correspond to a/

[can touch up on pull request]

> which need to be allowed as they are part of the non-shared storage
> migration workflow: failing to inactivate either will result in a
> failure during migration completion.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/block-backend.c          | 21 ++++++++++++++++-----
>  include/sysemu/block-backend.h |  1 +
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
>  
> -    return false;
> +    /* Inactivating means no more write to the image can be done, even if it's

s/write/writes/

> +     * guest invisible change. For block job BBs that satisfy this, we can just

reads awkwardly. Maybe 'even if it's changes invisible to the guest'?
But I can leave your wording if I don't get confirmation.

> +     * allow it.  This is the case for mirror job source, which is required by
> +     * libvirt non-shared block migration. */
> +    if (!(blk->perm & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED))) {
> +        return true;
> +    }
> +
> +    return blk->force_allow_inactivate;
>  }

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 0/4] block: Fix non-shared storage migration

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 486 bytes --]

On 08/23/2017 09:29 AM, Dr. David Alan Gilbert wrote:
> * Fam Zheng (famz@redhat.com) wrote:
>> This fixes the issue reported as https://bugs.launchpad.net/bugs/1711602
> 
> Yep, that fixes it in my test setup (1 of 1 passes!)

As mentioned on IRC, I'll treat this as a Tested-by: tag on 4/4 in my
pull request.  Thanks for helping check things.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 0/4] block: Fix non-shared storage migration

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On 08/23/2017 08:42 AM, Fam Zheng wrote:
> This fixes the issue reported as https://bugs.launchpad.net/bugs/1711602
> 
> Fam Zheng (3):
>   block-backend: Refactor inactivate check
>   block-backend: Allow more "can inactivate" cases
>   mirror: Mark target BB as "force allow inactivate"
> 
> Stefan Hajnoczi (1):
>   block: Update open_flags after ->inactivate() callback

Placing on my queue for a 2.10-rc4 pull request

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 2/4] block-backend: Allow more "can inactivate" cases

[Fam Zheng]

On Wed, 08/23 09:36, Eric Blake wrote:
> On 08/23/2017 08:42 AM, Fam Zheng wrote:
> > These two conditions corresponds to mirror job's source and target,
> 
> s/corresponds to/correspond to a/
> 
> [can touch up on pull request]
> 
> > which need to be allowed as they are part of the non-shared storage
> > migration workflow: failing to inactivate either will result in a
> > failure during migration completion.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/block-backend.c          | 21 ++++++++++++++++-----
> >  include/sysemu/block-backend.h |  1 +
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> > 
> >  
> > -    return false;
> > +    /* Inactivating means no more write to the image can be done, even if it's
> 
> s/write/writes/
> 
> > +     * guest invisible change. For block job BBs that satisfy this, we can just
> 
> reads awkwardly. Maybe 'even if it's changes invisible to the guest'?
> But I can leave your wording if I don't get confirmation.

Not sure I understand the grammar of "it's changes" but yours must be better.
Feel free to update it. Thanks,

Fam

> 
> > +     * allow it.  This is the case for mirror job source, which is required by
> > +     * libvirt non-shared block migration. */
> > +    if (!(blk->perm & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED))) {
> > +        return true;
> > +    }
> > +
> > +    return blk->force_allow_inactivate;
> >  }
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 

Re: [Qemu-devel] [PATCH for-2.10 2/4] block-backend: Allow more "can inactivate" cases

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

On 08/23/2017 09:47 AM, Fam Zheng wrote:
> On Wed, 08/23 09:36, Eric Blake wrote:
>> On 08/23/2017 08:42 AM, Fam Zheng wrote:
>>> These two conditions corresponds to mirror job's source and target,
>>
>> s/corresponds to/correspond to a/
>>
>> [can touch up on pull request]
>>
>>> which need to be allowed as they are part of the non-shared storage
>>> migration workflow: failing to inactivate either will result in a
>>> failure during migration completion.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  block/block-backend.c          | 21 ++++++++++++++++-----
>>>  include/sysemu/block-backend.h |  1 +
>>>  2 files changed, 17 insertions(+), 5 deletions(-)
>>>
>>>  
>>> -    return false;
>>> +    /* Inactivating means no more write to the image can be done, even if it's
>>
>> s/write/writes/
>>
>>> +     * guest invisible change. For block job BBs that satisfy this, we can just
>>
>> reads awkwardly. Maybe 'even if it's changes invisible to the guest'?
>> But I can leave your wording if I don't get confirmation.
> 
> Not sure I understand the grammar of "it's changes" but yours must be better.
> Feel free to update it. Thanks,

If it's confusing, we can still do better.  I settled on:

no more writes to the image can be done, even if those writes would be
changes invisible to the guest.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]