| Message ID | 20200625152215.941773-3-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Deal with filters | expand |
On 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 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -1382,6 +1382,9 @@ BdrvChild *bdrv_cow_child(BlockDriverState *bs); > BdrvChild *bdrv_filter_child(BlockDriverState *bs); > BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs); > BdrvChild *bdrv_primary_child(BlockDriverState *bs); > +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs); > +BlockDriverState *bdrv_skip_filters(BlockDriverState *bs); > +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs); > > static inline BlockDriverState *child_bs(BdrvChild *child) > { > diff --git a/block.c b/block.c > index 5a42ef49fd..0a0b855261 100644 > --- a/block.c > +++ b/block.c > @@ -7008,3 +7008,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState *bs) > > return NULL; > } > + > +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; Could it be child_bs(bs) ? Andrey > + } Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
On 08.07.20 19:20, Andrey Shinkevich wrote: > On 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 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -1382,6 +1382,9 @@ BdrvChild *bdrv_cow_child(BlockDriverState *bs); >> BdrvChild *bdrv_filter_child(BlockDriverState *bs); >> BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs); >> BdrvChild *bdrv_primary_child(BlockDriverState *bs); >> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs); >> +BlockDriverState *bdrv_skip_filters(BlockDriverState *bs); >> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs); >> static inline BlockDriverState *child_bs(BdrvChild *child) >> { >> diff --git a/block.c b/block.c >> index 5a42ef49fd..0a0b855261 100644 >> --- a/block.c >> +++ b/block.c >> @@ -7008,3 +7008,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState >> *bs) >> return NULL; >> } >> + >> +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; > > Could it be child_bs(bs) ? Well, in a sense, but not really. We need to check whether there is a child before overwriting @bs (because @bs must stay a non-NULL pointer), so we wouldn’t have fewer lines of code if we replaced “BdrvChild *c” by “BlockDriverState *child_bs”, and then used bdrv_child() to set child_bs. (And because we have to check whether @c is NULL anyway, there is no real reason to use child_bs(c) instead of c->bs afterwards.) >> + } > Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> Thanks a lot for reviewing!
On 09.07.2020 11:24, Max Reitz wrote: > On 08.07.20 19:20, Andrey Shinkevich wrote: >> On 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 >>> --- a/include/block/block_int.h >>> +++ b/include/block/block_int.h >>> @@ -1382,6 +1382,9 @@ BdrvChild *bdrv_cow_child(BlockDriverState *bs); >>> BdrvChild *bdrv_filter_child(BlockDriverState *bs); >>> BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs); >>> BdrvChild *bdrv_primary_child(BlockDriverState *bs); >>> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs); >>> +BlockDriverState *bdrv_skip_filters(BlockDriverState *bs); >>> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs); >>> static inline BlockDriverState *child_bs(BdrvChild *child) >>> { >>> diff --git a/block.c b/block.c >>> index 5a42ef49fd..0a0b855261 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -7008,3 +7008,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState >>> *bs) >>> return NULL; >>> } >>> + >>> +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; >> Could it be child_bs(bs) ? > Well, in a sense, but not really. We need to check whether there is a > child before overwriting @bs (because @bs must stay a non-NULL pointer), > so we wouldn’t have fewer lines of code if we replaced “BdrvChild *c” by > “BlockDriverState *child_bs”, and then used bdrv_child() to set child_bs. > > (And because we have to check whether @c is NULL anyway, there is no > real reason to use child_bs(c) instead of c->bs afterwards.) Got it, thanks. Andrey >>> + } >> Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Thanks a lot for reviewing! Pleasure! Andrey
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? - 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 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). - 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... 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. ... and the disadvantage is that we may return filter node, which may be not expected by caller ... > + */ > + > + return bs; > +} I think, I can live with it as is for now. If we don't drop is_filter, I think it worth documenting these corner cases somewhere (may be near .is_filter definition). Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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
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>
diff --git a/include/block/block_int.h b/include/block/block_int.h index bb3457c5e8..5da793bfc3 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1382,6 +1382,9 @@ BdrvChild *bdrv_cow_child(BlockDriverState *bs); BdrvChild *bdrv_filter_child(BlockDriverState *bs); BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs); BdrvChild *bdrv_primary_child(BlockDriverState *bs); +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs); +BlockDriverState *bdrv_skip_filters(BlockDriverState *bs); +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs); static inline BlockDriverState *child_bs(BdrvChild *child) { diff --git a/block.c b/block.c index 5a42ef49fd..0a0b855261 100644 --- a/block.c +++ b/block.c @@ -7008,3 +7008,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState *bs) return NULL; } + +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). + */ + + return bs; +} + +/* + * Return the first BDS that has not been added implicitly or that + * does not have a filtered child down the chain starting from @bs + * (including @bs itself). + */ +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs) +{ + return bdrv_do_skip_filters(bs, true); +} + +/* + * Return the first BDS that does not have a filtered child down the + * chain starting from @bs (including @bs itself). + */ +BlockDriverState *bdrv_skip_filters(BlockDriverState *bs) +{ + return bdrv_do_skip_filters(bs, false); +} + +/* + * For a backing chain, return the first non-filter backing image of + * the first non-filter image. + */ +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs) +{ + return bdrv_skip_filters(bdrv_cow_bs(bdrv_skip_filters(bs))); +}
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(+)