From: Kevin Wolf <kwolf@redhat.com>
To: Denis Plotnikov <dplotnikov@virtuozzo.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
"berrange@redhat.com" <berrange@redhat.com>,
"ehabkost@redhat.com" <ehabkost@redhat.com>,
"qemu-block@nongnu.org" <qemu-block@nongnu.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"mreitz@redhat.com" <mreitz@redhat.com>,
"pbonzini@redhat.com" <pbonzini@redhat.com>,
Denis Lunev <den@virtuozzo.com>
Subject: Re: [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device
Date: Fri, 13 Dec 2019 11:32:46 +0100 [thread overview]
Message-ID: <20191213103246.GB3428@localhost.localdomain> (raw)
In-Reply-To: <c38c1301-a28f-baf1-d441-ae8c7205feb1@virtuozzo.com>
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 <dplotnikov@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
next prev parent reply other threads:[~2019-12-13 10:34 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-10 19:03 [PATCH v0 0/2] allow to set 'drive' property on a realized block device Denis Plotnikov
2019-11-10 19:03 ` [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices Denis Plotnikov
2019-11-18 18:54 ` Eduardo Habkost
2019-11-22 11:36 ` Denis Plotnikov
2019-11-25 15:30 ` Eduardo Habkost
2019-11-26 6:49 ` Denis Plotnikov
2019-11-26 16:38 ` Kevin Wolf
2019-11-10 19:03 ` [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device Denis Plotnikov
2019-11-10 19:08 ` Denis Plotnikov
2019-11-18 10:50 ` Denis Plotnikov
2019-12-13 7:30 ` [PING]Re: " Denis Plotnikov
2019-12-13 10:32 ` Kevin Wolf [this message]
2019-12-16 14:51 ` Denis Plotnikov
2019-12-16 15:38 ` Kevin Wolf
2019-12-16 15:58 ` Denis Plotnikov
2019-11-18 10:30 ` [PATCH v0 0/2] " Denis Plotnikov
2020-03-02 13:38 ` Kevin Wolf
2020-03-02 13:55 ` Denis Plotnikov
2020-03-02 15:39 ` Kevin Wolf
2020-03-03 7:43 ` Vladimir Sementsov-Ogievskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191213103246.GB3428@localhost.localdomain \
--to=kwolf@redhat.com \
--cc=berrange@redhat.com \
--cc=den@virtuozzo.com \
--cc=dplotnikov@virtuozzo.com \
--cc=ehabkost@redhat.com \
--cc=mreitz@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).