[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH V4 1/6] ide/atapi: make PIO read re
From: |
John Snow |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH V4 1/6] ide/atapi: make PIO read requests async |
Date: |
Fri, 13 Nov 2015 18:12:17 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 11/13/2015 06:00 PM, Peter Lieven wrote:
>
>
>> Am 13.11.2015 um 23:42 schrieb John Snow <address@hidden>:
>>
>>
>>
>>> 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 <address@hidden>
>>> ---
>>> 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 <address@hidden>
>
> in Patch 1 the indent is off at the end. Otherwise looks good to me.
>
> Peter
>
Huh, so it is. That part wasn't even in the chunk that conflicted. :\
- [Qemu-block] [PATCH V4 0/6] ide: avoid main-loop hang on CDROM/NFS failure, Peter Lieven, 2015/11/12
- [Qemu-block] [PATCH V4 3/6] ide: add support for IDEBufferedRequest, Peter Lieven, 2015/11/12
- [Qemu-block] [PATCH V4 4/6] ide: orphan all buffered requests on DMA cancel, Peter Lieven, 2015/11/12
- [Qemu-block] [PATCH V4 2/6] block: add blk_abort_aio_request, Peter Lieven, 2015/11/12
- [Qemu-block] [PATCH V4 6/6] ide: enable buffered requests for PIO read requests, Peter Lieven, 2015/11/12
- [Qemu-block] [PATCH V4 5/6] ide: enable buffered requests for ATAPI devices, Peter Lieven, 2015/11/12
- Re: [Qemu-block] [Qemu-devel] [PATCH V4 0/6] ide: avoid main-loop hang on CDROM/NFS failure, Mark Cave-Ayland, 2015/11/13
- Re: [Qemu-block] [Qemu-devel] [PATCH V4 0/6] ide: avoid main-loop hang on CDROM/NFS failure, John Snow, 2015/11/13