[Qemu-devel] [PATCH 0/2] remove blk_pread_unthrottled()

[Qemu-devel] [PATCH 0/2] remove blk_pread_unthrottled()

[Manos Pitsidianakis]

Cleanups for minor stuff I noticed while looking around blk_root_drained_*

Manos Pitsidianakis (2):
  block/block-backend.c: add blk_check_byte_request call to
    blk_pread/blk_pwrite
  block/block-backend.c: remove blk_pread_unthrottled()

 include/sysemu/block-backend.h |  2 --
 block/block-backend.c          | 27 ++++++---------------------
 hw/block/hd-geometry.c         |  7 +------
 3 files changed, 7 insertions(+), 29 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/2] block/block-backend.c: add blk_check_byte_request call to blk_pread/blk_pwrite

[Manos Pitsidianakis]

blk_check_byte_request() is called from the blk_co_pwritev/blk_co_preadv to
check if the request offset and request bytes parameters are valid for the
given Blockbackend. Let's do that in blk_pread/blk_pwrite too.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/block-backend.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 45d9101be3..110a52d5b1 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1288,22 +1288,23 @@ BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
 
 int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int count)
 {
-    int ret = blk_prw(blk, offset, buf, count, blk_read_entry, 0);
+    int ret = blk_check_byte_request(blk, offset, count);
     if (ret < 0) {
         return ret;
     }
-    return count;
+    ret = blk_prw(blk, offset, buf, count, blk_read_entry, 0);
+    return ret < 0 ? ret : count;
 }
 
 int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count,
                BdrvRequestFlags flags)
 {
-    int ret = blk_prw(blk, offset, (void *) buf, count, blk_write_entry,
-                      flags);
+    int ret = blk_check_byte_request(blk, offset, count);
     if (ret < 0) {
         return ret;
     }
-    return count;
+    ret = blk_prw(blk, offset, (void *) buf, count, blk_write_entry, flags);
+    return ret < 0 ? ret : count;
 }
 
 int64_t blk_getlength(BlockBackend *blk)
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/2] block/block-backend.c: add blk_check_byte_request call to blk_pread/blk_pwrite

[Kevin Wolf]

Am 20.09.2017 um 13:43 hat Manos Pitsidianakis geschrieben:
> blk_check_byte_request() is called from the blk_co_pwritev/blk_co_preadv to
> check if the request offset and request bytes parameters are valid for the
> given Blockbackend. Let's do that in blk_pread/blk_pwrite too.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

I don't think this is necessary, blk_pread/pwrite() are only wrappers
around blk_co_preadv/pwritev(), so we're already checking the
parameters.

Kevin

Re: [Qemu-devel] [PATCH 1/2] block/block-backend.c: add blk_check_byte_request call to blk_pread/blk_pwrite

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Wed, Sep 20, 2017 at 02:24:21PM +0200, Kevin Wolf wrote:
>Am 20.09.2017 um 13:43 hat Manos Pitsidianakis geschrieben:
>> blk_check_byte_request() is called from the blk_co_pwritev/blk_co_preadv to
>> check if the request offset and request bytes parameters are valid for the
>> given Blockbackend. Let's do that in blk_pread/blk_pwrite too.
>>
>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>
>I don't think this is necessary, blk_pread/pwrite() are only wrappers
>around blk_co_preadv/pwritev(), so we're already checking the
>parameters.
>
>Kevin


Alright, this can be dropped then. Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 2/2] block/block-backend.c: remove blk_pread_unthrottled()

[Manos Pitsidianakis]

blk_pread_unthrottled was used to bypass I/O throttling on the BlockBackend in
the case of async I/O. This is not needed anymore and we can just call
blk_pread() directly.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 include/sysemu/block-backend.h |  2 --
 block/block-backend.c          | 16 ----------------
 hw/block/hd-geometry.c         |  7 +------
 3 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c4e52a5fa3..c881bfdfe4 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -117,8 +117,6 @@ char *blk_get_attached_dev_id(BlockBackend *blk);
 BlockBackend *blk_by_dev(void *dev);
 BlockBackend *blk_by_qdev_id(const char *id, Error **errp);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
-int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
-                          int bytes);
 int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
                                unsigned int bytes, QEMUIOVector *qiov,
                                BdrvRequestFlags flags);
diff --git a/block/block-backend.c b/block/block-backend.c
index 110a52d5b1..83329fce15 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1145,22 +1145,6 @@ static int blk_prw(BlockBackend *blk, int64_t offset, uint8_t *buf,
     return rwco.ret;
 }
 
-int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
-                          int count)
-{
-    int ret;
-
-    ret = blk_check_byte_request(blk, offset, count);
-    if (ret < 0) {
-        return ret;
-    }
-
-    blk_root_drained_begin(blk->root);
-    ret = blk_pread(blk, offset, buf, count);
-    blk_root_drained_end(blk->root);
-    return ret;
-}
-
 int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                       int bytes, BdrvRequestFlags flags)
 {
diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index 57ad5012a7..211307f73f 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -62,12 +62,7 @@ static int guess_disk_lchs(BlockBackend *blk,
 
     blk_get_geometry(blk, &nb_sectors);
 
-    /**
-     * The function will be invoked during startup not only in sync I/O mode,
-     * but also in async I/O mode. So the I/O throttling function has to
-     * be disabled temporarily here, not permanently.
-     */
-    if (blk_pread_unthrottled(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
+    if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
         return -1;
     }
     /* test msdos magic */
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/2] block/block-backend.c: remove blk_pread_unthrottled()

[Kevin Wolf]

Am 20.09.2017 um 13:43 hat Manos Pitsidianakis geschrieben:
> blk_pread_unthrottled was used to bypass I/O throttling on the BlockBackend in
> the case of async I/O. This is not needed anymore and we can just call
> blk_pread() directly.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

We already did a related commit to the same effect in block/io.c:

	commit 90c78624f157ba41c3761c1a54864de03a7ec350
	Author: Kevin Wolf <kwolf@redhat.com>
	Date:   Thu Apr 7 18:33:29 2016 +0200

		block: Don't disable I/O throttling on sync requests

		We had to disable I/O throttling with synchronous requests because we
		didn't use to run timers in nested event loops when the code was
		introduced. This isn't true any more, and throttling works just fine
		even when using the synchronous API.

		The removed code is in fact dead code since commit a8823a3b ('block: Use
		blk_co_pwritev() for blk_write()') because I/O throttling can only be
		set on the top layer, but BlockBackend always uses the coroutine
		interface now instead of using the sync API emulation in block.c.

		Signed-off-by: Kevin Wolf <kwolf@redhat.com>
		Message-Id: <1458660792-3035-2-git-send-email-kwolf@redhat.com>
		Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
		Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
		Signed-off-by: Kevin Wolf <kwolf@redhat.com>

The same reasoning (at least the first part) applies to this patch.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] block/block-backend.c: remove blk_pread_unthrottled()

[John Snow]

On 09/20/2017 07:43 AM, Manos Pitsidianakis wrote:
> blk_pread_unthrottled was used to bypass I/O throttling on the BlockBackend in
> the case of async I/O. This is not needed anymore and we can just call
> blk_pread() directly.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  include/sysemu/block-backend.h |  2 --
>  block/block-backend.c          | 16 ----------------
>  hw/block/hd-geometry.c         |  7 +------
>  3 files changed, 1 insertion(+), 24 deletions(-)
> 


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