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.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.)
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 <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?
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
