diff mbox series

[v4,4/5] block: drop bdrv_prwv

Message ID 20200525100801.13859-5-vsementsov@virtuozzo.com
State New
Headers show
Series coroutines: generate wrapper code | expand

Commit Message

Vladimir Sementsov-Ogievskiy May 25, 2020, 10:08 a.m. UTC
Now, when we are not more paying extra code for coroutine wrappers,
there no more sence in extra indirection layer: bdrv_prwv(). Let's drop
it and instead genereate 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.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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

Eric Blake May 26, 2020, 9:34 p.m. UTC | #1
On 5/25/20 5:08 AM, Vladimir Sementsov-Ogievskiy wrote:
> Now, when we are not more paying extra code for coroutine wrappers,
> there no more sence in extra indirection layer: bdrv_prwv(). Let's drop
> it and instead genereate pure bdrv_preadv() and bdrv_pwritev().

Typos and grammar; I suggest:

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.

Does returning bytes on success buy us anything useful?  We don't allow 
partial success, so blindly returning 0 on success is no less useful. 
True, we'd have to audit callers to make sure we aren't doing an 
inadvertent semantic change.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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(-)
> 

At any rate, I think this patch is reasonable.

Reviewed-by: Eric Blake <eblake@redhat.com>
Vladimir Sementsov-Ogievskiy May 27, 2020, 11:35 a.m. UTC | #2
27.05.2020 00:34, Eric Blake wrote:
> On 5/25/20 5:08 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Now, when we are not more paying extra code for coroutine wrappers,
>> there no more sence in extra indirection layer: bdrv_prwv(). Let's drop
>> it and instead genereate pure bdrv_preadv() and bdrv_pwritev().
> 
> Typos and grammar; I suggest:
> 
> 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.
> 
> Does returning bytes on success buy us anything useful?  We don't allow partial success, so blindly returning 0 on success is no less useful. True, we'd have to audit callers to make sure we aren't doing an inadvertent semantic change.

Not so simple.. Seems we have 151 calls in 23 files:

# git grep -l '\(bdrv_pread\|bdrv_pwrite\)\>' '*.[hc]' | wc -l
23
# git grep '\(bdrv_pread\|bdrv_pwrite\)\>' '*.[hc]' | wc -l
151

Amyway, let it be another series. And such series should probably change most of calls to _co_ variants.

> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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(-)
>>
> 
> At any rate, I think this patch is reasonable.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
diff mbox series

Patch

diff --git a/block/coroutines.h b/block/coroutines.h
index e2047697d6..233b8b3694 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -32,12 +32,12 @@  int coroutine_fn bdrv_co_check(BlockDriverState *bs,
                                BdrvCheckResult *res, BdrvCheckMode fix);
 void 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 aed6ffcc4f..dce99f8453 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -379,9 +379,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 f9700cc897..305ee7219a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -892,23 +892,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);
 }
 
 /*
@@ -952,41 +940,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:
@@ -997,13 +963,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 */