qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 1/3] block: detach devices from DriveInfo at unrealize time
Date: Mon, 21 Mar 2016 18:19:46 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Paolo Bonzini <address@hidden> writes:

> 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).

Why can't we keep the work in the property release() method
release_drive()?

The only reason blockdev_mark_auto_del() isn't there is that the device
decides whether to call it, not the property.

>>> 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.

Certainly debatable.

Current wart: virtio-blk and SCSI devices auto-delete block backends
with DriveInfo.

Possible alternate wart: all devices auto-delete block backends with
DriveInfo.  This is more regular, but the more regular wart is still a
wart.

I think only if some our users actually expect the alternate wart can we
seriosuly consider switching, because then we have to choose between two
breakages anyway:

* We can stick to the current wart, and leave these users broken.

* We can switch to the alternate wart, unbreak these users, and break
  the users that expect the current wart.

Without further evidence on who expects what, I'd stick to the current
wart.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]