If a node below a filter node is resized, the size of the filter node
is now also refreshed (recursively for filter parents).
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block.c | 12 ++++++++++++
include/block/block_int-common.h | 2 +-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index bfd4340b24..449f814ebe 100644
--- a/block.c
+++ b/block.c
@@ -1497,6 +1497,17 @@ static void GRAPH_WRLOCK bdrv_child_cb_detach(BdrvChild *child)
}
}
+static void coroutine_fn GRAPH_RDLOCK bdrv_child_cb_resize(BdrvChild *child)
+{
+ BlockDriverState *bs = child->opaque;
+
+ if (bs->drv && bs->drv->is_filter) {
+ /* Best effort, ignore errors. */
+ bdrv_co_refresh_total_sectors(bs, bs->total_sectors);
+ bdrv_co_parent_cb_resize(bs);
+ }
+}
+
static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
const char *filename,
bool backing_mask_protocol,
@@ -1529,6 +1540,7 @@ const BdrvChildClass child_of_bds = {
.detach = bdrv_child_cb_detach,
.inactivate = bdrv_child_cb_inactivate,
.change_aio_ctx = bdrv_child_cb_change_aio_ctx,
+ .resize = bdrv_child_cb_resize,
.update_filename = bdrv_child_cb_update_filename,
.get_parent_aio_context = child_of_bds_get_parent_aio_context,
};
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 692a9696d1..6670db2f85 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1023,7 +1023,7 @@ struct BdrvChildClass {
/*
* Notifies the parent that the child was resized.
*/
- void (*resize)(BdrvChild *child);
+ void GRAPH_RDLOCK_PTR (*resize)(BdrvChild *child);
/*
* Returns a name that is supposedly more useful for human users than the
--
2.47.2On 30.06.25 13:27, Fiona Ebner wrote:
> If a node below a filter node is resized, the size of the filter node
> is now also refreshed (recursively for filter parents).
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> block.c | 12 ++++++++++++
> include/block/block_int-common.h | 2 +-
> 2 files changed, 13 insertions(+), 1 deletion(-)
What does this do for block job filter nodes, like mirror? If they
changed their length, we might have to consider whether the job also
needs to be amended somehow.
However, I assume it’s a safe (conservative) change for them because
such drivers don’t implement bdrv_co_getlength(), so
bdrv_co_refresh_total_sectors() will not change anything. Is that right
and intended?
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
(Babbling below.)
> diff --git a/block.c b/block.c
> index bfd4340b24..449f814ebe 100644
> --- a/block.c
> +++ b/block.c
> @@ -1497,6 +1497,17 @@ static void GRAPH_WRLOCK bdrv_child_cb_detach(BdrvChild *child)
> }
> }
>
> +static void coroutine_fn GRAPH_RDLOCK bdrv_child_cb_resize(BdrvChild *child)
> +{
> + BlockDriverState *bs = child->opaque;
> +
> + if (bs->drv && bs->drv->is_filter) {
Checking the role for BDRV_CHILD_FILTERED would be more generic; but it
would cause 'raw' nodes to be updated, too. But I don’t know whether we
want that or not, and excluding raw (i.e. not changing behavior there)
is certainly the safe option.
> + /* Best effort, ignore errors. */
> + bdrv_co_refresh_total_sectors(bs, bs->total_sectors);
> + bdrv_co_parent_cb_resize(bs);
This makes me wonder whether bdrv_co_refresh_total_sectors() should
itself call bdrv_co_parent_cb_resize(). Still not quite sure; if the
underlying image file is resized by an external party (and we notice
this by accident, more or less), maybe the guest device should be informed.
(One thing to consider, maybe, are nodes that unshare the resize
permission for a child. It’s probably not clever to call the resize CB
if that permission is unshared, so maybe just from that perspective, we
should keep that CB strictly inside of that explicit truncate path that
checks that permission (at least when growing images...).)
Anyway, again, this is the safe option.
Hanna
> + }
> +}
> +
> static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
> const char *filename,
> bool backing_mask_protocol,
> @@ -1529,6 +1540,7 @@ const BdrvChildClass child_of_bds = {
> .detach = bdrv_child_cb_detach,
> .inactivate = bdrv_child_cb_inactivate,
> .change_aio_ctx = bdrv_child_cb_change_aio_ctx,
> + .resize = bdrv_child_cb_resize,
> .update_filename = bdrv_child_cb_update_filename,
> .get_parent_aio_context = child_of_bds_get_parent_aio_context,
> };
> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
> index 692a9696d1..6670db2f85 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -1023,7 +1023,7 @@ struct BdrvChildClass {
> /*
> * Notifies the parent that the child was resized.
> */
> - void (*resize)(BdrvChild *child);
> + void GRAPH_RDLOCK_PTR (*resize)(BdrvChild *child);
>
> /*
> * Returns a name that is supposedly more useful for human users than the
Am 01.07.2025 um 16:46 hat Hanna Czenczek geschrieben:
> On 30.06.25 13:27, Fiona Ebner wrote:
> > If a node below a filter node is resized, the size of the filter node
> > is now also refreshed (recursively for filter parents).
> >
> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> > ---
> > block.c | 12 ++++++++++++
> > include/block/block_int-common.h | 2 +-
> > 2 files changed, 13 insertions(+), 1 deletion(-)
>
> What does this do for block job filter nodes, like mirror? If they changed
> their length, we might have to consider whether the job also needs to be
> amended somehow.
mirror doesn't share BLK_PERM_RESIZE in its block_job_create() call, so
can this even happen? (If yes, that sounds bad.)
> However, I assume it’s a safe (conservative) change for them because such
> drivers don’t implement bdrv_co_getlength(), so
> bdrv_co_refresh_total_sectors() will not change anything. Is that right and
> intended?
>
> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
>
> (Babbling below.)
>
> > diff --git a/block.c b/block.c
> > index bfd4340b24..449f814ebe 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1497,6 +1497,17 @@ static void GRAPH_WRLOCK bdrv_child_cb_detach(BdrvChild *child)
> > }
> > }
> > +static void coroutine_fn GRAPH_RDLOCK bdrv_child_cb_resize(BdrvChild *child)
> > +{
> > + BlockDriverState *bs = child->opaque;
> > +
> > + if (bs->drv && bs->drv->is_filter) {
>
> Checking the role for BDRV_CHILD_FILTERED would be more generic; but it
> would cause 'raw' nodes to be updated, too. But I don’t know whether we
> want that or not, and excluding raw (i.e. not changing behavior there) is
> certainly the safe option.
If the size is not explicitly overridden in the node configuration, I
would certainly expect that a raw node reflects the size of its file
node.
Seems this is exactly the condition that makes it use
BDRV_CHILD_FILTERED, so it would probably be a good change?
> > + /* Best effort, ignore errors. */
> > + bdrv_co_refresh_total_sectors(bs, bs->total_sectors);
> > + bdrv_co_parent_cb_resize(bs);
>
> This makes me wonder whether bdrv_co_refresh_total_sectors() should itself
> call bdrv_co_parent_cb_resize(). Still not quite sure; if the underlying
> image file is resized by an external party (and we notice this by accident,
> more or less), maybe the guest device should be informed.
>
> (One thing to consider, maybe, are nodes that unshare the resize permission
> for a child. It’s probably not clever to call the resize CB if that
> permission is unshared, so maybe just from that perspective, we should keep
> that CB strictly inside of that explicit truncate path that checks that
> permission (at least when growing images...).)
>
> Anyway, again, this is the safe option.
The traditional solution for nodes that unshare resize, but get resized
in the background anyway would be bs->drv = NULL, and we probably still
aren't certain what happens after that. :-)
Kevin
Am 01.07.25 um 7:32 PM schrieb Kevin Wolf:
> Am 01.07.2025 um 16:46 hat Hanna Czenczek geschrieben:
>> On 30.06.25 13:27, Fiona Ebner wrote:
>>> If a node below a filter node is resized, the size of the filter node
>>> is now also refreshed (recursively for filter parents).
>>>
>>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>>> ---
>>> block.c | 12 ++++++++++++
>>> include/block/block_int-common.h | 2 +-
>>> 2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> What does this do for block job filter nodes, like mirror? If they changed
>> their length, we might have to consider whether the job also needs to be
>> amended somehow.
>
> mirror doesn't share BLK_PERM_RESIZE in its block_job_create() call, so
> can this even happen? (If yes, that sounds bad.)
Yes, for mirror, it cannot happen. There is a blocker that prevents
block_resize: "block device is in use by block job: mirror".
>> However, I assume it’s a safe (conservative) change for them because such
>> drivers don’t implement bdrv_co_getlength(), so
>> bdrv_co_refresh_total_sectors() will not change anything. Is that right and
>> intended?
I can see block_job_add_bdrv() uses bdrv_op_block_all(), so resize for
the nodes used directly by the job cannot happen via QMP block_resize
while a job is running.
However, if we go with checking for BDRV_CHILD_FILTERED like you brought
up, there also is the possibility that a 'raw' node gets resized,
because its filtered 'file' node is resized (via QMP block_resize).
But it seems like resizing the 'file' node already is prohibited too
(tested mirror, backup, commit and stream): "Permission conflict on
node 'file0': permissions 'resize' are both required by an unnamed block
device (uses node 'file0' as 'root' child) and unshared by node 'node0'
(uses node 'file0' as 'file' child)."
The only other case where .bdrv_co_getlength() is not implemented, but
.is_filter = true, is copy-before-write. There, the patch does not
change the behavior.
I'll this to the commit message in v2.
>>
>> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
>>
>> (Babbling below.)
>>
>>> diff --git a/block.c b/block.c
>>> index bfd4340b24..449f814ebe 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1497,6 +1497,17 @@ static void GRAPH_WRLOCK bdrv_child_cb_detach(BdrvChild *child)
>>> }
>>> }
>>> +static void coroutine_fn GRAPH_RDLOCK bdrv_child_cb_resize(BdrvChild *child)
>>> +{
>>> + BlockDriverState *bs = child->opaque;
>>> +
>>> + if (bs->drv && bs->drv->is_filter) {
>>
>> Checking the role for BDRV_CHILD_FILTERED would be more generic; but it
>> would cause 'raw' nodes to be updated, too. But I don’t know whether we
>> want that or not, and excluding raw (i.e. not changing behavior there) is
>> certainly the safe option.
>
> If the size is not explicitly overridden in the node configuration, I
> would certainly expect that a raw node reflects the size of its file
> node.
>
> Seems this is exactly the condition that makes it use
> BDRV_CHILD_FILTERED, so it would probably be a good change?
I agree that updating the size in the 'raw' parent node seems good. I'll
go with this in v2 and add a test for it too :)
>>> + /* Best effort, ignore errors. */
>>> + bdrv_co_refresh_total_sectors(bs, bs->total_sectors);
>>> + bdrv_co_parent_cb_resize(bs);
>>
>> This makes me wonder whether bdrv_co_refresh_total_sectors() should itself
>> call bdrv_co_parent_cb_resize(). Still not quite sure; if the underlying
>> image file is resized by an external party (and we notice this by accident,
>> more or less), maybe the guest device should be informed.
>>
>> (One thing to consider, maybe, are nodes that unshare the resize permission
>> for a child. It’s probably not clever to call the resize CB if that
>> permission is unshared, so maybe just from that perspective, we should keep
>> that CB strictly inside of that explicit truncate path that checks that
>> permission (at least when growing images...).)
>>
>> Anyway, again, this is the safe option.
>
> The traditional solution for nodes that unshare resize, but get resized
> in the background anyway would be bs->drv = NULL, and we probably still
> aren't certain what happens after that. :-)
>
> Kevin
Best Regards,
Fiona
© 2016 - 2025 Red Hat, Inc.