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
