[RFC PATCH-for-5.1] hw/ide: Do not block for AIO while resetting a drive

[RFC PATCH-for-5.1] hw/ide: Do not block for AIO while resetting a drive

[Philippe Mathieu-Daudé]

libFuzzer triggered the following assertion:

  cat << EOF | qemu-system-i386 -M pc-q35-5.0 \
    -nographic -monitor none -serial none \
    -qtest stdio -trace ide\*
  outl 0xcf8 0x8000fa24
  outl 0xcfc 0xe106c000
  outl 0xcf8 0x8000fa04
  outw 0xcfc 0x7
  outl 0xcf8 0x8000fb20
  write 0x0 0x3 0x2780e7
  write 0xe106c22c 0xd 0x1130c218021130c218021130c2
  write 0xe106c218 0x15 0x110010110010110010110010110010110010110010
  EOF
  ide_exec_cmd IDE exec cmd: bus 0x56170a77a2b8; state 0x56170a77a340; cmd 0xe7
  ide_reset IDEstate 0x56170a77a340
  Aborted (core dumped)

  (gdb) bt
  #1  0x00007ffff4f93895 in abort () at /lib64/libc.so.6
  #2  0x0000555555dc6c00 in bdrv_aio_cancel (acb=0x555556765550) at block/io.c:2745
  #3  0x0000555555dac202 in blk_aio_cancel (acb=0x555556765550) at block/block-backend.c:1546
  #4  0x0000555555b1bd74 in ide_reset (s=0x555557213340) at hw/ide/core.c:1318
  #5  0x0000555555b1e3a1 in ide_bus_reset (bus=0x5555572132b8) at hw/ide/core.c:2422
  #6  0x0000555555b2aa27 in ahci_reset_port (s=0x55555720eb50, port=2) at hw/ide/ahci.c:650
  #7  0x0000555555b29fd7 in ahci_port_write (s=0x55555720eb50, port=2, offset=44, val=16) at hw/ide/ahci.c:360
  #8  0x0000555555b2a564 in ahci_mem_write (opaque=0x55555720eb50, addr=556, val=16, size=1) at hw/ide/ahci.c:513
  #9  0x000055555598415b in memory_region_write_accessor (mr=0x55555720eb80, addr=556, value=0x7fffffffb838, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:483

Looking at bdrv_aio_cancel:

2728 /* async I/Os */
2729
2730 void bdrv_aio_cancel(BlockAIOCB *acb)
2731 {
2732     qemu_aio_ref(acb);
2733     bdrv_aio_cancel_async(acb);
2734     while (acb->refcnt > 1) {
2735         if (acb->aiocb_info->get_aio_context) {
2736             aio_poll(acb->aiocb_info->get_aio_context(acb), true);
2737         } else if (acb->bs) {
2738             /* qemu_aio_ref and qemu_aio_unref are not thread-safe, so
2739              * assert that we're not using an I/O thread.  Thread-safe
2740              * code should use bdrv_aio_cancel_async exclusively.
2741              */
2742             assert(bdrv_get_aio_context(acb->bs) == qemu_get_aio_context());
2743             aio_poll(bdrv_get_aio_context(acb->bs), true);
2744         } else {
2745             abort();     <===============
2746         }
2747     }
2748     qemu_aio_unref(acb);
2749 }

Our context is already referenced but we don't have a getter,
neither a block driver state. Maybe because we are called from
a vCPU context? Avoid this case by calling the pending callback
directly. In this case this is WIN_FLUSH_CACHE. I'm not sure for
the other READ/WRITE commands.

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Fixes: bef0fd5958 ("ide: convert ide_sector_read() to asynchronous I/O")
BugLink: https://bugs.launchpad.net/qemu/+bug/1878255
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because I don't understand AIO operations well.
After RFC Cc: qemu-stable@nongnu.org

ide_is_pio_out() verifies a PIO OUT checking:

  s->end_transfer_func == ide_dummy_transfer_stop

An alternative might be:

    if (s->pio_aiocb && s->end_transfer_func == ide_dummy_transfer_stop) {
        /* If there is a pending AIO callback, invoke it now. */
        blk_aio_cancel_async(s->pio_aiocb);
        s->pio_aiocb = NULL;
    }

Or if we want to limit to WIN_FLUSH_CACHE:

    if (s->pio_aiocb && s->bus->error_status & IDE_RETRY_FLUSH) {
        /* If there is a pending AIO callback, invoke it now. */
        blk_aio_cancel_async(s->pio_aiocb);
        s->pio_aiocb = NULL;
    }

Cc: Stefan Hajnoczi <stefanha@redhat.com>

Last minute chat:
19:01 <stefanha> f4bug: use bdrv_aio_cancel_async() if possible because it won't block the current thread.
19:02 <stefanha> f4bug: For example, in device emulation code where the guest has requested to cancel an I/O request it's often possible to use the async version.
19:02 <stefanha> f4bug: But in synchronous code like device reset it may be necessary to use the synchronous (blocking) bdrv_aio_cancel() API instead. :(
19:14 <stefanha> f4bug: The way to decide is: will the current function return to the event loop and is there someone who will handle the request completion callback when cancel finishes?
19:14 <stefanha> f4bug: If the next line of code requires the request to finished then async cancel cannot be used.
19:15 <stefanha> f4bug: On the other hand, if the function can return and it's okay for the request to cancel at some future time then you can use async.

So I'll revisit this patch :)
---
 hw/ide/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..e3a9ce7d25 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1315,7 +1315,8 @@ static void ide_reset(IDEState *s)
     trace_ide_reset(s);
 
     if (s->pio_aiocb) {
-        blk_aio_cancel(s->pio_aiocb);
+        /* If there is a pending AIO callback, invoke it now. */
+        blk_aio_cancel_async(s->pio_aiocb);
         s->pio_aiocb = NULL;
     }
 
-- 
2.21.3

Re: [RFC PATCH-for-5.1] hw/ide: Do not block for AIO while resetting a drive

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200717171938.1249-1-f4bug@amsat.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/target/arm/translate-sve.o
  CC      aarch64-softmmu/trace/generated-helpers.o
  LINK    aarch64-softmmu/qemu-system-aarch64
collect2: error: ld returned 1 exit status
collect2: error: ld returned 1 exit status
make[1]: *** [qemu-system-aarch64] Error 1
make[1]: *** [qemu-system-x86_64] Error 1
make: *** [aarch64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
make: *** [x86_64-softmmu/all] Error 2
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 708, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=3bb1a90ba07d44f8a68428ba8cc1aac8', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-s7o9muvt/src/docker-src.2020-07-17-13.24.31.3740:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=3bb1a90ba07d44f8a68428ba8cc1aac8
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-s7o9muvt/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m44.977s
user    0m8.984s


The full log is available at
http://patchew.org/logs/20200717171938.1249-1-f4bug@amsat.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC PATCH-for-5.1] hw/ide: Do not block for AIO while resetting a drive

[Stefan Hajnoczi]

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

On Fri, Jul 17, 2020 at 07:19:38PM +0200, Philippe Mathieu-Daudé wrote:
> Last minute chat:
> 19:01 <stefanha> f4bug: use bdrv_aio_cancel_async() if possible because it won't block the current thread.
> 19:02 <stefanha> f4bug: For example, in device emulation code where the guest has requested to cancel an I/O request it's often possible to use the async version.
> 19:02 <stefanha> f4bug: But in synchronous code like device reset it may be necessary to use the synchronous (blocking) bdrv_aio_cancel() API instead. :(
> 19:14 <stefanha> f4bug: The way to decide is: will the current function return to the event loop and is there someone who will handle the request completion callback when cancel finishes?
> 19:14 <stefanha> f4bug: If the next line of code requires the request to finished then async cancel cannot be used.
> 19:15 <stefanha> f4bug: On the other hand, if the function can return and it's okay for the request to cancel at some future time then you can use async.
> 
> So I'll revisit this patch :)
> ---
>  hw/ide/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d997a78e47..e3a9ce7d25 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1315,7 +1315,8 @@ static void ide_reset(IDEState *s)
>      trace_ide_reset(s);
>  
>      if (s->pio_aiocb) {
> -        blk_aio_cancel(s->pio_aiocb);
> +        /* If there is a pending AIO callback, invoke it now. */
> +        blk_aio_cancel_async(s->pio_aiocb);

This is a place where an async call is not allowed. The completion
function must be called right away (synchronously) before we can
continue resetting the device.

I sent a patch that allows bdrv_aio_cancel() to find the AioContext so
it can call aio_poll().

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH-for-5.1] hw/ide: Do not block for AIO while resetting a drive

[John Snow]

On 7/20/20 6:02 AM, Stefan Hajnoczi wrote:
> On Fri, Jul 17, 2020 at 07:19:38PM +0200, Philippe Mathieu-Daudé wrote:
>> Last minute chat:
>> 19:01 <stefanha> f4bug: use bdrv_aio_cancel_async() if possible because it won't block the current thread.
>> 19:02 <stefanha> f4bug: For example, in device emulation code where the guest has requested to cancel an I/O request it's often possible to use the async version.
>> 19:02 <stefanha> f4bug: But in synchronous code like device reset it may be necessary to use the synchronous (blocking) bdrv_aio_cancel() API instead. :(
>> 19:14 <stefanha> f4bug: The way to decide is: will the current function return to the event loop and is there someone who will handle the request completion callback when cancel finishes?
>> 19:14 <stefanha> f4bug: If the next line of code requires the request to finished then async cancel cannot be used.
>> 19:15 <stefanha> f4bug: On the other hand, if the function can return and it's okay for the request to cancel at some future time then you can use async.
>>
>> So I'll revisit this patch :)
>> ---
>>   hw/ide/core.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index d997a78e47..e3a9ce7d25 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -1315,7 +1315,8 @@ static void ide_reset(IDEState *s)
>>       trace_ide_reset(s);
>>   
>>       if (s->pio_aiocb) {
>> -        blk_aio_cancel(s->pio_aiocb);
>> +        /* If there is a pending AIO callback, invoke it now. */
>> +        blk_aio_cancel_async(s->pio_aiocb);
> 
> This is a place where an async call is not allowed. The completion
> function must be called right away (synchronously) before we can
> continue resetting the device.
> 
> I sent a patch that allows bdrv_aio_cancel() to find the AioContext so
> it can call aio_poll().
> 
> Stefan
> 

OK, dropping Phil's patch here.

--js