From 5bb047477807375e2d5e2494b1d1302d5cea4b73 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 26 Jul 2022 23:11:32 +0300 Subject: block: Manipulate bs->file / bs->backing pointers in .attach/.detach bs->file and bs->backing are a kind of duplication of part of bs->children. But very useful diplication, so let's not drop them at all:) We should manage bs->file and bs->backing in same place, where we manage bs->children, to keep them in sync. Moreover, generic io paths are unprepared to BdrvChild without a bs, so it's double good to clear bs->file / bs->backing when we detach the child. Detach is simple: if we detach bs->file or bs->backing child, just set corresponding field to NULL. Attach is a bit more complicated. But we still can precisely detect should we set one of bs->file / bs->backing or not: - if role is BDRV_CHILD_COW, we definitely deal with bs->backing - else, if role is BDRV_CHILD_FILTERED (it must be also BDRV_CHILD_PRIMARY), it's a filtered child. Use bs->drv->filtered_child_is_backing to chose the pointer field to modify. - else, if role is BDRV_CHILD_PRIMARY, we deal with bs->file - in all other cases, it's neither bs->backing nor bs->file. It's some other child and we shouldn't care OK. This change brings one more good thing: we can (and should) get rid of all indirect pointers in the block-graph-change transactions: bdrv_attach_child_common() stores BdrvChild** into transaction to clear it on abort. bdrv_attach_child_common() has two callers: bdrv_attach_child_noperm() just pass-through this feature, bdrv_root_attach_child() doesn't need the feature. Look at bdrv_attach_child_noperm() callers: - bdrv_attach_child() doesn't need the feature - bdrv_set_file_or_backing_noperm() uses the feature to manage bs->file and bs->backing, we don't want it anymore - bdrv_append() uses the feature to manage bs->backing, again we don't want it anymore So, we should drop this stuff! Great! We could probably keep BdrvChild** argument to keep the int return value, but it seems not worth the complexity. Finally, we now set .file / .backing automatically in generic code and want to restring setting them by hand outside of .attach/.detach. So, this patch cleanups all remaining places where they were set. To find such places I use: git grep '\->file =' git grep '\->backing =' git grep '&.*\' git grep '&.*\' Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Hanna Reitz Message-Id: <20220726201134.924743-14-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf --- include/block/block_int-common.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) (limited to 'include') diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 10feef9f0c..1f300ee7f6 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -1056,9 +1056,6 @@ struct BlockDriverState { QDict *full_open_options; char exact_filename[PATH_MAX]; - BdrvChild *backing; - BdrvChild *file; - /* I/O Limits */ BlockLimits bl; @@ -1117,7 +1114,19 @@ struct BlockDriverState { * parent node of this node. */ BlockDriverState *inherits_from; + + /* + * @backing and @file are some of @children or NULL. All these three fields + * (@file, @backing and @children) are modified only in + * bdrv_child_cb_attach() and bdrv_child_cb_detach(). + * + * See also comment in include/block/block.h, to learn how backing and file + * are connected with BdrvChildRole. + */ QLIST_HEAD(, BdrvChild) children; + BdrvChild *backing; + BdrvChild *file; + QLIST_HEAD(, BdrvChild) parents; QDict *options; -- cgit v1.2.3-55-g7522