On 21/03/2016 16:13, Markus Armbruster wrote:
> Meanwhile, commit 12bde0e added block job cancellation:
>
> block: cancel jobs when a device is ready to go away
>
> 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.
>
> 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.
>
> We now cancel even when !dinfo, i.e. even when we won't delete. Are you
> 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(DeviceState
>> *dev, Error **errp)
>> s->dataplane = 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 = NULL;
>> virtio_cleanup(vdev);
>> }
>
> 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 we
> know that warty deletion is wanted.
>
> Your patch inserts a copy of release_drive() and hacks it up a bit. Two
> hunks down, release_drive() gets hacked up to conditionally avoid
> repeating the job.
>
> This feels rather dirty to me.
The other possibility is to make blk_detach_dev do nothing if blk->dev
== 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 *xendev)
>>
>> if (blkdev->blk) {
>> blk_detach_dev(blkdev->blk, blkdev);
>> + blockdev_del_drive(blkdev->blk);
>> blk_unref(blkdev->blk);
>> blkdev->blk = NULL;
>> }
>
> 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 device
> itself.
>
> 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 = NULL;
>> if (!(i % 2)) {
>> idedev = pci_ide->bus[di->bus].master;
>
> 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)
>> }
>>
>> 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 = NULL;
>> }
>>
>> /* handle legacy '-drive if=scsi,...' cmd line args */
>
> 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 does not
>> + * call blockdev_del_drive.
>> */
>> blk_detach_dev(blk, &s->dev.qdev);
>> s->conf.blk = NULL;
>
> 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
