[1/1] block: add missed BDRV_O_NOCACHE when block device is opened without file
diff mbox

Message ID 1485362572-7246-1-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Jan. 25, 2017, 4:42 p.m. UTC
Technically there is a problem when the guest DVD is created by libvirt
with AIO mode 'native' on Linux. Current QEMU is unable to start the
domain configured as follows:
    <disk type='file' device='cdrom'>
      <driver name='qemu' type='raw' cache='none' io='native'/>
      <target dev='sdb' bus='scsi'/>
      <readonly/>
    </disk>
The problem comes from the combination of 'cache' and 'io' options.

'io' option is common option and it is removed from block driver
specific options. 'cache' originally is not. The patch makes 'cache'
option common. This works fine as long as cdrom media insertion
later on.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

May be this has already discussed, but still. AIO=native for CDROM without
media seems important case.

Comments

Max Reitz Jan. 25, 2017, 5:59 p.m. UTC | #1
[CC-ing John]

On 25.01.2017 17:42, Denis V. Lunev wrote:
> Technically there is a problem when the guest DVD is created by libvirt
> with AIO mode 'native' on Linux. Current QEMU is unable to start the
> domain configured as follows:
>     <disk type='file' device='cdrom'>
>       <driver name='qemu' type='raw' cache='none' io='native'/>
>       <target dev='sdb' bus='scsi'/>
>       <readonly/>
>     </disk>
> The problem comes from the combination of 'cache' and 'io' options.
> 
> 'io' option is common option and it is removed from block driver
> specific options. 'cache' originally is not. The patch makes 'cache'
> option common. This works fine as long as cdrom media insertion
> later on.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)

There was a Red Hat BZ for this:

https://bugzilla.redhat.com/show_bug.cgi?id=1342999

There is now a corresponding BZ for libvirt:

https://bugzilla.redhat.com/show_bug.cgi?id=1377321

The gist is that it was determined to be a problem with libvirt.

RHEL has a downstream commit to work around this issue by ignoring the
cache mode set for empty CD-ROMs -- but that is only because that was
simpler than fixing libvirt.

(Note that it's ignoring the cache mode instead of actually evaluating it.)

> May be this has already discussed, but still. AIO=native for CDROM without
> media seems important case.

First, I personally don't find aio=native very important for CD-ROM
drives. Yes, we shouldn't make IDE CD-ROM slower than necessary, but I
don't think aio=native will make the difference between "Slow like
CD-ROMs are in reality" and "As fast as virtio".

Second, all this patch does is revert some changes done by commit
91a097e7478940483e76d52217f05bc05b98d5a5, which was very deliberate.

Third, you may then be asking for the recommended way to put an
aio=native medium into a CD-ROM drive. Good thing you ask, because we
have a way that we want to recommend but can't because it's still
considered experimental:

The BDS is added using blockdev-add, with all of the appropriate caching
and aio options. Then it's inserted into the drive using the
x-blockdev-insert-medium command, and the drive is closed using
blockdev-close-tray.

There are a couple of issues with this: First, blockdev-add and
x-blockdev-insert-medium are still experimental. The good news are that
I think the reason why the latter is experimental has actually
disappeared and we can just drop the x- by now. The
not-so-good-but-not-so-bad-either news are that we plan to get
blockdev-add declared non-experimental for 2.9. Let's see how it goes.

The other issue is that of course it's more cumbersome to use than a
simple 'change' via HMP. I argue that if someone communicates with a VM
by hand, they either have to deal with this added complexity or they
cannot use aio=native for CD-ROM drives -- which, as I said, I don't
think is too bad.

However, there is a reason why you cannot set cache/aio for an empty
drive and it's simply that it doesn't make sense.

Max

Patch
diff mbox

diff --git a/blockdev.c b/blockdev.c
index 245e1e1..004dcde 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -374,6 +374,13 @@  static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
                return;
             }
         }
+
+        if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
+            *bdrv_flags |= BDRV_O_NO_FLUSH;
+        }
+        if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
+            *bdrv_flags |= BDRV_O_NOCACHE;
+        }
     }
 
     /* disk I/O throttling */
@@ -569,11 +576,8 @@  static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         /* bdrv_open() defaults to the values in bdrv_flags (for compatibility
          * with other callers) rather than what we want as the real defaults.
          * Apply the defaults here instead. */
-        qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
-        qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
         qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY,
                               read_only ? "on" : "off");
-        assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0);
 
         if (runstate_check(RUN_STATE_INMIGRATE)) {
             bdrv_flags |= BDRV_O_INACTIVE;
@@ -3996,6 +4000,14 @@  QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_STRING,
             .help = "write error action",
         },{
+            .name = BDRV_OPT_CACHE_DIRECT,
+            .type = QEMU_OPT_BOOL,
+            .help = "Bypass software writeback cache on the host",
+        }, {
+            .name = BDRV_OPT_CACHE_NO_FLUSH,
+            .type = QEMU_OPT_BOOL,
+            .help = "Ignore flush requests",
+        }, {
             .name = BDRV_OPT_READ_ONLY,
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",