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

[Kevin Wolf]

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