Re: [Qemu-devel] [PATCH v1 02/18] block/pcache: empty pcache driver filter

[Kevin Wolf <kwolf@redhat.com>]

Am 15.11.2016 um 07:36 hat Pavel Butsykin geschrieben:
> The basic version of pcache driver for easy preparation of a patch set.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  block/Makefile.objs |   1 +
>  block/pcache.c      | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 145 insertions(+)
>  create mode 100644 block/pcache.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 7d4031d..c60f680 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -4,6 +4,7 @@ block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
>  block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
>  block-obj-y += quorum.o
> +block-obj-y += pcache.o
>  block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o
>  block-obj-y += block-backend.o snapshot.o qapi.o
>  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> diff --git a/block/pcache.c b/block/pcache.c
> new file mode 100644
> index 0000000..59461df
> --- /dev/null
> +++ b/block/pcache.c
> @@ -0,0 +1,144 @@
> +/*
> + * Prefetch cache driver filter
> + *
> + * Copyright (C) 2015-2016 Parallels IP Holdings GmbH. All rights reserved.

"All rights reserved" doesn't really agree with licensing under the GPL.
It would be preferable to remove this note to avoid any ambiguity.

> + *
> + * Author: Pavel Butsykin <pbutsykin@virtuozzo.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later.  See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "block/block_int.h"
> +#include "qapi/error.h"
> +#include "qapi/qmp/qstring.h"
> +
> +
> +static QemuOptsList runtime_opts = {
> +    .name = "pcache",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = "x-image",
> +            .type = QEMU_OPT_STRING,
> +            .help = "[internal use only, will be removed]",
> +        },

blkdebug/blkverify have this because they have to support legacy syntax
like -drive file=blkdebug:blkdebug.conf:test.img, i.e. it has to deal
with filenames.

Here we don't have to support a legacy syntax, so I would completely
avoid this from the beginning. You already support the "image" option,
which should be good enough.

> +        { /* end of list */ }
> +    },
> +};
> +
> +typedef struct PCacheAIOCB {
> +    Coroutine *co;
> +    int ret;
> +} PCacheAIOCB;
> +
> +static void pcache_aio_cb(void *opaque, int ret)
> +{
> +    PCacheAIOCB *acb = opaque;
> +
> +    acb->ret = ret;
> +    qemu_coroutine_enter(acb->co);
> +}
> +
> +static coroutine_fn int pcache_co_preadv(BlockDriverState *bs, uint64_t offset,
> +                                         uint64_t bytes, QEMUIOVector *qiov,
> +                                         int flags)
> +{
> +    PCacheAIOCB acb = {
> +        .co = qemu_coroutine_self(),
> +    };
> +
> +    bdrv_aio_preadv(bs->file, offset, qiov, bytes, pcache_aio_cb, &acb);
> +
> +    qemu_coroutine_yield();
> +
> +    return acb.ret;
> +}

As I commented on patch 1, all of this can be replaced by a single line:

    return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);

> +static coroutine_fn int pcache_co_pwritev(BlockDriverState *bs, uint64_t offset,
> +                                          uint64_t bytes, QEMUIOVector *qiov,
> +                                          int flags)
> +{
> +    PCacheAIOCB acb = {
> +        .co = qemu_coroutine_self(),
> +    };
> +
> +    bdrv_aio_pwritev(bs->file, offset, qiov, bytes, pcache_aio_cb, &acb);
> +
> +    qemu_coroutine_yield();
> +
> +    return acb.ret;
> +}

Same here.

> +static int pcache_file_open(BlockDriverState *bs, QDict *options, int flags,
> +                            Error **errp)
> +{
> +    QemuOpts *opts;
> +    Error *local_err = NULL;
> +    int ret = 0;
> +
> +    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> +    qemu_opts_absorb_qdict(opts, options, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    assert(bs->file == NULL);
> +    bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options,
> +                               "image", bs, &child_format, false, &local_err);

When removing x-image, the first parameter becomes NULL here.

> +    if (local_err) {
> +        ret = -EINVAL;
> +        error_propagate(errp, local_err);
> +    }
> +fail:
> +    qemu_opts_del(opts);
> +    return ret;
> +}
> +
> +static void pcache_close(BlockDriverState *bs)
> +{
> +}
> +
> +static void pcache_parse_filename(const char *filename, QDict *options,
> +                                  Error **errp)
> +{
> +    qdict_put(options, "x-image", qstring_from_str(filename));
> +}

This one goes away.

> +static int64_t pcache_getlength(BlockDriverState *bs)
> +{
> +    return bdrv_getlength(bs->file->bs);
> +}
> +
> +static bool pcache_recurse_is_first_non_filter(BlockDriverState *bs,
> +                                               BlockDriverState *candidate)
> +{
> +    return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
> +}
> +
> +static BlockDriver bdrv_pcache = {
> +    .format_name                        = "pcache",
> +    .protocol_name                      = "pcache",

Remove this one, it would only be used for something like
"pcache:<filename>" syntax.

> +    .instance_size                      = 0,
> +
> +    .bdrv_parse_filename                = pcache_parse_filename,
> +    .bdrv_file_open                     = pcache_file_open,
> +    .bdrv_close                         = pcache_close,
> +    .bdrv_getlength                     = pcache_getlength,
> +
> +    .bdrv_co_preadv                     = pcache_co_preadv,
> +    .bdrv_co_pwritev                    = pcache_co_pwritev,
> +
> +    .is_filter                          = true,
> +    .bdrv_recurse_is_first_non_filter   = pcache_recurse_is_first_non_filter,
> +};
> +
> +static void bdrv_cache_init(void)
> +{
> +    bdrv_register(&bdrv_pcache);
> +}
> +
> +block_init(bdrv_cache_init);

You should also add pcache to the QAPI schema, so that blockdev-add can
create instances of it.

Kevin