Re: [PATCH v11 02/13] copy-on-read: add filter append/drop functions

[Andrey Shinkevich]

On 14.10.2020 13:44, Max Reitz wrote:
> On 12.10.20 19:43, 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>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/copy-on-read.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   block/copy-on-read.h | 35 +++++++++++++++++++++
> >   2 files changed, 123 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..bcccf0f 100644
> > --- a/block/copy-on-read.c
> > +++ b/block/copy-on-read.c
>
> [...]
>
> > @@ -159,4 +188,63 @@ static void bdrv_copy_on_read_init(void)
> >       bdrv_register(&bdrv_copy_on_read);
> >   }
> >   
> > +
> > +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
> > +                                         QDict *node_options,
> > +                                         int flags, Error **errp)
>
> I had hoped you could make this a generic block layer function. :(
>
> (Because it really is rather generic)
>
> *shrug*

Actually, I did (and still can do) that for the 'append node' function 
only but not for the 'drop node' one so far...

diff --git a/block.c b/block.c
index 11ab55f..f41e876 100644
--- a/block.c
+++ b/block.c
@@ -4669,6 +4669,55 @@ static void bdrv_delete(BlockDriverState *bs)
     g_free(bs);
 }

+BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict 
*node_options,
+                                   int flags, Error **errp)
+{
+    BlockDriverState *new_node_bs;
+    Error *local_err = NULL;
+
+    new_node_bs =  bdrv_open(NULL, NULL, node_options, flags, errp);
+    if (new_node_bs == NULL) {
+        error_prepend(errp, "Could not create node: ");
+        return NULL;
+    }
+
+    bdrv_drained_begin(bs);
+    bdrv_replace_node(bs, new_node_bs, &local_err);
+    bdrv_drained_end(bs);
+
+    if (local_err) {
+        bdrv_unref(new_node_bs);
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    return new_node_bs;
+}
+
+void bdrv_remove_node(BlockDriverState *bs)
+{
+    BdrvChild *child;
+    BlockDriverState *inferior_bs;
+
+    child = bdrv_filter_or_cow_child(bs);
+    if (!child) {
+        return;
+    }
+    inferior_bs = child->bs;
+
+    /* Retain the BDS until we complete the graph change. */
+    bdrv_ref(inferior_bs);
+    /* Hold a guest back from writing while permissions are being reset. */
+    bdrv_drained_begin(inferior_bs);
+    /* Refresh permissions before the graph change. */
+    bdrv_child_refresh_perms(bs, child, &error_abort);
+    bdrv_replace_node(bs, inferior_bs, &error_abort);
+
+    bdrv_drained_end(inferior_bs);
+    bdrv_unref(inferior_bs);
+    bdrv_unref(bs);
+}

So, it is an intermediate solution in this patch of the series. I am 
going to make both functions generic once Vladimir overhauls the QEMU 
permission update system. Otherwise, the COR-filter node cannot be 
removed from the backing chain gracefully.

Thank you for your r-b. If the next version comes, I can move the 
'append node' function only to the generic layer.

Andrey

> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> > +{
> > +    BlockDriverState *cor_filter_bs;
> > +    Error *local_err = NULL;
> > +
> > +    cor_filter_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
> > +    if (cor_filter_bs == NULL) {
> > +        error_prepend(errp, "Could not create COR-filter node: ");
> > +        return NULL;
> > +    }
> > +
> > +    if (!qdict_get_try_str(node_options, "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;
> > +    }
> > +
> > +    return cor_filter_bs;
> > +}