qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Lieven
Subject: Re: [Qemu-devel] [PATCH 1/4] ide/atapi: make PIO read requests async
Date: Tue, 3 Nov 2015 08:03:09 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

Am 03.11.2015 um 01:48 schrieb John Snow:

On 10/12/2015 08:27 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.

Signed-off-by: Peter Lieven <address@hidden>
---
  hw/ide/atapi.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
  1 file changed, 84 insertions(+), 9 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 747f466..2271ea2 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -105,11 +105,16 @@ 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 lba, uint8_t *buf)
  {
      int ret;
- switch(sector_size) {
+#ifdef DEBUG_IDE_ATAPI
+    printf("cd_read_sector_sync: lba=%d\n", 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);
@@ -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++;
+        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, 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) {
+        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;
+}
+
  void ide_atapi_cmd_ok(IDEState *s)
  {
      s->error = 0;
@@ -182,18 +249,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, s->lba, s->io_buffer);
+                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, s->lba, s->io_buffer);
+                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 +351,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);
  }
This patch looks good to me, apart from Stefan's other comments. Will
you be sending a V3? I can try to merge it for this week before the hard
freeze hits.

I will look at this today.

Peter




reply via email to

[Prev in Thread] Current Thread [Next in Thread]