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.eb...@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 <hre...@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
