[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 01/21] block: introduce bdrv_replace_child_bs()
From: |
Max Reitz |
Subject: |
Re: [PATCH 01/21] block: introduce bdrv_replace_child_bs() |
Date: |
Mon, 17 May 2021 14:09:02 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 |
On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
Add function to transactionally replace bs inside BdrvChild.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/block.h | 2 ++
block.c | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
As you may guess, I know little about the rewritten replacing functions,
so this is kind of difficult to review for me. However, nothing looks
out of place, and the function looks sufficiently similar to
bdrv_replace_node_common() to make me happy.
diff --git a/include/block/block.h b/include/block/block.h
index 82185965ff..f9d5fcb108 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -361,6 +361,8 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState
*bs_top,
Error **errp);
int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
Error **errp);
+int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
+ Error **errp);
BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
int flags, Error **errp);
int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
diff --git a/block.c b/block.c
index 9ad725d205..755fa53d85 100644
--- a/block.c
+++ b/block.c
@@ -4961,6 +4961,42 @@ out:
return ret;
}
+int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
+ Error **errp)
+{
+ int ret;
+ Transaction *tran = tran_new();
+ g_autoptr(GHashTable) found = NULL;
+ g_autoptr(GSList) refresh_list = NULL;
+ BlockDriverState *old_bs = child->bs;
+
+ if (old_bs) {
Hm. Can child->bs be ever NULL?
+ bdrv_ref(old_bs);
+ bdrv_drained_begin(old_bs);
+ }
+ bdrv_drained_begin(new_bs);
(I was wondering why we couldn’t handle the new_bs == NULL case here to
replace bdrv_remove_filter_or_cow_child(), but then I realized it’s
probably because that’s kind of difficult, precisely because child->bs
at least should generally be non-NULL. Which is why
bdrv_remove_filter_or_cow_child() needs to add its own transaction entry
to handle the BdrvChild object and the pointer to it.
Hence me wondering whether we could assume child->bs not to be NULL.)
+
+ bdrv_replace_child(child, new_bs, tran);
+
+ found = g_hash_table_new(NULL, NULL);
+ if (old_bs) {
+ refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
+ }
+ refresh_list = bdrv_topological_dfs(refresh_list, found, new_bs);
+
+ ret = bdrv_list_refresh_perms(refresh_list, NULL, tran, errp);
Speaking of bdrv_remove_filter_or_cow_child(): That function doesn’t
refresh permissions. I think it’s correct to do it here, so the
following question doesn’t really concern this patch, but: Why don’t we
do it there?
I guess it’s because we expect the node to go away anyway, so we don’t
need to refresh the permissions. And that assumption should hold true
right now, given its callers. But is that a safe assumption in general?
Would there be a problem if we refreshed permissions there? Or is not
refreshing permissions just part of the function’s interface?
Max
+
+ tran_finalize(tran, ret);
+
+ if (old_bs) {
+ bdrv_drained_end(old_bs);
+ bdrv_unref(old_bs);
+ }
+ bdrv_drained_end(new_bs);
+
+ return ret;
+}
+
static void bdrv_delete(BlockDriverState *bs)
{
assert(bdrv_op_blocker_is_empty(bs));
- [PATCH 00/21] block: publish backup-top filter, Vladimir Sementsov-Ogievskiy, 2021/05/17
- [PATCH] block/copy-on-read: use bdrv_drop_filter() and drop s->active, Vladimir Sementsov-Ogievskiy, 2021/05/17
- [PATCH 01/21] block: introduce bdrv_replace_child_bs(), Vladimir Sementsov-Ogievskiy, 2021/05/17
- Re: [PATCH 01/21] block: introduce bdrv_replace_child_bs(),
Max Reitz <=
- Re: [PATCH 01/21] block: introduce bdrv_replace_child_bs(), Vladimir Sementsov-Ogievskiy, 2021/05/19
- Re: [PATCH 01/21] block: introduce bdrv_replace_child_bs(), Max Reitz, 2021/05/19
- Re: [PATCH 01/21] block: introduce bdrv_replace_child_bs(), Vladimir Sementsov-Ogievskiy, 2021/05/19
[PATCH 02/21] block: introduce blk_replace_bs, Vladimir Sementsov-Ogievskiy, 2021/05/17
[PATCH 05/21] block: rename backup-top to copy-before-write, Vladimir Sementsov-Ogievskiy, 2021/05/17