[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v7 02/47] block: Add chain helper functions

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v7 02/47] block: Add chain helper functions
Date: Thu, 16 Jul 2020 18:24:58 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

16.07.2020 17:50, Max Reitz wrote:
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

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.

OK for me, but I'd prefer to have a comment and assertions.

- 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

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.

So, again some good comment and assertion won't hurt.

- 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

- 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
+    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.

Hmm. Still, the relationship in the comment seems unclear to me, the code 
itself is simpler :)

Okay, I'm actually OK with this all. I'd prefer to have assertions and comments 
on corner-cases I mentioned, but patch is OK as is:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]