Re: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Sep 04, 2020 at 10:47:37AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Sent: 04 September 2020 09:53
> > To: Paul Durrant <paul@xxxxxxx>
> > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant <pdurrant@xxxxxxxxxx>; Ian 
> > Jackson
> > <iwj@xxxxxxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Anthony PERARD 
> > <anthony.perard@xxxxxxxxxx>
> > Subject: Re: [PATCH 2/2] libxl: do not automatically force detach of block 
> > devices
> > 
> > On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@xxxxxxxxxx>
> > >
> > > The manpage for 'xl' documents that guest co-operation is required for a 
> > > (non-
> > > forced) block-detach operation and that it may consequently fail. 
> > > Currently,
> > > however, the implementation of generic device removal means that a 
> > > time-out
> > > of a block-detach is being automatically re-tried with the force flag set
> > > rather than failing. This patch stops such behaviour.
> > 
> > Won't this break cleanup on domain shutdown if the guest doesn't close
> > the devices itself?
> > 
> 
> I don't think so... AFAIK that's a destroy i.e. it's always forced (since 
> there's no way the guest can co-operate at the point).

Urgh, yes, sorry.

> > I think we need some special-casing on shutdown that keeps the current
> > behavior on that case.
> > 
> > >
> > > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> > > ---
> > > Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> > > Cc: Wei Liu <wl@xxxxxxx>
> > > Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > > ---
> > >  tools/libxl/libxl_device.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > > index 0381c5d509..d17ca78848 100644
> > > --- a/tools/libxl/libxl_device.c
> > > +++ b/tools/libxl/libxl_device.c
> > > @@ -1092,7 +1092,8 @@ static void device_backend_callback(libxl__egc 
> > > *egc, libxl__ev_devstate *ds,
> > >
> > >      if (rc == ERROR_TIMEDOUT &&
> > >          aodev->action == LIBXL__DEVICE_ACTION_REMOVE &&
> > > -        !aodev->force) {
> > > +        !aodev->force &&
> > > +        aodev->dev->kind != LIBXL__DEVICE_KIND_VBD) {
> > 
> > Doing this differentiation for block only seems weird, we should treat
> > all devices equally.
> > 
> 
> That is not how things are documented in the xl manpage though; block-detach 
> is the only one to have a force option. I assume that was deliberate.

Oh right, that's weird. I guess removing a block device without guest
cooperation will likely result in a guest crash, while the same it's
not true for other devices.

Thanks, Roger.