Bug 1402645

Also can reproduce the issue on qemu-kvm-rhev-2.6.0-28.el7_3.1.x86_64

This is essentially related to the same thing as bug 1342999.

Upstream will already reject setting a cache mode for an empty CD-ROM drive
because it has no effect. For compatibility reasons, we made RHEL still accept
and silently ignore the option. It doesn't make any sense to specify it, but
libvirt happened to do so.

We could probably extend this downstream-only hack and also ignore aio=native
silently for empty CD-ROM drives, but unless libvirt is making use of it, is it
really worth more downstream-only changes?

On the other hand, the fix could be as easy as this:

--- a/blockdev.c
+++ b/blockdev.c
@@ -603,7 +603,7 @@ 
         }
 
         blk_rs = blk_get_root_state(blk);
-        blk_rs->open_flags    = bdrv_flags;
+        blk_rs->open_flags    = bdrv_flags & ~BDRV_O_NATIVE_AIO;
         blk_rs->read_only     = !(bdrv_flags & BDRV_O_RDWR);
         blk_rs->detect_zeroes = detect_zeroes;


Anyway, reassigning to John who owned bug 1342999.

(In reply to Kevin Wolf from comment #5)
> This is essentially related to the same thing as bug 1342999.
> 
> Upstream will already reject setting a cache mode for an empty CD-ROM drive
> because it has no effect. For compatibility reasons, we made RHEL still
> accept
> and silently ignore the option. It doesn't make any sense to specify it, but
> libvirt happened to do so.
> 
> We could probably extend this downstream-only hack and also ignore aio=native
> silently for empty CD-ROM drives, but unless libvirt is making use of it, is
> it really worth more downstream-only changes?

Since we don't support direct usage of QEMU in RHEL-7, there's no need for the downstream-only change if libvirt is not using it.

Kevin, shouldn't we be *actively* rejecting the aio property on empty drives upstream? Especially if we can set it via blockdev_add, and the "Cache modes on removable media devices is already broken" genie is already out of the bottle.

I am not sure the exact right way to factor this out of the existing spaghetti, though. I'd suppose that the aio option needs to stay in bs_opts and not get parsed out into opts and subsequently bdrv_flags via extract_common_blockdev_options...

However, that does mean that:

(1) On initial insert, we'd be prohibiting an AIO mode for empty drives, but
(2) On subsequent changes, though, if a user added an AIO mode to an inserted medium, it would persist through the root state.

Which is also kind of madness, isn't it?

Just simply ignoring it downstream doesn't seem right as this exact same kind of behavior will still exist upstream, and it seems just as wrong there.

Looking for suggestions on what you feel the right way to untangle this knot is.

As discussed on IRC a while ago, yes, we should, and yes, users will complain if
we do it. We have messed up AIO by making it a BDRV_O_* flag when it is really
specific to file-posix and file-win32, and I'm not sure how to fix it without
breaking compatibility. The "correct" thing would be to remove it from the root
state, but I'm sure that people expect that it's kept across media change.

(I know, not that useful, I just need to get rid of the needinfo...)

(In reply to Kevin Wolf from comment #5)
> This is essentially related to the same thing as bug 1342999.
> 
> Upstream will already reject setting a cache mode for an empty CD-ROM drive
> because it has no effect. For compatibility reasons, we made RHEL still
> accept
> and silently ignore the option. It doesn't make any sense to specify it, but
> libvirt happened to do so.
> 
> We could probably extend this downstream-only hack and also ignore aio=native
> silently for empty CD-ROM drives, but unless libvirt is making use of it, is
> it
> really worth more downstream-only changes?
> 
> On the other hand, the fix could be as easy as this:
> 
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -603,7 +603,7 @@ 
>          }
>  
>          blk_rs = blk_get_root_state(blk);
> -        blk_rs->open_flags    = bdrv_flags;
> +        blk_rs->open_flags    = bdrv_flags & ~BDRV_O_NATIVE_AIO;
>          blk_rs->read_only     = !(bdrv_flags & BDRV_O_RDWR);
>          blk_rs->detect_zeroes = detect_zeroes;
> 
> 
> Anyway, reassigning to John who owned bug 1342999.

bug 1425003 comment 3 includes a formula where libvirt XML can indeed trigger this

*** Bug 1425002 has been marked as a duplicate of this bug. ***

Fix included in qemu-kvm-rhev-2.9.0-5.el7

Reproduced this bug on qemu-kvm-rhev-2.8.0-6.el7.x86_64

Steps:

1. Boot guest with following cli
/usr/libexec/qemu-kvm -drive id=drive_none,if=none,snapshot=off,aio=native,cache=none,media=cdrom -device ide-cd,id=none,drive=drive_none,bootindex=1,bus=ide.0,unit=0 -monitor stdio

2. Change cdrom
(qemu) change drive_none /root/test.iso 
aio=native was specified, but it requires cache.direct=on, which was not specified.
(qemu) info block
drive_none: [not inserted]
    Removable device: not locked, tray closed

floppy0: [not inserted]
    Removable device: not locked, tray closed

sd0: [not inserted]
    Removable device: not locked, tray closed


Verified this bug on qemu-kvm-rhev-2.9.0-5.el7.x86_64

(qemu) change drive_none /root/test.iso 
(qemu) info block
drive_none (#block144): /root/test.iso (raw, read-only)
    Removable device: not locked, tray closed
    Cache mode:       writeback

floppy0: [not inserted]
    Removable device: not locked, tray closed

sd0: [not inserted]
    Removable device: not locked, tray closed
 

Thanks.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2392

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2392

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2392

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2392

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2392

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2392