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

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

[John Snow]

When I said "Final re-send," I was lying. Here's a v5.
The title is also a misnomer by now :)

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 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.

v5:
 Fix bracket spacing in patch 1. By one space. :(
 Added third patch to remove blk_flush_all.

v4:
 Commit message update.

v3:
 Reworking approach and reinstating bdrv_flush_all at Kevin's suggestion.

________________________________________________________________________________

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-v5:
https://github.com/jnsnow/qemu/releases/tag/atapi-tray-migfix-v5

John Snow (3):
  block: reintroduce bdrv_flush_all
  qemu: use bdrv_flush_all for vm_stop et al
  block-backend: remove blk_flush_all

 block/block-backend.c          | 22 ----------------------
 block/io.c                     | 25 +++++++++++++++++++++++++
 cpus.c                         |  4 ++--
 hw/i386/xen/xen_platform.c     |  2 --
 hw/ide/piix.c                  |  4 ++++
 include/block/block.h          |  1 +
 include/sysemu/block-backend.h |  1 -
 7 files changed, 32 insertions(+), 27 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v5 1/3] 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>
Reviewed-by: Kevin Wolf <kwolf@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..57a2eeb 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 v5 2/3] qemu: use bdrv_flush_all for vm_stop et al

[John Snow]

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, 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.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@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

[Qemu-devel] [PATCH v5 3/3] block-backend: remove blk_flush_all

[John Snow]

We can teach Xen to drain and flush each device as it needs to, instead
of trying to flush ALL devices. This removes the last user of
blk_flush_all.

The function is therefore removed under the premise that any new uses
of blk_flush_all would be the wrong paradigm: either flush the single
device that requires flushing, or use an appropriate flush_all mechanism
from outside of the BlkBackend layer.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/block-backend.c          | 22 ----------------------
 hw/i386/xen/xen_platform.c     |  2 --
 hw/ide/piix.c                  |  4 ++++
 include/sysemu/block-backend.h |  1 -
 4 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index d1349d9..bfb1ddb 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1621,28 +1621,6 @@ int blk_commit_all(void)
     return 0;
 }
 
-int blk_flush_all(void)
-{
-    BlockBackend *blk = NULL;
-    int result = 0;
-
-    while ((blk = blk_all_next(blk)) != NULL) {
-        AioContext *aio_context = blk_get_aio_context(blk);
-        int ret;
-
-        aio_context_acquire(aio_context);
-        if (blk_is_inserted(blk)) {
-            ret = blk_flush(blk);
-            if (ret < 0 && !result) {
-                result = ret;
-            }
-        }
-        aio_context_release(aio_context);
-    }
-
-    return result;
-}
-
 
 /* throttling disk I/O limits */
 void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index aa78393..f85635c 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -134,8 +134,6 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
            devices, and bit 2 the non-primary-master IDE devices. */
         if (val & UNPLUG_ALL_IDE_DISKS) {
             DPRINTF("unplug disks\n");
-            blk_drain_all();
-            blk_flush_all();
             pci_unplug_disks(pci_dev->bus);
         }
         if (val & UNPLUG_ALL_NICS) {
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index c190fca..d5777fd 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -179,6 +179,10 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev)
         if (di != NULL && !di->media_cd) {
             BlockBackend *blk = blk_by_legacy_dinfo(di);
             DeviceState *ds = blk_get_attached_dev(blk);
+
+            blk_drain(blk);
+            blk_flush(blk);
+
             if (ds) {
                 blk_detach_dev(blk, ds);
             }
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 4808a96..3d43592 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -150,7 +150,6 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
 int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count);
 int blk_co_flush(BlockBackend *blk);
 int blk_flush(BlockBackend *blk);
-int blk_flush_all(void);
 int blk_commit_all(void);
 void blk_drain(BlockBackend *blk);
 void blk_drain_all(void);
-- 
2.7.4

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

[Fam Zheng]

On Thu, 09/22 21:45, John Snow wrote:
> When I said "Final re-send," I was lying. Here's a v5.
> The title is also a misnomer by now :)
> 
> 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 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.
> 
> v5:
>  Fix bracket spacing in patch 1. By one space. :(
>  Added third patch to remove blk_flush_all.
> 
> v4:
>  Commit message update.
> 
> v3:
>  Reworking approach and reinstating bdrv_flush_all at Kevin's suggestion.
> 
> ________________________________________________________________________________
> 
> 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-v5:
> https://github.com/jnsnow/qemu/releases/tag/atapi-tray-migfix-v5
> 
> John Snow (3):
>   block: reintroduce bdrv_flush_all
>   qemu: use bdrv_flush_all for vm_stop et al
>   block-backend: remove blk_flush_all
> 
>  block/block-backend.c          | 22 ----------------------
>  block/io.c                     | 25 +++++++++++++++++++++++++
>  cpus.c                         |  4 ++--
>  hw/i386/xen/xen_platform.c     |  2 --
>  hw/ide/piix.c                  |  4 ++++
>  include/block/block.h          |  1 +
>  include/sysemu/block-backend.h |  1 -
>  7 files changed, 32 insertions(+), 27 deletions(-)
> 
> -- 
> 2.7.4
> 
> 

Acked-by: Fam Zheng <famz@redhat.com>

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

[Max Reitz]

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

On 23.09.2016 03:45, John Snow wrote:
> When I said "Final re-send," I was lying. Here's a v5.
> The title is also a misnomer by now :)
> 
> 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 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.
> 
> v5:
>  Fix bracket spacing in patch 1. By one space. :(
>  Added third patch to remove blk_flush_all.
> 
> v4:
>  Commit message update.
> 
> v3:
>  Reworking approach and reinstating bdrv_flush_all at Kevin's suggestion.
> 
> ________________________________________________________________________________
> 
> 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-v5:
> https://github.com/jnsnow/qemu/releases/tag/atapi-tray-migfix-v5
> 
> John Snow (3):
>   block: reintroduce bdrv_flush_all
>   qemu: use bdrv_flush_all for vm_stop et al
>   block-backend: remove blk_flush_all
> 
>  block/block-backend.c          | 22 ----------------------
>  block/io.c                     | 25 +++++++++++++++++++++++++
>  cpus.c                         |  4 ++--
>  hw/i386/xen/xen_platform.c     |  2 --
>  hw/ide/piix.c                  |  4 ++++
>  include/block/block.h          |  1 +
>  include/sysemu/block-backend.h |  1 -
>  7 files changed, 32 insertions(+), 27 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

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

[John Snow]

On 09/23/2016 11:35 AM, Max Reitz wrote:
> On 23.09.2016 03:45, John Snow wrote:
>> When I said "Final re-send," I was lying. Here's a v5.
>> The title is also a misnomer by now :)
>>
>> 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 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.
>>
>> v5:
>>  Fix bracket spacing in patch 1. By one space. :(
>>  Added third patch to remove blk_flush_all.
>>
>> v4:
>>  Commit message update.
>>
>> v3:
>>  Reworking approach and reinstating bdrv_flush_all at Kevin's suggestion.
>>
>> ________________________________________________________________________________
>>
>> 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-v5:
>> https://github.com/jnsnow/qemu/releases/tag/atapi-tray-migfix-v5
>>
>> John Snow (3):
>>   block: reintroduce bdrv_flush_all
>>   qemu: use bdrv_flush_all for vm_stop et al
>>   block-backend: remove blk_flush_all
>>
>>  block/block-backend.c          | 22 ----------------------
>>  block/io.c                     | 25 +++++++++++++++++++++++++
>>  cpus.c                         |  4 ++--
>>  hw/i386/xen/xen_platform.c     |  2 --
>>  hw/ide/piix.c                  |  4 ++++
>>  include/block/block.h          |  1 +
>>  include/sysemu/block-backend.h |  1 -
>>  7 files changed, 32 insertions(+), 27 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>

Since Fam acked this, I suppose it's for Kevin's tree?

--js

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

[Kevin Wolf]

Am 23.09.2016 um 21:38 hat John Snow geschrieben:
> On 09/23/2016 11:35 AM, Max Reitz wrote:
> >On 23.09.2016 03:45, John Snow wrote:
> >> block/block-backend.c          | 22 ----------------------
> >> block/io.c                     | 25 +++++++++++++++++++++++++
> >> cpus.c                         |  4 ++--
> >> hw/i386/xen/xen_platform.c     |  2 --
> >> hw/ide/piix.c                  |  4 ++++
> >> include/block/block.h          |  1 +
> >> include/sysemu/block-backend.h |  1 -
> >> 7 files changed, 32 insertions(+), 27 deletions(-)
> >
> >Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> Since Fam acked this, I suppose it's for Kevin's tree?

If nobody else wants the patches, I'll take them.

Thanks, applied to the block branch.

Kevin