[V4,1/6] ide/atapi: make PIO read requests async

PIO read requests on the ATAPI interface used to be sync blk requests.
This has two significant drawbacks. First the main loop hangs util an
I/O request is completed and secondly if the I/O request does not
complete (e.g. due to an unresponsive storage) Qemu hangs completely.

Note: Due to possible race conditions requests during an ongoing
elementary transfer are still sync.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 hw/ide/atapi.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 85 insertions(+), 12 deletions(-)

On 11/12/2015 11:30 AM, Peter Lieven wrote:
> PIO read requests on the ATAPI interface used to be sync blk requests.
> This has two significant drawbacks. First the main loop hangs util an
> I/O request is completed and secondly if the I/O request does not
> complete (e.g. due to an unresponsive storage) Qemu hangs completely.
> 
> Note: Due to possible race conditions requests during an ongoing
> elementary transfer are still sync.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  hw/ide/atapi.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 85 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 747f466..cfd2d63 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -105,33 +105,98 @@ static void cd_data_to_raw(uint8_t *buf, int lba)
>      memset(buf, 0, 288);
>  }
>  
> -static int cd_read_sector(IDEState *s, int lba, uint8_t *buf, int sector_size)
> +static int
> +cd_read_sector_sync(IDEState *s)
>  {
>      int ret;
>  
> -    switch(sector_size) {
> +#ifdef DEBUG_IDE_ATAPI
> +    printf("cd_read_sector_sync: lba=%d\n", s->lba);
> +#endif
> +
> +    switch (s->cd_sector_size) {
>      case 2048:
>          block_acct_start(blk_get_stats(s->blk), &s->acct,
>                           4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
> -        ret = blk_read(s->blk, (int64_t)lba << 2, buf, 4);
> +        ret = blk_read(s->blk, (int64_t)s->lba << 2,
> +                       s->io_buffer, 4);
>          block_acct_done(blk_get_stats(s->blk), &s->acct);
>          break;
>      case 2352:
>          block_acct_start(blk_get_stats(s->blk), &s->acct,
>                           4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
> -        ret = blk_read(s->blk, (int64_t)lba << 2, buf + 16, 4);
> +        ret = blk_read(s->blk, (int64_t)s->lba << 2,
> +                       s->io_buffer + 16, 4);
>          block_acct_done(blk_get_stats(s->blk), &s->acct);
>          if (ret < 0)
>              return ret;
> -        cd_data_to_raw(buf, lba);
> +        cd_data_to_raw(s->io_buffer, s->lba);
>          break;
>      default:
>          ret = -EIO;
>          break;
>      }
> +
> +    if (!ret) {
> +        s->lba++;
> +        s->io_buffer_index = 0;
> +    }
> +
>      return ret;
>  }
>  
> +static void cd_read_sector_cb(void *opaque, int ret)
> +{
> +    IDEState *s = opaque;
> +
> +    block_acct_done(blk_get_stats(s->blk), &s->acct);
> +
> +#ifdef DEBUG_IDE_ATAPI
> +    printf("cd_read_sector_cb: lba=%d ret=%d\n", s->lba, ret);
> +#endif
> +
> +    if (ret < 0) {
> +        ide_atapi_io_error(s, ret);
> +        return;
> +    }
> +
> +    if (s->cd_sector_size == 2352) {
> +        cd_data_to_raw(s->io_buffer, s->lba);
> +    }
> +
> +    s->lba++;
> +    s->io_buffer_index = 0;
> +    s->status &= ~BUSY_STAT;
> +
> +    ide_atapi_cmd_reply_end(s);
> +}
> +
> +static int cd_read_sector(IDEState *s)
> +{
> +    if (s->cd_sector_size != 2048 && s->cd_sector_size != 2352) {
> +        return -EINVAL;
> +    }
> +
> +    s->iov.iov_base = (s->cd_sector_size == 2352) ?
> +                      s->io_buffer + 16 : s->io_buffer;
> +
> +    s->iov.iov_len = 4 * BDRV_SECTOR_SIZE;
> +    qemu_iovec_init_external(&s->qiov, &s->iov, 1);
> +
> +#ifdef DEBUG_IDE_ATAPI
> +    printf("cd_read_sector: lba=%d\n", s->lba);
> +#endif
> +
> +    block_acct_start(blk_get_stats(s->blk), &s->acct,
> +                     4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
> +
> +    blk_aio_readv(s->blk, (int64_t)s->lba << 2, &s->qiov, 4,
> +                  cd_read_sector_cb, s);
> +
> +    s->status |= BUSY_STAT;
> +    return 0;
> +}
> +
>  void ide_atapi_cmd_ok(IDEState *s)
>  {
>      s->error = 0;
> @@ -182,18 +247,27 @@ void ide_atapi_cmd_reply_end(IDEState *s)
>          ide_atapi_cmd_ok(s);
>          ide_set_irq(s->bus);
>  #ifdef DEBUG_IDE_ATAPI
> -        printf("status=0x%x\n", s->status);
> +        printf("end of transfer, status=0x%x\n", s->status);
>  #endif
>      } else {
>          /* see if a new sector must be read */
>          if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) {
> -            ret = cd_read_sector(s, s->lba, s->io_buffer, s->cd_sector_size);
> -            if (ret < 0) {
> -                ide_atapi_io_error(s, ret);
> +            if (!s->elementary_transfer_size) {
> +                ret = cd_read_sector(s);
> +                if (ret < 0) {
> +                    ide_atapi_io_error(s, ret);
> +                }
>                  return;
> +            } else {
> +                /* rebuffering within an elementary transfer is
> +                 * only possible with a sync request because we
> +                 * end up with a race condition otherwise */
> +                ret = cd_read_sector_sync(s);
> +                if (ret < 0) {
> +                    ide_atapi_io_error(s, ret);
> +                    return;
> +                }
>              }
> -            s->lba++;
> -            s->io_buffer_index = 0;
>          }
>          if (s->elementary_transfer_size > 0) {
>              /* there are some data left to transmit in this elementary
> @@ -275,7 +349,6 @@ static void ide_atapi_cmd_read_pio(IDEState *s, int lba, int nb_sectors,
>      s->io_buffer_index = sector_size;
>      s->cd_sector_size = sector_size;
>  
> -    s->status = READY_STAT | SEEK_STAT;
>      ide_atapi_cmd_reply_end(s);
>  }
>  
> 

Had to rebase against Berto's patches, but nothing major. You can see my
rebase at:

https://github.com/jnsnow/qemu/tree/review-plieven

Reviewed-by: John Snow <jsnow@redhat.com>

> Am 13.11.2015 um 23:42 schrieb John Snow <jsnow@redhat.com>:
> 
> 
> 
>> On 11/12/2015 11:30 AM, Peter Lieven wrote:
>> PIO read requests on the ATAPI interface used to be sync blk requests.
>> This has two significant drawbacks. First the main loop hangs util an
>> I/O request is completed and secondly if the I/O request does not
>> complete (e.g. due to an unresponsive storage) Qemu hangs completely.
>> 
>> Note: Due to possible race conditions requests during an ongoing
>> elementary transfer are still sync.
>> 
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> hw/ide/atapi.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 85 insertions(+), 12 deletions(-)
>> 
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 747f466..cfd2d63 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -105,33 +105,98 @@ static void cd_data_to_raw(uint8_t *buf, int lba)
>>     memset(buf, 0, 288);
>> }
>> 
>> -static int cd_read_sector(IDEState *s, int lba, uint8_t *buf, int sector_size)
>> +static int
>> +cd_read_sector_sync(IDEState *s)
>> {
>>     int ret;
>> 
>> -    switch(sector_size) {
>> +#ifdef DEBUG_IDE_ATAPI
>> +    printf("cd_read_sector_sync: lba=%d\n", s->lba);
>> +#endif
>> +
>> +    switch (s->cd_sector_size) {
>>     case 2048:
>>         block_acct_start(blk_get_stats(s->blk), &s->acct,
>>                          4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
>> -        ret = blk_read(s->blk, (int64_t)lba << 2, buf, 4);
>> +        ret = blk_read(s->blk, (int64_t)s->lba << 2,
>> +                       s->io_buffer, 4);
>>         block_acct_done(blk_get_stats(s->blk), &s->acct);
>>         break;
>>     case 2352:
>>         block_acct_start(blk_get_stats(s->blk), &s->acct,
>>                          4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
>> -        ret = blk_read(s->blk, (int64_t)lba << 2, buf + 16, 4);
>> +        ret = blk_read(s->blk, (int64_t)s->lba << 2,
>> +                       s->io_buffer + 16, 4);
>>         block_acct_done(blk_get_stats(s->blk), &s->acct);
>>         if (ret < 0)
>>             return ret;
>> -        cd_data_to_raw(buf, lba);
>> +        cd_data_to_raw(s->io_buffer, s->lba);
>>         break;
>>     default:
>>         ret = -EIO;
>>         break;
>>     }
>> +
>> +    if (!ret) {
>> +        s->lba++;
>> +        s->io_buffer_index = 0;
>> +    }
>> +
>>     return ret;
>> }
>> 
>> +static void cd_read_sector_cb(void *opaque, int ret)
>> +{
>> +    IDEState *s = opaque;
>> +
>> +    block_acct_done(blk_get_stats(s->blk), &s->acct);
>> +
>> +#ifdef DEBUG_IDE_ATAPI
>> +    printf("cd_read_sector_cb: lba=%d ret=%d\n", s->lba, ret);
>> +#endif
>> +
>> +    if (ret < 0) {
>> +        ide_atapi_io_error(s, ret);
>> +        return;
>> +    }
>> +
>> +    if (s->cd_sector_size == 2352) {
>> +        cd_data_to_raw(s->io_buffer, s->lba);
>> +    }
>> +
>> +    s->lba++;
>> +    s->io_buffer_index = 0;
>> +    s->status &= ~BUSY_STAT;
>> +
>> +    ide_atapi_cmd_reply_end(s);
>> +}
>> +
>> +static int cd_read_sector(IDEState *s)
>> +{
>> +    if (s->cd_sector_size != 2048 && s->cd_sector_size != 2352) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    s->iov.iov_base = (s->cd_sector_size == 2352) ?
>> +                      s->io_buffer + 16 : s->io_buffer;
>> +
>> +    s->iov.iov_len = 4 * BDRV_SECTOR_SIZE;
>> +    qemu_iovec_init_external(&s->qiov, &s->iov, 1);
>> +
>> +#ifdef DEBUG_IDE_ATAPI
>> +    printf("cd_read_sector: lba=%d\n", s->lba);
>> +#endif
>> +
>> +    block_acct_start(blk_get_stats(s->blk), &s->acct,
>> +                     4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
>> +
>> +    blk_aio_readv(s->blk, (int64_t)s->lba << 2, &s->qiov, 4,
>> +                  cd_read_sector_cb, s);
>> +
>> +    s->status |= BUSY_STAT;
>> +    return 0;
>> +}
>> +
>> void ide_atapi_cmd_ok(IDEState *s)
>> {
>>     s->error = 0;
>> @@ -182,18 +247,27 @@ void ide_atapi_cmd_reply_end(IDEState *s)
>>         ide_atapi_cmd_ok(s);
>>         ide_set_irq(s->bus);
>> #ifdef DEBUG_IDE_ATAPI
>> -        printf("status=0x%x\n", s->status);
>> +        printf("end of transfer, status=0x%x\n", s->status);
>> #endif
>>     } else {
>>         /* see if a new sector must be read */
>>         if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) {
>> -            ret = cd_read_sector(s, s->lba, s->io_buffer, s->cd_sector_size);
>> -            if (ret < 0) {
>> -                ide_atapi_io_error(s, ret);
>> +            if (!s->elementary_transfer_size) {
>> +                ret = cd_read_sector(s);
>> +                if (ret < 0) {
>> +                    ide_atapi_io_error(s, ret);
>> +                }
>>                 return;
>> +            } else {
>> +                /* rebuffering within an elementary transfer is
>> +                 * only possible with a sync request because we
>> +                 * end up with a race condition otherwise */
>> +                ret = cd_read_sector_sync(s);
>> +                if (ret < 0) {
>> +                    ide_atapi_io_error(s, ret);
>> +                    return;
>> +                }
>>             }
>> -            s->lba++;
>> -            s->io_buffer_index = 0;
>>         }
>>         if (s->elementary_transfer_size > 0) {
>>             /* there are some data left to transmit in this elementary
>> @@ -275,7 +349,6 @@ static void ide_atapi_cmd_read_pio(IDEState *s, int lba, int nb_sectors,
>>     s->io_buffer_index = sector_size;
>>     s->cd_sector_size = sector_size;
>> 
>> -    s->status = READY_STAT | SEEK_STAT;
>>     ide_atapi_cmd_reply_end(s);
>> }
> 
> Had to rebase against Berto's patches, but nothing major. You can see my
> rebase at:
> 
> https://github.com/jnsnow/qemu/tree/review-plieven
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

in Patch 1 the indent is off at the end. Otherwise looks good to me.

Peter