[Qemu-devel] [PATCH v3 0/2] block: allow flush on devices with open tray

[Qemu-devel] [PATCH v3 0/2] block: allow flush on devices with open tray

[John Snow]

One more try.

The move to blk_flush altered the behavior of migration and flushing
nodes that are not reachable via the guest, but are still reachable
via QEMU and may or may not need to be flushed.

This is likely the simplest solution for now until we nail down our
policy a bit more.

This is intended for 2.6.2 and/or 2.7.1, to fix problems with libvirt
et al being unable to migrate QEMU when the CDROM tray is open.

v3:
 Trying to take a hint from Kevin, reinstating bdrv_flush_all.
 If it's not what we want, we can try moving back to v2,
 acknowledging that a nicer solution in the future:
 (A) Can skip flushing on devices that just don't need it, and
 (B) Optionally institutes some sort of flush-on-eject policy.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch atapi-tray-migfix
https://github.com/jnsnow/qemu/tree/atapi-tray-migfix

This version is tagged atapi-tray-migfix-v3:
https://github.com/jnsnow/qemu/releases/tag/atapi-tray-migfix-v3

John Snow (2):
  block: reintroduce bdrv_flush_all
  qemu: use bdrv_flush_all for vm_stop et al

 block/io.c            | 25 +++++++++++++++++++++++++
 cpus.c                |  4 ++--
 include/block/block.h |  1 +
 3 files changed, 28 insertions(+), 2 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/2] block: reintroduce bdrv_flush_all

[John Snow]

Commit fe1a9cbc moved the flush_all routine from the bdrv layer to the
block-backend layer. In doing so, however, the semantics of the routine
changed slightly such that flush_all now used blk_flush instead of
bdrv_flush.

blk_flush can fail if the attached device model reports that it is not
"available," (i.e. the tray is open.) This changed the semantics of
flush_all such that it can now fail for e.g. open CDROM drives.

Reintroduce bdrv_flush_all to regain the old semantics without having to
alter the behavior of blk_flush or blk_flush_all, which are already
'doing the right thing.'

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/io.c            | 25 +++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/block/io.c b/block/io.c
index fdf7080..913df51 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1619,6 +1619,31 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
                            BDRV_REQ_ZERO_WRITE | flags);
 }
 
+/*
+ * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or not.
+ */
+int bdrv_flush_all(void)
+{
+    BdrvNextIterator it;
+    BlockDriverState *bs = NULL;
+    int result = 0;
+
+    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+        int ret;
+
+        aio_context_acquire(aio_context);
+        ret = bdrv_flush(bs);
+        if (ret < 0 && !result) {
+            result = ret;
+        }
+        aio_context_release(aio_context);
+   }
+
+    return result;
+}
+
+
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
diff --git a/include/block/block.h b/include/block/block.h
index ffecebf..5e81281 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -332,6 +332,7 @@ int bdrv_inactivate_all(void);
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
+int bdrv_flush_all(void);
 void bdrv_close_all(void);
 void bdrv_drain(BlockDriverState *bs);
 void coroutine_fn bdrv_co_drain(BlockDriverState *bs);
-- 
2.7.4

[Qemu-devel] [PATCH v3 2/2] qemu: use bdrv_flush_all for vm_stop et al

[John Snow]

Bypass the usual check to see if we are "allowed" to flush via the
block model, and manually flush the BDS nodes themselves instead.

This allows us to do things like migrate when we have a device with
an open tray, but has a node that may need to be flushed.

Specifically, this allows us to migrate when we have a CDROM with
an open tray.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 cpus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index e39ccb7..96d9352 100644
--- a/cpus.c
+++ b/cpus.c
@@ -751,7 +751,7 @@ static int do_vm_stop(RunState state)
     }
 
     bdrv_drain_all();
-    ret = blk_flush_all();
+    ret = bdrv_flush_all();
 
     return ret;
 }
@@ -1494,7 +1494,7 @@ int vm_stop_force_state(RunState state)
         bdrv_drain_all();
         /* Make sure to return an error if the flush in a previous vm_stop()
          * failed. */
-        return blk_flush_all();
+        return bdrv_flush_all();
     }
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 2/2] qemu: use bdrv_flush_all for vm_stop et al

[John Snow]

On 09/15/2016 07:42 PM, John Snow wrote:
> Bypass the usual check to see if we are "allowed" to flush via the
> block model, and manually flush the BDS nodes themselves instead.
>
> This allows us to do things like migrate when we have a device with
> an open tray, but has a node that may need to be flushed.
>
> Specifically, this allows us to migrate when we have a CDROM with
> an open tray.
>

How about:

Reimplement bdrv_flush_all for vm_stop. In contrast to blk_flush_all,
bdrv_flush_all does not have device model restrictions. This allows
us to flush and halt unconditionally without error.

This allows us to do things like migrate when we have a device with
an open tray, but has a node that may need to be flushed.

Specifically, this allows us to migrate when we have a CDROM with
an open tray.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  cpus.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index e39ccb7..96d9352 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -751,7 +751,7 @@ static int do_vm_stop(RunState state)
>      }
>
>      bdrv_drain_all();
> -    ret = blk_flush_all();
> +    ret = bdrv_flush_all();
>
>      return ret;
>  }
> @@ -1494,7 +1494,7 @@ int vm_stop_force_state(RunState state)
>          bdrv_drain_all();
>          /* Make sure to return an error if the flush in a previous vm_stop()
>           * failed. */
> -        return blk_flush_all();
> +        return bdrv_flush_all();
>      }
>  }
>
>

Re: [Qemu-devel] [PATCH v3 2/2] qemu: use bdrv_flush_all for vm_stop et al

[Kevin Wolf]

Am 16.09.2016 um 18:17 hat John Snow geschrieben:
> 
> 
> On 09/15/2016 07:42 PM, John Snow wrote:
> >Bypass the usual check to see if we are "allowed" to flush via the
> >block model, and manually flush the BDS nodes themselves instead.
> >
> >This allows us to do things like migrate when we have a device with
> >an open tray, but has a node that may need to be flushed.
> >
> >Specifically, this allows us to migrate when we have a CDROM with
> >an open tray.
> >
> 
> How about:
> 
> Reimplement bdrv_flush_all for vm_stop. In contrast to blk_flush_all,
> bdrv_flush_all does not have device model restrictions. This allows
> us to flush and halt unconditionally without error.
> 
> This allows us to do things like migrate when we have a device with
> an open tray, but has a node that may need to be flushed

I'd add:

    , or nodes that aren't currently attached to any device and need to
    be flushed.

> Specifically, this allows us to migrate when we have a CDROM with
> an open tray.

Looks good otherwise.

Kevin

Re: [Qemu-devel] [PATCH v3 0/2] block: allow flush on devices with open tray

[Paolo Bonzini]

On 16/09/2016 01:42, John Snow wrote:
> One more try.
> 
> The move to blk_flush altered the behavior of migration and flushing
> nodes that are not reachable via the guest, but are still reachable
> via QEMU and may or may not need to be flushed.
> 
> This is likely the simplest solution for now until we nail down our
> policy a bit more.
> 
> This is intended for 2.6.2 and/or 2.7.1, to fix problems with libvirt
> et al being unable to migrate QEMU when the CDROM tray is open.
> 
> v3:
>  Trying to take a hint from Kevin, reinstating bdrv_flush_all.
>  If it's not what we want, we can try moving back to v2,
>  acknowledging that a nicer solution in the future:
>  (A) Can skip flushing on devices that just don't need it, and
>  (B) Optionally institutes some sort of flush-on-eject policy.

If you move flushing from platform_fixed_ioport_writew's blk_flush_all
to blk_flush calls in pci_piix3_xen_ide_unplug, you can get rid of
blk_flush_all completely.

Paolo

Re: [Qemu-devel] [PATCH v3 0/2] block: allow flush on devices with open tray

[John Snow]

On 09/15/2016 08:06 PM, Paolo Bonzini wrote:
>
>
> On 16/09/2016 01:42, John Snow wrote:
>> One more try.
>>
>> The move to blk_flush altered the behavior of migration and flushing
>> nodes that are not reachable via the guest, but are still reachable
>> via QEMU and may or may not need to be flushed.
>>
>> This is likely the simplest solution for now until we nail down our
>> policy a bit more.
>>
>> This is intended for 2.6.2 and/or 2.7.1, to fix problems with libvirt
>> et al being unable to migrate QEMU when the CDROM tray is open.
>>
>> v3:
>>  Trying to take a hint from Kevin, reinstating bdrv_flush_all.
>>  If it's not what we want, we can try moving back to v2,
>>  acknowledging that a nicer solution in the future:
>>  (A) Can skip flushing on devices that just don't need it, and
>>  (B) Optionally institutes some sort of flush-on-eject policy.
>
> If you move flushing from platform_fixed_ioport_writew's blk_flush_all
> to blk_flush calls in pci_piix3_xen_ide_unplug, you can get rid of
> blk_flush_all completely.
>
> Paolo
>

I assume you mean in addition to re-adding bdrv_flush_all for the sake 
of vm_stop, we can also get rid of blk_flush_all from xen and avoid the 
duplication?

I can follow up with that if Max and Kevin agree to what I've already 
posted.

--js

Re: [Qemu-devel] [PATCH v3 0/2] block: allow flush on devices with open tray

[Paolo Bonzini]

On 16/09/2016 02:09, John Snow wrote:
> 
> 
> On 09/15/2016 08:06 PM, Paolo Bonzini wrote:
>>
>>
>> On 16/09/2016 01:42, John Snow wrote:
>>> One more try.
>>>
>>> The move to blk_flush altered the behavior of migration and flushing
>>> nodes that are not reachable via the guest, but are still reachable
>>> via QEMU and may or may not need to be flushed.
>>>
>>> This is likely the simplest solution for now until we nail down our
>>> policy a bit more.
>>>
>>> This is intended for 2.6.2 and/or 2.7.1, to fix problems with libvirt
>>> et al being unable to migrate QEMU when the CDROM tray is open.
>>>
>>> v3:
>>>  Trying to take a hint from Kevin, reinstating bdrv_flush_all.
>>>  If it's not what we want, we can try moving back to v2,
>>>  acknowledging that a nicer solution in the future:
>>>  (A) Can skip flushing on devices that just don't need it, and
>>>  (B) Optionally institutes some sort of flush-on-eject policy.
>>
>> If you move flushing from platform_fixed_ioport_writew's blk_flush_all
>> to blk_flush calls in pci_piix3_xen_ide_unplug, you can get rid of
>> blk_flush_all completely.
> 
> I assume you mean in addition to re-adding bdrv_flush_all for the sake
> of vm_stop, we can also get rid of blk_flush_all from xen and avoid the
> duplication?

Yep.  Devices really should not use the *_all() variants.

Paolo

Re: [Qemu-devel] [PATCH v3 0/2] block: allow flush on devices with open tray

[Kevin Wolf]

Am 16.09.2016 um 01:42 hat John Snow geschrieben:
> One more try.
> 
> The move to blk_flush altered the behavior of migration and flushing
> nodes that are not reachable via the guest, but are still reachable
> via QEMU and may or may not need to be flushed.
> 
> This is likely the simplest solution for now until we nail down our
> policy a bit more.
> 
> This is intended for 2.6.2 and/or 2.7.1, to fix problems with libvirt
> et al being unable to migrate QEMU when the CDROM tray is open.
> 
> v3:
>  Trying to take a hint from Kevin, reinstating bdrv_flush_all.
>  If it's not what we want, we can try moving back to v2,
>  acknowledging that a nicer solution in the future:
>  (A) Can skip flushing on devices that just don't need it, and
>  (B) Optionally institutes some sort of flush-on-eject policy.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

The only part that I'm not completely happy with are the commit messages
because there is a theme of "bypassing checks" in there. That's not
really what happens because the requests don't come from something
attached to the BlockBackends in the first place, and it also completely
ignores the case of BDSes that aren't currently attached to any BB and
are fixed by these changes as well.

Anyway, that's a minor problem as long as the code is right.

Kevin

Re: [Qemu-devel] [PATCH v3 0/2] block: allow flush on devices with open tray

[John Snow]

On 09/16/2016 04:33 AM, Kevin Wolf wrote:
> Am 16.09.2016 um 01:42 hat John Snow geschrieben:
>> One more try.
>>
>> The move to blk_flush altered the behavior of migration and flushing
>> nodes that are not reachable via the guest, but are still reachable
>> via QEMU and may or may not need to be flushed.
>>
>> This is likely the simplest solution for now until we nail down our
>> policy a bit more.
>>
>> This is intended for 2.6.2 and/or 2.7.1, to fix problems with libvirt
>> et al being unable to migrate QEMU when the CDROM tray is open.
>>
>> v3:
>>  Trying to take a hint from Kevin, reinstating bdrv_flush_all.
>>  If it's not what we want, we can try moving back to v2,
>>  acknowledging that a nicer solution in the future:
>>  (A) Can skip flushing on devices that just don't need it, and
>>  (B) Optionally institutes some sort of flush-on-eject policy.
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
> The only part that I'm not completely happy with are the commit messages
> because there is a theme of "bypassing checks" in there. That's not
> really what happens because the requests don't come from something
> attached to the BlockBackends in the first place, and it also completely
> ignores the case of BDSes that aren't currently attached to any BB and
> are fixed by these changes as well.
>

I see your point. I just meant to say that it bypasses the checks in 
contrast to the other interface, but yes, there's a reason we're 
removing the checks here.

> Anyway, that's a minor problem as long as the code is right.
>

Let's fix it up. I'll reply with a suggestion.

> Kevin
>

Thank you.