Re: [PATCH 2/8] block: add BlockParentClass class

2021-09-19 Thread Markus Armbruster
Vladimir Sementsov-Ogievskiy  writes:

> Add a class that will unify block parents for blockdev-replace
> functionality we are going to add.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/block-parent.h | 32 +
>  block/block-parent.c | 66 
>  block/meson.build|  1 +
>  3 files changed, 99 insertions(+)
>  create mode 100644 include/block/block-parent.h
>  create mode 100644 block/block-parent.c
>
> diff --git a/include/block/block-parent.h b/include/block/block-parent.h
> new file mode 100644
> index 00..bd9c9d91e6
> --- /dev/null
> +++ b/include/block/block-parent.h
> @@ -0,0 +1,32 @@
> +/*
> + * Block Parent class
> + *
> + * Copyright (c) 2021 Virtuozzo International GmbH.
> + *
> + * Authors:
> + *  Vladimir Sementsov-Ogievskiy 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef BLOCK_PARENT_H
> +#define BLOCK_PARENT_H
> +
> +#include "block/block.h"
> +
> +typedef struct BlockParentClass {
> +const char *name;
> +
> +int (*find_child)(const char *parent_id, const char *child_name,
> +  BlockDriverState *child_bs, BdrvChild **child,
> +  Error **errp);
> +QTAILQ_ENTRY(BlockParentClass) next;
> +} BlockParentClass;
> +
> +void block_parent_class_register(BlockParentClass *cls);
> +
> +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
> +BlockDriverState *child_bs, Error **errp);
> +
> +#endif /* BLOCK_PARENT_H */
> diff --git a/block/block-parent.c b/block/block-parent.c
> new file mode 100644
> index 00..73b6026c42
> --- /dev/null
> +++ b/block/block-parent.c
> @@ -0,0 +1,66 @@
> +/*
> + * Block Parent class
> + *
> + * Copyright (c) 2021 Virtuozzo International GmbH.
> + *
> + * Authors:
> + *  Vladimir Sementsov-Ogievskiy 
> + *
> + * 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-parent.h"
> +#include "qapi/error.h"
> +
> +static QTAILQ_HEAD(, BlockParentClass) block_parent_classes =
> +QTAILQ_HEAD_INITIALIZER(block_parent_classes);
> +
> +void block_parent_class_register(BlockParentClass *cls)
> +{
> +QTAILQ_INSERT_HEAD(_parent_classes, cls, next);
> +}
> +
> +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
> +BlockDriverState *child_bs, Error **errp)
> +{
> +BdrvChild *found_child = NULL;
> +BlockParentClass *found_cls = NULL, *cls;
> +
> +QTAILQ_FOREACH(cls, _parent_classes, next) {
> +int ret;
> +BdrvChild *c;
> +
> +/*
> + * Note that .find_child must fail if parent is found but doesn't 
> have
> + * corresponding child.
> + */
> +ret = cls->find_child(parent_id, child_name, child_bs, , errp);
> +if (ret < 0) {
> +return NULL;

Interesting: when one method fails, the entire function fails, even when
other methods succeed.  The contract should probably spell this out.

> +}
> +if (ret == 0) {
> +continue;
> +}
> +
> +if (!found_child) {
> +found_cls = cls;
> +found_child = c;
> +continue;
> +}
> +
> +error_setg(errp, "{%s, %s} parent-child pair is ambiguous: it match "
> +   "both %s and %s", parent_id, child_name, found_cls->name,
> +   cls->name);
> +return NULL;
> +}

Style recommendation: if very much prefer

   if bad
   error out
   normal

over

   if ok
   normal
   bad

In this case:

   if (found_child) {
   error_setg(...);
   return 0;
   }

   found_cls = cls;
   found_child = c;
   }

> +
> +if (!found_child) {
> +error_setg(errp, "{%s, %s} parent-child pair not found", parent_id,
> +   child_name);
> +return NULL;
> +}
> +
> +return found_child;
> +}
> diff --git a/block/meson.build b/block/meson.build
> index 0450914c7a..5200e0ffce 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -10,6 +10,7 @@ block_ss.add(files(
>'blkverify.c',
>'block-backend.c',
>'block-copy.c',
> +  'block-parent.c',
>'commit.c',
>'copy-on-read.c',
>'preallocate.c',




Re: [PATCH] qemu-nbd: Change default cache mode to writeback

2021-09-19 Thread Nir Soffer
On Mon, Aug 16, 2021 at 6:50 PM Eric Blake  wrote:
>
> On Fri, Aug 13, 2021 at 11:55:19PM +0300, Nir Soffer wrote:
> > Both qemu and qemu-img use writeback cache mode by default, which is
> > already documented in qemu(1). qemu-nbd uses writethrough cache mode by
> > default, and the default cache mode is not documented.
> >
> > According to the qemu-nbd(8):
> >
> >--cache=CACHE
> >   The  cache  mode  to be used with the file.  See the
> >   documentation of the emulator's -drive cache=... option for
> >   allowed values.
> >
> > qemu(1) says:
> >
> > The default mode is cache=writeback.
> >
> > So users have no reason to assume that qemu-nbd is using writethough
> > cache mode. The only hint is the painfully slow writing when using the
> > defaults.
>
> Oh, good catch.  Unfortunately too late for 6.1 proper, but I'll add
> qemu-stable in cc and queue this through my NBD tree for 6.2.

I don't see this in master, lost in your NBD tree?

> > Users can avoid the issue by using --cache=writeback[1] but the defaults
> > should give good performance for the common use case.
> >
> > [1] https://bugzilla.redhat.com/1990656
> >
> > Signed-off-by: Nir Soffer 
> > ---
>
> Reviewed-by: Eric Blake 
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org