Re: [Qemu-devel] [PATCH v5 09/42] block: Include filters when freezing backing chain

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7449 bytes --]

On 13.06.19 15:04, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> In order to make filters work in backing chains, the associated
>> functions must be able to deal with them and freeze all filter links, be
>> they COW or R/W filter links.
>>
>> While at it, add some comments that note which functions require their
>> caller to ensure that a given child link is not frozen, and how the
>> callers do so.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c | 45 ++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 8438b0699e..45882a3470 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2214,12 +2214,15 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
>>    * If @new_bs is not NULL, bdrv_check_perm() must be called beforehand, as this
>>    * function uses bdrv_set_perm() to update the permissions according to the new
>>    * reference that @new_bs gets.
>> + *
>> + * Callers must ensure that child->frozen is false.
>>    */
>>   static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>>   {
>>       BlockDriverState *old_bs = child->bs;
>>       uint64_t perm, shared_perm;
>>   
>> +    /* Asserts that child->frozen == false */
>>       bdrv_replace_child_noperm(child, new_bs);
>>   
>>       if (old_bs) {
>> @@ -2360,6 +2363,7 @@ static void bdrv_detach_child(BdrvChild *child)
>>       g_free(child);
>>   }
>>   
>> +/* Callers must ensure that child->frozen is false. */
> 
> Is such a comment better than one-line extra assertion at start of the function body?

Well, there already is an assertion, it is in
bdrv_replace_child_noperm().  I personally prefer to read comments than
assertions.

>>   void bdrv_root_unref_child(BdrvChild *child)
>>   {
>>       BlockDriverState *child_bs;
>> @@ -2369,6 +2373,7 @@ void bdrv_root_unref_child(BdrvChild *child)
>>       bdrv_unref(child_bs);
>>   }
>>   
>> +/* Callers must ensure that child->frozen is false. */
>>   void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
>>   {
>>       if (child == NULL) {
>> @@ -2435,6 +2440,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>>       }
>>   
>>       if (bs->backing) {
>> +        /* Cannot be frozen, we checked that above */
>>           bdrv_unref_child(bs, bs->backing);
>>       }
>>   
>> @@ -3908,6 +3914,7 @@ static void bdrv_close(BlockDriverState *bs)
>>   
>>       if (bs->drv) {
>>           if (bs->drv->bdrv_close) {
>> +            /* Must unfreeze all children, so bdrv_unref_child() works */
>>               bs->drv->bdrv_close(bs);
>>           }
>>           bs->drv = NULL;
>> @@ -4281,17 +4288,20 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
>>    * Return true if at least one of the backing links between @bs and
>>    * @base is frozen. @errp is set if that's the case.
>>    * @base must be reachable from @bs, or NULL.
>> + * (Filters are treated as normal elements of the backing chain.)
>>    */
>>   bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
>>                                     Error **errp)
>>   {
>>       BlockDriverState *i;
>> +    BdrvChild *child;
>>   
>> -    for (i = bs; i != base; i = backing_bs(i)) {
>> -        if (i->backing && i->backing->frozen) {
>> +    for (i = bs; i != base; i = child_bs(child)) {
>> +        child = bdrv_filtered_child(i);
>> +
>> +        if (child && child->frozen) {
>>               error_setg(errp, "Cannot change '%s' link from '%s' to '%s'",
>> -                       i->backing->name, i->node_name,
>> -                       backing_bs(i)->node_name);
>> +                       child->name, i->node_name, child->bs->node_name);
>>               return true;
>>           }
>>       }
>> @@ -4305,19 +4315,22 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
>>    * none of the links are modified.
>>    * @base must be reachable from @bs, or NULL.
>>    * Returns 0 on success. On failure returns < 0 and sets @errp.
>> + * (Filters are treated as normal elements of the backing chain.)
>>    */
>>   int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
>>                                 Error **errp)
>>   {
>>       BlockDriverState *i;
>> +    BdrvChild *child;
>>   
>>       if (bdrv_is_backing_chain_frozen(bs, base, errp)) {
>>           return -EPERM;
>>       }
>>   
>> -    for (i = bs; i != base; i = backing_bs(i)) {
>> -        if (i->backing) {
>> -            i->backing->frozen = true;
>> +    for (i = bs; i != base; i = child_bs(child)) {
>> +        child = bdrv_filtered_child(i);
>> +        if (child) {
>> +            child->frozen = true;
>>           }
>>       }
>>   
>> @@ -4328,15 +4341,18 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
>>    * Unfreeze all backing links between @bs and @base. The caller must
>>    * ensure that all links are frozen before using this function.
>>    * @base must be reachable from @bs, or NULL.
>> + * (Filters are treated as normal elements of the backing chain.)
>>    */
>>   void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base)
>>   {
>>       BlockDriverState *i;
>> +    BdrvChild *child;
>>   
>> -    for (i = bs; i != base; i = backing_bs(i)) {
>> -        if (i->backing) {
>> -            assert(i->backing->frozen);
>> -            i->backing->frozen = false;
>> +    for (i = bs; i != base; i = child_bs(child)) {
>> +        child = bdrv_filtered_child(i);
>> +        if (child) {
>> +            assert(child->frozen);
>> +            child->frozen = false;
>>           }
>>       }
>>   }
>> @@ -4438,8 +4454,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
>>               }
>>           }
>>   
>> -        /* Do the actual switch in the in-memory graph.
>> -         * Completes bdrv_check_update_perm() transaction internally. */
>> +        /*
>> +         * Do the actual switch in the in-memory graph.
>> +         * Completes bdrv_check_update_perm() transaction internally.
>> +         * c->frozen is false, we have checked that above.
>> +         */
>>           bdrv_ref(base);
>>           bdrv_replace_child(c, base);
>>           bdrv_unref(top);
>>
> 
> Hmm, OK, it's better than it was, so:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> But I have one thought: we check that frozen is false at some point, and then
> do some logic around this child. Where is guarantee, that someone else will not
> set frozen=true during some yield, for example? So, do we need a kind of child_mutex,
> or something like this?

The guarantee is that the caller does not do anything that could cause
the child link to become frozen between checking whether it is and
performing an operation that relies on it not being frozen.

Freezing (currently) only happens when starting block jobs, which can
only happen because of monitor commands.  Even in
bdrv_drop_intermediate(), which has quite a bit of code between checking
the frozen status and calling bdrv_replace_child(), I don’t see how a
new block job could sneak in.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]