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

[Pavel Butsykin <pbutsykin@virtuozzo.com>]

On 23.11.2016 18:15, Kevin Wolf wrote:
> 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.

Yes, you're right. It seems that 'All rights reserved' is directly
inconsistent with GPL.

>> + *
>> + * 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.q
>
> 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.

Then the command line would look like this:

-drive 
file=/img/harddisk.hdd,if=none,id=drive-scsi0-0-0-0,cache=none,aio=native
-drive driver=pcache,image=scsi0-0-0-0,if=virtio

Ok.

>> +        { /* 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.

Ok.

> Kevin
>