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

[Wei Liu <wl@xxxxxxx>]

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.
> 
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>

I'm two-minded here.

On the one hand, special-casing VBD in libxl to conform to xl's
behaviour seems wrong to me.

On the other hand, if we want to treat all devices the same in libxl,
libxl should drop its force-removal behaviour all together, and the
retry behaviour would need to be implemented in xl for all devices
excepts for VBD. This requires a bit of code churn and, more
importantly, changes how those device removal APIs behave. In the end
this effort may not worth it.

If we go with the patch here, we should document this special case on
libxl level somehow.

Anthony and Ian, do you have any opinion on this?

Wei.

> ---
> 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) {
>          LOGD(DEBUG, aodev->dev->domid, "Timeout reached, initiating forced 
> remove");
>          aodev->force = 1;
>          libxl__initiate_device_generic_remove(egc, aodev);
> @@ -1103,7 +1104,8 @@ static void device_backend_callback(libxl__egc *egc, 
> libxl__ev_devstate *ds,
>          LOGD(ERROR, aodev->dev->domid, "unable to %s device with path %s",
>                      libxl__device_action_to_string(aodev->action),
>                      libxl__device_backend_path(gc, aodev->dev));
> -        goto out;
> +        if (!aodev->force)
> +            goto out;
>      }
>  
>      device_hotplug(egc, aodev);
> @@ -1319,7 +1321,8 @@ static void device_hotplug_done(libxl__egc *egc, 
> libxl__ao_device *aodev)
>      device_hotplug_clean(gc, aodev);
>  
>      /* Clean xenstore if it's a disconnection */
> -    if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE) {
> +    if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE &&
> +        (aodev->force || !aodev->rc)) {
>          rc = libxl__device_destroy(gc, aodev->dev);
>          if (!aodev->rc)
>              aodev->rc = rc;
> -- 
> 2.20.1
>