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

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(a)virtuozzo.com&gt; >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; >> --- >>   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.