diff mbox

[01/10] block/io: add bdrv_co_write_compressed

Message ID 1463229957-14253-2-git-send-email-den@openvz.org (mailing list archive)
State New, archived
Headers show

Commit Message

Denis V. Lunev May 14, 2016, 12:45 p.m. UTC 
From: Pavel Butsykin <pbutsykin@virtuozzo.com>

This patch just adds the interface to the bdrv_co_write_compressed, which
is currently not used but will be useful for safe implementation of the
bdrv_co_write_compressed callback in format drivers.

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/io.c                | 71 ++++++++++++++++++++++++++++++++++++++++++++---
 include/block/block.h     |  2 ++
 include/block/block_int.h |  3 ++
 qemu-img.c                |  2 +-
 4 files changed, 73 insertions(+), 5 deletions(-)

Comments

Eric Blake May 16, 2016, 4:52 p.m. UTC | #1 
On 05/14/2016 06:45 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> This patch just adds the interface to the bdrv_co_write_compressed, which
> is currently not used but will be useful for safe implementation of the
> bdrv_co_write_compressed callback in format drivers.
> 
> 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/block/io.c
> @@ -1828,8 +1828,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>      return 0;
>  }
>  
> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> -                          const uint8_t *buf, int nb_sectors)
> +int bdrv_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
> +                             int nb_sectors, QEMUIOVector *qiov)

As long as we're adding a new public interface, I'd really like us to
make it byte-based.  int64_t sector_num might be better represented as a
byte offset, and int nb_sectors seems redundant with qiov->size.

>  {
>      BlockDriver *drv = bs->drv;
>      int ret;
> @@ -1837,7 +1837,7 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>      if (!drv) {
>          return -ENOMEDIUM;
>      }
> -    if (!drv->bdrv_write_compressed) {
> +    if (!drv->bdrv_co_write_compressed) {
>          return -ENOTSUP;
>      }
>      ret = bdrv_check_request(bs, sector_num, nb_sectors);
> @@ -1846,8 +1846,71 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>      }
>  
>      assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> +    assert(qemu_in_coroutine());
> +
> +    return drv->bdrv_co_write_compressed(bs, sector_num, nb_sectors, qiov);

Of course, if you make the public interface byte-based, then calling
into the back end will have to scale back to sectors (after first
asserting that we aren't violating the scaling); see how Kevin did it in
commit 166fe9605.

> +}
> +
> +typedef struct BdrvWriteCompressedCo {
> +    BlockDriverState *bs;
> +    int64_t sector_num;

Again, I think a byte offset is smarter than a sector number.
Pavel Butsykin May 17, 2016, 3:01 p.m. UTC | #2 
On 16.05.2016 19:52, Eric Blake wrote:
> On 05/14/2016 06:45 AM, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> This patch just adds the interface to the bdrv_co_write_compressed, which
>> is currently not used but will be useful for safe implementation of the
>> bdrv_co_write_compressed callback in format drivers.
>>
>> 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/block/io.c
>> @@ -1828,8 +1828,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>>       return 0;
>>   }
>>
>> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> -                          const uint8_t *buf, int nb_sectors)
>> +int bdrv_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> +                             int nb_sectors, QEMUIOVector *qiov)
>
> As long as we're adding a new public interface, I'd really like us to
> make it byte-based.  int64_t sector_num might be better represented as a
> byte offset, and int nb_sectors seems redundant with qiov->size.
>
>>   {
>>       BlockDriver *drv = bs->drv;
>>       int ret;
>> @@ -1837,7 +1837,7 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>       if (!drv) {
>>           return -ENOMEDIUM;
>>       }
>> -    if (!drv->bdrv_write_compressed) {
>> +    if (!drv->bdrv_co_write_compressed) {
>>           return -ENOTSUP;
>>       }
>>       ret = bdrv_check_request(bs, sector_num, nb_sectors);
>> @@ -1846,8 +1846,71 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>       }
>>
>>       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>> +    assert(qemu_in_coroutine());
>> +
>> +    return drv->bdrv_co_write_compressed(bs, sector_num, nb_sectors, qiov);
>
> Of course, if you make the public interface byte-based, then calling
> into the back end will have to scale back to sectors (after first
> asserting that we aren't violating the scaling); see how Kevin did it in
> commit 166fe9605.
>
>> +}
>> +
>> +typedef struct BdrvWriteCompressedCo {
>> +    BlockDriverState *bs;
>> +    int64_t sector_num;
>
> Again, I think a byte offset is smarter than a sector number.
>

Kevin used the byte offset for functions bdrv_driver_pread/_pwrite(It
looks like just an additional interface), which is not the same thing.
Here the bdrv_co/bdrv_write_compressed functions are analogues of the
bdrv_co/bdrv_write functions that still use sectors in the arguments.
So I'm not sure that the interface there needs to be some other.


Kevin, what do you think about this?
Stefan Hajnoczi May 19, 2016, 9:25 p.m. UTC | #3 
On Sat, May 14, 2016 at 03:45:49PM +0300, Denis V. Lunev wrote:
> diff --git a/block/io.c b/block/io.c
> index cd6d71a..88af10c 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1828,8 +1828,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>      return 0;
>  }
>  
> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> -                          const uint8_t *buf, int nb_sectors)
> +int bdrv_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
> +                             int nb_sectors, QEMUIOVector *qiov)

Please use the coroutine_fn attribute to declare that this function must
be called in coroutine context.

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index a029c20..3c93ddb 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -208,6 +208,9 @@ struct BlockDriver {
>      int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>                                   const uint8_t *buf, int nb_sectors);
>  
> +    int (*bdrv_co_write_compressed)(BlockDriverState *bs, int64_t sector_num,
> +                                    int nb_sectors, QEMUIOVector *qiov);

Please add the coroutine_fn attribute just like .bdrv_co_readv() and
friends.
Denis V. Lunev May 19, 2016, 9:39 p.m. UTC | #4 
On 05/20/2016 12:25 AM, Stefan Hajnoczi wrote:
> On Sat, May 14, 2016 at 03:45:49PM +0300, Denis V. Lunev wrote:
>> diff --git a/block/io.c b/block/io.c
>> index cd6d71a..88af10c 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1828,8 +1828,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>>       return 0;
>>   }
>>   
>> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> -                          const uint8_t *buf, int nb_sectors)
>> +int bdrv_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> +                             int nb_sectors, QEMUIOVector *qiov)
> Please use the coroutine_fn attribute to declare that this function must
> be called in coroutine context.
>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index a029c20..3c93ddb 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -208,6 +208,9 @@ struct BlockDriver {
>>       int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>>                                    const uint8_t *buf, int nb_sectors);
>>   
>> +    int (*bdrv_co_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>> +                                    int nb_sectors, QEMUIOVector *qiov);
> Please add the coroutine_fn attribute just like .bdrv_co_readv() and
> friends.
yep. This seems right thing to do.
diff mbox

Patch

diff --git a/block/io.c b/block/io.c
index cd6d71a..88af10c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1828,8 +1828,8 @@  int bdrv_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
-int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
-                          const uint8_t *buf, int nb_sectors)
+int bdrv_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
+                             int nb_sectors, QEMUIOVector *qiov)
 {
     BlockDriver *drv = bs->drv;
     int ret;
@@ -1837,7 +1837,7 @@  int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
     if (!drv) {
         return -ENOMEDIUM;
     }
-    if (!drv->bdrv_write_compressed) {
+    if (!drv->bdrv_co_write_compressed) {
         return -ENOTSUP;
     }
     ret = bdrv_check_request(bs, sector_num, nb_sectors);
@@ -1846,8 +1846,71 @@  int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
     }
 
     assert(QLIST_EMPTY(&bs->dirty_bitmaps));
+    assert(qemu_in_coroutine());
+
+    return drv->bdrv_co_write_compressed(bs, sector_num, nb_sectors, qiov);
+}
+
+typedef struct BdrvWriteCompressedCo {
+    BlockDriverState *bs;
+    int64_t sector_num;
+    const uint8_t *buf;
+    int nb_sectors;
+    int ret;
+} BdrvWriteCompressedCo;
+
+static void bdrv_write_compressed_co_entry(void *opaque)
+{
+    BdrvWriteCompressedCo *co = opaque;
+    QEMUIOVector qiov;
+    struct iovec iov = {
+        .iov_base   = (uint8_t *)co->buf,
+        .iov_len    = co->nb_sectors << BDRV_SECTOR_BITS,
+    };
+    qemu_iovec_init_external(&qiov, &iov, 1);
+
+    co->ret = bdrv_co_write_compressed(co->bs, co->sector_num,
+                                       co->nb_sectors, &qiov);
+}
+
+int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
+                          const uint8_t *buf, int nb_sectors)
+{
+    BlockDriver *drv = bs->drv;
+    BdrvWriteCompressedCo data = {
+        .bs         = bs,
+        .sector_num = sector_num,
+        .buf        = buf,
+        .nb_sectors = nb_sectors,
+        .ret        = -EINPROGRESS,
+    };
+
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
+    if (drv->bdrv_write_compressed) {
+        int ret = bdrv_check_request(bs, sector_num, nb_sectors);
+        if (ret < 0) {
+            return ret;
+        }
+        assert(QLIST_EMPTY(&bs->dirty_bitmaps));
+        return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
+    }
 
-    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
+    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,
diff --git a/include/block/block.h b/include/block/block.h
index b210832..ae67fd8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -423,6 +423,8 @@  const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
                           const uint8_t *buf, int nb_sectors);
+int bdrv_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
+                             int nb_sectors, QEMUIOVector *qiov);
 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/block/block_int.h b/include/block/block_int.h
index a029c20..3c93ddb 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -208,6 +208,9 @@  struct BlockDriver {
     int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
                                  const uint8_t *buf, int nb_sectors);
 
+    int (*bdrv_co_write_compressed)(BlockDriverState *bs, int64_t sector_num,
+                                    int nb_sectors, QEMUIOVector *qiov);
+
     int (*bdrv_snapshot_create)(BlockDriverState *bs,
                                 QEMUSnapshotInfo *sn_info);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
diff --git a/qemu-img.c b/qemu-img.c
index 4792366..0d38eac 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2022,7 +2022,7 @@  static int img_convert(int argc, char **argv)
         const char *preallocation =
             qemu_opt_get(opts, BLOCK_OPT_PREALLOC);
 
-        if (!drv->bdrv_write_compressed) {
+        if (!drv->bdrv_write_compressed && !drv->bdrv_co_write_compressed) {
             error_report("Compression not supported for this file format");
             ret = -1;
             goto out;