Re: [Qemu-devel] [PATCH 2/3] block: keep BlockBackend alive until device finalize time

[Paolo Bonzini]

On 21/03/2016 16:22, Markus Armbruster wrote:
> Paolo Bonzini <address@hidden> writes:
> 
>> While the next patch will anticipate the death of the DriveInfo
>> data structure, the BlockBackend must survive after unrealize,
>> for example in case there are outstanding operations on it.
>> The good thing is that we can just use reference counting to
>> do it.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  hw/core/qdev-properties-system.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/core/qdev-properties-system.c 
>> b/hw/core/qdev-properties-system.c
>> index 469ba8a..5e84b55 100644
>> --- a/hw/core/qdev-properties-system.c
>> +++ b/hw/core/qdev-properties-system.c
>> @@ -93,6 +93,7 @@ static void parse_drive(DeviceState *dev, const char *str, 
>> void **ptr,
>        if (blk_attach_dev(blk, dev) < 0) {
>            DriveInfo *dinfo = blk_legacy_dinfo(blk);
> 
>            if (dinfo->type != IF_NONE) {
>                error_setg(errp, "Drive '%s' is already in use because "
>                           "it has been automatically connected to another "
>                           "device (did you need 'if=none' in the drive 
> options?)",
>                           str);
>            } else {
>                error_setg(errp, "Drive '%s' is already in use by another 
> device",
>                           str);
>            }
>>          return;
>>      }
>>      *ptr = blk;
>> +    blk_ref(blk);
> 
> blk_attach_dev() already takes a reference.  I'm not sure I understand
> why you need to take a second one.  You say "in case there are
> outstanding operations on it."  What operations could that be?

There could be asynchronous I/O operations which are still active after
unrealize.  The device would not be finalized until they are completed.

> And shouldn't they take their own reference?

Generally the block layer doesn't try to ref/unref on every use.  It
assumes that someone else does it for you.  A better justification for
this patch is that blk_attach_dev/blk_detach_dev actually does not need
to take a reference, so we can add it to parse_drive/release_drive and
remove it from blk_attach_dev/blk_detach_dev instead.

Paolo

> I hasten to add that I'm not going to demand you fix them to take their
> own references.  It's okay to take a hacky second reference here, then
> fix "them" at our leisure.  But I need to understand what exactly this
> second reference protects.  It probably needs to be explained in the
> source, too.
> 
>>  }
>>  
>>  static void release_drive(Object *obj, const char *name, void *opaque)
>> @@ -101,13 +102,17 @@ static void release_drive(Object *obj, const char 
>> *name, void *opaque)
>>      Property *prop = opaque;
>>      BlockBackend **ptr = qdev_get_prop_ptr(dev, prop);
>>  
>> -    if (*ptr && blk_get_attached_dev(*ptr) != NULL) {
>> -        /* Unrealize has already called blk_detach_dev and 
>> blockdev_del_drive
>> -         * if the device has been realized; in that case, 
>> blk_get_attached_dev
>> -         * returns NULL.  Thus, we get here if the device failed to realize,
>> -         * and the -drive must not be released.
>> -         */
>> -        blk_detach_dev(*ptr, dev);
>> +    if (*ptr) {
>> +        if (blk_get_attached_dev(*ptr) != NULL) {
>> +            /* Unrealize has already called blk_detach_dev and
>> +             * blockdev_del_drive if the device has been realized;
>> +             * in that case, blk_get_attached_dev returns NULL.  Thus,
>> +             * we get here if the device failed to realize, and the
>> +             * -drive must not be released.
>> +             */
>> +            blk_detach_dev(*ptr, dev);
>> +        }
>> +        blk_unref(*ptr);
>>      }
>>  }