diff mbox series

[v8,6/7] block: drop bdrv_prwv

Message ID 20200915164411.20590-7-vsementsov@virtuozzo.com
State Superseded
Headers show
Series coroutines: generate wrapper code | expand

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 15, 2020, 4:44 p.m. UTC 
Now that we are not maintaining boilerplate code for coroutine
wrappers, there is no more sense in keeping the extra indirection layer
of bdrv_prwv().  Let's drop it and instead generate pure bdrv_preadv()
and bdrv_pwritev().

Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on
success, auto generated functions will instead return zero, as their
_co_ prototype. Still, it's simple to make the conversion safe: the
only external user of bdrv_pwritev() is test-bdrv-drain, and it is
comfortable enough with bdrv_co_pwritev() instead. So prototypes are
moved to local block/coroutines.h. Next, the only internal use is
bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on
success.

Of course, it would be great to convert bdrv_pread() and bdrv_pwrite()
to return 0 on success. But this requires audit (and probably
conversion) of all their users, let's leave it for another day
refactoring.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/coroutines.h      | 10 ++++-----
 include/block/block.h   |  2 --
 block/io.c              | 49 ++++++++---------------------------------
 tests/test-bdrv-drain.c |  2 +-
 4 files changed, 15 insertions(+), 48 deletions(-)

Comments

Philippe Mathieu-Daudé Sept. 24, 2020, 8:31 a.m. UTC | #1 
On 9/15/20 6:44 PM, Vladimir Sementsov-Ogievskiy wrote:
> Now that we are not maintaining boilerplate code for coroutine

> wrappers, there is no more sense in keeping the extra indirection layer

> of bdrv_prwv().  Let's drop it and instead generate pure bdrv_preadv()

> and bdrv_pwritev().

> 

> Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on

> success, auto generated functions will instead return zero, as their

> _co_ prototype. Still, it's simple to make the conversion safe: the

> only external user of bdrv_pwritev() is test-bdrv-drain, and it is

> comfortable enough with bdrv_co_pwritev() instead. So prototypes are

> moved to local block/coroutines.h. Next, the only internal use is

> bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on

> success.

> 

> Of course, it would be great to convert bdrv_pread() and bdrv_pwrite()

> to return 0 on success. But this requires audit (and probably

> conversion) of all their users, let's leave it for another day

> refactoring.

> 

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> Reviewed-by: Eric Blake <eblake@redhat.com>

> ---

>  block/coroutines.h      | 10 ++++-----

>  include/block/block.h   |  2 --

>  block/io.c              | 49 ++++++++---------------------------------

>  tests/test-bdrv-drain.c |  2 +-

>  4 files changed, 15 insertions(+), 48 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Stefan Hajnoczi Sept. 24, 2020, 12:15 p.m. UTC | #2 
On Tue, Sep 15, 2020 at 07:44:10PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Now that we are not maintaining boilerplate code for coroutine

> wrappers, there is no more sense in keeping the extra indirection layer

> of bdrv_prwv().  Let's drop it and instead generate pure bdrv_preadv()

> and bdrv_pwritev().

> 

> Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on

> success, auto generated functions will instead return zero, as their

> _co_ prototype. Still, it's simple to make the conversion safe: the

> only external user of bdrv_pwritev() is test-bdrv-drain, and it is

> comfortable enough with bdrv_co_pwritev() instead. So prototypes are

> moved to local block/coroutines.h. Next, the only internal use is

> bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on

> success.

> 

> Of course, it would be great to convert bdrv_pread() and bdrv_pwrite()

> to return 0 on success. But this requires audit (and probably

> conversion) of all their users, let's leave it for another day

> refactoring.

> 

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> Reviewed-by: Eric Blake <eblake@redhat.com>

> ---

>  block/coroutines.h      | 10 ++++-----

>  include/block/block.h   |  2 --

>  block/io.c              | 49 ++++++++---------------------------------

>  tests/test-bdrv-drain.c |  2 +-

>  4 files changed, 15 insertions(+), 48 deletions(-)


Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox series

Patch

diff --git a/block/coroutines.h b/block/coroutines.h
index c62b3a2697..6c63a819c9 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -31,12 +31,12 @@  int coroutine_fn bdrv_co_check(BlockDriverState *bs,
                                BdrvCheckResult *res, BdrvCheckMode fix);
 int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
 
-int coroutine_fn
-bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
-             bool is_write, BdrvRequestFlags flags);
 int generated_co_wrapper
-bdrv_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
-          bool is_write, BdrvRequestFlags flags);
+bdrv_preadv(BdrvChild *child, int64_t offset, unsigned int bytes,
+            QEMUIOVector *qiov, BdrvRequestFlags flags);
+int generated_co_wrapper
+bdrv_pwritev(BdrvChild *child, int64_t offset, unsigned int bytes,
+             QEMUIOVector *qiov, BdrvRequestFlags flags);
 
 int coroutine_fn
 bdrv_co_common_block_status_above(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index d8fb02fa2a..b8b4c177de 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -383,9 +383,7 @@  int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags);
 int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
 int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes);
-int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
 int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes);
-int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
 int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
                      const void *buf, int count);
 /*
diff --git a/block/io.c b/block/io.c
index 5270d68d72..68d7d9cf80 100644
--- a/block/io.c
+++ b/block/io.c
@@ -890,23 +890,11 @@  static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
     return 0;
 }
 
-int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
-                              QEMUIOVector *qiov, bool is_write,
-                              BdrvRequestFlags flags)
-{
-    if (is_write) {
-        return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
-    } else {
-        return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
-    }
-}
-
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags)
 {
-    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
-
-    return bdrv_prwv(child, offset, &qiov, true, BDRV_REQ_ZERO_WRITE | flags);
+    return bdrv_pwritev(child, offset, bytes, NULL,
+                        BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /*
@@ -950,41 +938,19 @@  int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
     }
 }
 
-/* return < 0 if error. See bdrv_pwrite() for the return codes */
-int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
-{
-    int ret;
-
-    ret = bdrv_prwv(child, offset, qiov, false, 0);
-    if (ret < 0) {
-        return ret;
-    }
-
-    return qiov->size;
-}
-
 /* See bdrv_pwrite() for the return codes */
 int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
 {
+    int ret;
     QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
 
     if (bytes < 0) {
         return -EINVAL;
     }
 
-    return bdrv_preadv(child, offset, &qiov);
-}
-
-int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
-{
-    int ret;
+    ret = bdrv_preadv(child, offset, bytes, &qiov,  0);
 
-    ret = bdrv_prwv(child, offset, qiov, true, 0);
-    if (ret < 0) {
-        return ret;
-    }
-
-    return qiov->size;
+    return ret < 0 ? ret : bytes;
 }
 
 /* Return no. of bytes on success or < 0 on error. Important errors are:
@@ -995,13 +961,16 @@  int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
 */
 int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
 {
+    int ret;
     QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
 
     if (bytes < 0) {
         return -EINVAL;
     }
 
-    return bdrv_pwritev(child, offset, &qiov);
+    ret = bdrv_pwritev(child, offset, bytes, &qiov, 0);
+
+    return ret < 0 ? ret : bytes;
 }
 
 /*
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 1107271840..1595bbc92e 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -1872,7 +1872,7 @@  static int coroutine_fn bdrv_replace_test_co_preadv(BlockDriverState *bs,
         }
         s->io_co = NULL;
 
-        ret = bdrv_preadv(bs->backing, offset, qiov);
+        ret = bdrv_co_preadv(bs->backing, offset, bytes, qiov, 0);
         s->has_read = true;
 
         /* Wake up drain_co if it runs */