From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35498) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjOQq-0003tF-1g for qemu-devel@nongnu.org; Tue, 06 Oct 2015 05:21:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZjOQm-0000yI-Ry for qemu-devel@nongnu.org; Tue, 06 Oct 2015 05:21:12 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:59731 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjOQm-0000xi-AG for qemu-devel@nongnu.org; Tue, 06 Oct 2015 05:21:08 -0400 References: <1442838328-23117-1-git-send-email-pl@kamp.de> <1442838328-23117-2-git-send-email-pl@kamp.de> <5612E873.1090503@redhat.com> <20151006085736.GA3707@noname.str.redhat.com> From: Peter Lieven Message-ID: <56139278.2090002@kamp.de> Date: Tue, 6 Oct 2015 11:20:56 +0200 MIME-Version: 1.0 In-Reply-To: <20151006085736.GA3707@noname.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/5] ide/atapi: make PIO read requests async List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , John Snow Cc: stefanha@gmail.com, jcody@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org Am 06.10.2015 um 10:57 schrieb Kevin Wolf: > Am 05.10.2015 um 23:15 hat John Snow geschrieben: >> >> On 09/21/2015 08:25 AM, Peter Lieven wrote: >>> PIO read requests on the ATAPI interface used to be sync blk requests. >>> This has to siginificant 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. >>> >>> Signed-off-by: Peter Lieven >>> --- >>> hw/ide/atapi.c | 69 ++++++++++++++++++++++++++++++++++++---------------------- >>> 1 file changed, 43 insertions(+), 26 deletions(-) >>> >>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c >>> index 747f466..9257e1c 100644 >>> --- a/hw/ide/atapi.c >>> +++ b/hw/ide/atapi.c >>> @@ -105,31 +105,51 @@ 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 void cd_read_sector_cb(void *opaque, int ret) >>> { >>> - int ret; >>> + IDEState *s = opaque; >>> >>> - switch(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); >>> - 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); >>> - block_acct_done(blk_get_stats(s->blk), &s->acct); >>> - if (ret < 0) >>> - return ret; >>> - cd_data_to_raw(buf, lba); >>> - break; >>> - default: >>> - ret = -EIO; >>> - break; >>> + block_acct_done(blk_get_stats(s->blk), &s->acct); >>> + >>> + 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); >>> } >>> - return ret; >>> + >>> + s->lba++; >>> + s->io_buffer_index = 0; >>> + s->status &= ~BUSY_STAT; >>> + >>> + ide_atapi_cmd_reply_end(s); >>> +} >>> + >>> +static int cd_read_sector(IDEState *s, int lba, void *buf, int sector_size) >>> +{ >>> + if (sector_size != 2048 && sector_size != 2352) { >>> + return -EINVAL; >>> + } >>> + >>> + s->iov.iov_base = buf; >>> + if (sector_size == 2352) { >>> + buf += 4; >>> + } > This doesn't look quite right, buf is never read after this. > > Also, why +=4 when it was originally buf + 16? You are right. I mixed that up. > >>> + >>> + s->iov.iov_len = 4 * BDRV_SECTOR_SIZE; >>> + qemu_iovec_init_external(&s->qiov, &s->iov, 1); >>> + >>> + if (blk_aio_readv(s->blk, (int64_t)lba << 2, &s->qiov, 4, >>> + cd_read_sector_cb, s) == NULL) { >>> + return -EIO; >>> + } >>> + >>> + block_acct_start(blk_get_stats(s->blk), &s->acct, >>> + 4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ); >>> + s->status |= BUSY_STAT; >>> + return 0; >>> } >>> >> We discussed this off-list a bit, but for upstream synchronization: >> >> Unfortunately, I believe making cd_read_sector here non-blocking makes >> ide_atapi_cmd_reply_end non-blocking, and as a result makes calls to >> s->end_transfer_func() nonblocking, which functions like ide_data_readw >> are not prepared to cope with. > I don't think that's a problem as long as BSY is set while the > asynchronous command is running and DRQ is cleared. The latter will > protect ide_data_readw(). ide_sector_read() does essentially the same > thing. I was thinking the same. Without the BSY its not working at all. > > Or maybe I'm just missing what you're trying to say. > >> My suggestion is to buffer an entire DRQ block of data at once >> (byte_count_limit) to avoid the problem. > No matter whether there is a problem or not, buffering more data at once > (and therefore doing less requests) is better for performance anyway. Its possible to do only one read in the backend and read the whole request into the IO buffer. I send a follow-up. Maybe do you have a pointer to the test tool that John mentioned? Peter