[07/11] block: optimization blk_pwrite_compressed()
diff mbox

Message ID 1464686130-12265-8-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev May 31, 2016, 9:15 a.m. UTC
From: Pavel Butsykin <pbutsykin@virtuozzo.com>

For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
and use the blk_prw() as a generic one.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 19 +++++++++++------
 block/io.c                     | 48 ------------------------------------------
 include/block/block.h          |  3 +--
 include/sysemu/block-backend.h |  3 +++
 4 files changed, 17 insertions(+), 56 deletions(-)

Comments

Stefan Hajnoczi June 13, 2016, 1:11 p.m. UTC | #1
On Tue, May 31, 2016 at 12:15:26PM +0300, Denis V. Lunev wrote:
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 57df069..3d7b446 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
>                                        int count, BdrvRequestFlags flags);
>  int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>                            int count);
> +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
> +                                           unsigned int bytes,
> +                                           QEMUIOVector *qiov);

Perhaps blk_co_pwritev_compressed() isn't necessary at all since
blk_co_pwritev() already exists and has the flags argument:

int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
                               unsigned int bytes, QEMUIOVector *qiov,
                               BdrvRequestFlags flags);
Eric Blake June 13, 2016, 8:16 p.m. UTC | #2
On 06/13/2016 07:11 AM, Stefan Hajnoczi wrote:
> On Tue, May 31, 2016 at 12:15:26PM +0300, Denis V. Lunev wrote:
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> index 57df069..3d7b446 100644
>> --- a/include/sysemu/block-backend.h
>> +++ b/include/sysemu/block-backend.h
>> @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
>>                                        int count, BdrvRequestFlags flags);
>>  int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>>                            int count);
>> +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
>> +                                           unsigned int bytes,
>> +                                           QEMUIOVector *qiov);
> 
> Perhaps blk_co_pwritev_compressed() isn't necessary at all since
> blk_co_pwritev() already exists and has the flags argument:
> 
> int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
>                                unsigned int bytes, QEMUIOVector *qiov,
>                                BdrvRequestFlags flags);

Are you arguing that we should have a new BDRV_REQ_COMPRESSED flag that
can be set in .supported_write_flags for drivers that know how to do a
compressed write?
Stefan Hajnoczi June 15, 2016, 10:22 a.m. UTC | #3
On Mon, Jun 13, 2016 at 02:16:08PM -0600, Eric Blake wrote:
> On 06/13/2016 07:11 AM, Stefan Hajnoczi wrote:
> > On Tue, May 31, 2016 at 12:15:26PM +0300, Denis V. Lunev wrote:
> >> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> >> index 57df069..3d7b446 100644
> >> --- a/include/sysemu/block-backend.h
> >> +++ b/include/sysemu/block-backend.h
> >> @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
> >>                                        int count, BdrvRequestFlags flags);
> >>  int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
> >>                            int count);
> >> +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
> >> +                                           unsigned int bytes,
> >> +                                           QEMUIOVector *qiov);
> > 
> > Perhaps blk_co_pwritev_compressed() isn't necessary at all since
> > blk_co_pwritev() already exists and has the flags argument:
> > 
> > int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
> >                                unsigned int bytes, QEMUIOVector *qiov,
> >                                BdrvRequestFlags flags);
> 
> Are you arguing that we should have a new BDRV_REQ_COMPRESSED flag that
> can be set in .supported_write_flags for drivers that know how to do a
> compressed write?

Never mind, it's too much noise and out of scope for this series.

I'm fine with blk_co_pwritev_compressed().

Stefan
Eric Blake June 24, 2016, 3:42 p.m. UTC | #4
On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
> is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
> and use the blk_prw() as a generic one.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---

> +++ b/include/block/block.h
> @@ -65,6 +65,7 @@ typedef enum {
>      BDRV_REQ_MAY_UNMAP          = 0x4,
>      BDRV_REQ_NO_SERIALISING     = 0x8,
>      BDRV_REQ_FUA                = 0x10,
> +    BDRV_REQ_WRITE_COMPRESSED   = 0x20,
>  } BdrvRequestFlags;
>  

Needs to be rebased on top of commit fa166538, and adjust BDRV_REQ_MASK
at that time.
Kevin Wolf June 28, 2016, 11:47 a.m. UTC | #5
Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
> is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
> and use the blk_prw() as a generic one.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>

Oh, so you already do use a flag. Nice. :-)

> diff --git a/block/block-backend.c b/block/block-backend.c
> index 3c1fc50..9e1c793 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
>          flags |= BDRV_REQ_FUA;
>      }
>  
> -    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> +    if (flags & BDRV_REQ_WRITE_COMPRESSED) {
> +        return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov);
> +    } else {
> +        return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> +    }
>  }

If you move the processing of the flag inside bdrv_co_pwritev(), where I
think it belongs anyway, you could use the flag from the start (by going
through bdrv_prwv_co()) instead of temporarily introducing your own
coroutine wrapper. I think that would make the initial conversion
patches quite a bit simpler.

Kevin
Pavel Butsykin June 28, 2016, 12:32 p.m. UTC | #6
On 28.06.2016 14:47, Kevin Wolf wrote:
> Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
>> is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
>> and use the blk_prw() as a generic one.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: John Snow <jsnow@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>
> Oh, so you already do use a flag. Nice. :-)
>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 3c1fc50..9e1c793 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
>>           flags |= BDRV_REQ_FUA;
>>       }
>>
>> -    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
>> +    if (flags & BDRV_REQ_WRITE_COMPRESSED) {
>> +        return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov);
>> +    } else {
>> +        return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
>> +    }
>>   }
>
> If you move the processing of the flag inside bdrv_co_pwritev(), where I
> think it belongs anyway, you could use the flag from the start (by going
> through bdrv_prwv_co()) instead of temporarily introducing your own
> coroutine wrapper. I think that would make the initial conversion
> patches quite a bit simpler.

You propose to add bdrv_driver_compressed and call it from 
bdrv_aligned_pwritev ?

> Kevin
>
Kevin Wolf June 28, 2016, 1:05 p.m. UTC | #7
Am 28.06.2016 um 14:32 hat Pavel Butsykin geschrieben:
> On 28.06.2016 14:47, Kevin Wolf wrote:
> >Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
> >>From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> >>
> >>For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
> >>is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
> >>and use the blk_prw() as a generic one.
> >>
> >>Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>CC: Jeff Cody <jcody@redhat.com>
> >>CC: Markus Armbruster <armbru@redhat.com>
> >>CC: Eric Blake <eblake@redhat.com>
> >>CC: John Snow <jsnow@redhat.com>
> >>CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>CC: Kevin Wolf <kwolf@redhat.com>
> >
> >Oh, so you already do use a flag. Nice. :-)
> >
> >>diff --git a/block/block-backend.c b/block/block-backend.c
> >>index 3c1fc50..9e1c793 100644
> >>--- a/block/block-backend.c
> >>+++ b/block/block-backend.c
> >>@@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
> >>          flags |= BDRV_REQ_FUA;
> >>      }
> >>
> >>-    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> >>+    if (flags & BDRV_REQ_WRITE_COMPRESSED) {
> >>+        return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov);
> >>+    } else {
> >>+        return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> >>+    }
> >>  }
> >
> >If you move the processing of the flag inside bdrv_co_pwritev(), where I
> >think it belongs anyway, you could use the flag from the start (by going
> >through bdrv_prwv_co()) instead of temporarily introducing your own
> >coroutine wrapper. I think that would make the initial conversion
> >patches quite a bit simpler.
> 
> You propose to add bdrv_driver_compressed and call it from
> bdrv_aligned_pwritev ?

I'm not sure if we can easily move it that much down the caller chain.
If we can, great. But here I was just thinking of making the switch at
the start of bdrv_co_pwritev() so that you can reuse the existing
wrappers like bdrv_prwv_co().

The long-term advantage of putting it into bdrv_aligned_pwritev() could
be that we would ge the common alignment handling. But I think qcow2
still errors out if you rewrite an already allocated cluster with
qcow2_pwritev_compressed(), so currently doing sub-cluster (or even
sub-sector) writes doesn't make much sense anyway. So I doubt it's worth
the hassle now.

Kevin

Patch
diff mbox

diff --git a/block/block-backend.c b/block/block-backend.c
index 3c1fc50..9e1c793 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -785,7 +785,11 @@  int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
         flags |= BDRV_REQ_FUA;
     }
 
-    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
+    if (flags & BDRV_REQ_WRITE_COMPRESSED) {
+        return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov);
+    } else {
+        return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
+    }
 }
 
 typedef struct BlkRwCo {
@@ -1480,12 +1484,15 @@  int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
 int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
                           int count)
 {
-    int ret = blk_check_byte_request(blk, offset, count);
-    if (ret < 0) {
-        return ret;
-    }
+    return blk_prw(blk, offset, (void *) buf, count, blk_write_entry,
+                   BDRV_REQ_WRITE_COMPRESSED);
+}
 
-    return bdrv_pwrite_compressed(blk_bs(blk), offset, buf, count);
+int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
+                                           unsigned int bytes,
+                                           QEMUIOVector *qiov)
+{
+    return blk_co_pwritev(blk, offset, bytes, qiov, BDRV_REQ_WRITE_COMPRESSED);
 }
 
 int blk_truncate(BlockBackend *blk, int64_t offset)
diff --git a/block/io.c b/block/io.c
index 4b06de8..29f5962 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1805,54 +1805,6 @@  int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
                                          bytes >> BDRV_SECTOR_BITS, qiov);
 }
 
-typedef struct BdrvWriteCompressedCo {
-    BlockDriverState *bs;
-    int64_t offset;
-    QEMUIOVector *qiov;
-    int ret;
-} BdrvWriteCompressedCo;
-
-static void bdrv_write_compressed_co_entry(void *opaque)
-{
-    BdrvWriteCompressedCo *co = opaque;
-
-    co->ret = bdrv_co_pwritev_compressed(co->bs, co->offset, co->qiov->size,
-                                         co->qiov);
-}
-
-int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
-                           const void *buf, int count)
-{
-    BdrvWriteCompressedCo data;
-    QEMUIOVector qiov;
-    struct iovec iov = {
-        .iov_base = (void *)buf,
-        .iov_len = count,
-    };
-    qemu_iovec_init_external(&qiov, &iov, 1);
-
-    data = (BdrvWriteCompressedCo) {
-        .bs     = bs,
-        .offset = offset,
-        .qiov   = &qiov,
-        .ret    = -EINPROGRESS,
-    };
-
-    if (qemu_in_coroutine()) {
-        /* Fast-path if already in coroutine context */
-        bdrv_write_compressed_co_entry(&data);
-    } else {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-
-        Coroutine *co = qemu_coroutine_create(bdrv_write_compressed_co_entry);
-        qemu_coroutine_enter(co, &data);
-        while (data.ret == -EINPROGRESS) {
-            aio_poll(aio_context, true);
-        }
-    }
-    return data.ret;
-}
-
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size)
 {
diff --git a/include/block/block.h b/include/block/block.h
index b687c0d..56fcab6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -65,6 +65,7 @@  typedef enum {
     BDRV_REQ_MAY_UNMAP          = 0x4,
     BDRV_REQ_NO_SERIALISING     = 0x8,
     BDRV_REQ_FUA                = 0x10,
+    BDRV_REQ_WRITE_COMPRESSED   = 0x20,
 } BdrvRequestFlags;
 
 typedef struct BlockSizes {
@@ -421,8 +422,6 @@  const char *bdrv_get_node_name(const BlockDriverState *bs);
 const char *bdrv_get_device_name(const BlockDriverState *bs);
 const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
-int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
-                           const void *buf, int count);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
 void bdrv_round_to_clusters(BlockDriverState *bs,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 57df069..3d7b446 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -205,6 +205,9 @@  int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                                       int count, BdrvRequestFlags flags);
 int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
                           int count);
+int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
+                                           unsigned int bytes,
+                                           QEMUIOVector *qiov);
 int blk_truncate(BlockBackend *blk, int64_t offset);
 int blk_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,