[Qemu-devel] [PATCH v2 0/2] block: warn about aio=native if libaio is unavailable

[Qemu-devel] [PATCH v2 0/2] block: warn about aio=native if libaio is unavailable

[Stefan Hajnoczi]

v2:
 * Banish CONFIG_LINUX_AIO from blockdev.c, that is raw-posix.c's business
   [Kevin]
 * Print the warning in the same way as the aio=native,cache.direct=off
   deprecation warning [Kevin]

Open question: what about the Windows case?  We now pass the
FILE_FLAG_OVERLAPPED flag which we didn't do before for -drive aio=native.

Stefan Hajnoczi (2):
  raw-posix: warn about BDRV_O_NATIVE_AIO if libaio is unavailable
  blockdev: always compile in -drive aio= parsing

 block/raw-posix.c | 11 ++++++++++-
 blockdev.c        |  2 --
 2 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 1/2] raw-posix: warn about BDRV_O_NATIVE_AIO if libaio is unavailable

[Stefan Hajnoczi]

raw-posix.c silently ignores BDRV_O_NATIVE_AIO if libaio is unavailable.
It is confusing when aio=native performance is identical to aio=threads
because the binary was accidentally built without libaio.

Print a deprecation warning if -drive aio=native is used with a binary
that does not support libaio.  There are probably users using aio=native
who would be inconvenienced if QEMU suddenly refused to start their
guests.  In the future this will become an error.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/raw-posix.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 855febe..e09019c 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -519,7 +519,16 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
                      "future QEMU versions.\n",
                      bs->filename);
     }
-#endif
+#else
+    if (bdrv_flags & BDRV_O_NATIVE_AIO) {
+        error_printf("WARNING: aio=native was specified for '%s', but "
+                     "is not supported in this build. Falling back to "
+                     "aio=threads.\n"
+                     "         This will become an error condition in "
+                     "future QEMU versions.\n",
+                     bs->filename);
+    }
+#endif /* !defined(CONFIG_LINUX_AIO) */
 
     s->has_discard = true;
     s->has_write_zeroes = true;
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 1/2] raw-posix: warn about BDRV_O_NATIVE_AIO if libaio is unavailable

[Christian Borntraeger]

Am 17.07.2015 um 16:23 schrieb Stefan Hajnoczi:
> raw-posix.c silently ignores BDRV_O_NATIVE_AIO if libaio is unavailable.
> It is confusing when aio=native performance is identical to aio=threads
> because the binary was accidentally built without libaio.
> 
> Print a deprecation warning if -drive aio=native is used with a binary
> that does not support libaio.  There are probably users using aio=native
> who would be inconvenienced if QEMU suddenly refused to start their
> guests.  In the future this will become an error.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

had that myself on a freshly installed system without libaio-devel.
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>


Another thing. Would it make sense to change the default to aio=native somewhen?
>From what I can tell this seems to outperform aio=threads in most cases.


> ---
>  block/raw-posix.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 855febe..e09019c 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -519,7 +519,16 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>                       "future QEMU versions.\n",
>                       bs->filename);
>      }
> -#endif
> +#else
> +    if (bdrv_flags & BDRV_O_NATIVE_AIO) {
> +        error_printf("WARNING: aio=native was specified for '%s', but "
> +                     "is not supported in this build. Falling back to "
> +                     "aio=threads.\n"
> +                     "         This will become an error condition in "
> +                     "future QEMU versions.\n",
> +                     bs->filename);
> +    }
> +#endif /* !defined(CONFIG_LINUX_AIO) */
> 
>      s->has_discard = true;
>      s->has_write_zeroes = true;
> 

Re: [Qemu-devel] [PATCH v2 1/2] raw-posix: warn about BDRV_O_NATIVE_AIO if libaio is unavailable

[Denis V. Lunev]

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

On 07/23/2015 01:09 PM, Christian Borntraeger wrote:
> Am 17.07.2015 um 16:23 schrieb Stefan Hajnoczi:
>> raw-posix.c silently ignores BDRV_O_NATIVE_AIO if libaio is unavailable.
>> It is confusing when aio=native performance is identical to aio=threads
>> because the binary was accidentally built without libaio.
>>
>> Print a deprecation warning if -drive aio=native is used with a binary
>> that does not support libaio.  There are probably users using aio=native
>> who would be inconvenienced if QEMU suddenly refused to start their
>> guests.  In the future this will become an error.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> had that myself on a freshly installed system without libaio-devel.
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
>
> Another thing. Would it make sense to change the default to aio=native somewhen?
>  From what I can tell this seems to outperform aio=threads in most cases.
>

this seems a good idea to me, we are always changing
from threads to native in our installations

>> ---
>>   block/raw-posix.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 855febe..e09019c 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -519,7 +519,16 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>                        "future QEMU versions.\n",
>>                        bs->filename);
>>       }
>> -#endif
>> +#else
>> +    if (bdrv_flags & BDRV_O_NATIVE_AIO) {
>> +        error_printf("WARNING: aio=native was specified for '%s', but "
>> +                     "is not supported in this build. Falling back to "
>> +                     "aio=threads.\n"
>> +                     "         This will become an error condition in "
>> +                     "future QEMU versions.\n",
>> +                     bs->filename);
>> +    }
>> +#endif /* !defined(CONFIG_LINUX_AIO) */
>>
>>       s->has_discard = true;
>>       s->has_write_zeroes = true;
>>
>


[-- Attachment #2: Type: text/html, Size: 2791 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] raw-posix: warn about BDRV_O_NATIVE_AIO if libaio is unavailable

[Kevin Wolf]

Am 23.07.2015 um 12:09 hat Christian Borntraeger geschrieben:
> Am 17.07.2015 um 16:23 schrieb Stefan Hajnoczi:
> > raw-posix.c silently ignores BDRV_O_NATIVE_AIO if libaio is unavailable.
> > It is confusing when aio=native performance is identical to aio=threads
> > because the binary was accidentally built without libaio.
> > 
> > Print a deprecation warning if -drive aio=native is used with a binary
> > that does not support libaio.  There are probably users using aio=native
> > who would be inconvenienced if QEMU suddenly refused to start their
> > guests.  In the future this will become an error.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> had that myself on a freshly installed system without libaio-devel.
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 
> Another thing. Would it make sense to change the default to aio=native somewhen?
> From what I can tell this seems to outperform aio=threads in most cases.

aio=native requires cache.direct=on, which is not portable to all
platforms that qemu supports, and also doesn't work with all filesystems
on Linux (most notably tmpfs fails). Also recent benchmarks seem to
suggest that currently there is no clear winner between aio=native and
aio=threads, it depends too much on the host storage and the workload.

When we discussed the default cache mode a while back (with the options
cache=writeback and cache=none), it was considered more important to
have a default setting that works everywhere and performs good for quick
ad-hoc VMs and development/debugging work than one that performs best in
enterprise setups that should use a management tool anyway.

Kevin

[Qemu-devel] [PATCH v2 2/2] blockdev: always compile in -drive aio= parsing

[Stefan Hajnoczi]

CONFIG_LINUX_AIO is an implementation detail of raw-posix.c.  Don't
mention CONFIG_LINUX_AIO in blockdev.c.  Let raw-posix.c decide what to
do with BDRV_O_NATIVE_AIO if CONFIG_LINUX_AIO is not defined.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 62a4586..37b91c8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -405,7 +405,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         bdrv_flags |= BDRV_O_NO_FLUSH;
     }
 
-#ifdef CONFIG_LINUX_AIO
     if ((buf = qemu_opt_get(opts, "aio")) != NULL) {
         if (!strcmp(buf, "native")) {
             bdrv_flags |= BDRV_O_NATIVE_AIO;
@@ -416,7 +415,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
            goto early_err;
         }
     }
-#endif
 
     if ((buf = qemu_opt_get(opts, "format")) != NULL) {
         if (is_help_option(buf)) {
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 2/2] blockdev: always compile in -drive aio= parsing

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> CONFIG_LINUX_AIO is an implementation detail of raw-posix.c.  Don't
> mention CONFIG_LINUX_AIO in blockdev.c.  Let raw-posix.c decide what to

To be precise: "raw-posix.c or raw-win32.c", or maybe "the block
driver".

> do with BDRV_O_NATIVE_AIO if CONFIG_LINUX_AIO is not defined.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 62a4586..37b91c8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -405,7 +405,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>          bdrv_flags |= BDRV_O_NO_FLUSH;
>      }
>  
> -#ifdef CONFIG_LINUX_AIO
>      if ((buf = qemu_opt_get(opts, "aio")) != NULL) {
>          if (!strcmp(buf, "native")) {
>              bdrv_flags |= BDRV_O_NATIVE_AIO;
> @@ -416,7 +415,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>             goto early_err;
>          }
>      }
> -#endif
>  
>      if ((buf = qemu_opt_get(opts, "format")) != NULL) {
>          if (is_help_option(buf)) {

Re: [Qemu-devel] [PATCH v2 0/2] block: warn about aio=native if libaio is unavailable

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> v2:
>  * Banish CONFIG_LINUX_AIO from blockdev.c, that is raw-posix.c's business
>    [Kevin]
>  * Print the warning in the same way as the aio=native,cache.direct=off
>    deprecation warning [Kevin]
>
> Open question: what about the Windows case?  We now pass the
> FILE_FLAG_OVERLAPPED flag which we didn't do before for -drive aio=native.

Odd.  Commit a273652 takes the trouble to implement native asynchronous
I/O there, but unless CONFIG_LINUX_AIO somehow gets defined, it's
unreachable, isn't it?  Paolo?

I think PATCH 2's commit message needs to be updated to discuss the
impact.

Re: [Qemu-devel] [PATCH v2 0/2] block: warn about aio=native if libaio is unavailable

[Paolo Bonzini]

On 23/07/2015 10:03, Markus Armbruster wrote:
> 
>> > v2:
>> >  * Banish CONFIG_LINUX_AIO from blockdev.c, that is raw-posix.c's business
>> >    [Kevin]
>> >  * Print the warning in the same way as the aio=native,cache.direct=off
>> >    deprecation warning [Kevin]
>> >
>> > Open question: what about the Windows case?  We now pass the
>> > FILE_FLAG_OVERLAPPED flag which we didn't do before for -drive aio=native.
> Odd.  Commit a273652 takes the trouble to implement native asynchronous
> I/O there, but unless CONFIG_LINUX_AIO somehow gets defined, it's
> unreachable, isn't it?  Paolo?

I don't remember how I tested that code, but it's probably been dead
code since it was committed.  If it's bitrot and thus these patches
break it, I will fix it.

Paolo

> I think PATCH 2's commit message needs to be updated to discuss the
> impact.

Re: [Qemu-devel] [PATCH v2 0/2] block: warn about aio=native if libaio is unavailable

[Kevin Wolf]

Am 23.07.2015 um 10:08 hat Paolo Bonzini geschrieben:
> 
> 
> On 23/07/2015 10:03, Markus Armbruster wrote:
> > 
> >> > v2:
> >> >  * Banish CONFIG_LINUX_AIO from blockdev.c, that is raw-posix.c's business
> >> >    [Kevin]
> >> >  * Print the warning in the same way as the aio=native,cache.direct=off
> >> >    deprecation warning [Kevin]
> >> >
> >> > Open question: what about the Windows case?  We now pass the
> >> > FILE_FLAG_OVERLAPPED flag which we didn't do before for -drive aio=native.
> > Odd.  Commit a273652 takes the trouble to implement native asynchronous
> > I/O there, but unless CONFIG_LINUX_AIO somehow gets defined, it's
> > unreachable, isn't it?  Paolo?
> 
> I don't remember how I tested that code, but it's probably been dead
> code since it was committed.  If it's bitrot and thus these patches
> break it, I will fix it.

The tools use a different path, so qemu-img/io would have correctly used
AIO on Windows.

Kevin

Re: [Qemu-devel] [PATCH v2 0/2] block: warn about aio=native if libaio is unavailable

[Stefan Hajnoczi]

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

On Thu, Jul 23, 2015 at 10:03:32AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > v2:
> >  * Banish CONFIG_LINUX_AIO from blockdev.c, that is raw-posix.c's business
> >    [Kevin]
> >  * Print the warning in the same way as the aio=native,cache.direct=off
> >    deprecation warning [Kevin]
> >
> > Open question: what about the Windows case?  We now pass the
> > FILE_FLAG_OVERLAPPED flag which we didn't do before for -drive aio=native.
> 
> Odd.  Commit a273652 takes the trouble to implement native asynchronous
> I/O there, but unless CONFIG_LINUX_AIO somehow gets defined, it's
> unreachable, isn't it?  Paolo?
> 
> I think PATCH 2's commit message needs to be updated to discuss the
> impact.

Nice find!

I'll send a v2.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]