Re: [Qemu-block] [PATCH v6 09/42] block: Include filters when freezing backing chain
Am 09.09.2019 um 10:02 hat Max Reitz geschrieben: > On 05.09.19 15:05, Kevin Wolf wrote: > > Am 09.08.2019 um 18:13 hat Max Reitz geschrieben: > >> In order to make filters work in backing chains, the associated > >> functions must be able to deal with them and freeze all filter links, be > >> they COW or R/W filter links. > >> > >> In the process, rename these functions to reflect that they now act on > >> generalized chains of filter nodes instead of backing chains alone. > > > > I don't think this is a good idea. The functions are still following the > > backing chain. A generic "chain" could mean following the bs->file links > > or any other children, so the new name is confusing because it doesn't > > really tell you any more what the function does. I'd prefer the name to > > stay specific. > They’re following backing chains, among others. > > It would make sense to rename s/backing_chain/filter_chain/, that is, in > case you don‘t find lumping COW and R/W filters together under “filter” > too offensive. > > (Naming things is hard. I’m open for suggestions, but I found the > “filter” concept succinct, even if it does not fully align with our > existing parlance.) As you could see in my reply to patch 4, I didn't. :-) I think it makes a lot more sense to just broaden the meaning of "backing chain" to be what you call a "filter chain" (following the backing file links, but accept filter nodes in between), because of how unspecific "filter chain" is. The primary thing we're interested in is still the backing files. Kevin signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v6 09/42] block: Include filters when freezing backing chain
On 05.09.19 15:05, Kevin Wolf wrote: > Am 09.08.2019 um 18:13 hat Max Reitz geschrieben: >> In order to make filters work in backing chains, the associated >> functions must be able to deal with them and freeze all filter links, be >> they COW or R/W filter links. >> >> In the process, rename these functions to reflect that they now act on >> generalized chains of filter nodes instead of backing chains alone. > > I don't think this is a good idea. The functions are still following the > backing chain. A generic "chain" could mean following the bs->file links > or any other children, so the new name is confusing because it doesn't > really tell you any more what the function does. I'd prefer the name to > stay specific. They’re following backing chains, among others. It would make sense to rename s/backing_chain/filter_chain/, that is, in case you don‘t find lumping COW and R/W filters together under “filter” too offensive. (Naming things is hard. I’m open for suggestions, but I found the “filter” concept succinct, even if it does not fully align with our existing parlance.) Max >> While at it, add some comments that note which functions require their >> caller to ensure that a given child link is not frozen, and how the >> callers do so. >> >> Signed-off-by: Max Reitz > > Kevin > signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v6 09/42] block: Include filters when freezing backing chain
Am 09.08.2019 um 18:13 hat Max Reitz geschrieben: > In order to make filters work in backing chains, the associated > functions must be able to deal with them and freeze all filter links, be > they COW or R/W filter links. > > In the process, rename these functions to reflect that they now act on > generalized chains of filter nodes instead of backing chains alone. I don't think this is a good idea. The functions are still following the backing chain. A generic "chain" could mean following the bs->file links or any other children, so the new name is confusing because it doesn't really tell you any more what the function does. I'd prefer the name to stay specific. > While at it, add some comments that note which functions require their > caller to ensure that a given child link is not frozen, and how the > callers do so. > > Signed-off-by: Max Reitz Kevin
Re: [Qemu-block] [PATCH v6 09/42] block: Include filters when freezing backing chain
On 10.08.19 15:32, Vladimir Sementsov-Ogievskiy wrote: > 09.08.2019 19:13, Max Reitz wrote: >> In order to make filters work in backing chains, the associated >> functions must be able to deal with them and freeze all filter links, be >> they COW or R/W filter links. >> >> In the process, rename these functions to reflect that they now act on >> generalized chains of filter nodes instead of backing chains alone. >> >> While at it, add some comments that note which functions require their >> caller to ensure that a given child link is not frozen, and how the >> callers do so. >> >> Signed-off-by: Max Reitz >> --- >> include/block/block.h | 10 +++--- >> block.c | 81 +-- >> block/commit.c| 8 ++--- >> block/mirror.c| 4 +-- >> block/stream.c| 8 ++--- >> 5 files changed, 62 insertions(+), 49 deletions(-) [...] >> @@ -2477,10 +2481,6 @@ void bdrv_root_unref_child(BdrvChild *child) >> bdrv_unref(child_bs); >> } >> >> -/** >> - * Clear all inherits_from pointers from children and grandchildren of >> - * @root that point to @root, where necessary. >> - */ > > Hmm, unrelated chunk? Without it: > Reviewed-by: Vladimir Sementsov-Ogievskiy I don’t know how that slipped in, sorry... Once again, thanks for reviewing! Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v6 09/42] block: Include filters when freezing backing chain
09.08.2019 19:13, Max Reitz wrote:
> In order to make filters work in backing chains, the associated
> functions must be able to deal with them and freeze all filter links, be
> they COW or R/W filter links.
>
> In the process, rename these functions to reflect that they now act on
> generalized chains of filter nodes instead of backing chains alone.
>
> While at it, add some comments that note which functions require their
> caller to ensure that a given child link is not frozen, and how the
> callers do so.
>
> Signed-off-by: Max Reitz
> ---
> include/block/block.h | 10 +++---
> block.c | 81 +--
> block/commit.c| 8 ++---
> block/mirror.c| 4 +--
> block/stream.c| 8 ++---
> 5 files changed, 62 insertions(+), 49 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 50a07c1c33..f6f09b95cd 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -364,11 +364,11 @@ int bdrv_drop_intermediate(BlockDriverState *top,
> BlockDriverState *base,
> BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
> BlockDriverState *bs);
> BlockDriverState *bdrv_find_base(BlockDriverState *bs);
> -bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState
> *base,
> - Error **errp);
> -int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
> - Error **errp);
> -void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState
> *base);
> +bool bdrv_is_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
> + Error **errp);
> +int bdrv_freeze_chain(BlockDriverState *bs, BlockDriverState *base,
> + Error **errp);
> +void bdrv_unfreeze_chain(BlockDriverState *bs, BlockDriverState *base);
>
>
> typedef struct BdrvCheckResult {
> diff --git a/block.c b/block.c
> index adf82efb0e..650c00d182 100644
> --- a/block.c
> +++ b/block.c
> @@ -2303,12 +2303,15 @@ static void bdrv_replace_child_noperm(BdrvChild
> *child,
>* If @new_bs is not NULL, bdrv_check_perm() must be called beforehand, as
> this
>* function uses bdrv_set_perm() to update the permissions according to the
> new
>* reference that @new_bs gets.
> + *
> + * Callers must ensure that child->frozen is false.
>*/
> static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
> {
> BlockDriverState *old_bs = child->bs;
> uint64_t perm, shared_perm;
>
> +/* Asserts that child->frozen == false */
> bdrv_replace_child_noperm(child, new_bs);
>
> /*
> @@ -2468,6 +2471,7 @@ static void bdrv_detach_child(BdrvChild *child)
> g_free(child);
> }
>
> +/* Callers must ensure that child->frozen is false. */
> void bdrv_root_unref_child(BdrvChild *child)
> {
> BlockDriverState *child_bs;
> @@ -2477,10 +2481,6 @@ void bdrv_root_unref_child(BdrvChild *child)
> bdrv_unref(child_bs);
> }
>
> -/**
> - * Clear all inherits_from pointers from children and grandchildren of
> - * @root that point to @root, where necessary.
> - */
Hmm, unrelated chunk? Without it:
Reviewed-by: Vladimir Sementsov-Ogievskiy
> static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild
> *child)
> {
> BdrvChild *c;
> @@ -2505,6 +2505,7 @@ static void bdrv_unset_inherits_from(BlockDriverState
> *root, BdrvChild *child)
[..]
--
Best regards,
Vladimir
[Qemu-block] [PATCH v6 09/42] block: Include filters when freezing backing chain
In order to make filters work in backing chains, the associated
functions must be able to deal with them and freeze all filter links, be
they COW or R/W filter links.
In the process, rename these functions to reflect that they now act on
generalized chains of filter nodes instead of backing chains alone.
While at it, add some comments that note which functions require their
caller to ensure that a given child link is not frozen, and how the
callers do so.
Signed-off-by: Max Reitz
---
include/block/block.h | 10 +++---
block.c | 81 +--
block/commit.c| 8 ++---
block/mirror.c| 4 +--
block/stream.c| 8 ++---
5 files changed, 62 insertions(+), 49 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 50a07c1c33..f6f09b95cd 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -364,11 +364,11 @@ int bdrv_drop_intermediate(BlockDriverState *top,
BlockDriverState *base,
BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
BlockDriverState *bs);
BlockDriverState *bdrv_find_base(BlockDriverState *bs);
-bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
- Error **errp);
-int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
- Error **errp);
-void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
+bool bdrv_is_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
+ Error **errp);
+int bdrv_freeze_chain(BlockDriverState *bs, BlockDriverState *base,
+ Error **errp);
+void bdrv_unfreeze_chain(BlockDriverState *bs, BlockDriverState *base);
typedef struct BdrvCheckResult {
diff --git a/block.c b/block.c
index adf82efb0e..650c00d182 100644
--- a/block.c
+++ b/block.c
@@ -2303,12 +2303,15 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
* If @new_bs is not NULL, bdrv_check_perm() must be called beforehand, as this
* function uses bdrv_set_perm() to update the permissions according to the new
* reference that @new_bs gets.
+ *
+ * Callers must ensure that child->frozen is false.
*/
static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
{
BlockDriverState *old_bs = child->bs;
uint64_t perm, shared_perm;
+/* Asserts that child->frozen == false */
bdrv_replace_child_noperm(child, new_bs);
/*
@@ -2468,6 +2471,7 @@ static void bdrv_detach_child(BdrvChild *child)
g_free(child);
}
+/* Callers must ensure that child->frozen is false. */
void bdrv_root_unref_child(BdrvChild *child)
{
BlockDriverState *child_bs;
@@ -2477,10 +2481,6 @@ void bdrv_root_unref_child(BdrvChild *child)
bdrv_unref(child_bs);
}
-/**
- * Clear all inherits_from pointers from children and grandchildren of
- * @root that point to @root, where necessary.
- */
static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child)
{
BdrvChild *c;
@@ -2505,6 +2505,7 @@ static void bdrv_unset_inherits_from(BlockDriverState
*root, BdrvChild *child)
}
}
+/* Callers must ensure that child->frozen is false. */
void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
{
if (child == NULL) {
@@ -2548,7 +2549,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs,
BlockDriverState *backing_hd,
bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
bdrv_inherits_from_recursive(backing_hd, bs);
-if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
+if (bdrv_is_chain_frozen(bs, child_bs(bs->backing), errp)) {
return;
}
@@ -2557,6 +2558,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs,
BlockDriverState *backing_hd,
}
if (bs->backing) {
+/* Cannot be frozen, we checked that above */
bdrv_unref_child(bs, bs->backing);
}
@@ -3674,8 +3676,7 @@ static int bdrv_reopen_parse_backing(BDRVReopenState
*reopen_state,
return -EPERM;
}
/* Check if the backing link that we want to replace is frozen */
-if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
- errp)) {
+if (bdrv_is_chain_frozen(overlay_bs, backing_bs(overlay_bs), errp)) {
return -EPERM;
}
reopen_state->replace_backing_bs = true;
@@ -4029,6 +4030,7 @@ static void bdrv_close(BlockDriverState *bs)
if (bs->drv) {
if (bs->drv->bdrv_close) {
+/* Must unfreeze all children, so bdrv_unref_child() works */
bs->drv->bdrv_close(bs);
}
bs->drv = NULL;
@@ -4398,20 +4400,22 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
}
/*
- * Return true if at least one of the backing links between @bs and
- * @base is frozen. @errp is set if that's the case.
+
