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

[Andrey Shinkevich]

On 19.08.2020 13:21, Vladimir Sementsov-Ogievskiy wrote:
> 19.08.2020 00:24, Andrey Shinkevich wrote:
> > Provide API for the COR-filter insertion/removal.
> > Also, drop the filter child permissions for an inactive state when the
> > filter node is being removed.
> >
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> >   block/copy-on-read.c | 103 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   block/copy-on-read.h |  36 ++++++++++++++++++
> >   2 files changed, 139 insertions(+)
> >   create mode 100644 block/copy-on-read.h
> >
> > diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> > index cb03e0f..150d9b7 100644
> > --- a/block/copy-on-read.c
> > +++ b/block/copy-on-read.c
> > @@ -23,11 +23,21 @@
> >   #include "qemu/osdep.h"
> >   #include "block/block_int.h"
> >   #include "qemu/module.h"
> > +#include "qapi/error.h"
> > +#include "qapi/qmp/qdict.h"
> > +#include "block/copy-on-read.h"
> > +
> > +
> > +typedef struct BDRVStateCOR {
> > +    bool active;
> > +} BDRVStateCOR;
> >       static int cor_open(BlockDriverState *bs, QDict *options, int 
> > flags,
> >                       Error **errp)
> >   {
> > +    BDRVStateCOR *state = bs->opaque;
> > +
> >       bs->file = bdrv_open_child(NULL, options, "file", bs, 
> > &child_of_bds,
> >                                  BDRV_CHILD_FILTERED | 
> > BDRV_CHILD_PRIMARY,
> >                                  false, errp);
> > @@ -42,6 +52,8 @@ static int cor_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >           ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
> >               bs->file->bs->supported_zero_flags);
> >   +    state->active = true;
>
> We don't need to call bdrv_child_refresh_perms() here, as permissions 
> will be
> updated soon, when the filter node get its parent, yes?
>
> Let's add at least a comment on this.
>
> > +
> >       return 0;
> >   }
> >   @@ -57,6 +69,17 @@ static void cor_child_perm(BlockDriverState *bs, 
> > BdrvChild *c,
> >                              uint64_t perm, uint64_t shared,
> >                              uint64_t *nperm, uint64_t *nshared)
> >   {
> > +    BDRVStateCOR *s = bs->opaque;
> > +
> > +    if (!s->active) {
> > +        /*
> > +         * While the filter is being removed
> > +         */
> > +        *nperm = 0;
> > +        *nshared = BLK_PERM_ALL;
> > +        return;
> > +    }
> > +
> >       *nperm = perm & PERM_PASSTHROUGH;
> >       *nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED;
> >   @@ -135,6 +158,7 @@ static void cor_lock_medium(BlockDriverState 
> > *bs, bool locked)
> >     static BlockDriver bdrv_copy_on_read = {
> >       .format_name                        = "copy-on-read",
> > +    .instance_size                      = sizeof(BDRVStateCOR),
> >         .bdrv_open                          = cor_open,
> >       .bdrv_child_perm                    = cor_child_perm,
> > @@ -159,4 +183,83 @@ static void bdrv_copy_on_read_init(void)
> >       bdrv_register(&bdrv_copy_on_read);
> >   }
> >   +
> > +static BlockDriverState *create_filter_node(BlockDriverState *bs,
> > +                                            const char 
> > *filter_node_name,
> > +                                            Error **errp)
> > +{
> > +    QDict *opts = qdict_new();
> > +
> > +    qdict_put_str(opts, "driver", "copy-on-read");
> > +    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
> > +    if (filter_node_name) {
> > +        qdict_put_str(opts, "node-name", filter_node_name);
> > +    }
> > +
> > +    return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp);
> > +}
> > +
> > +
> > +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
> > +                                         const char *filter_node_name,
> > +                                         Error **errp)
> > +{
> > +    BlockDriverState *cor_filter_bs;
> > +    BDRVStateCOR *state;
> > +    Error *local_err = NULL;
> > +
> > +    cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
> > +    if (cor_filter_bs == NULL) {
> > +        error_prepend(errp, "Could not create filter node: ");
> > +        return NULL;
> > +    }
> > +
> > +    if (!filter_node_name) {
> > +        cor_filter_bs->implicit = true;
> > +    }
> > +
> > +    bdrv_drained_begin(bs);
> > +    bdrv_replace_node(bs, cor_filter_bs, &local_err);
> > +    bdrv_drained_end(bs);
> > +
> > +    if (local_err) {
> > +        bdrv_unref(cor_filter_bs);
> > +        error_propagate(errp, local_err);
> > +        return NULL;
> > +    }
> > +
> > +    state = cor_filter_bs->opaque;
> > +    state->active = true;
>
> hmm stop. it's already active, after create_filter_node, so you don't 
> need to set it again, isn't it?


I will remove the extra assignment from the cor_open() for consistancy.

Andrey


> > +
> > +    return cor_filter_bs;
> > +}
> > +
> > +
> > +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs)
> > +{
> > +    BdrvChild *child;
> > +    BlockDriverState *bs;
> > +    BDRVStateCOR *s = cor_filter_bs->opaque;
> > +
> > +    child = bdrv_filter_child(cor_filter_bs);
> > +    if (!child) {
> > +        return;
> > +    }
> > +    bs = child->bs;
> > +
> > +    /* Retain the BDS until we complete the graph change. */
> > +    bdrv_ref(bs);
> > +    /* Hold a guest back from writing while permissions are being 
> > reset. */
> > +    bdrv_drained_begin(bs);
> > +    /* Drop permissions before the graph change. */
> > +    s->active = false;
> > +    bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort);
> > +    bdrv_replace_node(cor_filter_bs, bs, &error_abort);
> > +
> > +    bdrv_drained_end(bs);
> > +    bdrv_unref(bs);
> > +    bdrv_unref(cor_filter_bs);
> > +}
> > +
> > +
> >   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..db03c6c
> > --- /dev/null
> > +++ b/block/copy-on-read.h
> > @@ -0,0 +1,36 @@
> > +/*
> > + * 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
> > +
> > +#include "block/block_int.h"
> > +#include "block/block-copy.h"
>
> do you really need block-copy.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 */
>
> Code seems correct to me, with extra "state->active = true;" dropped:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>