| Message ID | 20200625152215.941773-18-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: > Reopening a node's backing child needs a bit of special handling because > the "backing" child has different defaults than all other children > (among other things). Adding filter support here is a bit more > difficult than just using the child access functions. In fact, we often > have to directly use bs->backing because these functions are about the > "backing" child (which may or may not be the COW backing file). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 46 ++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 38 insertions(+), 8 deletions(-) > > diff --git a/block.c b/block.c > index 712230ef5c..8131d0b5eb 100644 > --- a/block.c > +++ b/block.c > @@ -4026,26 +4026,56 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, > } > } > > + /* > + * Ensure that @bs can really handle backing files, because we are > + * about to give it one (or swap the existing one) > + */ > + if (bs->drv->is_filter) { > + /* Filters always have a file or a backing child */ > + if (!bs->backing) { > + error_setg(errp, "'%s' is a %s filter node that does not support a " > + "backing child", bs->node_name, bs->drv->format_name); > + return -EINVAL; > + } > + } else if (!bs->drv->supports_backing) { > + error_setg(errp, "Driver '%s' of node '%s' does not support backing " > + "files", bs->drv->format_name, bs->node_name); > + return -EINVAL; > + } > + > /* > * Find the "actual" backing file by skipping all links that point > * to an implicit node, if any (e.g. a commit filter node). > + * We cannot use any of the bdrv_skip_*() functions here because > + * those return the first explicit node, while we are looking for > + * its overlay here. > */ > overlay_bs = bs; > - while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) { > - overlay_bs = backing_bs(overlay_bs); > + while (bdrv_filter_or_cow_bs(overlay_bs) && > + bdrv_filter_or_cow_bs(overlay_bs)->implicit) > + { > + overlay_bs = bdrv_filter_or_cow_bs(overlay_bs); > } I believe that little optimization would work properly: for (BlockDriverState *below_bs = bdrv_filter_or_cow_bs(overlay_bs); below_bs && below_bs->implicit; below_bs = bdrv_filter_or_cow_bs(overlay_bs)) { overlay_bs = below_bs; } > > /* If we want to replace the backing file we need some extra checks */ > - if (new_backing_bs != backing_bs(overlay_bs)) { > + if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) { > /* Check for implicit nodes between bs and its backing file */ > if (bs != overlay_bs) { > error_setg(errp, "Cannot change backing link if '%s' has " > "an implicit backing file", bs->node_name); > return -EPERM; > } > - /* Check if the backing link that we want to replace is frozen */ > - if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs), > - errp)) { > + /* > + * Check if the backing link that we want to replace is frozen. > + * Note that > + * bdrv_filter_or_cow_child(overlay_bs) == overlay_bs->backing, > + * because we know that overlay_bs == bs, and that @bs > + * either is a filter that uses ->backing or a COW format BDS > + * with bs->drv->supports_backing == true. > + */ > + if (bdrv_is_backing_chain_frozen(overlay_bs, > + child_bs(overlay_bs->backing), errp)) What would be wrong with bdrv_filter_or_cow_bs(overlay_bs) here? > + { > return -EPERM; > } > reopen_state->replace_backing_bs = true; > @@ -4196,7 +4226,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, > * its metadata. Otherwise the 'backing' option can be omitted. > */ > if (drv->supports_backing && reopen_state->backing_missing && > - (backing_bs(reopen_state->bs) || reopen_state->bs->backing_file[0])) { = BlockDriverState* > + (reopen_state->bs->backing || reopen_state->bs->backing_file[0])) { = BdrvChild* Are we OK with that? > error_setg(errp, "backing is missing for '%s'", > reopen_state->bs->node_name); > ret = -EINVAL; > @@ -4337,7 +4367,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) > * from bdrv_set_backing_hd()) has the new values. > */ > if (reopen_state->replace_backing_bs) { > - BlockDriverState *old_backing_bs = backing_bs(bs); > + BlockDriverState *old_backing_bs = child_bs(bs->backing); > assert(!old_backing_bs || !old_backing_bs->implicit); > /* Abort the permission update on the backing bs we're detaching */ > if (old_backing_bs) {
On 10.07.20 21:42, Andrey Shinkevich wrote: > On 25.06.2020 18:21, Max Reitz wrote: >> Reopening a node's backing child needs a bit of special handling because >> the "backing" child has different defaults than all other children >> (among other things). Adding filter support here is a bit more >> difficult than just using the child access functions. In fact, we often >> have to directly use bs->backing because these functions are about the >> "backing" child (which may or may not be the COW backing file). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 46 ++++++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 38 insertions(+), 8 deletions(-) >> >> diff --git a/block.c b/block.c >> index 712230ef5c..8131d0b5eb 100644 >> --- a/block.c >> +++ b/block.c >> @@ -4026,26 +4026,56 @@ static int >> bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, >> } >> } >> + /* >> + * Ensure that @bs can really handle backing files, because we are >> + * about to give it one (or swap the existing one) >> + */ >> + if (bs->drv->is_filter) { >> + /* Filters always have a file or a backing child */ >> + if (!bs->backing) { >> + error_setg(errp, "'%s' is a %s filter node that does not >> support a " >> + "backing child", bs->node_name, >> bs->drv->format_name); >> + return -EINVAL; >> + } >> + } else if (!bs->drv->supports_backing) { >> + error_setg(errp, "Driver '%s' of node '%s' does not support >> backing " >> + "files", bs->drv->format_name, bs->node_name); >> + return -EINVAL; >> + } >> + >> /* >> * Find the "actual" backing file by skipping all links that point >> * to an implicit node, if any (e.g. a commit filter node). >> + * We cannot use any of the bdrv_skip_*() functions here because >> + * those return the first explicit node, while we are looking for >> + * its overlay here. >> */ >> overlay_bs = bs; >> - while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) { >> - overlay_bs = backing_bs(overlay_bs); >> + while (bdrv_filter_or_cow_bs(overlay_bs) && >> + bdrv_filter_or_cow_bs(overlay_bs)->implicit) >> + { >> + overlay_bs = bdrv_filter_or_cow_bs(overlay_bs); >> } > > I believe that little optimization would work properly: > > > for (BlockDriverState *below_bs = bdrv_filter_or_cow_bs(overlay_bs); > below_bs && below_bs->implicit; > below_bs = bdrv_filter_or_cow_bs(overlay_bs)) { > overlay_bs = below_bs; > } Looks good, thanks. >> /* If we want to replace the backing file we need some extra >> checks */ >> - if (new_backing_bs != backing_bs(overlay_bs)) { >> + if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) { >> /* Check for implicit nodes between bs and its backing file */ >> if (bs != overlay_bs) { >> error_setg(errp, "Cannot change backing link if '%s' has " >> "an implicit backing file", bs->node_name); >> return -EPERM; >> } >> - /* Check if the backing link that we want to replace is >> frozen */ >> - if (bdrv_is_backing_chain_frozen(overlay_bs, >> backing_bs(overlay_bs), >> - errp)) { >> + /* >> + * Check if the backing link that we want to replace is frozen. >> + * Note that >> + * bdrv_filter_or_cow_child(overlay_bs) == overlay_bs->backing, >> + * because we know that overlay_bs == bs, and that @bs >> + * either is a filter that uses ->backing or a COW format BDS >> + * with bs->drv->supports_backing == true. >> + */ >> + if (bdrv_is_backing_chain_frozen(overlay_bs, >> + >> child_bs(overlay_bs->backing), errp)) > What would be wrong with bdrv_filter_or_cow_bs(overlay_bs) here? As the comment says, it’s the same thing. I prefer ->backing here, because this function is about reopening the ->backing child. >> + { >> return -EPERM; >> } >> reopen_state->replace_backing_bs = true; >> @@ -4196,7 +4226,7 @@ int bdrv_reopen_prepare(BDRVReopenState >> *reopen_state, BlockReopenQueue *queue, >> * its metadata. Otherwise the 'backing' option can be omitted. >> */ >> if (drv->supports_backing && reopen_state->backing_missing && >> - (backing_bs(reopen_state->bs) || >> reopen_state->bs->backing_file[0])) { > = BlockDriverState* >> + (reopen_state->bs->backing || >> reopen_state->bs->backing_file[0])) { > > = BdrvChild* > > Are we OK with that? Sure, the question is whether it’s non-NULL, and BdrvChild.bs is always non-NULL. Max
diff --git a/block.c b/block.c index 712230ef5c..8131d0b5eb 100644 --- a/block.c +++ b/block.c @@ -4026,26 +4026,56 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, } } + /* + * Ensure that @bs can really handle backing files, because we are + * about to give it one (or swap the existing one) + */ + if (bs->drv->is_filter) { + /* Filters always have a file or a backing child */ + if (!bs->backing) { + error_setg(errp, "'%s' is a %s filter node that does not support a " + "backing child", bs->node_name, bs->drv->format_name); + return -EINVAL; + } + } else if (!bs->drv->supports_backing) { + error_setg(errp, "Driver '%s' of node '%s' does not support backing " + "files", bs->drv->format_name, bs->node_name); + return -EINVAL; + } + /* * Find the "actual" backing file by skipping all links that point * to an implicit node, if any (e.g. a commit filter node). + * We cannot use any of the bdrv_skip_*() functions here because + * those return the first explicit node, while we are looking for + * its overlay here. */ overlay_bs = bs; - while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) { - overlay_bs = backing_bs(overlay_bs); + while (bdrv_filter_or_cow_bs(overlay_bs) && + bdrv_filter_or_cow_bs(overlay_bs)->implicit) + { + overlay_bs = bdrv_filter_or_cow_bs(overlay_bs); } /* If we want to replace the backing file we need some extra checks */ - if (new_backing_bs != backing_bs(overlay_bs)) { + if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) { /* Check for implicit nodes between bs and its backing file */ if (bs != overlay_bs) { error_setg(errp, "Cannot change backing link if '%s' has " "an implicit backing file", bs->node_name); return -EPERM; } - /* Check if the backing link that we want to replace is frozen */ - if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs), - errp)) { + /* + * Check if the backing link that we want to replace is frozen. + * Note that + * bdrv_filter_or_cow_child(overlay_bs) == overlay_bs->backing, + * because we know that overlay_bs == bs, and that @bs + * either is a filter that uses ->backing or a COW format BDS + * with bs->drv->supports_backing == true. + */ + if (bdrv_is_backing_chain_frozen(overlay_bs, + child_bs(overlay_bs->backing), errp)) + { return -EPERM; } reopen_state->replace_backing_bs = true; @@ -4196,7 +4226,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, * its metadata. Otherwise the 'backing' option can be omitted. */ if (drv->supports_backing && reopen_state->backing_missing && - (backing_bs(reopen_state->bs) || reopen_state->bs->backing_file[0])) { + (reopen_state->bs->backing || reopen_state->bs->backing_file[0])) { error_setg(errp, "backing is missing for '%s'", reopen_state->bs->node_name); ret = -EINVAL; @@ -4337,7 +4367,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) * from bdrv_set_backing_hd()) has the new values. */ if (reopen_state->replace_backing_bs) { - BlockDriverState *old_backing_bs = backing_bs(bs); + BlockDriverState *old_backing_bs = child_bs(bs->backing); assert(!old_backing_bs || !old_backing_bs->implicit); /* Abort the permission update on the backing bs we're detaching */ if (old_backing_bs) {
Reopening a node's backing child needs a bit of special handling because the "backing" child has different defaults than all other children (among other things). Adding filter support here is a bit more difficult than just using the child access functions. In fact, we often have to directly use bs->backing because these functions are about the "backing" child (which may or may not be the COW backing file). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 46 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 8 deletions(-)