Re: [PATCH v8 2/7] copy-on-read: add filter append/drop functions

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3900 bytes --]

On 28.08.20 18:52, Andrey Shinkevich wrote:
> Provide API for the COR-filter insertion/removal.

Hm.  Why?

I see the implementation is just bdrv_open() + bdrv_replace_node(),
which is what I would have expected.  Why can’t the caller just do that?

Or maybe it would even make sense to just make it a generic block driver
function, like

BlockDriverState *
bdrv_insert_node(BlockDriverState *bs, const char *node_driver,
                 const char *node_name, QDict *node_options,
                 Error **errp);

?

(Similarly for dropping a node from a chain.)

> Also, drop the filter child permissions for an inactive state when the
> filter node is being removed.

Do we need .active for that?  Shouldn’t it be sufficient to just not
require BLK_PERM_WRITE_UNCHANGED when no permissions are taken on the
node (i.e. perm == 0 in cor_child_perm())?

Of course, using an .active field works, too.  But Vladimir wanted a
similar field in the preallocate filter, and there already is in
backup-top.  I feel like there shouldn’t be a need for this.

.bdrv_child_perm() should generally be able to decide what permissions
it needs based on the information it gets.  If every driver needs to
keep track of whether it has any parents and feed that information into
.bdrv_child_perm() as external state, then something feels wrong.

If perm == 0 is not sufficient to rule out any parents, we should just
explicitly add a boolean that tells .bdrv_child_perm() whether there are
any parents or not – shouldn’t be too difficult.

> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/copy-on-read.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/copy-on-read.h |  35 +++++++++++++++++
>  2 files changed, 139 insertions(+)
>  create mode 100644 block/copy-on-read.h

[...]

>  block_init(bdrv_copy_on_read_init);
> diff --git a/block/copy-on-read.h b/block/copy-on-read.h
> new file mode 100644
> index 0000000..1686e4e
> --- /dev/null
> +++ b/block/copy-on-read.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copy-on-read filter block driver
> + *
> + * The filter driver performs Copy-On-Read (COR) operations
> + *
> + * Copyright (c) 2018-2020 Virtuozzo International GmbH.
> + *
> + * Author:
> + *   Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef COPY_ON_READ_FILTER
> +#define COPY_ON_READ_FILTER

Bit of a weird include guard, seeing that most header files in qemu
(certainly under block/) base their name on their filename.  So this
would be BLOCK_COPY_ON_READ_H (or COPY_ON_READ_H).

(It’s just that COPY_ON_READ_FILTER kind of sounds like a configured
option, i.e. whether the filter is present or not.)

Max

> +
> +#include "block/block_int.h"
> +
> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
> +                                         const char *filter_node_name,
> +                                         Error **errp);
> +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
> +
> +#endif /* COPY_ON_READ_FILTER */
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]