From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58453) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9wU4-00051u-KE for qemu-devel@nongnu.org; Thu, 24 Nov 2016 11:02:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9wU0-00024L-4g for qemu-devel@nongnu.org; Thu, 24 Nov 2016 11:02:48 -0500 References: <20161115063715.12561-1-pbutsykin@virtuozzo.com> <20161115063715.12561-3-pbutsykin@virtuozzo.com> <20161123151542.GC5068@noname.redhat.com> From: Pavel Butsykin Message-ID: <58370BBC.1050504@virtuozzo.com> Date: Thu, 24 Nov 2016 18:48:12 +0300 MIME-Version: 1.0 In-Reply-To: <20161123151542.GC5068@noname.redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v1 02/18] block/pcache: empty pcache driver filter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, den@openvz.org, famz@redhat.com, stefanha@redhat.com, mreitz@redhat.com, eblake@redhat.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 >> --- >> 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 >> + * >> + * 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:" 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 >