[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/11] block/io: add bdrv_co_write_compressed
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 02/11] block/io: add bdrv_co_write_compressed |
Date: |
Mon, 13 Jun 2016 08:32:02 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <address@hidden>
>
> This patch just adds the interface to the bdrv_co_pwritev_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 <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Jeff Cody <address@hidden>
> CC: Markus Armbruster <address@hidden>
> CC: Eric Blake <address@hidden>
> CC: John Snow <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> CC: Kevin Wolf <address@hidden>
> ---
> block/io.c | 78
> +++++++++++++++++++++++++++++++++++++++++++----
> include/block/block_int.h | 5 +++
> qemu-img.c | 2 +-
> 3 files changed, 78 insertions(+), 7 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index c5bb6ae..54cd9a4 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1779,8 +1779,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
> return 0;
> }
>
> -int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
> - const void *buf, int count)
> +int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
> + int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
Why the rename s/count/bytes/? Would it be better to get the name right
in 1/11?
> {
> BlockDriver *drv = bs->drv;
> int ret;
> @@ -1788,18 +1788,84 @@ int bdrv_pwrite_compressed(BlockDriverState *bs,
> int64_t offset,
> if (!drv) {
> return -ENOMEDIUM;
> }
> - if (!drv->bdrv_write_compressed) {
> +
> + if (!drv->bdrv_co_write_compressed) {
> return -ENOTSUP;
This is a (temporary) regression - since none of the drivers have
.bdrv_co_write_compressed yet, you will always fail. Rather, the
transition period should support both interfaces at once...
> }
> - ret = bdrv_check_byte_request(bs, offset, count);
> +
> + ret = bdrv_check_byte_request(bs, offset, bytes);
> if (ret < 0) {
> return ret;
> }
>
> assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> + assert(qemu_in_coroutine());
> +
> + return drv->bdrv_co_write_compressed(bs, offset >> BDRV_SECTOR_BITS,
> + bytes >> BDRV_SECTOR_BITS, qiov);
...and call into either the old or the new interface according to what
is present.
> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
> + const void *buf, int count)
> +{
> + BdrvWriteCompressedCo data;
> + QEMUIOVector qiov;
> + BlockDriver *drv = bs->drv;
> + struct iovec iov = {
> + .iov_base = (void *)buf,
> + .iov_len = count,
> + };
> + qemu_iovec_init_external(&qiov, &iov, 1);
>
> - return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
> - count >> BDRV_SECTOR_BITS);
> + data = (BdrvWriteCompressedCo) {
> + .bs = bs,
> + .offset = offset,
> + .qiov = &qiov,
> + .ret = -EINPROGRESS,
> + };
> +
> + if (!drv) {
> + return -ENOMEDIUM;
> + }
> +
> + if (drv->bdrv_write_compressed) {
> + int ret = bdrv_check_byte_request(bs, offset, count);
> + if (ret < 0) {
> + return ret;
> + }
> + assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> + return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS,
> buf,
> + count >> BDRV_SECTOR_BITS);
> + }
Oh, you're catering to the old code up front, without a coroutine, and
only the new code gets coroutine treatment. Maybe it's okay after all.
> +
> + 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_int.h b/include/block/block_int.h
> index 30a9717..ccba9c9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -207,6 +207,9 @@ struct BlockDriver {
> int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
> const uint8_t *buf, int nb_sectors);
>
> + int coroutine_fn (*bdrv_co_write_compressed)(BlockDriverState *bs,
Might be better to name this bdrv_co_pwrite_compressed if we want to
make it byte-based...
> + int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
...it seems odd that you have to add a new sector-based interface given
that you are trying to convert to byte-based.
> +
> int (*bdrv_snapshot_create)(BlockDriverState *bs,
> QEMUSnapshotInfo *sn_info);
> int (*bdrv_snapshot_goto)(BlockDriverState *bs,
> @@ -535,6 +538,8 @@ int coroutine_fn bdrv_co_preadv(BlockDriverState *bs,
> int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
> int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
> BdrvRequestFlags flags);
> +int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
> + int64_t offset, unsigned int bytes, QEMUIOVector *qiov);
>
> int get_tmp_filename(char *filename, int size);
> BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
> diff --git a/qemu-img.c b/qemu-img.c
> index eb744d4..ab54027 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2024,7 +2024,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;
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature