Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1680892 > unrolled thread

[PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device

Back to article view | Back to linux.kernel

Contents

  [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device Vincent Legout <vincent.legout@gandi.net> - 2017-07-04 14:00 +0200
    Re: [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching  device Roger Pau Monné <roger.pau@citrix.com> - 2017-07-04 19:00 +0200
      Re: [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching  device Vincent Legout <vincent.legout@gandi.net> - 2017-07-05 10:10 +0200
        Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent  when detaching device "Jan Beulich" <JBeulich@suse.com> - 2017-07-05 10:20 +0200
          Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when  detaching device Vincent Legout <vincent.legout@gandi.net> - 2017-07-05 14:40 +0200
            Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent  when detaching device "Jan Beulich" <JBeulich@suse.com> - 2017-07-05 15:00 +0200
              Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when  detaching device Vincent Legout <vincent.legout@gandi.net> - 2017-07-05 15:40 +0200
                Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when  detaching device Roger Pau Monné <roger.pau@citrix.com> - 2017-07-07 10:20 +0200

#1680892 — [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device

Devices are not unmounted inside a domU after a xl block-detach.

After xl block-detach, blkfront_closing() is called with state ==
XenbusStateConnected, it detects that the device is still in use and
only switches state to XenbusStateClosing. blkfront_closing() is called
a second time but returns immediately because state ==
XenbusStateClosing. Thus the device keeps being mounted inside the domU.

To fix this, emit a KOBJ_OFFLINE uevent even if the device has users.

With this patch, inside domU, udev has:

KERNEL[16994.526789] offline  /devices/vbd-51728/block/xvdb (block)
KERNEL[16994.796197] remove   /devices/virtual/bdi/202:16 (bdi)
KERNEL[16994.797167] remove   /devices/vbd-51728/block/xvdb (block)
UDEV  [16994.798035] remove   /devices/virtual/bdi/202:16 (bdi)
UDEV  [16994.809429] offline  /devices/vbd-51728/block/xvdb (block)
UDEV  [16994.842365] remove   /devices/vbd-51728/block/xvdb (block)
KERNEL[16995.461991] remove   /devices/vbd-51728 (xen)
UDEV  [16995.462549] remove   /devices/vbd-51728 (xen)

While without, it had:

KERNEL[30.862764] remove   /devices/vbd-51728 (xen)
UDEV  [30.867838] remove   /devices/vbd-51728 (xen)

Signed-off-by: Pascal Bouchareine <pascal@gandi.net>
Signed-off-by: Fatih Acar <fatih.acar@gandi.net>
Signed-off-by: Vincent Legout <vincent.legout@gandi.net>
---
 drivers/block/xen-blkfront.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 39459631667c..da0b0444ee1f 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -2185,8 +2185,10 @@ static void blkfront_closing(struct blkfront_info *info)
 	mutex_lock(&bdev->bd_mutex);
 
 	if (bdev->bd_openers) {
-		xenbus_dev_error(xbdev, -EBUSY,
-				 "Device in use; refusing to close");
+		dev_warn(disk_to_dev(info->gd),
+			 "detaching %s with pending users\n",
+			 xbdev->nodename);
+		kobject_uevent(&disk_to_dev(info->gd)->kobj, KOBJ_OFFLINE);
 		xenbus_switch_state(xbdev, XenbusStateClosing);
 	} else {
 		xlvbd_release_gendisk(info);
-- 
2.13.2

[toc] | [next] | [standalone]

#1681094 — Re: [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device

On Tue, Jul 04, 2017 at 01:48:32PM +0200, Vincent Legout wrote:
> Devices are not unmounted inside a domU after a xl block-detach.
> 
> After xl block-detach, blkfront_closing() is called with state ==
> XenbusStateConnected, it detects that the device is still in use and
> only switches state to XenbusStateClosing. blkfront_closing() is called
> a second time but returns immediately because state ==
> XenbusStateClosing. Thus the device keeps being mounted inside the domU.
> 
> To fix this, emit a KOBJ_OFFLINE uevent even if the device has users.
> 
> With this patch, inside domU, udev has:
> 
> KERNEL[16994.526789] offline  /devices/vbd-51728/block/xvdb (block)
> KERNEL[16994.796197] remove   /devices/virtual/bdi/202:16 (bdi)
> KERNEL[16994.797167] remove   /devices/vbd-51728/block/xvdb (block)
> UDEV  [16994.798035] remove   /devices/virtual/bdi/202:16 (bdi)
> UDEV  [16994.809429] offline  /devices/vbd-51728/block/xvdb (block)
> UDEV  [16994.842365] remove   /devices/vbd-51728/block/xvdb (block)
> KERNEL[16995.461991] remove   /devices/vbd-51728 (xen)
> UDEV  [16995.462549] remove   /devices/vbd-51728 (xen)

I'm not an expect on udev, but aren't those messages duplicated? You
seem to get one message from udev and another one from the kernel.

> While without, it had:
> 
> KERNEL[30.862764] remove   /devices/vbd-51728 (xen)
> UDEV  [30.867838] remove   /devices/vbd-51728 (xen)
> 
> Signed-off-by: Pascal Bouchareine <pascal@gandi.net>
> Signed-off-by: Fatih Acar <fatih.acar@gandi.net>
> Signed-off-by: Vincent Legout <vincent.legout@gandi.net>
>
>  drivers/block/xen-blkfront.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 39459631667c..da0b0444ee1f 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -2185,8 +2185,10 @@ static void blkfront_closing(struct blkfront_info *info)
>  	mutex_lock(&bdev->bd_mutex);
>  
>  	if (bdev->bd_openers) {
> -		xenbus_dev_error(xbdev, -EBUSY,
> -				 "Device in use; refusing to close");
> +		dev_warn(disk_to_dev(info->gd),
> +			 "detaching %s with pending users\n",
> +			 xbdev->nodename);
> +		kobject_uevent(&disk_to_dev(info->gd)->kobj, KOBJ_OFFLINE);

What happens if you simply remove the xenbus_dev_error but don't add
the kobject_uevent?

I'm asking because I don't see any other block device calling
directly kobject_uevent, and I'm sure this should be pretty similar to
what virtio or USB do when a block device is hot-unplugged.

For example blk_unregister_queue already contains a call to trigger a
kobject_uevent.

Thanks, Roger.

[toc] | [prev] | [next] | [standalone]

#1681351 — Re: [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device

On Tue, Jul 04, 2017 at 05:59:27PM +0100, Roger Pau Monné wrote :
> On Tue, Jul 04, 2017 at 01:48:32PM +0200, Vincent Legout wrote:
> > Devices are not unmounted inside a domU after a xl block-detach.
> > 
> > After xl block-detach, blkfront_closing() is called with state ==
> > XenbusStateConnected, it detects that the device is still in use and
> > only switches state to XenbusStateClosing. blkfront_closing() is called
> > a second time but returns immediately because state ==
> > XenbusStateClosing. Thus the device keeps being mounted inside the domU.
> > 
> > To fix this, emit a KOBJ_OFFLINE uevent even if the device has users.
> > 
> > With this patch, inside domU, udev has:
> > 
> > KERNEL[16994.526789] offline  /devices/vbd-51728/block/xvdb (block)
> > KERNEL[16994.796197] remove   /devices/virtual/bdi/202:16 (bdi)
> > KERNEL[16994.797167] remove   /devices/vbd-51728/block/xvdb (block)
> > UDEV  [16994.798035] remove   /devices/virtual/bdi/202:16 (bdi)
> > UDEV  [16994.809429] offline  /devices/vbd-51728/block/xvdb (block)
> > UDEV  [16994.842365] remove   /devices/vbd-51728/block/xvdb (block)
> > KERNEL[16995.461991] remove   /devices/vbd-51728 (xen)
> > UDEV  [16995.462549] remove   /devices/vbd-51728 (xen)
> 
> I'm not an expect on udev, but aren't those messages duplicated? You
> seem to get one message from udev and another one from the kernel.

I'm not either, but this seems to be the expected behavior, at least
that's what I get on a few different setups.

> > While without, it had:
> > 
> > KERNEL[30.862764] remove   /devices/vbd-51728 (xen)
> > UDEV  [30.867838] remove   /devices/vbd-51728 (xen)
> > 
> > Signed-off-by: Pascal Bouchareine <pascal@gandi.net>
> > Signed-off-by: Fatih Acar <fatih.acar@gandi.net>
> > Signed-off-by: Vincent Legout <vincent.legout@gandi.net>
> >
> >  drivers/block/xen-blkfront.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 39459631667c..da0b0444ee1f 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -2185,8 +2185,10 @@ static void blkfront_closing(struct blkfront_info *info)
> >  	mutex_lock(&bdev->bd_mutex);
> >  
> >  	if (bdev->bd_openers) {
> > -		xenbus_dev_error(xbdev, -EBUSY,
> > -				 "Device in use; refusing to close");
> > +		dev_warn(disk_to_dev(info->gd),
> > +			 "detaching %s with pending users\n",
> > +			 xbdev->nodename);
> > +		kobject_uevent(&disk_to_dev(info->gd)->kobj, KOBJ_OFFLINE);
> 
> What happens if you simply remove the xenbus_dev_error but don't add
> the kobject_uevent?

I just tested and I've got the same behavior as before if I do that
(i.e. no unmount inside domU).

> I'm asking because I don't see any other block device calling
> directly kobject_uevent, and I'm sure this should be pretty similar to
> what virtio or USB do when a block device is hot-unplugged.

I don't know if this is the right thing to do, but a call to
kobject_uevent_env was added in xen-blkfront a few months ago:

 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=89515d0255c918e08aa4085956c79bf17615fda5

> For example blk_unregister_queue already contains a call to trigger a
> kobject_uevent.

Without the patch, blkif_release and xlvbd_release_gendisk are never
called, and no call to blk_unregister_queue is made.

blkif_release expects the device to be unused. And calling directly
xlvbd_release_gendisk instead of kobject_uevent seems to block at
del_gendisk while calling invalidate_partition and then fsync_bdev.


Vincent

[toc] | [prev] | [next] | [standalone]

#1681358 — Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device

>>> On 05.07.17 at 10:08, <vincent.legout@gandi.net> wrote:
> Without the patch, blkif_release and xlvbd_release_gendisk are never
> called, and no call to blk_unregister_queue is made.

But isn't that what needs to be fixed then? The device should be
removed once its last user goes away (which would be at the time
the umount is eventually done aiui).

Jan

[toc] | [prev] | [next] | [standalone]

#1681512 — Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device

On Wed, Jul 05, 2017 at 02:17:24AM -0600, Jan Beulich wrote :
> >>> On 05.07.17 at 10:08, <vincent.legout@gandi.net> wrote:
> > Without the patch, blkif_release and xlvbd_release_gendisk are never
> > called, and no call to blk_unregister_queue is made.
> 
> But isn't that what needs to be fixed then? The device should be
> removed once its last user goes away (which would be at the time
> the umount is eventually done aiui).

You mean that block-detach should fail if the device is still mounted?
or find a way to wait until all the users are gone?

I don't say that's not what should be done, but that's not what I get.
The device is removed after a block-detach, even if still mounted. So
the system is left in an unstable state without the patch.

I also just saw the --force option of xl block-detach, but from a quick
look it seems this option was actually only in xm and never in xl.


Vincent

[toc] | [prev] | [next] | [standalone]

#1681518 — Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device

>>> On 05.07.17 at 14:37, <vincent.legout@gandi.net> wrote:
> On Wed, Jul 05, 2017 at 02:17:24AM -0600, Jan Beulich wrote :
>> >>> On 05.07.17 at 10:08, <vincent.legout@gandi.net> wrote:
>> > Without the patch, blkif_release and xlvbd_release_gendisk are never
>> > called, and no call to blk_unregister_queue is made.
>> 
>> But isn't that what needs to be fixed then? The device should be
>> removed once its last user goes away (which would be at the time
>> the umount is eventually done aiui).
> 
> You mean that block-detach should fail if the device is still mounted?
> or find a way to wait until all the users are gone?
> 
> I don't say that's not what should be done, but that's not what I get.
> The device is removed after a block-detach, even if still mounted. So
> the system is left in an unstable state without the patch.

Unstable? I'd expect subsequent I/O to fail for that device, yes, but
that's still a stable system. Are you observing anything else?

Jan

[toc] | [prev] | [next] | [standalone]

#1681538 — Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device

On Wed, Jul 05, 2017 at 06:53:25AM -0600, Jan Beulich wrote :
> >>> On 05.07.17 at 14:37, <vincent.legout@gandi.net> wrote:
> > On Wed, Jul 05, 2017 at 02:17:24AM -0600, Jan Beulich wrote :
> >> >>> On 05.07.17 at 10:08, <vincent.legout@gandi.net> wrote:
> >> > Without the patch, blkif_release and xlvbd_release_gendisk are never
> >> > called, and no call to blk_unregister_queue is made.
> >> 
> >> But isn't that what needs to be fixed then? The device should be
> >> removed once its last user goes away (which would be at the time
> >> the umount is eventually done aiui).
> > 
> > You mean that block-detach should fail if the device is still mounted?
> > or find a way to wait until all the users are gone?
> > 
> > I don't say that's not what should be done, but that's not what I get.
> > The device is removed after a block-detach, even if still mounted. So
> > the system is left in an unstable state without the patch.
> 
> Unstable? I'd expect subsequent I/O to fail for that device, yes, but
> that's still a stable system. Are you observing anything else?

Yes, that's what I meant by unstable, nothing else. Sorry for the
confusion.

Vincent

[toc] | [prev] | [next] | [standalone]

#1683010 — Re: [Xen-devel] [PATCH] xen-blkfront: emit KOBJ_OFFLINE uevent when detaching device

On Wed, Jul 05, 2017 at 03:30:00PM +0200, Vincent Legout wrote:
> On Wed, Jul 05, 2017 at 06:53:25AM -0600, Jan Beulich wrote :
> > >>> On 05.07.17 at 14:37, <vincent.legout@gandi.net> wrote:
> > > On Wed, Jul 05, 2017 at 02:17:24AM -0600, Jan Beulich wrote :
> > >> >>> On 05.07.17 at 10:08, <vincent.legout@gandi.net> wrote:
> > >> > Without the patch, blkif_release and xlvbd_release_gendisk are never
> > >> > called, and no call to blk_unregister_queue is made.
> > >> 
> > >> But isn't that what needs to be fixed then? The device should be
> > >> removed once its last user goes away (which would be at the time
> > >> the umount is eventually done aiui).
> > > 
> > > You mean that block-detach should fail if the device is still mounted?
> > > or find a way to wait until all the users are gone?
> > > 
> > > I don't say that's not what should be done, but that's not what I get.
> > > The device is removed after a block-detach, even if still mounted. So
> > > the system is left in an unstable state without the patch.
> > 
> > Unstable? I'd expect subsequent I/O to fail for that device, yes, but
> > that's still a stable system. Are you observing anything else?
> 
> Yes, that's what I meant by unstable, nothing else. Sorry for the
> confusion.

IMHO, this should behave in the same exact way as hot-unplugging a USB
drive that's mounted, can you confirm that's correct?

Roger.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web