[Qemu-devel] [PATCH 00/12] file-posix: Simplify delegation to worker thread

[Qemu-devel] [PATCH 00/12] file-posix: Simplify delegation to worker thread

[Kevin Wolf]

This series cleans up and simplifies the code that calls worker thread
functions for the various operations in the file-posix driver. This
results in less indirection and better readability as well as reduced
heap allocations because we can store ACBs on the coroutine stack now.

Kevin Wolf (12):
  file-posix: Reorganise RawPosixAIOData
  file-posix: Factor out raw_thread_pool_submit()
  file-posix: Avoid aio_worker() for QEMU_AIO_TRUNCATE
  file-posix: Avoid aio_worker() for QEMU_AIO_COPY_RANGE
  file-posix: Avoid aio_worker() for QEMU_AIO_WRITE_ZEROES
  file-posix: Avoid aio_worker() for QEMU_AIO_DISCARD
  file-posix: Avoid aio_worker() for QEMU_AIO_FLUSH
  file-posix: Move read/write operation logic out of aio_worker()
  file-posix: Avoid aio_worker() for QEMU_AIO_READ/WRITE
  file-posix: Remove paio_submit_co()
  file-posix: Switch to .bdrv_co_ioctl
  file-posix: Avoid aio_worker() for QEMU_AIO_IOCTL

 include/scsi/pr-manager.h |   8 +-
 block/file-posix.c        | 380 ++++++++++++++++++++------------------
 scsi/pr-manager.c         |  21 +--
 scsi/trace-events         |   2 +-
 4 files changed, 210 insertions(+), 201 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 01/12] file-posix: Reorganise RawPosixAIOData

[Kevin Wolf]

RawPosixAIOData contains a lot of fields for several separate operations
that are to be processed in a worker thread and that need different
parameters. The struct is currently rather unorganised, with unions that
cover some, but not all operations, and even one #define for field names
instead of a union.

Clean this up to have some common fields and a single union. As a side
effect, on x86_64 the struct shrinks from 72 to 48 bytes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 89 +++++++++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 40 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 0c1b81ce4b..68cea7685a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -178,25 +178,29 @@ static int64_t raw_getlength(BlockDriverState *bs);
 
 typedef struct RawPosixAIOData {
     BlockDriverState *bs;
+    int aio_type;
     int aio_fildes;
-    union {
-        struct iovec *aio_iov;
-        void *aio_ioctl_buf;
-    };
-    int aio_niov;
-    uint64_t aio_nbytes;
-#define aio_ioctl_cmd   aio_nbytes /* for QEMU_AIO_IOCTL */
+
     off_t aio_offset;
-    int aio_type;
+    uint64_t aio_nbytes;
+
     union {
+        struct {
+            struct iovec *iov;
+            int niov;
+        } io;
+        struct {
+            uint64_t cmd;
+            void *buf;
+        } ioctl;
         struct {
             int aio_fd2;
             off_t aio_offset2;
-        };
+        } copy_range;
         struct {
             PreallocMode prealloc;
             Error **errp;
-        };
+        } truncate;
     };
 } RawPosixAIOData;
 
@@ -1131,7 +1135,7 @@ static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
 {
     int ret;
 
-    ret = ioctl(aiocb->aio_fildes, aiocb->aio_ioctl_cmd, aiocb->aio_ioctl_buf);
+    ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
     if (ret == -1) {
         return -errno;
     }
@@ -1212,13 +1216,13 @@ static ssize_t handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
     do {
         if (aiocb->aio_type & QEMU_AIO_WRITE)
             len = qemu_pwritev(aiocb->aio_fildes,
-                               aiocb->aio_iov,
-                               aiocb->aio_niov,
+                               aiocb->io.iov,
+                               aiocb->io.niov,
                                aiocb->aio_offset);
          else
             len = qemu_preadv(aiocb->aio_fildes,
-                              aiocb->aio_iov,
-                              aiocb->aio_niov,
+                              aiocb->io.iov,
+                              aiocb->io.niov,
                               aiocb->aio_offset);
     } while (len == -1 && errno == EINTR);
 
@@ -1284,8 +1288,8 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
          * If there is just a single buffer, and it is properly aligned
          * we can just use plain pread/pwrite without any problems.
          */
-        if (aiocb->aio_niov == 1) {
-             return handle_aiocb_rw_linear(aiocb, aiocb->aio_iov->iov_base);
+        if (aiocb->io.niov == 1) {
+             return handle_aiocb_rw_linear(aiocb, aiocb->io.iov->iov_base);
         }
         /*
          * We have more than one iovec, and all are properly aligned.
@@ -1322,9 +1326,9 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
         char *p = buf;
         int i;
 
-        for (i = 0; i < aiocb->aio_niov; ++i) {
-            memcpy(p, aiocb->aio_iov[i].iov_base, aiocb->aio_iov[i].iov_len);
-            p += aiocb->aio_iov[i].iov_len;
+        for (i = 0; i < aiocb->io.niov; ++i) {
+            memcpy(p, aiocb->io.iov[i].iov_base, aiocb->io.iov[i].iov_len);
+            p += aiocb->io.iov[i].iov_len;
         }
         assert(p - buf == aiocb->aio_nbytes);
     }
@@ -1335,12 +1339,12 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
         size_t count = aiocb->aio_nbytes, copy;
         int i;
 
-        for (i = 0; i < aiocb->aio_niov && count; ++i) {
+        for (i = 0; i < aiocb->io.niov && count; ++i) {
             copy = count;
-            if (copy > aiocb->aio_iov[i].iov_len) {
-                copy = aiocb->aio_iov[i].iov_len;
+            if (copy > aiocb->io.iov[i].iov_len) {
+                copy = aiocb->io.iov[i].iov_len;
             }
-            memcpy(aiocb->aio_iov[i].iov_base, p, copy);
+            memcpy(aiocb->io.iov[i].iov_base, p, copy);
             assert(count >= copy);
             p     += copy;
             count -= copy;
@@ -1551,14 +1555,15 @@ static ssize_t handle_aiocb_copy_range(RawPosixAIOData *aiocb)
 {
     uint64_t bytes = aiocb->aio_nbytes;
     off_t in_off = aiocb->aio_offset;
-    off_t out_off = aiocb->aio_offset2;
+    off_t out_off = aiocb->copy_range.aio_offset2;
 
     while (bytes) {
         ssize_t ret = copy_file_range(aiocb->aio_fildes, &in_off,
-                                      aiocb->aio_fd2, &out_off,
+                                      aiocb->copy_range.aio_fd2, &out_off,
                                       bytes, 0);
         trace_file_copy_file_range(aiocb->bs, aiocb->aio_fildes, in_off,
-                                   aiocb->aio_fd2, out_off, bytes, 0, ret);
+                                   aiocb->copy_range.aio_fd2, out_off, bytes,
+                                   0, ret);
         if (ret == 0) {
             /* No progress (e.g. when beyond EOF), let the caller fall back to
              * buffer I/O. */
@@ -1627,7 +1632,8 @@ static int handle_aiocb_truncate(RawPosixAIOData *aiocb)
     struct stat st;
     int fd = aiocb->aio_fildes;
     int64_t offset = aiocb->aio_offset;
-    Error **errp = aiocb->errp;
+    PreallocMode prealloc = aiocb->truncate.prealloc;
+    Error **errp = aiocb->truncate.errp;
 
     if (fstat(fd, &st) < 0) {
         result = -errno;
@@ -1636,12 +1642,12 @@ static int handle_aiocb_truncate(RawPosixAIOData *aiocb)
     }
 
     current_length = st.st_size;
-    if (current_length > offset && aiocb->prealloc != PREALLOC_MODE_OFF) {
+    if (current_length > offset && prealloc != PREALLOC_MODE_OFF) {
         error_setg(errp, "Cannot use preallocation for shrinking files");
         return -ENOTSUP;
     }
 
-    switch (aiocb->prealloc) {
+    switch (prealloc) {
 #ifdef CONFIG_POSIX_FALLOCATE
     case PREALLOC_MODE_FALLOC:
         /*
@@ -1722,7 +1728,7 @@ static int handle_aiocb_truncate(RawPosixAIOData *aiocb)
     default:
         result = -ENOTSUP;
         error_setg(errp, "Unsupported preallocation mode: %s",
-                   PreallocMode_str(aiocb->prealloc));
+                   PreallocMode_str(prealloc));
         return result;
     }
 
@@ -1747,7 +1753,7 @@ static int aio_worker(void *arg)
     case QEMU_AIO_READ:
         ret = handle_aiocb_rw(aiocb);
         if (ret >= 0 && ret < aiocb->aio_nbytes) {
-            iov_memset(aiocb->aio_iov, aiocb->aio_niov, ret,
+            iov_memset(aiocb->io.iov, aiocb->io.niov, ret,
                       0, aiocb->aio_nbytes - ret);
 
             ret = aiocb->aio_nbytes;
@@ -1808,16 +1814,17 @@ static int paio_submit_co_full(BlockDriverState *bs, int fd,
     acb->bs = bs;
     acb->aio_type = type;
     acb->aio_fildes = fd;
-    acb->aio_fd2 = fd2;
-    acb->aio_offset2 = offset2;
 
     acb->aio_nbytes = bytes;
     acb->aio_offset = offset;
 
     if (qiov) {
-        acb->aio_iov = qiov->iov;
-        acb->aio_niov = qiov->niov;
+        acb->io.iov = qiov->iov;
+        acb->io.niov = qiov->niov;
         assert(qiov->size == bytes);
+    } else {
+        acb->copy_range.aio_fd2 = fd2;
+        acb->copy_range.aio_offset2 = offset2;
     }
 
     trace_file_paio_submit_co(offset, bytes, type);
@@ -1959,8 +1966,10 @@ raw_regular_truncate(BlockDriverState *bs, int fd, int64_t offset,
         .aio_fildes     = fd,
         .aio_type       = QEMU_AIO_TRUNCATE,
         .aio_offset     = offset,
-        .prealloc       = prealloc,
-        .errp           = errp,
+        .truncate       = {
+            .prealloc       = prealloc,
+            .errp           = errp,
+        },
     };
 
     /* @bs can be NULL, bdrv_get_aio_context() returns the main context then */
@@ -3072,8 +3081,8 @@ static BlockAIOCB *hdev_aio_ioctl(BlockDriverState *bs,
     acb->aio_type = QEMU_AIO_IOCTL;
     acb->aio_fildes = s->fd;
     acb->aio_offset = 0;
-    acb->aio_ioctl_buf = buf;
-    acb->aio_ioctl_cmd = req;
+    acb->ioctl.buf = buf;
+    acb->ioctl.cmd = req;
     pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
     return thread_pool_submit_aio(pool, aio_worker, acb, cb, opaque);
 }
-- 
2.19.1

[Qemu-devel] [PATCH 02/12] file-posix: Factor out raw_thread_pool_submit()

[Kevin Wolf]

Getting the thread pool of the AioContext of a block node and scheduling
some work in it is an operation that is already done twice, and we'll
get more instances. Factor it out into a separate function.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 68cea7685a..28eb55c29e 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1803,13 +1803,20 @@ static int aio_worker(void *arg)
     return ret;
 }
 
+static int coroutine_fn raw_thread_pool_submit(BlockDriverState *bs,
+                                               ThreadPoolFunc func, void *arg)
+{
+    /* @bs can be NULL, bdrv_get_aio_context() returns the main context then */
+    ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+    return thread_pool_submit_co(pool, func, arg);
+}
+
 static int paio_submit_co_full(BlockDriverState *bs, int fd,
                                int64_t offset, int fd2, int64_t offset2,
                                QEMUIOVector *qiov,
                                int bytes, int type)
 {
     RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
-    ThreadPool *pool;
 
     acb->bs = bs;
     acb->aio_type = type;
@@ -1828,8 +1835,7 @@ static int paio_submit_co_full(BlockDriverState *bs, int fd,
     }
 
     trace_file_paio_submit_co(offset, bytes, type);
-    pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
-    return thread_pool_submit_co(pool, aio_worker, acb);
+    return raw_thread_pool_submit(bs, aio_worker, acb);
 }
 
 static inline int paio_submit_co(BlockDriverState *bs, int fd,
@@ -1959,7 +1965,6 @@ raw_regular_truncate(BlockDriverState *bs, int fd, int64_t offset,
                      PreallocMode prealloc, Error **errp)
 {
     RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
-    ThreadPool *pool;
 
     *acb = (RawPosixAIOData) {
         .bs             = bs,
@@ -1972,9 +1977,7 @@ raw_regular_truncate(BlockDriverState *bs, int fd, int64_t offset,
         },
     };
 
-    /* @bs can be NULL, bdrv_get_aio_context() returns the main context then */
-    pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
-    return thread_pool_submit_co(pool, aio_worker, acb);
+    return raw_thread_pool_submit(bs, aio_worker, acb);
 }
 
 static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
-- 
2.19.1

[Qemu-devel] [PATCH 03/12] file-posix: Avoid aio_worker() for QEMU_AIO_TRUNCATE

[Kevin Wolf]

aio_worker() doesn't add anything interesting, it's only a useless
indirection. Call the handler function directly instead.

As we know that this handler function is only called from coroutine
context and the coroutine stays around until the worker thread finishes,
we can keep RawPosixAIOData on the stack.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 28eb55c29e..69e1b761b4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1624,8 +1624,9 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
     return ret;
 }
 
-static int handle_aiocb_truncate(RawPosixAIOData *aiocb)
+static int handle_aiocb_truncate(void *opaque)
 {
+    RawPosixAIOData *aiocb = opaque;
     int result = 0;
     int64_t current_length = 0;
     char *buf = NULL;
@@ -1791,8 +1792,7 @@ static int aio_worker(void *arg)
         ret = handle_aiocb_copy_range(aiocb);
         break;
     case QEMU_AIO_TRUNCATE:
-        ret = handle_aiocb_truncate(aiocb);
-        break;
+        g_assert_not_reached();
     default:
         fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
         ret = -EINVAL;
@@ -1964,9 +1964,9 @@ static int coroutine_fn
 raw_regular_truncate(BlockDriverState *bs, int fd, int64_t offset,
                      PreallocMode prealloc, Error **errp)
 {
-    RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
+    RawPosixAIOData acb;
 
-    *acb = (RawPosixAIOData) {
+    acb = (RawPosixAIOData) {
         .bs             = bs,
         .aio_fildes     = fd,
         .aio_type       = QEMU_AIO_TRUNCATE,
@@ -1977,7 +1977,7 @@ raw_regular_truncate(BlockDriverState *bs, int fd, int64_t offset,
         },
     };
 
-    return raw_thread_pool_submit(bs, aio_worker, acb);
+    return raw_thread_pool_submit(bs, handle_aiocb_truncate, &acb);
 }
 
 static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
-- 
2.19.1

[Qemu-devel] [PATCH 04/12] file-posix: Avoid aio_worker() for QEMU_AIO_COPY_RANGE

[Kevin Wolf]

aio_worker() doesn't add anything interesting, it's only a useless
indirection. Call the handler function directly instead.

As we know that this handler function is only called from coroutine
context and the coroutine stays around until the worker thread finishes,
we can keep RawPosixAIOData on the stack.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 69e1b761b4..fe5e95cc0a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1551,8 +1551,9 @@ static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
 }
 #endif
 
-static ssize_t handle_aiocb_copy_range(RawPosixAIOData *aiocb)
+static int handle_aiocb_copy_range(void *opaque)
 {
+    RawPosixAIOData *aiocb = opaque;
     uint64_t bytes = aiocb->aio_nbytes;
     off_t in_off = aiocb->aio_offset;
     off_t out_off = aiocb->copy_range.aio_offset2;
@@ -1789,8 +1790,6 @@ static int aio_worker(void *arg)
         ret = handle_aiocb_write_zeroes_unmap(aiocb);
         break;
     case QEMU_AIO_COPY_RANGE:
-        ret = handle_aiocb_copy_range(aiocb);
-        break;
     case QEMU_AIO_TRUNCATE:
         g_assert_not_reached();
     default:
@@ -2697,6 +2696,7 @@ static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
                                              BdrvRequestFlags read_flags,
                                              BdrvRequestFlags write_flags)
 {
+    RawPosixAIOData acb;
     BDRVRawState *s = bs->opaque;
     BDRVRawState *src_s;
 
@@ -2709,8 +2709,20 @@ static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
     if (fd_open(src->bs) < 0 || fd_open(dst->bs) < 0) {
         return -EIO;
     }
-    return paio_submit_co_full(bs, src_s->fd, src_offset, s->fd, dst_offset,
-                               NULL, bytes, QEMU_AIO_COPY_RANGE);
+
+    acb = (RawPosixAIOData) {
+        .bs             = bs,
+        .aio_type       = QEMU_AIO_COPY_RANGE,
+        .aio_fildes     = src_s->fd,
+        .aio_offset     = src_offset,
+        .aio_nbytes     = bytes,
+        .copy_range     = {
+            .aio_fd2        = s->fd,
+            .aio_offset2    = dst_offset,
+        },
+    };
+
+    return raw_thread_pool_submit(bs, handle_aiocb_copy_range, &acb);
 }
 
 BlockDriver bdrv_file = {
-- 
2.19.1

[Qemu-devel] [PATCH 05/12] file-posix: Avoid aio_worker() for QEMU_AIO_WRITE_ZEROES

[Kevin Wolf]

aio_worker() doesn't add anything interesting, it's only a useless
indirection. Call the handler function directly instead.

As we know that this handler function is only called from coroutine
context and the coroutine stays around until the worker thread finishes,
we can keep RawPosixAIOData on the stack.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 53 +++++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index fe5e95cc0a..3d3bddd511 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1443,8 +1443,9 @@ static ssize_t handle_aiocb_write_zeroes_block(RawPosixAIOData *aiocb)
     return ret;
 }
 
-static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
+static int handle_aiocb_write_zeroes(void *opaque)
 {
+    RawPosixAIOData *aiocb = opaque;
 #if defined(CONFIG_FALLOCATE) || defined(CONFIG_XFS)
     BDRVRawState *s = aiocb->bs->opaque;
 #endif
@@ -1508,8 +1509,9 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
     return -ENOTSUP;
 }
 
-static ssize_t handle_aiocb_write_zeroes_unmap(RawPosixAIOData *aiocb)
+static int handle_aiocb_write_zeroes_unmap(void *opaque)
 {
+    RawPosixAIOData *aiocb = opaque;
     BDRVRawState *s G_GNUC_UNUSED = aiocb->bs->opaque;
     int ret;
 
@@ -1784,11 +1786,7 @@ static int aio_worker(void *arg)
         ret = handle_aiocb_discard(aiocb);
         break;
     case QEMU_AIO_WRITE_ZEROES:
-        ret = handle_aiocb_write_zeroes(aiocb);
-        break;
     case QEMU_AIO_WRITE_ZEROES | QEMU_AIO_DISCARD:
-        ret = handle_aiocb_write_zeroes_unmap(aiocb);
-        break;
     case QEMU_AIO_COPY_RANGE:
     case QEMU_AIO_TRUNCATE:
         g_assert_not_reached();
@@ -2614,18 +2612,41 @@ raw_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
     return paio_submit_co(bs, s->fd, offset, NULL, bytes, QEMU_AIO_DISCARD);
 }
 
-static int coroutine_fn raw_co_pwrite_zeroes(
-    BlockDriverState *bs, int64_t offset,
-    int bytes, BdrvRequestFlags flags)
+static int coroutine_fn
+raw_do_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes,
+                     BdrvRequestFlags flags, bool blkdev)
 {
     BDRVRawState *s = bs->opaque;
-    int operation = QEMU_AIO_WRITE_ZEROES;
+    RawPosixAIOData acb;
+    ThreadPoolFunc *handler;
+
+    acb = (RawPosixAIOData) {
+        .bs             = bs,
+        .aio_fildes     = s->fd,
+        .aio_type       = QEMU_AIO_WRITE_ZEROES,
+        .aio_offset     = offset,
+        .aio_nbytes     = bytes,
+    };
+
+    if (blkdev) {
+        acb.aio_type |= QEMU_AIO_BLKDEV;
+    }
 
     if (flags & BDRV_REQ_MAY_UNMAP) {
-        operation |= QEMU_AIO_DISCARD;
+        acb.aio_type |= QEMU_AIO_DISCARD;
+        handler = handle_aiocb_write_zeroes_unmap;
+    } else {
+        handler = handle_aiocb_write_zeroes;
     }
 
-    return paio_submit_co(bs, s->fd, offset, NULL, bytes, operation);
+    return raw_thread_pool_submit(bs, handler, &acb);
+}
+
+static int coroutine_fn raw_co_pwrite_zeroes(
+    BlockDriverState *bs, int64_t offset,
+    int bytes, BdrvRequestFlags flags)
+{
+    return raw_do_pwrite_zeroes(bs, offset, bytes, flags, false);
 }
 
 static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
@@ -3130,8 +3151,6 @@ hdev_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
 static coroutine_fn int hdev_co_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int bytes, BdrvRequestFlags flags)
 {
-    BDRVRawState *s = bs->opaque;
-    int operation = QEMU_AIO_WRITE_ZEROES | QEMU_AIO_BLKDEV;
     int rc;
 
     rc = fd_open(bs);
@@ -3139,11 +3158,7 @@ static coroutine_fn int hdev_co_pwrite_zeroes(BlockDriverState *bs,
         return rc;
     }
 
-    if (flags & BDRV_REQ_MAY_UNMAP) {
-        operation |= QEMU_AIO_DISCARD;
-    }
-
-    return paio_submit_co(bs, s->fd, offset, NULL, bytes, operation);
+    return raw_do_pwrite_zeroes(bs, offset, bytes, flags, true);
 }
 
 static int coroutine_fn hdev_co_create_opts(const char *filename, QemuOpts *opts,
-- 
2.19.1

[Qemu-devel] [PATCH 06/12] file-posix: Avoid aio_worker() for QEMU_AIO_DISCARD

[Kevin Wolf]

aio_worker() doesn't add anything interesting, it's only a useless
indirection. Call the handler function directly instead.

As we know that this handler function is only called from coroutine
context and the coroutine stays around until the worker thread finishes,
we can keep RawPosixAIOData on the stack.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 3d3bddd511..291ffaeded 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1587,8 +1587,9 @@ static int handle_aiocb_copy_range(void *opaque)
     return 0;
 }
 
-static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
+static int handle_aiocb_discard(void *opaque)
 {
+    RawPosixAIOData *aiocb = opaque;
     int ret = -EOPNOTSUPP;
     BDRVRawState *s = aiocb->bs->opaque;
 
@@ -1783,8 +1784,6 @@ static int aio_worker(void *arg)
         ret = handle_aiocb_ioctl(aiocb);
         break;
     case QEMU_AIO_DISCARD:
-        ret = handle_aiocb_discard(aiocb);
-        break;
     case QEMU_AIO_WRITE_ZEROES:
     case QEMU_AIO_WRITE_ZEROES | QEMU_AIO_DISCARD:
     case QEMU_AIO_COPY_RANGE:
@@ -2605,11 +2604,30 @@ static void coroutine_fn raw_co_invalidate_cache(BlockDriverState *bs,
 }
 
 static coroutine_fn int
-raw_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
+raw_do_pdiscard(BlockDriverState *bs, int64_t offset, int bytes, bool blkdev)
 {
     BDRVRawState *s = bs->opaque;
+    RawPosixAIOData acb;
+
+    acb = (RawPosixAIOData) {
+        .bs             = bs,
+        .aio_fildes     = s->fd,
+        .aio_type       = QEMU_AIO_DISCARD,
+        .aio_offset     = offset,
+        .aio_nbytes     = bytes,
+    };
 
-    return paio_submit_co(bs, s->fd, offset, NULL, bytes, QEMU_AIO_DISCARD);
+    if (blkdev) {
+        acb.aio_type |= QEMU_AIO_BLKDEV;
+    }
+
+    return raw_thread_pool_submit(bs, handle_aiocb_discard, &acb);
+}
+
+static coroutine_fn int
+raw_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
+{
+    return raw_do_pdiscard(bs, offset, bytes, false);
 }
 
 static int coroutine_fn
@@ -3137,15 +3155,13 @@ static int fd_open(BlockDriverState *bs)
 static coroutine_fn int
 hdev_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
 {
-    BDRVRawState *s = bs->opaque;
     int ret;
 
     ret = fd_open(bs);
     if (ret < 0) {
         return ret;
     }
-    return paio_submit_co(bs, s->fd, offset, NULL, bytes,
-                          QEMU_AIO_DISCARD | QEMU_AIO_BLKDEV);
+    return raw_do_pdiscard(bs, offset, bytes, true);
 }
 
 static coroutine_fn int hdev_co_pwrite_zeroes(BlockDriverState *bs,
-- 
2.19.1

[Qemu-devel] [PATCH 07/12] file-posix: Avoid aio_worker() for QEMU_AIO_FLUSH

[Kevin Wolf]

aio_worker() doesn't add anything interesting, it's only a useless
indirection. Call the handler function directly instead.

As we know that this handler function is only called from coroutine
context and the coroutine stays around until the worker thread finishes,
we can keep RawPosixAIOData on the stack.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 291ffaeded..074848ed1f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1143,8 +1143,9 @@ static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
     return 0;
 }
 
-static ssize_t handle_aiocb_flush(RawPosixAIOData *aiocb)
+static int handle_aiocb_flush(void *opaque)
 {
+    RawPosixAIOData *aiocb = opaque;
     BDRVRawState *s = aiocb->bs->opaque;
     int ret;
 
@@ -1777,12 +1778,10 @@ static int aio_worker(void *arg)
             ret = -EINVAL;
         }
         break;
-    case QEMU_AIO_FLUSH:
-        ret = handle_aiocb_flush(aiocb);
-        break;
     case QEMU_AIO_IOCTL:
         ret = handle_aiocb_ioctl(aiocb);
         break;
+    case QEMU_AIO_FLUSH:
     case QEMU_AIO_DISCARD:
     case QEMU_AIO_WRITE_ZEROES:
     case QEMU_AIO_WRITE_ZEROES | QEMU_AIO_DISCARD:
@@ -1910,6 +1909,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
 static int raw_co_flush_to_disk(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
+    RawPosixAIOData acb;
     int ret;
 
     ret = fd_open(bs);
@@ -1917,7 +1917,13 @@ static int raw_co_flush_to_disk(BlockDriverState *bs)
         return ret;
     }
 
-    return paio_submit_co(bs, s->fd, 0, NULL, 0, QEMU_AIO_FLUSH);
+    acb = (RawPosixAIOData) {
+        .bs             = bs,
+        .aio_fildes     = s->fd,
+        .aio_type       = QEMU_AIO_FLUSH,
+    };
+
+    return raw_thread_pool_submit(bs, handle_aiocb_flush, &acb);
 }
 
 static void raw_aio_attach_aio_context(BlockDriverState *bs,
-- 
2.19.1

[Qemu-devel] [PATCH 08/12] file-posix: Move read/write operation logic out of aio_worker()

[Kevin Wolf]

aio_worker() for reads and writes isn't boring enough yet. It still does
some postprocessing for handling short reads and turning the result into
the right return value.

However, there is no reason why handle_aiocb_rw() couldn't do the same,
and even without duplicating code between the read and write path. So
move the code there.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 074848ed1f..4989208ba3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1290,7 +1290,8 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
          * we can just use plain pread/pwrite without any problems.
          */
         if (aiocb->io.niov == 1) {
-             return handle_aiocb_rw_linear(aiocb, aiocb->io.iov->iov_base);
+             nbytes = handle_aiocb_rw_linear(aiocb, aiocb->io.iov->iov_base);
+             goto out;
         }
         /*
          * We have more than one iovec, and all are properly aligned.
@@ -1302,7 +1303,7 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
             nbytes = handle_aiocb_rw_vector(aiocb);
             if (nbytes == aiocb->aio_nbytes ||
                 (nbytes < 0 && nbytes != -ENOSYS)) {
-                return nbytes;
+                goto out;
             }
             preadv_present = false;
         }
@@ -1320,7 +1321,8 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
      */
     buf = qemu_try_blockalign(aiocb->bs, aiocb->aio_nbytes);
     if (buf == NULL) {
-        return -ENOMEM;
+        nbytes = -ENOMEM;
+        goto out;
     }
 
     if (aiocb->aio_type & QEMU_AIO_WRITE) {
@@ -1354,7 +1356,21 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
     }
     qemu_vfree(buf);
 
-    return nbytes;
+out:
+    if (nbytes == aiocb->aio_nbytes) {
+        return 0;
+    } else if (nbytes >= 0 && nbytes < aiocb->aio_nbytes) {
+        if (aiocb->aio_type & QEMU_AIO_WRITE) {
+            return -EINVAL;
+        } else {
+            iov_memset(aiocb->io.iov, aiocb->io.niov, nbytes,
+                      0, aiocb->aio_nbytes - nbytes);
+            return aiocb->aio_nbytes;
+        }
+    } else {
+        assert(nbytes < 0);
+        return nbytes;
+    }
 }
 
 #ifdef CONFIG_XFS
@@ -1758,25 +1774,9 @@ static int aio_worker(void *arg)
     switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
     case QEMU_AIO_READ:
         ret = handle_aiocb_rw(aiocb);
-        if (ret >= 0 && ret < aiocb->aio_nbytes) {
-            iov_memset(aiocb->io.iov, aiocb->io.niov, ret,
-                      0, aiocb->aio_nbytes - ret);
-
-            ret = aiocb->aio_nbytes;
-        }
-        if (ret == aiocb->aio_nbytes) {
-            ret = 0;
-        } else if (ret >= 0 && ret < aiocb->aio_nbytes) {
-            ret = -EINVAL;
-        }
         break;
     case QEMU_AIO_WRITE:
         ret = handle_aiocb_rw(aiocb);
-        if (ret == aiocb->aio_nbytes) {
-            ret = 0;
-        } else if (ret >= 0 && ret < aiocb->aio_nbytes) {
-            ret = -EINVAL;
-        }
         break;
     case QEMU_AIO_IOCTL:
         ret = handle_aiocb_ioctl(aiocb);
-- 
2.19.1

[Qemu-devel] [PATCH 09/12] file-posix: Avoid aio_worker() for QEMU_AIO_READ/WRITE

[Kevin Wolf]

aio_worker() doesn't add anything interesting, it's only a useless
indirection. Call the handler function directly instead.

As we know that this handler function is only called from coroutine
context and the coroutine stays around until the worker thread finishes,
we can keep RawPosixAIOData on the stack.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 4989208ba3..c5e83140d3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1279,8 +1279,9 @@ static ssize_t handle_aiocb_rw_linear(RawPosixAIOData *aiocb, char *buf)
     return offset;
 }
 
-static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
+static int handle_aiocb_rw(void *opaque)
 {
+    RawPosixAIOData *aiocb = opaque;
     ssize_t nbytes;
     char *buf;
 
@@ -1772,15 +1773,11 @@ static int aio_worker(void *arg)
     ssize_t ret = 0;
 
     switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
-    case QEMU_AIO_READ:
-        ret = handle_aiocb_rw(aiocb);
-        break;
-    case QEMU_AIO_WRITE:
-        ret = handle_aiocb_rw(aiocb);
-        break;
     case QEMU_AIO_IOCTL:
         ret = handle_aiocb_ioctl(aiocb);
         break;
+    case QEMU_AIO_READ:
+    case QEMU_AIO_WRITE:
     case QEMU_AIO_FLUSH:
     case QEMU_AIO_DISCARD:
     case QEMU_AIO_WRITE_ZEROES:
@@ -1844,6 +1841,7 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
                                    uint64_t bytes, QEMUIOVector *qiov, int type)
 {
     BDRVRawState *s = bs->opaque;
+    RawPosixAIOData acb;
 
     if (fd_open(bs) < 0)
         return -EIO;
@@ -1866,7 +1864,20 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
         }
     }
 
-    return paio_submit_co(bs, s->fd, offset, qiov, bytes, type);
+    acb = (RawPosixAIOData) {
+        .bs             = bs,
+        .aio_fildes     = s->fd,
+        .aio_type       = type,
+        .aio_offset     = offset,
+        .aio_nbytes     = bytes,
+        .io             = {
+            .iov            = qiov->iov,
+            .niov           = qiov->niov,
+        },
+    };
+
+    assert(qiov->size == bytes);
+    return raw_thread_pool_submit(bs, handle_aiocb_rw, &acb);
 }
 
 static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset,
-- 
2.19.1

[Qemu-devel] [PATCH 10/12] file-posix: Remove paio_submit_co()

[Kevin Wolf]

The function is not used any more, remove it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 34 ----------------------------------
 1 file changed, 34 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index c5e83140d3..821743d2b2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1803,40 +1803,6 @@ static int coroutine_fn raw_thread_pool_submit(BlockDriverState *bs,
     return thread_pool_submit_co(pool, func, arg);
 }
 
-static int paio_submit_co_full(BlockDriverState *bs, int fd,
-                               int64_t offset, int fd2, int64_t offset2,
-                               QEMUIOVector *qiov,
-                               int bytes, int type)
-{
-    RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
-
-    acb->bs = bs;
-    acb->aio_type = type;
-    acb->aio_fildes = fd;
-
-    acb->aio_nbytes = bytes;
-    acb->aio_offset = offset;
-
-    if (qiov) {
-        acb->io.iov = qiov->iov;
-        acb->io.niov = qiov->niov;
-        assert(qiov->size == bytes);
-    } else {
-        acb->copy_range.aio_fd2 = fd2;
-        acb->copy_range.aio_offset2 = offset2;
-    }
-
-    trace_file_paio_submit_co(offset, bytes, type);
-    return raw_thread_pool_submit(bs, aio_worker, acb);
-}
-
-static inline int paio_submit_co(BlockDriverState *bs, int fd,
-                                 int64_t offset, QEMUIOVector *qiov,
-                                 int bytes, int type)
-{
-    return paio_submit_co_full(bs, fd, offset, -1, 0, qiov, bytes, type);
-}
-
 static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
                                    uint64_t bytes, QEMUIOVector *qiov, int type)
 {
-- 
2.19.1

[Qemu-devel] [PATCH 11/12] file-posix: Switch to .bdrv_co_ioctl

[Kevin Wolf]

No real reason to keep using the callback based mechanism here when the
rest of the file-posix driver is coroutine based. Changing it brings
ioctls more in line with how other request types work.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/scsi/pr-manager.h |  8 +++-----
 block/file-posix.c        | 21 +++++++++++----------
 scsi/pr-manager.c         | 21 +++++++++------------
 scsi/trace-events         |  2 +-
 4 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/include/scsi/pr-manager.h b/include/scsi/pr-manager.h
index 50a77b08fc..6ad5fd1ff7 100644
--- a/include/scsi/pr-manager.h
+++ b/include/scsi/pr-manager.h
@@ -5,6 +5,7 @@
 #include "qapi/visitor.h"
 #include "qom/object_interfaces.h"
 #include "block/aio.h"
+#include "qemu/coroutine.h"
 
 #define TYPE_PR_MANAGER "pr-manager"
 
@@ -37,11 +38,8 @@ typedef struct PRManagerClass {
 } PRManagerClass;
 
 bool pr_manager_is_connected(PRManager *pr_mgr);
-BlockAIOCB *pr_manager_execute(PRManager *pr_mgr,
-                               AioContext *ctx, int fd,
-                               struct sg_io_hdr *hdr,
-                               BlockCompletionFunc *complete,
-                               void *opaque);
+int coroutine_fn pr_manager_execute(PRManager *pr_mgr, AioContext *ctx, int fd,
+                                    struct sg_io_hdr *hdr);
 
 PRManager *pr_manager_lookup(const char *id, Error **errp);
 
diff --git a/block/file-posix.c b/block/file-posix.c
index 821743d2b2..9439e8c054 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3092,24 +3092,25 @@ hdev_open_Mac_error:
 }
 
 #if defined(__linux__)
-
-static BlockAIOCB *hdev_aio_ioctl(BlockDriverState *bs,
-        unsigned long int req, void *buf,
-        BlockCompletionFunc *cb, void *opaque)
+static int coroutine_fn
+hdev_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 {
     BDRVRawState *s = bs->opaque;
     RawPosixAIOData *acb;
     ThreadPool *pool;
+    int ret;
 
-    if (fd_open(bs) < 0)
-        return NULL;
+    ret = fd_open(bs);
+    if (ret < 0) {
+        return ret;
+    }
 
     if (req == SG_IO && s->pr_mgr) {
         struct sg_io_hdr *io_hdr = buf;
         if (io_hdr->cmdp[0] == PERSISTENT_RESERVE_OUT ||
             io_hdr->cmdp[0] == PERSISTENT_RESERVE_IN) {
             return pr_manager_execute(s->pr_mgr, bdrv_get_aio_context(bs),
-                                      s->fd, io_hdr, cb, opaque);
+                                      s->fd, io_hdr);
         }
     }
 
@@ -3121,7 +3122,7 @@ static BlockAIOCB *hdev_aio_ioctl(BlockDriverState *bs,
     acb->ioctl.buf = buf;
     acb->ioctl.cmd = req;
     pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
-    return thread_pool_submit_aio(pool, aio_worker, acb, cb, opaque);
+    return thread_pool_submit_co(pool, aio_worker, acb);
 }
 #endif /* linux */
 
@@ -3263,7 +3264,7 @@ static BlockDriver bdrv_host_device = {
 
     /* generic scsi device */
 #ifdef __linux__
-    .bdrv_aio_ioctl     = hdev_aio_ioctl,
+    .bdrv_co_ioctl          = hdev_co_ioctl,
 #endif
 };
 
@@ -3385,7 +3386,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_lock_medium   = cdrom_lock_medium,
 
     /* generic scsi device */
-    .bdrv_aio_ioctl     = hdev_aio_ioctl,
+    .bdrv_co_ioctl      = hdev_co_ioctl,
 };
 #endif /* __linux__ */
 
diff --git a/scsi/pr-manager.c b/scsi/pr-manager.c
index 2a8f300dde..d9f4e8c3ad 100644
--- a/scsi/pr-manager.c
+++ b/scsi/pr-manager.c
@@ -48,24 +48,21 @@ static int pr_manager_worker(void *opaque)
 }
 
 
-BlockAIOCB *pr_manager_execute(PRManager *pr_mgr,
-                               AioContext *ctx, int fd,
-                               struct sg_io_hdr *hdr,
-                               BlockCompletionFunc *complete,
-                               void *opaque)
+int coroutine_fn pr_manager_execute(PRManager *pr_mgr, AioContext *ctx, int fd,
+                                    struct sg_io_hdr *hdr)
 {
-    PRManagerData *data = g_new(PRManagerData, 1);
     ThreadPool *pool = aio_get_thread_pool(ctx);
+    PRManagerData data = {
+        .pr_mgr = pr_mgr,
+        .fd     = fd,
+        .hdr    = hdr,
+    };
 
-    trace_pr_manager_execute(fd, hdr->cmdp[0], hdr->cmdp[1], opaque);
-    data->pr_mgr = pr_mgr;
-    data->fd = fd;
-    data->hdr = hdr;
+    trace_pr_manager_execute(fd, hdr->cmdp[0], hdr->cmdp[1]);
 
     /* The matching object_unref is in pr_manager_worker.  */
     object_ref(OBJECT(pr_mgr));
-    return thread_pool_submit_aio(pool, pr_manager_worker,
-                                  data, complete, opaque);
+    return thread_pool_submit_co(pool, pr_manager_worker, &data);
 }
 
 bool pr_manager_is_connected(PRManager *pr_mgr)
diff --git a/scsi/trace-events b/scsi/trace-events
index 45f5b6e49b..f8a68b11eb 100644
--- a/scsi/trace-events
+++ b/scsi/trace-events
@@ -1,3 +1,3 @@
 # scsi/pr-manager.c
-pr_manager_execute(int fd, int cmd, int sa, void *opaque) "fd=%d cmd=0x%02x service action=0x%02x opaque=%p"
+pr_manager_execute(int fd, int cmd, int sa) "fd=%d cmd=0x%02x service action=0x%02x"
 pr_manager_run(int fd, int cmd, int sa) "fd=%d cmd=0x%02x service action=0x%02x"
-- 
2.19.1

[Qemu-devel] [PATCH 12/12] file-posix: Avoid aio_worker() for QEMU_AIO_IOCTL

[Kevin Wolf]

aio_worker() doesn't add anything interesting, it's only a useless
indirection. Call the handler function directly instead.

As we know that this handler function is only called from coroutine
context and the coroutine stays around until the worker thread finishes,
we can keep RawPosixAIOData on the stack.

This was the last user of aio_worker(), so the function goes away now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 55 +++++++++++++---------------------------------
 1 file changed, 15 insertions(+), 40 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 9439e8c054..9f305f3d49 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1131,8 +1131,9 @@ static int hdev_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
 }
 #endif
 
-static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
+static int handle_aiocb_ioctl(void *opaque)
 {
+    RawPosixAIOData *aiocb = opaque;
     int ret;
 
     ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
@@ -1767,34 +1768,6 @@ out:
     return result;
 }
 
-static int aio_worker(void *arg)
-{
-    RawPosixAIOData *aiocb = arg;
-    ssize_t ret = 0;
-
-    switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
-    case QEMU_AIO_IOCTL:
-        ret = handle_aiocb_ioctl(aiocb);
-        break;
-    case QEMU_AIO_READ:
-    case QEMU_AIO_WRITE:
-    case QEMU_AIO_FLUSH:
-    case QEMU_AIO_DISCARD:
-    case QEMU_AIO_WRITE_ZEROES:
-    case QEMU_AIO_WRITE_ZEROES | QEMU_AIO_DISCARD:
-    case QEMU_AIO_COPY_RANGE:
-    case QEMU_AIO_TRUNCATE:
-        g_assert_not_reached();
-    default:
-        fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
-        ret = -EINVAL;
-        break;
-    }
-
-    g_free(aiocb);
-    return ret;
-}
-
 static int coroutine_fn raw_thread_pool_submit(BlockDriverState *bs,
                                                ThreadPoolFunc func, void *arg)
 {
@@ -3096,8 +3069,7 @@ static int coroutine_fn
 hdev_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 {
     BDRVRawState *s = bs->opaque;
-    RawPosixAIOData *acb;
-    ThreadPool *pool;
+    RawPosixAIOData acb;
     int ret;
 
     ret = fd_open(bs);
@@ -3114,15 +3086,18 @@ hdev_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
         }
     }
 
-    acb = g_new(RawPosixAIOData, 1);
-    acb->bs = bs;
-    acb->aio_type = QEMU_AIO_IOCTL;
-    acb->aio_fildes = s->fd;
-    acb->aio_offset = 0;
-    acb->ioctl.buf = buf;
-    acb->ioctl.cmd = req;
-    pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
-    return thread_pool_submit_co(pool, aio_worker, acb);
+    acb = (RawPosixAIOData) {
+        .bs         = bs,
+        .aio_type   = QEMU_AIO_IOCTL,
+        .aio_fildes = s->fd,
+        .aio_offset = 0,
+        .ioctl      = {
+            .buf        = buf,
+            .cmd        = req,
+        },
+    };
+
+    return raw_thread_pool_submit(bs, handle_aiocb_ioctl, &acb);
 }
 #endif /* linux */
 
-- 
2.19.1

Re: [Qemu-devel] [PATCH 00/12] file-posix: Simplify delegation to worker thread

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20181031215622.27690-1-kwolf@redhat.com
Subject: [Qemu-devel] [PATCH 00/12] file-posix: Simplify delegation to worker thread

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f3d24f259d file-posix: Avoid aio_worker() for QEMU_AIO_IOCTL
a20af5e7d5 file-posix: Switch to .bdrv_co_ioctl
86c8cd0ca9 file-posix: Remove paio_submit_co()
a6798de879 file-posix: Avoid aio_worker() for QEMU_AIO_READ/WRITE
f667c83e70 file-posix: Move read/write operation logic out of aio_worker()
028fa45b36 file-posix: Avoid aio_worker() for QEMU_AIO_FLUSH
992060b92c file-posix: Avoid aio_worker() for QEMU_AIO_DISCARD
b27306c38f file-posix: Avoid aio_worker() for QEMU_AIO_WRITE_ZEROES
934194f753 file-posix: Avoid aio_worker() for QEMU_AIO_COPY_RANGE
ab3479aa9e file-posix: Avoid aio_worker() for QEMU_AIO_TRUNCATE
055cea24a4 file-posix: Factor out raw_thread_pool_submit()
91834b3fb8 file-posix: Reorganise RawPosixAIOData

=== OUTPUT BEGIN ===
Checking PATCH 1/12: file-posix: Reorganise RawPosixAIOData...
ERROR: suspect code indent for conditional statements (8, 13)
#96: FILE: block/file-posix.c:1278:
+        if (aiocb->io.niov == 1) {
+             return handle_aiocb_rw_linear(aiocb, aiocb->io.iov->iov_base);

total: 1 errors, 0 warnings, 203 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/12: file-posix: Factor out raw_thread_pool_submit()...
Checking PATCH 3/12: file-posix: Avoid aio_worker() for QEMU_AIO_TRUNCATE...
Checking PATCH 4/12: file-posix: Avoid aio_worker() for QEMU_AIO_COPY_RANGE...
Checking PATCH 5/12: file-posix: Avoid aio_worker() for QEMU_AIO_WRITE_ZEROES...
Checking PATCH 6/12: file-posix: Avoid aio_worker() for QEMU_AIO_DISCARD...
Checking PATCH 7/12: file-posix: Avoid aio_worker() for QEMU_AIO_FLUSH...
Checking PATCH 8/12: file-posix: Move read/write operation logic out of aio_worker()...
ERROR: suspect code indent for conditional statements (8, 13)
#25: FILE: block/file-posix.c:1279:
         if (aiocb->io.niov == 1) {
+             nbytes = handle_aiocb_rw_linear(aiocb, aiocb->io.iov->iov_base);

total: 1 errors, 0 warnings, 73 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 9/12: file-posix: Avoid aio_worker() for QEMU_AIO_READ/WRITE...
Checking PATCH 10/12: file-posix: Remove paio_submit_co()...
Checking PATCH 11/12: file-posix: Switch to .bdrv_co_ioctl...
Checking PATCH 12/12: file-posix: Avoid aio_worker() for QEMU_AIO_IOCTL...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 00/12] file-posix: Simplify delegation to worker thread

[Kevin Wolf]

Am 31.10.2018 um 22:56 hat Kevin Wolf geschrieben:
> This series cleans up and simplifies the code that calls worker thread
> functions for the various operations in the file-posix driver. This
> results in less indirection and better readability as well as reduced
> heap allocations because we can store ACBs on the coroutine stack now.

ping?

Re: [Qemu-devel] [Qemu-block] [PATCH 00/12] file-posix: Simplify delegation to worker thread

[Kevin Wolf]

Am 12.11.2018 um 17:50 hat Kevin Wolf geschrieben:
> Am 31.10.2018 um 22:56 hat Kevin Wolf geschrieben:
> > This series cleans up and simplifies the code that calls worker thread
> > functions for the various operations in the file-posix driver. This
> > results in less indirection and better readability as well as reduced
> > heap allocations because we can store ACBs on the coroutine stack now.
> 
> ping?

Seems nobody has objections, so applied to block-next.

Kevin

Re: [Qemu-devel] [PATCH 08/12] file-posix: Move read/write operation logic out of aio_worker()

[Kevin Wolf]

Am 31.10.2018 um 22:56 hat Kevin Wolf geschrieben:
> aio_worker() for reads and writes isn't boring enough yet. It still does
> some postprocessing for handling short reads and turning the result into
> the right return value.
> 
> However, there is no reason why handle_aiocb_rw() couldn't do the same,
> and even without duplicating code between the read and write path. So
> move the code there.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> @@ -1354,7 +1356,21 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
>      }
>      qemu_vfree(buf);
>  
> -    return nbytes;
> +out:
> +    if (nbytes == aiocb->aio_nbytes) {
> +        return 0;
> +    } else if (nbytes >= 0 && nbytes < aiocb->aio_nbytes) {
> +        if (aiocb->aio_type & QEMU_AIO_WRITE) {
> +            return -EINVAL;
> +        } else {
> +            iov_memset(aiocb->io.iov, aiocb->io.niov, nbytes,
> +                      0, aiocb->aio_nbytes - nbytes);
> +            return aiocb->aio_nbytes;

While reviewing Eric's patches, I noticed that this should be return 0.
Fixing it in my queue.

Kevin