Re: [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device

[Denis Plotnikov]

On 13.12.2019 13:32, Kevin Wolf wrote:
> Am 18.11.2019 um 11:50 hat Denis Plotnikov geschrieben:
>>
>> On 10.11.2019 22:08, Denis Plotnikov wrote:
>>> On 10.11.2019 22:03, Denis Plotnikov wrote:
>>>> This allows to change (replace) the file on a block device and is useful
>>>> to workaround exclusive file access restrictions, e.g. to implement VM
>>>> migration with a shared disk stored on some storage with the exclusive
>>>> file opening model: a destination VM is started waiting for incomming
>>>> migration with a fake image drive, and later, on the last migration
>>>> phase, the fake image file is replaced with the real one.
>>>>
>>>> Signed-off-by: Denis Plotnikov <address@hidden>
>>>> ---
>>>>    hw/core/qdev-properties-system.c | 89 +++++++++++++++++++++++++++-----
>>>>    1 file changed, 77 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/core/qdev-properties-system.c
>>>> b/hw/core/qdev-properties-system.c
>>>> index c534590dcd..aaab1370a4 100644
>>>> --- a/hw/core/qdev-properties-system.c
>>>> +++ b/hw/core/qdev-properties-system.c
>>>> @@ -79,8 +79,55 @@ static void set_pointer(Object *obj, Visitor *v,
>>>> Property *prop,
>>>>      /* --- drive --- */
>>>>    -static void do_parse_drive(DeviceState *dev, const char *str, void
>>>> **ptr,
>>>> -                           const char *propname, bool iothread,
>>>> Error **errp)
>>>> +static void do_parse_drive_realized(DeviceState *dev, const char *str,
>>>> +                                    void **ptr, const char *propname,
>>>> +                                    bool iothread, Error **errp)
>>>> +{
>>>> +    BlockBackend *blk = *ptr;
>>>> +    BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);
>>>> +    int ret;
>>>> +    bool blk_created = false;
>>>> +
>>>> +    if (!bs) {
>>>> +        error_setg(errp, "Can't find blockdev '%s'", str);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (!blk) {
>>>> +        AioContext *ctx = iothread ? bdrv_get_aio_context(bs) :
>>>> +                                     qemu_get_aio_context();
>>>> +        blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL);
>>>> +        blk_created = true;
>>> Actually, I have concerns about situation where blk=null.
>>>
>>> Is there any case when scsi-hd (or others) doesn't have a blk assigned
>>> and it's legal?
> No, block devices will always have a BlockBackend, even if it doesn't
> have a root node inserted.
>
>>>> +    } else {
>>>> +        if (blk_bs(blk)) {
>>>> +            blk_remove_bs(blk);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    ret = blk_insert_bs(blk, bs, errp);
>>>> +
>>>> +    if (!ret && blk_created) {
>>>> +        if (blk_attach_dev(blk, dev) < 0) {
>>>> +            /*
>>>> +             * Shouldn't be any errors here since we just created
>>>> +             * the new blk because the device doesn't have any.
>>>> +             * Leave the message here in case blk_attach_dev is changed
>>>> +             */
>>>> +             error_setg(errp, "Can't attach drive '%s' to device '%s'",
>>>> +                        str, object_get_typename(OBJECT(dev)));
>>>> +        } else {
>>>> +            *ptr = blk;
>>>> +        }
>>>> +    }
>> Another problem here, is that the "size" of the device dev may not match
>> after setting a drive.
>> So, we should update it after the drive setting.
>> It was found, that it could be done by calling
>> BlockDevOps.bdrv_parent_cb_resize.
>>
>> But I have some concerns about doing it so. In the case of virtio scsi
>> disk we have the following callstack
>>
>>       bdrv_parent_cb_resize calls() ->
>>           scsi_device_report_change(dev, SENSE_CODE(CAPACITY_CHANGED)) ->
>>               virtio_scsi_change ->
>>                   virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE,
>>                                                       sense.asc |
>> (sense.ascq << 8));
> I think the safest option for now (and which should solve the case you
> want to address) is checking whether old and new size match and
> returning an error otherwise.
>
>> virtio_scsi_change  pushes the event to the guest to make the guest
>> ask for size refreshing.  If I'm not mistaken, here we can get a race
>> condition when some another request is processed with an unchanged
>> size and then the size changing request is processed.
> I think this is actually a problem even without resizing: We need to
> quiesce the device between removing the old root and inserting the new
> one. They way to achieve this is probably by splitting blk_drain() into
> a blk_drain_begin()/end() and then draining the BlockBackend here while
> we're working on it.
>
> Kevin
Why don't we use bdrv_drained_begin/end directly? This is what blk_drain 
does.
If we want to split blk_drain we must keep track if blk's brdv isn't 
change otherwise we can end up with drain_begin one and drain end 
another bdrv if we do remove/insert in between.

Another thing is should we really care about this if we have VM stopped 
and the sizes matched?

Denis
>