[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 02/47] block: Add chain helper functions
From: |
Max Reitz |
Subject: |
Re: [PATCH v7 02/47] block: Add chain helper functions |
Date: |
Thu, 16 Jul 2020 16:50:50 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
On 13.07.20 12:18, Vladimir Sementsov-Ogievskiy wrote:
> 25.06.2020 18:21, Max Reitz wrote:
>> Add some helper functions for skipping filters in a chain of block
>> nodes.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> include/block/block_int.h | 3 +++
>> block.c | 55 +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 58 insertions(+)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index bb3457c5e8..5da793bfc3 100644
>
>
> This patch raises two questions:
>
> 1. How to treat filters at the end of the backing chain?
It was my understanding that this configuration would be impossible.
> - child-access function will return no filter child for such nodes, it's
> correct of course
> - filer skipping functions will return this filter.. How much is it
> correct - I don't know.
>
>
> Consider a chain
>
> top --- backing ---> filter-with-no-child
How would it be possible to have filter without a child?
> if bdrv_backing_chain_next(top) returns NULL, it's incorrect, because
> top actually have backing, and on read it will read from it for
> unallocated clusters (and this should crash). So, probably, returning
> filter as a backing-chain-next is a valid thing to do. Or we should
> assert that we are not in such situation (which may crash more often
> than trying to really read from nonexistent child).
>
> so, returning NULL, may even less correct than returning a filter..
>
>
> 2. How to tread nodes with drv=NULL, but with filter child (with
> BDRV_CHILD_FILTERED role).
drv=NULL is a bug on its own that should be fixed... (The idea we had
at some point was to introduce a special driver that just always returns
-EIO on everything, and to replace corrupt qcow2 nodes by that. Or,
well, we might just return -EIO from the qcow2 driver, actually, if we
never use drv=NULL anywhere else.)
In any case, drv=NULL is an edge case that I think never has anything to
do with filters.
> - child-access functions returns no filtered child for such nodes
> - filter skipping functions will stop on it..
>
> =======
>
> Isn't it better to drop drv->is_filter at all? And call filter nodes
> with a bs->file or bs->backing
> child in BDRV_CHILD_FILTERED role? This automatically closes the two
> questions:
>
> - node without a child in BDRV_CHILD_FILTERED is automatically
> non-filter. So, filter driver is responsible for having such child.
> - node without a drv may still be a filter if it have
> BDRV_CHILD_FILTERED.. Still, not very useful.
>
> Anyway, is_filter and BDRV_CHILD_FILTERED are in contradiction, and it
> seems good to get rid of is_filter. But I may miss something.
>
> [..]
>
>> +
>> +static BlockDriverState *bdrv_do_skip_filters(BlockDriverState *bs,
>> + bool
>> stop_on_explicit_filter)
>> +{
>> + BdrvChild *c;
>> +
>> + if (!bs) {
>> + return NULL;
>> + }
>> +
>> + while (!(stop_on_explicit_filter && !bs->implicit)) {
>> + c = bdrv_filter_child(bs);
>> + if (!c) {
>> + break;
>> + }
>> + bs = c->bs;
>> + }
>> + /*
>> + * Note that this treats nodes with bs->drv == NULL as not being
>> + * filters (bs->drv == NULL should be replaced by something else
>> + * anyway).
>> + * The advantage of this behavior is that this function will thus
>> + * always return a non-NULL value (given a non-NULL @bs).
>
> I don't see, how it is follows from first sentence? We can skip nodes
> with a child of BDRV_CHILD_FILTERED and drv=NULL as well, and still return
> non-NULL bs at the end...
My idea was that nodes with bs->drv == NULL might not even have
children. If we treated them like filters and skipped through them, we
would have to return NULL if there is no child.
> Didn't you mean "treat nodes without filter child as not being filters,
> even if they have drv->is_filter == true"? This is a real reason for the
> second sentence.
Hm. I implicitly always assume that filters always have a filter child,
so I tend to not even question that part.
Max
signature.asc
Description: OpenPGP digital signature