[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 14/42] block: Use CAFs when working with back
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v6 14/42] block: Use CAFs when working with backing chains |
Date: |
Mon, 9 Sep 2019 16:08:23 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 09.09.19 11:55, Kevin Wolf wrote:
> Am 09.09.2019 um 10:25 hat Max Reitz geschrieben:
>> On 05.09.19 16:05, Kevin Wolf wrote:
>>> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>>>> Use child access functions when iterating through backing chains so
>>>> filters do not break the chain.
>>>>
>>>> Signed-off-by: Max Reitz <address@hidden>
>>>> ---
>>>> block.c | 40 ++++++++++++++++++++++++++++------------
>>>> 1 file changed, 28 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 86b84bea21..42abbaf0ba 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -4376,7 +4376,8 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>>>> }
>>>>
>>>> /*
>>>> - * Finds the image layer in the chain that has 'bs' as its backing file.
>>>> + * Finds the image layer in the chain that has 'bs' (or a filter on
>>>> + * top of it) as its backing file.
>>>> *
>>>> * active is the current topmost image.
>>>> *
>>>> @@ -4388,11 +4389,18 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>>>> BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>>>> BlockDriverState *bs)
>>>> {
>>>> - while (active && bs != backing_bs(active)) {
>>>> - active = backing_bs(active);
>>>> + bs = bdrv_skip_rw_filters(bs);
>>>> + active = bdrv_skip_rw_filters(active);
>>>
>>> This does more than the commit message says. In addition to iterating
>>> through filters instead of stopping, it also changes the semantics of
>>> the function to return the next non-filter on top of bs instead of the
>>> next node.
>>
>> Which is to say the overlay.
>>
>> (I think we only ever use “overlay” in the COW sense.)
>
> I think we do, but so far also only ever for immediate COW childs, not
> for skipping through intermediate node.
Yes, because it’s broken so far.
>>> The block jobs seem to use it only for bdrv_is_allocated_above(), which
>>> should return the same thing in both cases, so the behaviour stays the
>>> same. qmp_block_commit() will check op blockers on a different node now,
>>> which could be a fix or a bug, I can't tell offhand. Probably the
>>> blocking doesn't really work anyway.
>>
>> You mean that the op blocker could have been on a block job filter node
>> before? I think that‘s pretty much the point of this fix; that that
>> doesn’t make sense. (We didn’t have mirror_top_bs and the like at
>> 058223a6e3b.)
>
> On the off chance that the op blocker actually works, it can't be a job
> filter. I was thinking more of throttling, blkdebug etc.
Well, that’s just broken altogether right now.
>>> All of this should be mentioned in the commit message at least. Maybe
>>> it's also worth splitting in two patches.
>>
>> I don’t know. The function was written when there were no filters.
>
> I doubt it. blkdebug is a really old filter.
>
>> This change would have been a no-op then. The fact that it isn’t to me
>> just means that introducing filters broke it.
>>
>> So I don’t know what I would write. Maybe “bdrv_find_overlay() now
>> actually finds the overlay, that is, it will not return a filter node.
>> This is the behavior that all callers expect (because they work on COW
>> backing chains).”
>
> Maybe just something like "In addition, filter nodes are not returned as
> overlays any more. Instead, the first non-filter node on top of bs is
> considered the overlay now."?
Isn’t that basically the same? :-)
Max
signature.asc
Description: OpenPGP digital signature