[Qemu-devel] [PATCH V3 0/6] ide: avoid main-loop hang on CDROM/NFS failure

[Qemu-devel] [PATCH V3 0/6] ide: avoid main-loop hang on CDROM/NFS failure

[Peter Lieven]

This series aims at avoiding a hanging main-loop if a vserver has a
CDROM image mounted from a NFS share and that NFS share goes down.
Typical situation is that users mount an CDROM ISO to install something
and then forget to eject that CDROM afterwards.
As a consequence this mounted CD is able to bring down the
whole vserver if the backend NFS share is unreachable. This is bad
especially if the CDROM itself is not needed anymore at this point.

v2->v3: - adressed Stefans comments on Patch 1
        - added patches 2,4,5,6
        - avoided the term cancel in Patch 3. Added an iovec,
          added a BH [Stefan]
v1->v2: - fix offset for 2352 byte sector size [Kevin]
        - use a sync request if we continue an elementary transfer.
          As John pointed out we enter a race condition between next
          IDE command and async transfer otherwise. This is sill not
          optimal, but it fixes the NFS down problems for all cases where
          the NFS server goes down while there is no PIO CD activity.
          Of course, it could still happen during a PIO transfer, but I
          expect this to be the unlikelier case.
          I spent some effort trying to read more sectors at once and
          avoiding continuation of elementary transfers, but with
          whatever I came up it was destroying migration between different
          Qemu versions. I have a quite hackish patch that works and
          should survive migration, but I am not happy with it. So I
          would like to start with this version as it is a big improvement
          already.
        - Dropped Patch 5 because it is upstream meanwhile.

Peter Lieven (6):
  ide/atapi: make PIO read requests async
  block: add blk_abort_aio_request
  ide: add support for IDEBufferedRequest
  ide: orphan all buffered requests on DMA cancel
  ide: enable buffered requests for ATAPI devices
  ide: enable buffered requests for PIO read requests

 block/block-backend.c          |  17 +++----
 hw/ide/atapi.c                 | 103 +++++++++++++++++++++++++++++++++++------
 hw/ide/core.c                  |  50 +++++++++++++++++++-
 hw/ide/internal.h              |  14 ++++++
 hw/ide/pci.c                   |  19 ++++++++
 include/sysemu/block-backend.h |   3 ++
 6 files changed, 181 insertions(+), 25 deletions(-)

-- 
1.9.1

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

[Peter Lieven]

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 <pl@kamp.de>
---
 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..29fd131 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, uint8_t *buf)
 {
     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, 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);
+        ret = blk_read(s->blk, (int64_t)s->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);
+        cd_data_to_raw(buf, 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, 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", 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, 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->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 +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);
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH V3 2/6] block: add blk_abort_aio_request

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/block-backend.c          | 17 +++++++++--------
 include/sysemu/block-backend.h |  3 +++
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 19fdaae..b13dc4e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -627,8 +627,9 @@ static void error_callback_bh(void *opaque)
     qemu_aio_unref(acb);
 }
 
-static BlockAIOCB *abort_aio_request(BlockBackend *blk, BlockCompletionFunc *cb,
-                                     void *opaque, int ret)
+BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
+                                   BlockCompletionFunc *cb,
+                                   void *opaque, int ret)
 {
     struct BlockBackendAIOCB *acb;
     QEMUBH *bh;
@@ -650,7 +651,7 @@ BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
 {
     int ret = blk_check_request(blk, sector_num, nb_sectors);
     if (ret < 0) {
-        return abort_aio_request(blk, cb, opaque, ret);
+        return blk_abort_aio_request(blk, cb, opaque, ret);
     }
 
     return bdrv_aio_write_zeroes(blk->bs, sector_num, nb_sectors, flags,
@@ -710,7 +711,7 @@ BlockAIOCB *blk_aio_readv(BlockBackend *blk, int64_t sector_num,
 {
     int ret = blk_check_request(blk, sector_num, nb_sectors);
     if (ret < 0) {
-        return abort_aio_request(blk, cb, opaque, ret);
+        return blk_abort_aio_request(blk, cb, opaque, ret);
     }
 
     return bdrv_aio_readv(blk->bs, sector_num, iov, nb_sectors, cb, opaque);
@@ -722,7 +723,7 @@ BlockAIOCB *blk_aio_writev(BlockBackend *blk, int64_t sector_num,
 {
     int ret = blk_check_request(blk, sector_num, nb_sectors);
     if (ret < 0) {
-        return abort_aio_request(blk, cb, opaque, ret);
+        return blk_abort_aio_request(blk, cb, opaque, ret);
     }
 
     return bdrv_aio_writev(blk->bs, sector_num, iov, nb_sectors, cb, opaque);
@@ -732,7 +733,7 @@ BlockAIOCB *blk_aio_flush(BlockBackend *blk,
                           BlockCompletionFunc *cb, void *opaque)
 {
     if (!blk_is_available(blk)) {
-        return abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
+        return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
     }
 
     return bdrv_aio_flush(blk->bs, cb, opaque);
@@ -744,7 +745,7 @@ BlockAIOCB *blk_aio_discard(BlockBackend *blk,
 {
     int ret = blk_check_request(blk, sector_num, nb_sectors);
     if (ret < 0) {
-        return abort_aio_request(blk, cb, opaque, ret);
+        return blk_abort_aio_request(blk, cb, opaque, ret);
     }
 
     return bdrv_aio_discard(blk->bs, sector_num, nb_sectors, cb, opaque);
@@ -787,7 +788,7 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
                           BlockCompletionFunc *cb, void *opaque)
 {
     if (!blk_is_available(blk)) {
-        return abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
+        return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
     }
 
     return bdrv_aio_ioctl(blk->bs, req, buf, cb, opaque);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9306a52..b5267a8 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -180,5 +180,8 @@ int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
 int blk_load_vmstate(BlockBackend *blk, uint8_t *buf, int64_t pos, int size);
 int blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz);
 int blk_probe_geometry(BlockBackend *blk, HDGeometry *geo);
+BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
+                                   BlockCompletionFunc *cb,
+                                   void *opaque, int ret);
 
 #endif
-- 
1.9.1

[Qemu-devel] [PATCH V3 3/6] ide: add support for IDEBufferedRequest

[Peter Lieven]

this patch adds a new aio readv compatible function which copies
all data through a bounce buffer. These buffered requests can be
flagged as orphaned which means that their original callback has
already been invoked and the request has just not been completed
by the backend storage. The bounce buffer guarantees that guest
memory corruption is avoided when such a orphaned request is
completed by the backend at a later stage.

This trick only works for read requests as a write request completed
at a later stage might corrupt data as there is no way to control
if and what data has already been written to the storage.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 hw/ide/core.c     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/ide/internal.h | 14 ++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 364ba21..53f9c2c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -561,6 +561,52 @@ static bool ide_sect_range_ok(IDEState *s,
     return true;
 }
 
+static void ide_buffered_readv_cb(void *opaque, int ret)
+{
+    IDEBufferedRequest *req = opaque;
+    if (!req->orphaned) {
+        if (!ret) {
+            qemu_iovec_from_buf(req->original_qiov, 0, req->iov.iov_base,
+                                req->original_qiov->size);
+        }
+        req->original_cb(req->original_opaque, ret);
+    }
+    QLIST_REMOVE(req, list);
+    g_free(req);
+}
+
+#define MAX_BUFFERED_REQS 16
+
+BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num,
+                               QEMUIOVector *iov, int nb_sectors,
+                               BlockCompletionFunc *cb, void *opaque)
+{
+    BlockAIOCB *aioreq;
+    IDEBufferedRequest *req;
+    int c = 0;
+
+    QLIST_FOREACH(req, &s->buffered_requests, list) {
+        c++;
+    }
+    if (c > MAX_BUFFERED_REQS) {
+        return blk_abort_aio_request(s->blk, cb, opaque, -EIO);
+    }
+
+    req = g_new0(IDEBufferedRequest, 1);
+    req->original_qiov = iov;
+    req->original_cb = cb;
+    req->original_opaque = opaque;
+    req->iov.iov_base = qemu_blockalign(blk_bs(s->blk), iov->size);
+    req->iov.iov_len = iov->size;
+    qemu_iovec_init_external(&req->qiov, &req->iov, 1);
+
+    aioreq = blk_aio_readv(s->blk, sector_num, &req->qiov, nb_sectors,
+                           ide_buffered_readv_cb, req);
+
+    QLIST_INSERT_HEAD(&s->buffered_requests, req, list);
+    return aioreq;
+}
+
 static void ide_sector_read(IDEState *s);
 
 static void ide_sector_read_cb(void *opaque, int ret)
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 05e93ff..3d1116f 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -343,6 +343,16 @@ enum ide_dma_cmd {
 #define ide_cmd_is_read(s) \
 	((s)->dma_cmd == IDE_DMA_READ)
 
+typedef struct IDEBufferedRequest {
+    QLIST_ENTRY(IDEBufferedRequest) list;
+    struct iovec iov;
+    QEMUIOVector qiov;
+    QEMUIOVector *original_qiov;
+    BlockCompletionFunc *original_cb;
+    void *original_opaque;
+    bool orphaned;
+} IDEBufferedRequest;
+
 /* NOTE: IDEState represents in fact one drive */
 struct IDEState {
     IDEBus *bus;
@@ -396,6 +406,7 @@ struct IDEState {
     BlockAIOCB *pio_aiocb;
     struct iovec iov;
     QEMUIOVector qiov;
+    QLIST_HEAD(, IDEBufferedRequest) buffered_requests;
     /* ATA DMA state */
     int32_t io_buffer_offset;
     int32_t io_buffer_size;
@@ -572,6 +583,9 @@ void ide_set_inactive(IDEState *s, bool more);
 BlockAIOCB *ide_issue_trim(BlockBackend *blk,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockCompletionFunc *cb, void *opaque);
+BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num,
+                               QEMUIOVector *iov, int nb_sectors,
+                               BlockCompletionFunc *cb, void *opaque);
 
 /* hw/ide/atapi.c */
 void ide_atapi_cmd(IDEState *s);
-- 
1.9.1

[Qemu-devel] [PATCH V3 4/6] ide: orphan all buffered requests on DMA cancel

[Peter Lieven]

If the guests canceles a DMA request we can prematurely
invoke all callbacks of buffered requests and flag all them
as orphaned. Ideally this avoids the need for draining all
requests. For CDROM devices this works in 100% of all cases.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 hw/ide/pci.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index d31ff88..a9e164e 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -240,6 +240,22 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
     /* Ignore writes to SSBM if it keeps the old value */
     if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
         if (!(val & BM_CMD_START)) {
+            /* First invoke the callbacks of all buffered requests
+             * and flag those requests as orphaned. Ideally there
+             * are no unbuffered (Scatter Gather DMA Requests or
+             * write requests) pending and we can avoid to drain. */
+            IDEBufferedRequest *req;
+            IDEState *s = idebus_active_if(bm->bus);
+            QLIST_FOREACH(req, &s->buffered_requests, list) {
+                if (!req->orphaned) {
+#ifdef DEBUG_IDE
+                    printf("%s: invoking cb %p of buffered request %p with"
+                           " -ECANCELED\n", __func__, req->original_cb, req);
+#endif
+                    req->original_cb(req->original_opaque, -ECANCELED);
+                }
+                req->orphaned = true;
+            }
             /*
              * We can't cancel Scatter Gather DMA in the middle of the
              * operation or a partial (not full) DMA transfer would reach
@@ -253,6 +269,9 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
              * aio operation with preadv/pwritev.
              */
             if (bm->bus->dma->aiocb) {
+#ifdef DEBUG_IDE
+                printf("%s: draining all remaining requests", __func__);
+#endif
                 blk_drain_all();
                 assert(bm->bus->dma->aiocb == NULL);
             }
-- 
1.9.1

[Qemu-devel] [PATCH V3 5/6] ide: enable buffered requests for ATAPI devices

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 hw/ide/atapi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 29fd131..2f6d018 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -190,8 +190,8 @@ static int cd_read_sector(IDEState *s, void *buf)
     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);
+    ide_buffered_readv(s, (int64_t)s->lba << 2, &s->qiov, 4,
+                       cd_read_sector_cb, s);
 
     s->status |= BUSY_STAT;
     return 0;
@@ -424,9 +424,9 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
     s->bus->dma->iov.iov_len = n * 4 * 512;
     qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
 
-    s->bus->dma->aiocb = blk_aio_readv(s->blk, (int64_t)s->lba << 2,
-                                       &s->bus->dma->qiov, n * 4,
-                                       ide_atapi_cmd_read_dma_cb, s);
+    s->bus->dma->aiocb = ide_buffered_readv(s, (int64_t)s->lba << 2,
+                                            &s->bus->dma->qiov, n * 4,
+                                            ide_atapi_cmd_read_dma_cb, s);
     return;
 
 eot:
-- 
1.9.1

[Qemu-devel] [PATCH V3 6/6] ide: enable buffered requests for PIO read requests

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 hw/ide/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 53f9c2c..d1feae2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -676,8 +676,8 @@ static void ide_sector_read(IDEState *s)
 
     block_acct_start(blk_get_stats(s->blk), &s->acct,
                      n * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
-    s->pio_aiocb = blk_aio_readv(s->blk, sector_num, &s->qiov, n,
-                                 ide_sector_read_cb, s);
+    s->pio_aiocb = ide_buffered_readv(s, sector_num, &s->qiov, n,
+                                      ide_sector_read_cb, s);
 }
 
 void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
-- 
1.9.1

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

[John Snow]

On 11/06/2015 03:42 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 <pl@kamp.de>
> ---
>  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..29fd131 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, uint8_t *buf)
>  {
>      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, 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);
> +        ret = blk_read(s->blk, (int64_t)s->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);
> +        cd_data_to_raw(buf, 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, 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", 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, 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->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 +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);
>  }
>  
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Stefan, do you have time to re-review the latter portion of this series
before hard freeze? I'd like to get this series in before rc0 if at all
possible, to benefit from all the RC testing.

--js

Re: [Qemu-devel] [PATCH V3 2/6] block: add blk_abort_aio_request

[Fam Zheng]

On Fri, 11/06 09:42, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/block-backend.c          | 17 +++++++++--------
>  include/sysemu/block-backend.h |  3 +++
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 19fdaae..b13dc4e 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -627,8 +627,9 @@ static void error_callback_bh(void *opaque)
>      qemu_aio_unref(acb);
>  }
>  
> -static BlockAIOCB *abort_aio_request(BlockBackend *blk, BlockCompletionFunc *cb,
> -                                     void *opaque, int ret)
> +BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
> +                                   BlockCompletionFunc *cb,
> +                                   void *opaque, int ret)

Parameter list identation is off by one.

>  {
>      struct BlockBackendAIOCB *acb;
>      QEMUBH *bh;
> @@ -650,7 +651,7 @@ BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
>  {
>      int ret = blk_check_request(blk, sector_num, nb_sectors);
>      if (ret < 0) {
> -        return abort_aio_request(blk, cb, opaque, ret);
> +        return blk_abort_aio_request(blk, cb, opaque, ret);
>      }
>  
>      return bdrv_aio_write_zeroes(blk->bs, sector_num, nb_sectors, flags,
> @@ -710,7 +711,7 @@ BlockAIOCB *blk_aio_readv(BlockBackend *blk, int64_t sector_num,
>  {
>      int ret = blk_check_request(blk, sector_num, nb_sectors);
>      if (ret < 0) {
> -        return abort_aio_request(blk, cb, opaque, ret);
> +        return blk_abort_aio_request(blk, cb, opaque, ret);
>      }
>  
>      return bdrv_aio_readv(blk->bs, sector_num, iov, nb_sectors, cb, opaque);
> @@ -722,7 +723,7 @@ BlockAIOCB *blk_aio_writev(BlockBackend *blk, int64_t sector_num,
>  {
>      int ret = blk_check_request(blk, sector_num, nb_sectors);
>      if (ret < 0) {
> -        return abort_aio_request(blk, cb, opaque, ret);
> +        return blk_abort_aio_request(blk, cb, opaque, ret);
>      }
>  
>      return bdrv_aio_writev(blk->bs, sector_num, iov, nb_sectors, cb, opaque);
> @@ -732,7 +733,7 @@ BlockAIOCB *blk_aio_flush(BlockBackend *blk,
>                            BlockCompletionFunc *cb, void *opaque)
>  {
>      if (!blk_is_available(blk)) {
> -        return abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
> +        return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
>      }
>  
>      return bdrv_aio_flush(blk->bs, cb, opaque);
> @@ -744,7 +745,7 @@ BlockAIOCB *blk_aio_discard(BlockBackend *blk,
>  {
>      int ret = blk_check_request(blk, sector_num, nb_sectors);
>      if (ret < 0) {
> -        return abort_aio_request(blk, cb, opaque, ret);
> +        return blk_abort_aio_request(blk, cb, opaque, ret);
>      }
>  
>      return bdrv_aio_discard(blk->bs, sector_num, nb_sectors, cb, opaque);
> @@ -787,7 +788,7 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
>                            BlockCompletionFunc *cb, void *opaque)
>  {
>      if (!blk_is_available(blk)) {
> -        return abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
> +        return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
>      }
>  
>      return bdrv_aio_ioctl(blk->bs, req, buf, cb, opaque);
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 9306a52..b5267a8 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -180,5 +180,8 @@ int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
>  int blk_load_vmstate(BlockBackend *blk, uint8_t *buf, int64_t pos, int size);
>  int blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz);
>  int blk_probe_geometry(BlockBackend *blk, HDGeometry *geo);
> +BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
> +                                   BlockCompletionFunc *cb,
> +                                   void *opaque, int ret);
>  

Same here.

>  #endif
> -- 
> 1.9.1
> 
> 

Re: [Qemu-devel] [PATCH V3 4/6] ide: orphan all buffered requests on DMA cancel

[Fam Zheng]

On Fri, 11/06 09:42, Peter Lieven wrote:
> If the guests canceles a DMA request we can prematurely
> invoke all callbacks of buffered requests and flag all them
> as orphaned. Ideally this avoids the need for draining all
> requests. For CDROM devices this works in 100% of all cases.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  hw/ide/pci.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index d31ff88..a9e164e 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -240,6 +240,22 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
>      /* Ignore writes to SSBM if it keeps the old value */
>      if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
>          if (!(val & BM_CMD_START)) {
> +            /* First invoke the callbacks of all buffered requests
> +             * and flag those requests as orphaned. Ideally there
> +             * are no unbuffered (Scatter Gather DMA Requests or
> +             * write requests) pending and we can avoid to drain. */
> +            IDEBufferedRequest *req;
> +            IDEState *s = idebus_active_if(bm->bus);
> +            QLIST_FOREACH(req, &s->buffered_requests, list) {
> +                if (!req->orphaned) {
> +#ifdef DEBUG_IDE
> +                    printf("%s: invoking cb %p of buffered request %p with"
> +                           " -ECANCELED\n", __func__, req->original_cb, req);
> +#endif
> +                    req->original_cb(req->original_opaque, -ECANCELED);
> +                }
> +                req->orphaned = true;
> +            }

Why not use bdrv_aio_cancel or bdrv_aio_cancel_async with the aio returned by
bdrv_aio_cancel?

Fam

>              /*
>               * We can't cancel Scatter Gather DMA in the middle of the
>               * operation or a partial (not full) DMA transfer would reach
> @@ -253,6 +269,9 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
>               * aio operation with preadv/pwritev.
>               */
>              if (bm->bus->dma->aiocb) {
> +#ifdef DEBUG_IDE
> +                printf("%s: draining all remaining requests", __func__);
> +#endif
>                  blk_drain_all();
>                  assert(bm->bus->dma->aiocb == NULL);
>              }
> -- 
> 1.9.1
> 
> 

Re: [Qemu-devel] [PATCH V3 4/6] ide: orphan all buffered requests on DMA cancel

[Peter Lieven]

Am 12.11.2015 um 09:27 schrieb Fam Zheng:
> On Fri, 11/06 09:42, Peter Lieven wrote:
>> If the guests canceles a DMA request we can prematurely
>> invoke all callbacks of buffered requests and flag all them
>> as orphaned. Ideally this avoids the need for draining all
>> requests. For CDROM devices this works in 100% of all cases.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   hw/ide/pci.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index d31ff88..a9e164e 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -240,6 +240,22 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
>>       /* Ignore writes to SSBM if it keeps the old value */
>>       if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
>>           if (!(val & BM_CMD_START)) {
>> +            /* First invoke the callbacks of all buffered requests
>> +             * and flag those requests as orphaned. Ideally there
>> +             * are no unbuffered (Scatter Gather DMA Requests or
>> +             * write requests) pending and we can avoid to drain. */
>> +            IDEBufferedRequest *req;
>> +            IDEState *s = idebus_active_if(bm->bus);
>> +            QLIST_FOREACH(req, &s->buffered_requests, list) {
>> +                if (!req->orphaned) {
>> +#ifdef DEBUG_IDE
>> +                    printf("%s: invoking cb %p of buffered request %p with"
>> +                           " -ECANCELED\n", __func__, req->original_cb, req);
>> +#endif
>> +                    req->original_cb(req->original_opaque, -ECANCELED);
>> +                }
>> +                req->orphaned = true;
>> +            }
> Why not use bdrv_aio_cancel or bdrv_aio_cancel_async with the aio returned by
> bdrv_aio_cancel?

bdrv_aio_cancel would block until the request is completed, that wouldn't help if
the storage is no longer responsive.

The trick with the buffered request is that we can avoid waiting for the storage and
guarantee that a later completion on the storage won't corrupt guest memory.

Peter

Re: [Qemu-devel] [PATCH V3 3/6] ide: add support for IDEBufferedRequest

[Fam Zheng]

On Fri, 11/06 09:42, Peter Lieven wrote:
> +BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num,
> +                               QEMUIOVector *iov, int nb_sectors,
> +                               BlockCompletionFunc *cb, void *opaque)
> +{
> +    BlockAIOCB *aioreq;
> +    IDEBufferedRequest *req;
> +    int c = 0;
> +
> +    QLIST_FOREACH(req, &s->buffered_requests, list) {
> +        c++;
> +    }
> +    if (c > MAX_BUFFERED_REQS) {
> +        return blk_abort_aio_request(s->blk, cb, opaque, -EIO);
> +    }
> +
> +    req = g_new0(IDEBufferedRequest, 1);
> +    req->original_qiov = iov;
> +    req->original_cb = cb;
> +    req->original_opaque = opaque;
> +    req->iov.iov_base = qemu_blockalign(blk_bs(s->blk), iov->size);

Where is this bounce buffer freed?

> +    req->iov.iov_len = iov->size;
> +    qemu_iovec_init_external(&req->qiov, &req->iov, 1);
> +
> +    aioreq = blk_aio_readv(s->blk, sector_num, &req->qiov, nb_sectors,
> +                           ide_buffered_readv_cb, req);
> +
> +    QLIST_INSERT_HEAD(&s->buffered_requests, req, list);
> +    return aioreq;
> +}
> +
>  static void ide_sector_read(IDEState *s);
>  
>  static void ide_sector_read_cb(void *opaque, int ret)

Re: [Qemu-devel] [PATCH V3 3/6] ide: add support for IDEBufferedRequest

[Peter Lieven]

Am 12.11.2015 um 10:57 schrieb Fam Zheng:
> On Fri, 11/06 09:42, Peter Lieven wrote:
>> +BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num,
>> +                               QEMUIOVector *iov, int nb_sectors,
>> +                               BlockCompletionFunc *cb, void *opaque)
>> +{
>> +    BlockAIOCB *aioreq;
>> +    IDEBufferedRequest *req;
>> +    int c = 0;
>> +
>> +    QLIST_FOREACH(req, &s->buffered_requests, list) {
>> +        c++;
>> +    }
>> +    if (c > MAX_BUFFERED_REQS) {
>> +        return blk_abort_aio_request(s->blk, cb, opaque, -EIO);
>> +    }
>> +
>> +    req = g_new0(IDEBufferedRequest, 1);
>> +    req->original_qiov = iov;
>> +    req->original_cb = cb;
>> +    req->original_opaque = opaque;
>> +    req->iov.iov_base = qemu_blockalign(blk_bs(s->blk), iov->size);
> Where is this bounce buffer freed?

Oops, during conversion form req->buf to req->iov this got lost.
It should be freed in ide_buffered_readv_cb.

I will respin after you had a look at the other patches as well.

Thanks,
Peter

Re: [Qemu-devel] [PATCH V3 5/6] ide: enable buffered requests for ATAPI devices

[Fam Zheng]

On Fri, 11/06 09:42, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  hw/ide/atapi.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 29fd131..2f6d018 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -190,8 +190,8 @@ static int cd_read_sector(IDEState *s, void *buf)
>      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);
> +    ide_buffered_readv(s, (int64_t)s->lba << 2, &s->qiov, 4,
> +                       cd_read_sector_cb, s);
>  
>      s->status |= BUSY_STAT;
>      return 0;
> @@ -424,9 +424,9 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
>      s->bus->dma->iov.iov_len = n * 4 * 512;
>      qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
>  
> -    s->bus->dma->aiocb = blk_aio_readv(s->blk, (int64_t)s->lba << 2,
> -                                       &s->bus->dma->qiov, n * 4,
> -                                       ide_atapi_cmd_read_dma_cb, s);
> +    s->bus->dma->aiocb = ide_buffered_readv(s, (int64_t)s->lba << 2,
> +                                            &s->bus->dma->qiov, n * 4,
> +                                            ide_atapi_cmd_read_dma_cb, s);

IIRC the dma aiocb are still going to be drained in bmdma_cmd_writeb, so why do
we need the bounce buffer?

>      return;
>  
>  eot:
> -- 
> 1.9.1
> 
> 

Re: [Qemu-devel] [PATCH V3 0/6] ide: avoid main-loop hang on CDROM/NFS failure

[Fam Zheng]

On Fri, 11/06 09:42, Peter Lieven wrote:
> This series aims at avoiding a hanging main-loop if a vserver has a
> CDROM image mounted from a NFS share and that NFS share goes down.
> Typical situation is that users mount an CDROM ISO to install something
> and then forget to eject that CDROM afterwards.
> As a consequence this mounted CD is able to bring down the
> whole vserver if the backend NFS share is unreachable. This is bad
> especially if the CDROM itself is not needed anymore at this point.

If a storage backend is lost, would QEMU hang on guest reboot with this patch?
If so, just for understanding the problem, what is the use case this series
addresses?

The code looks good to me apart from the two questions I left, and that I
didn't fully understand the elementary transfer part.

Thanks,

Fam

Re: [Qemu-devel] [PATCH V3 5/6] ide: enable buffered requests for ATAPI devices

[Peter Lieven]

Am 12.11.2015 um 12:25 schrieb Fam Zheng:
> On Fri, 11/06 09:42, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   hw/ide/atapi.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 29fd131..2f6d018 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -190,8 +190,8 @@ static int cd_read_sector(IDEState *s, void *buf)
>>       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);
>> +    ide_buffered_readv(s, (int64_t)s->lba << 2, &s->qiov, 4,
>> +                       cd_read_sector_cb, s);
>>   
>>       s->status |= BUSY_STAT;
>>       return 0;
>> @@ -424,9 +424,9 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
>>       s->bus->dma->iov.iov_len = n * 4 * 512;
>>       qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
>>   
>> -    s->bus->dma->aiocb = blk_aio_readv(s->blk, (int64_t)s->lba << 2,
>> -                                       &s->bus->dma->qiov, n * 4,
>> -                                       ide_atapi_cmd_read_dma_cb, s);
>> +    s->bus->dma->aiocb = ide_buffered_readv(s, (int64_t)s->lba << 2,
>> +                                            &s->bus->dma->qiov, n * 4,
>> +                                            ide_atapi_cmd_read_dma_cb, s);
> IIRC the dma aiocb are still going to be drained in bmdma_cmd_writeb, so why do
> we need the bounce buffer?

They dont ;-)

If s->bus->dma->aiocb is a buffered Request, it will be set to NULL in this
call:

req->original_cb(req->original_opaque, -ECANCELED);

Peter

Re: [Qemu-devel] [PATCH V3 0/6] ide: avoid main-loop hang on CDROM/NFS failure

[Peter Lieven]

Am 12.11.2015 um 12:33 schrieb Fam Zheng:
> On Fri, 11/06 09:42, Peter Lieven wrote:
>> This series aims at avoiding a hanging main-loop if a vserver has a
>> CDROM image mounted from a NFS share and that NFS share goes down.
>> Typical situation is that users mount an CDROM ISO to install something
>> and then forget to eject that CDROM afterwards.
>> As a consequence this mounted CD is able to bring down the
>> whole vserver if the backend NFS share is unreachable. This is bad
>> especially if the CDROM itself is not needed anymore at this point.
> If a storage backend is lost, would QEMU hang on guest reboot with this patch?
> If so, just for understanding the problem, what is the use case this series
> addresses?

Qemu would hang whenever the next request to the CDROM is made.
This usually happens as soon as the CDROM is mounted (at least in Windows guests)
even if there is no CDROM activity intiated by the user. This brings down every guest
where the user just forgot to unmount the CDROM just because some NFS storage is
unreachable.

>
> The code looks good to me apart from the two questions I left, and that I
> didn't fully understand the elementary transfer part.

That indeed is not obvious. There was a long discussion on that topic in V1 of this series.

Thanks,
Peter