* [PATCH 0/2] fix 'xl block-detach'
@ 2020-09-03 10:05 Paul Durrant
2020-09-03 10:05 ` [PATCH 1/2] xl: implement documented --force option for block-detach Paul Durrant
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Paul Durrant @ 2020-09-03 10:05 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant
From: Paul Durrant <pdurrant@amazon.com>
This series makes it behave as the documentation states it should
Paul Durrant (2):
xl: implement documented --force option for block-detach
libxl: do not automatically force detach of block devices
docs/man/xl.1.pod.in | 4 ++--
tools/libxl/libxl_device.c | 9 ++++++---
tools/xl/xl_block.c | 21 ++++++++++++++++-----
tools/xl/xl_cmdtable.c | 3 ++-
4 files changed, 26 insertions(+), 11 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] xl: implement documented --force option for block-detach
2020-09-03 10:05 [PATCH 0/2] fix 'xl block-detach' Paul Durrant
@ 2020-09-03 10:05 ` Paul Durrant
2020-09-08 14:02 ` Wei Liu
2020-09-03 10:05 ` [PATCH 2/2] libxl: do not automatically force detach of block devices Paul Durrant
2020-09-08 10:52 ` [PATCH 0/2] fix 'xl block-detach' Paul Durrant
2 siblings, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2020-09-03 10:05 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD
From: Paul Durrant <pdurrant@amazon.com>
The manpage for 'xl' documents an option to force a block device to be
released even if the domain to which it is attached does not co-operate.
This option, however, is not implemented. This patch implements the option.
NOTE: The documentation is also adjusted since the normal positioning of
options is before compulsory parameters. It is also noted that use of
the --force option may lead to a guest crash.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
docs/man/xl.1.pod.in | 4 ++--
tools/xl/xl_block.c | 21 ++++++++++++++++-----
tools/xl/xl_cmdtable.c | 3 ++-
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index 52a47a6fbd..5f7d3a7134 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -1389,7 +1389,7 @@ Note that only PV block devices are supported by block-attach.
Requests to attach emulated devices (eg, vdev=hdc) will result in only
the PV view being available to the guest.
-=item B<block-detach> I<domain-id> I<devid> [I<OPTIONS>]
+=item B<block-detach> [I<OPTIONS>] I<domain-id> I<devid>
Detach a domain's virtual block device. I<devid> may be the symbolic
name or the numeric device id given to the device by domain 0. You
@@ -1406,7 +1406,7 @@ B<OPTIONS>
=item B<--force>
If this parameter is specified the device will be forcefully detached, which
-may cause IO errors in the domain.
+may cause IO errors in the domain and possibly a guest crash
=back
diff --git a/tools/xl/xl_block.c b/tools/xl/xl_block.c
index acaf9b96b8..05696643bf 100644
--- a/tools/xl/xl_block.c
+++ b/tools/xl/xl_block.c
@@ -96,12 +96,21 @@ int main_blocklist(int argc, char **argv)
int main_blockdetach(int argc, char **argv)
{
+ static struct option opts[] = {
+ {"force", 0, 0, 'f'},
+ COMMON_LONG_OPTS
+ };
uint32_t domid;
int opt, rc = 0;
libxl_device_disk disk;
-
- SWITCH_FOREACH_OPT(opt, "", NULL, "block-detach", 2) {
- /* No options */
+ bool force = false;
+
+ SWITCH_FOREACH_OPT(opt, "f", opts, "block-detach", 2) {
+ case 'f':
+ force = true;
+ break;
+ default:
+ break;
}
domid = find_domain(argv[optind]);
@@ -110,9 +119,11 @@ int main_blockdetach(int argc, char **argv)
fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]);
return 1;
}
- rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
+ rc = !force ? libxl_device_disk_remove(ctx, domid, &disk, 0) :
+ libxl_device_disk_destroy(ctx, domid, &disk, 0);
if (rc) {
- fprintf(stderr, "libxl_device_disk_remove failed.\n");
+ fprintf(stderr, "libxl_device_disk_%s failed.\n",
+ !force ? "remove" : "destroy");
return 1;
}
libxl_device_disk_dispose(&disk);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 2b8e1b321a..7da6c1b927 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -368,7 +368,8 @@ struct cmd_spec cmd_table[] = {
{ "block-detach",
&main_blockdetach, 0, 1,
"Destroy a domain's virtual block device",
- "<Domain> <DevId>",
+ "[option] <Domain> <DevId>",
+ "-f, --force do not wait for the domain to release the device"
},
{ "vtpm-attach",
&main_vtpmattach, 1, 1,
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] libxl: do not automatically force detach of block devices
2020-09-03 10:05 [PATCH 0/2] fix 'xl block-detach' Paul Durrant
2020-09-03 10:05 ` [PATCH 1/2] xl: implement documented --force option for block-detach Paul Durrant
@ 2020-09-03 10:05 ` Paul Durrant
2020-09-04 8:52 ` Roger Pau Monné
2020-09-08 14:19 ` Wei Liu
2020-09-08 10:52 ` [PATCH 0/2] fix 'xl block-detach' Paul Durrant
2 siblings, 2 replies; 13+ messages in thread
From: Paul Durrant @ 2020-09-03 10:05 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD
From: Paul Durrant <pdurrant@amazon.com>
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@amazon.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] libxl: do not automatically force detach of block devices
2020-09-03 10:05 ` [PATCH 2/2] libxl: do not automatically force detach of block devices Paul Durrant
@ 2020-09-04 8:52 ` Roger Pau Monné
2020-09-04 9:47 ` Paul Durrant
2020-09-08 14:19 ` Wei Liu
1 sibling, 1 reply; 13+ messages in thread
From: Roger Pau Monné @ 2020-09-04 8:52 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD
On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> 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 think we need some special-casing on shutdown that keeps the current
behavior on that case.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 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.
Thanks, Roger.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 2/2] libxl: do not automatically force detach of block devices
2020-09-04 8:52 ` Roger Pau Monné
@ 2020-09-04 9:47 ` Paul Durrant
2020-09-04 13:50 ` Roger Pau Monné
0 siblings, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2020-09-04 9:47 UTC (permalink / raw)
To: 'Roger Pau Monné'
Cc: xen-devel, 'Paul Durrant', 'Ian Jackson',
'Wei Liu', 'Anthony PERARD'
> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 04 September 2020 09:53
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Ian Jackson
> <iwj@xenproject.org>; Wei Liu <wl@xen.org>; Anthony PERARD <anthony.perard@citrix.com>
> 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@amazon.com>
> >
> > 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).
> I think we need some special-casing on shutdown that keeps the current
> behavior on that case.
>
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > ---
> > Cc: Ian Jackson <iwj@xenproject.org>
> > Cc: Wei Liu <wl@xen.org>
> > Cc: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > 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.
Paul
> Thanks, Roger.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] libxl: do not automatically force detach of block devices
2020-09-04 9:47 ` Paul Durrant
@ 2020-09-04 13:50 ` Roger Pau Monné
0 siblings, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2020-09-04 13:50 UTC (permalink / raw)
To: paul
Cc: xen-devel, 'Paul Durrant', 'Ian Jackson',
'Wei Liu', 'Anthony PERARD'
On Fri, Sep 04, 2020 at 10:47:37AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: 04 September 2020 09:53
> > To: Paul Durrant <paul@xen.org>
> > Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Ian Jackson
> > <iwj@xenproject.org>; Wei Liu <wl@xen.org>; Anthony PERARD <anthony.perard@citrix.com>
> > 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@amazon.com>
> > >
> > > 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@amazon.com>
> > > ---
> > > Cc: Ian Jackson <iwj@xenproject.org>
> > > Cc: Wei Liu <wl@xen.org>
> > > Cc: Anthony PERARD <anthony.perard@citrix.com>
> > > ---
> > > 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 0/2] fix 'xl block-detach'
2020-09-03 10:05 [PATCH 0/2] fix 'xl block-detach' Paul Durrant
2020-09-03 10:05 ` [PATCH 1/2] xl: implement documented --force option for block-detach Paul Durrant
2020-09-03 10:05 ` [PATCH 2/2] libxl: do not automatically force detach of block devices Paul Durrant
@ 2020-09-08 10:52 ` Paul Durrant
2020-09-08 13:38 ` Wei Liu
2 siblings, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2020-09-08 10:52 UTC (permalink / raw)
To: wei.liu, 'Ian Jackson', 'Anthony PERARD'; +Cc: xen-devel
Ping? Can I get a toolstack maintainer opinion on this?
Thanks,
Paul
> -----Original Message-----
> From: Paul Durrant <paul@xen.org>
> Sent: 03 September 2020 11:06
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>
> Subject: [PATCH 0/2] fix 'xl block-detach'
>
> From: Paul Durrant <pdurrant@amazon.com>
>
> This series makes it behave as the documentation states it should
>
> Paul Durrant (2):
> xl: implement documented --force option for block-detach
> libxl: do not automatically force detach of block devices
>
> docs/man/xl.1.pod.in | 4 ++--
> tools/libxl/libxl_device.c | 9 ++++++---
> tools/xl/xl_block.c | 21 ++++++++++++++++-----
> tools/xl/xl_cmdtable.c | 3 ++-
> 4 files changed, 26 insertions(+), 11 deletions(-)
>
> --
> 2.20.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] fix 'xl block-detach'
2020-09-08 10:52 ` [PATCH 0/2] fix 'xl block-detach' Paul Durrant
@ 2020-09-08 13:38 ` Wei Liu
0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2020-09-08 13:38 UTC (permalink / raw)
To: paul; +Cc: wei.liu, 'Ian Jackson', 'Anthony PERARD', xen-devel
On Tue, Sep 08, 2020 at 11:52:48AM +0100, Paul Durrant wrote:
> Ping? Can I get a toolstack maintainer opinion on this?
This series landed in my @xen.org inbox just fine but I haven't got
around reviewing it.
Wei.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xl: implement documented --force option for block-detach
2020-09-03 10:05 ` [PATCH 1/2] xl: implement documented --force option for block-detach Paul Durrant
@ 2020-09-08 14:02 ` Wei Liu
0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2020-09-08 14:02 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD
On Thu, Sep 03, 2020 at 11:05:36AM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> The manpage for 'xl' documents an option to force a block device to be
> released even if the domain to which it is attached does not co-operate.
> This option, however, is not implemented. This patch implements the option.
>
> NOTE: The documentation is also adjusted since the normal positioning of
> options is before compulsory parameters. It is also noted that use of
> the --force option may lead to a guest crash.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
> docs/man/xl.1.pod.in | 4 ++--
> tools/xl/xl_block.c | 21 ++++++++++++++++-----
> tools/xl/xl_cmdtable.c | 3 ++-
> 3 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
> index 52a47a6fbd..5f7d3a7134 100644
> --- a/docs/man/xl.1.pod.in
> +++ b/docs/man/xl.1.pod.in
> @@ -1389,7 +1389,7 @@ Note that only PV block devices are supported by block-attach.
> Requests to attach emulated devices (eg, vdev=hdc) will result in only
> the PV view being available to the guest.
>
> -=item B<block-detach> I<domain-id> I<devid> [I<OPTIONS>]
> +=item B<block-detach> [I<OPTIONS>] I<domain-id> I<devid>
>
> Detach a domain's virtual block device. I<devid> may be the symbolic
> name or the numeric device id given to the device by domain 0. You
> @@ -1406,7 +1406,7 @@ B<OPTIONS>
> =item B<--force>
>
> If this parameter is specified the device will be forcefully detached, which
> -may cause IO errors in the domain.
> +may cause IO errors in the domain and possibly a guest crash
Missing "." at the end. This can be fixed while committing.
Acked-by: Wei Liu <wl@xen.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] libxl: do not automatically force detach of block devices
2020-09-03 10:05 ` [PATCH 2/2] libxl: do not automatically force detach of block devices Paul Durrant
2020-09-04 8:52 ` Roger Pau Monné
@ 2020-09-08 14:19 ` Wei Liu
2020-09-14 10:41 ` Roger Pau Monné
1 sibling, 1 reply; 13+ messages in thread
From: Wei Liu @ 2020-09-08 14:19 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD
On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> 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@amazon.com>
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@xenproject.org>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] libxl: do not automatically force detach of block devices
2020-09-08 14:19 ` Wei Liu
@ 2020-09-14 10:41 ` Roger Pau Monné
2020-09-14 12:26 ` Wei Liu
0 siblings, 1 reply; 13+ messages in thread
From: Roger Pau Monné @ 2020-09-14 10:41 UTC (permalink / raw)
To: Wei Liu
Cc: Paul Durrant, xen-devel, Paul Durrant, Ian Jackson, Anthony PERARD
On Tue, Sep 08, 2020 at 02:19:03PM +0000, Wei Liu wrote:
> On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > 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@amazon.com>
>
> 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.
I would be worried about those changes, as we would likely have to
also change libvirt or any other downstreams?
> 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?
Maybe a new function should be introduced instead, that attempts to
remove a device gracefully and fail otherwise?
Then none of the current APIs would change, and xl could use this new
function to handle VBD removal?
Roger.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] libxl: do not automatically force detach of block devices
2020-09-14 10:41 ` Roger Pau Monné
@ 2020-09-14 12:26 ` Wei Liu
2020-09-28 13:43 ` Ian Jackson
0 siblings, 1 reply; 13+ messages in thread
From: Wei Liu @ 2020-09-14 12:26 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Wei Liu, Paul Durrant, xen-devel, Paul Durrant, Ian Jackson,
Anthony PERARD
On Mon, Sep 14, 2020 at 12:41:09PM +0200, Roger Pau Monné wrote:
> On Tue, Sep 08, 2020 at 02:19:03PM +0000, Wei Liu wrote:
> > On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > >
> > > 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@amazon.com>
> >
> > 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.
>
> I would be worried about those changes, as we would likely have to
> also change libvirt or any other downstreams?
>
> > 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?
>
> Maybe a new function should be introduced instead, that attempts to
> remove a device gracefully and fail otherwise?
>
> Then none of the current APIs would change, and xl could use this new
> function to handle VBD removal?
This sounds fine to me.
Wei.
>
> Roger.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] libxl: do not automatically force detach of block devices
2020-09-14 12:26 ` Wei Liu
@ 2020-09-28 13:43 ` Ian Jackson
0 siblings, 0 replies; 13+ messages in thread
From: Ian Jackson @ 2020-09-28 13:43 UTC (permalink / raw)
To: Wei Liu
Cc: Roger Pau Monné,
Paul Durrant, xen-devel, Paul Durrant, Anthony PERARD
Wei Liu writes ("Re: [PATCH 2/2] libxl: do not automatically force detach of block devices"):
> On Mon, Sep 14, 2020 at 12:41:09PM +0200, Roger Pau Monné wrote:
> > Maybe a new function should be introduced instead, that attempts to
> > remove a device gracefully and fail otherwise?
> >
> > Then none of the current APIs would change, and xl could use this new
> > function to handle VBD removal?
>
> This sounds fine to me.
I agree.
If there is going to be different default policy for different devices
it ought to be in xl, not libxl, but frankly I think this is an
anomaly.
I suggest we have a new entrypoint that specifies the fallback
behaviour explicitly. ISTM that there are three possible behaviours:
- fail if the guest does not cooperate
- fall back to force remove
- rip the device out immediately
The last of these would be useful only in rare situations. IDK if the
length of the timeout (for the first two cases) ought to be a
parameter too.
Ian.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-09-28 13:44 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-03 10:05 [PATCH 0/2] fix 'xl block-detach' Paul Durrant
2020-09-03 10:05 ` [PATCH 1/2] xl: implement documented --force option for block-detach Paul Durrant
2020-09-08 14:02 ` Wei Liu
2020-09-03 10:05 ` [PATCH 2/2] libxl: do not automatically force detach of block devices Paul Durrant
2020-09-04 8:52 ` Roger Pau Monné
2020-09-04 9:47 ` Paul Durrant
2020-09-04 13:50 ` Roger Pau Monné
2020-09-08 14:19 ` Wei Liu
2020-09-14 10:41 ` Roger Pau Monné
2020-09-14 12:26 ` Wei Liu
2020-09-28 13:43 ` Ian Jackson
2020-09-08 10:52 ` [PATCH 0/2] fix 'xl block-detach' Paul Durrant
2020-09-08 13:38 ` Wei Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).