Re: an issue for device hot-unplug

[Igor Mammedov]

On Tue, 4 Apr 2023 18:00:06 +0200
Yu Zhang <yu.zhang@ionos.com> wrote:

> > this patch targets corner case of early boot where
> > guest hasn't initialized ACPI subsystem yet and 'broken'
> > management asking to unplug device too early which leads
> > to device stuck in being unplugged state due to regression
> > in QEMU.
> > However, It doesn't apply to fully booted guest.  
> 
> by adding a few debug lines I see that in
> acpi_pcihp_device_unplug_request_cb(),
> 
>     pdev->qdev.pending_deleted_event = true;
> 
> was executed, which then directly triggered the error in:

If you do repeat unplug request right away after the 1st one,
then getting this error is expected behavior
(as guest needs time to react and unplug device).

> void qmp_device_del(const char *id, Error **errp)
> {
>     DeviceState *dev = find_device_state(id, errp);
>     if (dev != NULL) {
>         if (dev->pending_deleted_event &&
>             (dev->pending_deleted_expires_ms == 0 ||
>              dev->pending_deleted_expires_ms >
> qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL))) {
>             error_setg(errp, "Device %s is already in the "
>                              "process of unplug", id);
>             return;
>         }
> 
>         qdev_unplug(dev, errp);
>     }
> }
> 
> In QEMU code, there are 6 lines where this flag is changed:
> 
> hw/core/qdev.c:564:        dev->pending_deleted_event = false;
> hw/core/qdev.c:601:        dev->pending_deleted_event = true;
> hw/acpi/pcihp.c:219:                    qdev->pending_deleted_event = false;
> hw/acpi/pcihp.c:359:    pdev->qdev.pending_deleted_event = true;
> hw/pci/pcie.c:516:        dev->qdev.pending_deleted_event = false;
> hw/pci/pcie.c:573:    dev->pending_deleted_event = true;
> 
> Considering the complexity of the code, the logic for setting and clearing
> this flag
> seems not quite straightforward. I doubt that the setting of
> pending_deleted_event in
> acpi_pcihp_device_unplug_request_cb() is the appropriate approach to
> accomplish its target.

It's true that pending_deleted_event is abused by failover and later by
pci hotplug.
see comment 15 where Paolo suggest how to fix it
   https://bugzilla.redhat.com/show_bug.cgi?id=1878659#c15

and than see comment 17, explaining that cleaning up pending_deleted_event
usage won't change current behavior.

Anyways, clean up patches are welcome if you wish to follow up
on Paolo's suggestion.

> On Tue, Apr 4, 2023 at 2:25 PM Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Tue, 4 Apr 2023 08:45:54 +0200
> > Jinpu Wang <jinpu.wang@ionos.com> wrote:
> >  
> > > Hi Yu,
> > >
> > > On Mon, Apr 3, 2023 at 6:59 PM Yu Zhang <yu.zhang@ionos.com> wrote:  
> > > >
> > > > Dear Laurent,
> > > >
> > > > Thank you for your quick reply. We used qemu-7.1, but it is  
> > reproducible with qemu from v6.2 to the recent v8.0 release candidates.  
> > > > I found that it's introduced by the commit  9323f892b39 (between  
> > v6.2.0-rc2 and v6.2.0-rc3).  
> > > >
> > > > If it doesn't break anything else, it suffices to remove the line  
> > below from acpi_pcihp_device_unplug_request_cb():  
> > > >
> > > >     pdev->qdev.pending_deleted_event = true;
> > > >
> > > > but you may have a reason to keep it. First of all, I'll open a bug in  
> > the bug tracker and let you know.  
> > > >
> > > > Best regards,
> > > > Yu Zhang  
> > > This patch from Igor Mammedov seems relevant,
> > >  
> > 20230403131833-mutt-send-email-mst@kernel.org/T/#t">https://lore.kernel.org/qemu-devel/20230403131833-mutt-send-email-mst@kernel.org/T/#t
> >
> > this patch targets corner case of early boot where
> > guest hasn't initialized ACPI subsystem yet and 'broken'
> > management asking to unplug device too early which leads
> > to device stuck in being unplugged state due to regression
> > in QEMU.
> > However, It doesn't apply to fully booted guest.
> >
> > [...]
> >  
> > > >> > The purpose is for detecting the end of the PCI device hot-unplug.  
> > However, we feel the  
> > > >> > error confusing. How is it possible that a disk "is already in the  
> > process of unplug"  
> > > >> > during the first hot-unplug attempt? So far as I know, the issue  
> > was also encountered by  
> > > >> > libvirt, but they simply ignored it:
> > > >> >
> > > >> > https://bugzilla.redhat.com/show_bug.cgi?id=1878659
> > > >> > <https://bugzilla.redhat.com/show_bug.cgi?id=1878659>  
> > see my other reply email/BZ comment 17.
> >
> > [...]
> >
> >