[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v5 14/42] block: Use CAFs when working with back
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH v5 14/42] block: Use CAFs when working with backing chains |
Date: |
Fri, 14 Jun 2019 14:31:38 +0000 |
14.06.2019 16:50, Max Reitz wrote:
> On 14.06.19 15:26, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 1:09, Max Reitz wrote:
>>> 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 11f37983d9..505b3e9a01 100644
>>> --- a/block.c
>>> +++ b/block.c
>
> [...]
>
>>> @@ -4273,11 +4274,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);
>>> +
>>> + while (active) {
>>> + BlockDriverState *next = bdrv_backing_chain_next(active);
>>> + if (bs == next) {
>>> + return active;
>>> + }
>>> + active = next;
>>> }
>>>
>>> - return active;
>>> + return NULL;
>>> }
>>
>> Semantics changed for this function.
>> It is used in two places
>> 1. from bdrv_find_base wtih @bs=NULL, it should be unchanged, as I hope we
>> will never have
>> filter node as a bottom of some valid chain
>>
>> 2. from qmp_block_commit, only to check op-blocker... hmmm. I really don't
>> understand,
>> why do we check BLOCK_OP_TYPE_COMMIT_TARGET on top_bs overlay.. top_bs
>> overlay is out of the job,
>> what is this check for?
>
> There is a loop before this check which checks that the same blocker is
> not set on any nodes between top and base (both inclusive). I guess
> non-active commit checks the node above @top, too, because its backing
> file will change.
So in this case frozen chain works better.
>
>>> /* Given a BDS, searches for the base layer. */
>
> [...]
>
>>> @@ -5149,7 +5165,7 @@ BlockDriverState
>>> *bdrv_find_backing_image(BlockDriverState *bs,
>>> char *backing_file_full_ret;
>>>
>>> if (strcmp(backing_file, curr_bs->backing_file) == 0) {
>>
>> hmm, interesting, what bs->backing_file now means? It's strange enough to
>> store such field on
>> bds, when we have backing link anyway..
>
> Patch 37 has you covered. :-)
>
Hmm, if it has removed this field, but it doesn't)
So, we finished with some object, called "overlay", but it is not an overlay of
bs, it's overlay of
first non-implicit filtered node in bs backing chain, it may be found by
bdrv_find_overlay() helper (which is
almost unused and my be safely dropped), and filename of this "overlay" is
stored in bs->backing_file string
variable, keeping in mind that bs->backing is pointer to backing child of bs
which is completely another thing?
Oh, no, everything related to filename-based backing chain logic is not for me
o_O. If something doesn't work
with filename-based logic users should use node-names.. And I'd prefer to
deprecate filename based interfaces
at all.
--
Best regards,
Vladimir
- [Qemu-block] [PATCH v5 09/42] block: Include filters when freezing backing chain, (continued)
[Qemu-block] [PATCH v5 15/42] block: Re-evaluate backing file handling in reopen, Max Reitz, 2019/06/12
[Qemu-block] [PATCH v5 16/42] block: Use child access functions when flushing, Max Reitz, 2019/06/12
[Qemu-block] [PATCH v5 17/42] block: Use CAFs in bdrv_refresh_limits(), Max Reitz, 2019/06/12