Re: [PATCH 3/4] block: implement 'resize' callback for child_of_bds class

[Kevin Wolf <kwolf@redhat.com>]

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