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 <dplotni...@virtuozzo.com>
>>>> ---
>>>> 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
>
