From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38623) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ai1oN-0004pK-E3 for qemu-devel@nongnu.org; Mon, 21 Mar 2016 11:32:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ai1oK-0003jp-76 for qemu-devel@nongnu.org; Mon, 21 Mar 2016 11:32:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55308) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ai1oJ-0003jg-Ve for qemu-devel@nongnu.org; Mon, 21 Mar 2016 11:32:04 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 71C92804F9 for ; Mon, 21 Mar 2016 15:32:03 +0000 (UTC) References: <1456151945-11225-1-git-send-email-pbonzini@redhat.com> <1456151945-11225-2-git-send-email-pbonzini@redhat.com> <87a8lr27b7.fsf@blackfin.pond.sub.org> From: Paolo Bonzini Message-ID: <56F013EF.1000401@redhat.com> Date: Mon, 21 Mar 2016 16:31:59 +0100 MIME-Version: 1.0 In-Reply-To: <87a8lr27b7.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/3] block: detach devices from DriveInfo at unrealize time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , qemu-devel@nongnu.org, Max Reitz On 21/03/2016 16:13, Markus Armbruster wrote: > Meanwhile, commit 12bde0e added block job cancellation: >=20 > block: cancel jobs when a device is ready to go away > =20 > We do not want jobs to keep a device busy for a possibly very long > time, and management could become confused because they thought a > device was not even there anymore. So, cancel long-running jobs > as soon as their device is going to disappear. >=20 > The automatic block job cancellation is an extension of the automatic > deletion wart. We cancel exactly when we schedule the warty deletion. > Note that this made blockdev_mark_auto_del() do more than its name > promises. I never liked that, and always wondered why we don't cancel > in blockdev_auto_del() instead. Because management would fall prey of exactly the bug we're trying to fix. For example by getting a BLOCK_JOB_CANCELLED event for a block device that (according to the earlier DEVICE_DELETED event) should have gone away already. > Instead of delaying the unref to blockdev_auto_del(), which made sense > back when it was a hard delete, you unref right away, leaving > blockdev_auto_del() with nothing to do. Swaps the order of the two > unrefs, but that's just fine. >=20 > We now cancel even when !dinfo, i.e. even when we won't delete. Are yo= u > sure that's correct? If it is, then it needs to be explained in the > commit message. Will avoid that. >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index c427698..0582787 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -945,7 +945,9 @@ static void virtio_blk_device_unrealize(DeviceStat= e *dev, Error **errp) >> s->dataplane =3D NULL; >> qemu_del_vm_change_state_handler(s->change); >> unregister_savevm(dev, "virtio-blk", s); >> - blockdev_mark_auto_del(s->blk); >> + blk_detach_dev(s->blk, dev); >> + blockdev_del_drive(s->blk); >> + s->blk =3D NULL; >> virtio_cleanup(vdev); >> } >=20 > Before your patch, we leave finalization of the property to its > release() callback release_drive(), as we should. All we do here is > schedule warty deletion. And that we must do here, because only here w= e > know that warty deletion is wanted. >=20 > Your patch inserts a copy of release_drive() and hacks it up a bit. Tw= o > hunks down, release_drive() gets hacked up to conditionally avoid > repeating the job. >=20 > This feels rather dirty to me. The other possibility is to make blk_detach_dev do nothing if blk->dev =3D=3D NULL, i.e. make it idempotent. On one hand, who doesn't like idempotency; on the other hand, removing an assertion is also dirty. I chose the easy way here (changing as fewer contracts as possible). >> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c >> index 7bd5bde..39a72e4 100644 >> --- a/hw/block/xen_disk.c >> +++ b/hw/block/xen_disk.c >> @@ -1041,6 +1041,7 @@ static void blk_disconnect(struct XenDevice *xen= dev) >> =20 >> if (blkdev->blk) { >> blk_detach_dev(blkdev->blk, blkdev); >> + blockdev_del_drive(blkdev->blk); >> blk_unref(blkdev->blk); >> blkdev->blk =3D NULL; >> } >=20 > This is a non-qdevified device, where the link to the backend is not a > property, and the link to the backend has to be dismantled by the devic= e > itself. >=20 > I believe inserting blockdev_del_drive() extends the automatic deletion > wart to this device. That's an incompatible change, isn't it? This is why I wanted a careful review. :) I can surely get rid of it. >> diff --git a/hw/ide/piix.c b/hw/ide/piix.c >> index df46147..cf8fa58 100644 >> --- a/hw/ide/piix.c >> +++ b/hw/ide/piix.c >> @@ -182,6 +182,9 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev) >> if (ds) { >> blk_detach_dev(blk, ds); >> } >> + if (pci_ide->bus[di->bus].ifs[di->unit].blk) { >> + blockdev_del_drive(blk); >> + } >> pci_ide->bus[di->bus].ifs[di->unit].blk =3D NULL; >> if (!(i % 2)) { >> idedev =3D pci_ide->bus[di->bus].master; >=20 > Same comment as for xen_disk.c. Same here. >> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c >> index 6dcdbc0..3b2b766 100644 >> --- a/hw/scsi/scsi-bus.c >> +++ b/hw/scsi/scsi-bus.c >> @@ -214,7 +214,9 @@ static void scsi_qdev_unrealize(DeviceState *qdev,= Error **errp) >> } >> =20 >> scsi_device_purge_requests(dev, SENSE_CODE(NO_SENSE)); >> - blockdev_mark_auto_del(dev->conf.blk); >> + blk_detach_dev(dev->conf.blk, qdev); >> + blockdev_del_drive(dev->conf.blk); >> + dev->conf.blk =3D NULL; >> } >> =20 >> /* handle legacy '-drive if=3Dscsi,...' cmd line args */ >=20 > Same comment as for virtio-blk.c. Same answer... >> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c >> index 5ae0424..1c00211 100644 >> --- a/hw/usb/dev-storage.c >> +++ b/hw/usb/dev-storage.c >> @@ -643,7 +643,8 @@ static void usb_msd_realize_storage(USBDevice *dev= , Error **errp) >> * blockdev, or else scsi_bus_legacy_add_drive() dies when it >> * attaches again. >> * >> - * The hack is probably a bad idea. >> + * The hack is probably a bad idea. Anyway, this is why this doe= s not >> + * call blockdev_del_drive. >> */ >> blk_detach_dev(blk, &s->dev.qdev); >> s->conf.blk =3D NULL; >=20 > Note that other qdevified block devices (such as nvme) are *not* > touched. Warty auto deletion is extended only to some, but not all > cases. I wonder if we actually _should_ extend to all of them, i.e. which way is the bug. That would of course change what to do with Xen and IDE. Paolo