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

[Vladimir Sementsov-Ogievskiy]

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

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

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

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,
Vladimir