| Message ID | 20190809161407.11920-15-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Deal with filters | expand |
09.08.2019 19:13, Max Reitz wrote: > Use child access functions when iterating through backing chains so > filters do not break the chain. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Am 09.08.2019 um 18:13 hat Max Reitz geschrieben: > Use child access functions when iterating through backing chains so > filters do not break the chain. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 40 ++++++++++++++++++++++++++++------------ > 1 file changed, 28 insertions(+), 12 deletions(-) > > diff --git a/block.c b/block.c > index 86b84bea21..42abbaf0ba 100644 > --- a/block.c > +++ b/block.c > @@ -4376,7 +4376,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, > } > > /* > - * Finds the image layer in the chain that has 'bs' as its backing file. > + * Finds the image layer in the chain that has 'bs' (or a filter on > + * top of it) as its backing file. > * > * active is the current topmost image. > * > @@ -4388,11 +4389,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); This does more than the commit message says. In addition to iterating through filters instead of stopping, it also changes the semantics of the function to return the next non-filter on top of bs instead of the next node. The block jobs seem to use it only for bdrv_is_allocated_above(), which should return the same thing in both cases, so the behaviour stays the same. qmp_block_commit() will check op blockers on a different node now, which could be a fix or a bug, I can't tell offhand. Probably the blocking doesn't really work anyway. All of this should be mentioned in the commit message at least. Maybe it's also worth splitting in two patches. > + while (active) { > + BlockDriverState *next = bdrv_backing_chain_next(active); > + if (bs == next) { > + return active; > + } > + active = next; > } > > - return active; > + return NULL; > } > > /* Given a BDS, searches for the base layer. */ > @@ -4544,9 +4552,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, > * other intermediate nodes have been dropped. > * If 'top' is an implicit node (e.g. "commit_top") we should skip > * it because no one inherits from it. We use explicit_top for that. */ > - while (explicit_top && explicit_top->implicit) { > - explicit_top = backing_bs(explicit_top); > - } > + explicit_top = bdrv_skip_implicit_filters(explicit_top); > update_inherits_from = bdrv_inherits_from_recursive(base, explicit_top); > > /* success - we can delete the intermediate states, and link top->base */ > @@ -5014,7 +5020,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device, > bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base) > { > while (top && top != base) { > - top = backing_bs(top); > + top = bdrv_filtered_bs(top); > } > > return top != NULL; > @@ -5253,7 +5259,17 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, > > is_protocol = path_has_protocol(backing_file); > > - for (curr_bs = bs; curr_bs->backing; curr_bs = curr_bs->backing->bs) { > + /* > + * Being largely a legacy function, skip any filters here > + * (because filters do not have normal filenames, so they cannot > + * match anyway; and allowing json:{} filenames is a bit out of > + * scope). > + */ > + for (curr_bs = bdrv_skip_rw_filters(bs); > + bdrv_filtered_cow_child(curr_bs) != NULL; > + curr_bs = bdrv_backing_chain_next(curr_bs)) This could just use bs_below instead of recalculating the node if you moved the declaration of bs_below to the function scope. > + { > + BlockDriverState *bs_below = bdrv_backing_chain_next(curr_bs); > > /* If either of the filename paths is actually a protocol, then > * compare unmodified paths; otherwise make paths relative */ > @@ -5261,7 +5277,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, > char *backing_file_full_ret; > > if (strcmp(backing_file, curr_bs->backing_file) == 0) { > - retval = curr_bs->backing->bs; > + retval = bs_below; > break; > } > /* Also check against the full backing filename for the image */ > @@ -5271,7 +5287,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, > bool equal = strcmp(backing_file, backing_file_full_ret) == 0; > g_free(backing_file_full_ret); > if (equal) { > - retval = curr_bs->backing->bs; > + retval = bs_below; > break; > } > } > @@ -5297,7 +5313,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, > g_free(filename_tmp); > > if (strcmp(backing_file_full, filename_full) == 0) { > - retval = curr_bs->backing->bs; > + retval = bs_below; > break; > } > } Kevin
On 05.09.19 16:05, Kevin Wolf wrote: > Am 09.08.2019 um 18:13 hat Max Reitz geschrieben: >> Use child access functions when iterating through backing chains so >> filters do not break the chain. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 40 ++++++++++++++++++++++++++++------------ >> 1 file changed, 28 insertions(+), 12 deletions(-) >> >> diff --git a/block.c b/block.c >> index 86b84bea21..42abbaf0ba 100644 >> --- a/block.c >> +++ b/block.c >> @@ -4376,7 +4376,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, >> } >> >> /* >> - * Finds the image layer in the chain that has 'bs' as its backing file. >> + * Finds the image layer in the chain that has 'bs' (or a filter on >> + * top of it) as its backing file. >> * >> * active is the current topmost image. >> * >> @@ -4388,11 +4389,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); > > This does more than the commit message says. In addition to iterating > through filters instead of stopping, it also changes the semantics of > the function to return the next non-filter on top of bs instead of the > next node. Which is to say the overlay. (I think we only ever use “overlay” in the COW sense.) > The block jobs seem to use it only for bdrv_is_allocated_above(), which > should return the same thing in both cases, so the behaviour stays the > same. qmp_block_commit() will check op blockers on a different node now, > which could be a fix or a bug, I can't tell offhand. Probably the > blocking doesn't really work anyway. You mean that the op blocker could have been on a block job filter node before? I think that‘s pretty much the point of this fix; that that doesn’t make sense. (We didn’t have mirror_top_bs and the like at 058223a6e3b.) > All of this should be mentioned in the commit message at least. Maybe > it's also worth splitting in two patches. I don’t know. The function was written when there were no filters. This change would have been a no-op then. The fact that it isn’t to me just means that introducing filters broke it. So I don’t know what I would write. Maybe “bdrv_find_overlay() now actually finds the overlay, that is, it will not return a filter node. This is the behavior that all callers expect (because they work on COW backing chains).” >> + while (active) { >> + BlockDriverState *next = bdrv_backing_chain_next(active); >> + if (bs == next) { >> + return active; >> + } >> + active = next; >> } >> >> - return active; >> + return NULL; >> } >> >> /* Given a BDS, searches for the base layer. */ >> @@ -4544,9 +4552,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, >> * other intermediate nodes have been dropped. >> * If 'top' is an implicit node (e.g. "commit_top") we should skip >> * it because no one inherits from it. We use explicit_top for that. */ >> - while (explicit_top && explicit_top->implicit) { >> - explicit_top = backing_bs(explicit_top); >> - } >> + explicit_top = bdrv_skip_implicit_filters(explicit_top); >> update_inherits_from = bdrv_inherits_from_recursive(base, explicit_top); >> >> /* success - we can delete the intermediate states, and link top->base */ >> @@ -5014,7 +5020,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device, >> bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base) >> { >> while (top && top != base) { >> - top = backing_bs(top); >> + top = bdrv_filtered_bs(top); >> } >> >> return top != NULL; >> @@ -5253,7 +5259,17 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, >> >> is_protocol = path_has_protocol(backing_file); >> >> - for (curr_bs = bs; curr_bs->backing; curr_bs = curr_bs->backing->bs) { >> + /* >> + * Being largely a legacy function, skip any filters here >> + * (because filters do not have normal filenames, so they cannot >> + * match anyway; and allowing json:{} filenames is a bit out of >> + * scope). >> + */ >> + for (curr_bs = bdrv_skip_rw_filters(bs); >> + bdrv_filtered_cow_child(curr_bs) != NULL; >> + curr_bs = bdrv_backing_chain_next(curr_bs)) > > This could just use bs_below instead of recalculating the node if you > moved the declaration of bs_below to the function scope. Indeed, thanks. Max >> + { >> + BlockDriverState *bs_below = bdrv_backing_chain_next(curr_bs); >> >> /* If either of the filename paths is actually a protocol, then >> * compare unmodified paths; otherwise make paths relative */
Am 09.09.2019 um 10:25 hat Max Reitz geschrieben: > On 05.09.19 16:05, Kevin Wolf wrote: > > Am 09.08.2019 um 18:13 hat Max Reitz geschrieben: > >> Use child access functions when iterating through backing chains so > >> filters do not break the chain. > >> > >> Signed-off-by: Max Reitz <mreitz@redhat.com> > >> --- > >> block.c | 40 ++++++++++++++++++++++++++++------------ > >> 1 file changed, 28 insertions(+), 12 deletions(-) > >> > >> diff --git a/block.c b/block.c > >> index 86b84bea21..42abbaf0ba 100644 > >> --- a/block.c > >> +++ b/block.c > >> @@ -4376,7 +4376,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, > >> } > >> > >> /* > >> - * Finds the image layer in the chain that has 'bs' as its backing file. > >> + * Finds the image layer in the chain that has 'bs' (or a filter on > >> + * top of it) as its backing file. > >> * > >> * active is the current topmost image. > >> * > >> @@ -4388,11 +4389,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); > > > > This does more than the commit message says. In addition to iterating > > through filters instead of stopping, it also changes the semantics of > > the function to return the next non-filter on top of bs instead of the > > next node. > > Which is to say the overlay. > > (I think we only ever use “overlay” in the COW sense.) I think we do, but so far also only ever for immediate COW childs, not for skipping through intermediate node. > > The block jobs seem to use it only for bdrv_is_allocated_above(), which > > should return the same thing in both cases, so the behaviour stays the > > same. qmp_block_commit() will check op blockers on a different node now, > > which could be a fix or a bug, I can't tell offhand. Probably the > > blocking doesn't really work anyway. > > You mean that the op blocker could have been on a block job filter node > before? I think that‘s pretty much the point of this fix; that that > doesn’t make sense. (We didn’t have mirror_top_bs and the like at > 058223a6e3b.) On the off chance that the op blocker actually works, it can't be a job filter. I was thinking more of throttling, blkdebug etc. > > All of this should be mentioned in the commit message at least. Maybe > > it's also worth splitting in two patches. > > I don’t know. The function was written when there were no filters. I doubt it. blkdebug is a really old filter. > This change would have been a no-op then. The fact that it isn’t to me > just means that introducing filters broke it. > > So I don’t know what I would write. Maybe “bdrv_find_overlay() now > actually finds the overlay, that is, it will not return a filter node. > This is the behavior that all callers expect (because they work on COW > backing chains).” Maybe just something like "In addition, filter nodes are not returned as overlays any more. Instead, the first non-filter node on top of bs is considered the overlay now."? Kevin
On 09.09.19 11:55, Kevin Wolf wrote: > Am 09.09.2019 um 10:25 hat Max Reitz geschrieben: >> On 05.09.19 16:05, Kevin Wolf wrote: >>> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben: >>>> Use child access functions when iterating through backing chains so >>>> filters do not break the chain. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> block.c | 40 ++++++++++++++++++++++++++++------------ >>>> 1 file changed, 28 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index 86b84bea21..42abbaf0ba 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -4376,7 +4376,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, >>>> } >>>> >>>> /* >>>> - * Finds the image layer in the chain that has 'bs' as its backing file. >>>> + * Finds the image layer in the chain that has 'bs' (or a filter on >>>> + * top of it) as its backing file. >>>> * >>>> * active is the current topmost image. >>>> * >>>> @@ -4388,11 +4389,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); >>> >>> This does more than the commit message says. In addition to iterating >>> through filters instead of stopping, it also changes the semantics of >>> the function to return the next non-filter on top of bs instead of the >>> next node. >> >> Which is to say the overlay. >> >> (I think we only ever use “overlay” in the COW sense.) > > I think we do, but so far also only ever for immediate COW childs, not > for skipping through intermediate node. Yes, because it’s broken so far. >>> The block jobs seem to use it only for bdrv_is_allocated_above(), which >>> should return the same thing in both cases, so the behaviour stays the >>> same. qmp_block_commit() will check op blockers on a different node now, >>> which could be a fix or a bug, I can't tell offhand. Probably the >>> blocking doesn't really work anyway. >> >> You mean that the op blocker could have been on a block job filter node >> before? I think that‘s pretty much the point of this fix; that that >> doesn’t make sense. (We didn’t have mirror_top_bs and the like at >> 058223a6e3b.) > > On the off chance that the op blocker actually works, it can't be a job > filter. I was thinking more of throttling, blkdebug etc. Well, that’s just broken altogether right now. >>> All of this should be mentioned in the commit message at least. Maybe >>> it's also worth splitting in two patches. >> >> I don’t know. The function was written when there were no filters. > > I doubt it. blkdebug is a really old filter. > >> This change would have been a no-op then. The fact that it isn’t to me >> just means that introducing filters broke it. >> >> So I don’t know what I would write. Maybe “bdrv_find_overlay() now >> actually finds the overlay, that is, it will not return a filter node. >> This is the behavior that all callers expect (because they work on COW >> backing chains).” > > Maybe just something like "In addition, filter nodes are not returned as > overlays any more. Instead, the first non-filter node on top of bs is > considered the overlay now."? Isn’t that basically the same? :-) Max
diff --git a/block.c b/block.c index 86b84bea21..42abbaf0ba 100644 --- a/block.c +++ b/block.c @@ -4376,7 +4376,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, } /* - * Finds the image layer in the chain that has 'bs' as its backing file. + * Finds the image layer in the chain that has 'bs' (or a filter on + * top of it) as its backing file. * * active is the current topmost image. * @@ -4388,11 +4389,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; } /* Given a BDS, searches for the base layer. */ @@ -4544,9 +4552,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, * other intermediate nodes have been dropped. * If 'top' is an implicit node (e.g. "commit_top") we should skip * it because no one inherits from it. We use explicit_top for that. */ - while (explicit_top && explicit_top->implicit) { - explicit_top = backing_bs(explicit_top); - } + explicit_top = bdrv_skip_implicit_filters(explicit_top); update_inherits_from = bdrv_inherits_from_recursive(base, explicit_top); /* success - we can delete the intermediate states, and link top->base */ @@ -5014,7 +5020,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device, bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base) { while (top && top != base) { - top = backing_bs(top); + top = bdrv_filtered_bs(top); } return top != NULL; @@ -5253,7 +5259,17 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, is_protocol = path_has_protocol(backing_file); - for (curr_bs = bs; curr_bs->backing; curr_bs = curr_bs->backing->bs) { + /* + * Being largely a legacy function, skip any filters here + * (because filters do not have normal filenames, so they cannot + * match anyway; and allowing json:{} filenames is a bit out of + * scope). + */ + for (curr_bs = bdrv_skip_rw_filters(bs); + bdrv_filtered_cow_child(curr_bs) != NULL; + curr_bs = bdrv_backing_chain_next(curr_bs)) + { + BlockDriverState *bs_below = bdrv_backing_chain_next(curr_bs); /* If either of the filename paths is actually a protocol, then * compare unmodified paths; otherwise make paths relative */ @@ -5261,7 +5277,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, char *backing_file_full_ret; if (strcmp(backing_file, curr_bs->backing_file) == 0) { - retval = curr_bs->backing->bs; + retval = bs_below; break; } /* Also check against the full backing filename for the image */ @@ -5271,7 +5287,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, bool equal = strcmp(backing_file, backing_file_full_ret) == 0; g_free(backing_file_full_ret); if (equal) { - retval = curr_bs->backing->bs; + retval = bs_below; break; } } @@ -5297,7 +5313,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, g_free(filename_tmp); if (strcmp(backing_file_full, filename_full) == 0) { - retval = curr_bs->backing->bs; + retval = bs_below; break; } }
Use child access functions when iterating through backing chains so filters do not break the chain. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-)