Re: [Qemu-devel] [PATCH 1/4] ide/atapi: make PIO read requests async

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, Oct 12, 2015 at 02:27:22PM +0200, Peter Lieven wrote:
> @@ -129,9 +134,71 @@ static int cd_read_sector(IDEState *s, int lba, uint8_t *buf, int sector_size)
>          ret = -EIO;
>          break;
>      }
> +
> +    if (!ret) {
> +        s->lba++;

This function probably shouldn't take the lba argument if it modifies
s->lba.  You dropped the sector_size argument and I think the lba
argument should be dropped for the same reason.

> +static int cd_read_sector(IDEState *s, int lba, void *buf)
> +{
> +    if (s->cd_sector_size != 2048 && s->cd_sector_size != 2352) {
> +        return -EINVAL;
> +    }
> +
> +    s->iov.iov_base = buf;
> +    if (s->cd_sector_size == 2352) {
> +        buf += 16;
> +    }
> +
> +    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", lba);
> +#endif
> +
> +    if (blk_aio_readv(s->blk, (int64_t)lba << 2, &s->qiov, 4,
> +                      cd_read_sector_cb, s) == NULL) {

This function never returns NULL, the if statement can be removed.

Doesn't the aiocb need to be stored for cancellation (e.g. device reset)?

> +        return -EIO;
> +    }
> +
> +    block_acct_start(blk_get_stats(s->blk), &s->acct,
> +                     4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);

Why does accounting start *after* the read request has been submitted?