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

[Peter Lieven <pl@kamp.de>]

Am 22.10.2015 um 18:17 schrieb Stefan Hajnoczi:
> 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.

Okay, that was my fault. I was believing it could return NULL.

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

The ide_readv_cancelable function introduced in Patch 3 will store
the aiocb. At this point there is blk_drain_all called when the device is 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?

You are right it has to start before the request.

Peter