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 <mre...@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
signature.asc
Description: OpenPGP digital signature
