qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v5 15/42] block: Re-evaluate backing file handli


From: Max Reitz
Subject: Re: [Qemu-block] [PATCH v5 15/42] block: Re-evaluate backing file handling in reopen
Date: Fri, 14 Jun 2019 17:52:56 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0

On 14.06.19 15:42, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, 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 <address@hidden>
>> ---
>>   block.c | 36 +++++++++++++++++++++++++++++-------
>>   1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 505b3e9a01..db2759c10d 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3542,17 +3542,39 @@ 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;
>> +    }
> 
> hmm, shouldn't we have these checks for overlay_bs?

I think this is correct here because this is the only node the user has
control over, so this is the only one we can reasonably complain about.

And I do think it is reasonable to complain about.

>> +
>>       /*
>>        * 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_filtered_bs(overlay_bs) &&
>> +           bdrv_filtered_bs(overlay_bs)->implicit)
>> +    {
>> +        overlay_bs = bdrv_filtered_bs(overlay_bs);
>>       }
> 
> here, overlay_bs may be some filter with file child ..
> 
>>   
>>       /* 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_filtered_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 "
>> @@ -3560,8 +3582,8 @@ static int bdrv_reopen_parse_backing(BDRVReopenState 
>> *reopen_state,
>>               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)) {
>> +        if (bdrv_is_backing_chain_frozen(overlay_bs,
>> +                                         child_bs(overlay_bs->backing), 
>> errp)) {
> 
> .. and here we are doing wrong thing, as it don't have backing child
> 
> Aha, you use the fact that we now don't have implicit filters with file 
> child. Then, should
> we add an assertion for this?

No, that wasn’t my intention.  The real reason is that all of this is a
mess.

Here is the full context:

>     overlay_bs = bs;
>     while (bdrv_filtered_bs(overlay_bs) &&
>            bdrv_filtered_bs(overlay_bs)->implicit)
>     {
>         overlay_bs = bdrv_filtered_bs(overlay_bs);
>     }
> 
>     /* If we want to replace the backing file we need some extra checks */
>     if (new_backing_bs != bdrv_filtered_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,
>                                          child_bs(overlay_bs->backing), 
> errp)) {
>             return -EPERM;
>         }

Note the “Check for implicit nodes” thing.  If we get to the frozen
check, we have already confirmed that overlay_bs == bs, so we then know
that overlay_bs->backing works.

I can add an additional comment to make that more clear.  It took myself
quite a bit of digging to figure that out again...

(The reason for the loop is that we want to be able to recognize when
the user tries to not change the backing file.  In that case, we don’t
have to do anything, but because the user doesn’t know about implicit
nodes, we have to skip them in order to check whether the user actually
doesn’t want to change anything.)

Max

>>               return -EPERM;
>>           }
>>           reopen_state->replace_backing_bs = true;
>> @@ -3712,7 +3734,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;
>> @@ -3857,7 +3879,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) {
>>
> 
> 


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]