Re: [Qemu-devel] [PATCH 1/3] block: detach devices from DriveInfo at unrealize time

[Paolo Bonzini]

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