[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 14/42] block: Use CAFs when working with back
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v5 14/42] block: Use CAFs when working with backing chains |
Date: |
Fri, 14 Jun 2019 18:02:19 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
On 14.06.19 16:31, Vladimir Sementsov-Ogievskiy wrote:
> 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.
Perhaps. The op blockers are in this weird state anyway. I don’t think
we even need them any more, because the permissions were intended to
replace them (they were originally called “fine-grained op blockers”, I
seem to remember).
I dare not touch them.
>>>> /* 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)
Because it’s needed. (Just not in the current form, but that’s what 37
is for.)
> 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?
I don’t quite see what you mean. There is no “overlay” in this function.
> 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..
In theory yes, but that isn’t an option for qemu-img commit, for example.
> And I'd prefer to deprecate filename based interfaces at all.
Me too.
https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04878.html
:-/
Max
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v5 11/42] block: Add bdrv_supports_compressed_writes(), (continued)
[Qemu-devel] [PATCH v5 15/42] block: Re-evaluate backing file handling in reopen, Max Reitz, 2019/06/12
[Qemu-devel] [PATCH v5 16/42] block: Use child access functions when flushing, Max Reitz, 2019/06/12
[Qemu-devel] [PATCH v5 17/42] block: Use CAFs in bdrv_refresh_limits(), Max Reitz, 2019/06/12