On Wed, Sep 05, 2018 at 01:35:15PM +0200, Valentin Vidic wrote:
AFAICT, this will cause the backend to never switch to 'Closed' state until the toolstack sets online to 0, which is not good IMO.
If for example a frontend decides to close a device, the backend will stay in state 'Closing' until the toolstack actually removes the disk by setting online to 0.
This will prevent resetting blk connections, as blkback will refuse to switch to state XenbusStateInitWait unless it's at XenbusStateClosed (see the XenbusStateInitialising case in frontend_changed), which will never be reached with your patch.
Would it be possible to call xen_vbd_free before the state change?
case XenbusStateClosed: xen_blkif_disconnect(be->blkif); xen_vbd_free(&be->blkif->vbd); xenbus_switch_state(dev, XenbusStateClosed);
On Wed, Sep 05, 2018 at 06:28:01PM +0200, Valentin Vidic wrote:
On Wed, Sep 05, 2018 at 01:35:15PM +0200, Valentin Vidic wrote:
AFAICT, this will cause the backend to never switch to 'Closed' state until the toolstack sets online to 0, which is not good IMO.
If for example a frontend decides to close a device, the backend will stay in state 'Closing' until the toolstack actually removes the disk by setting online to 0.
This will prevent resetting blk connections, as blkback will refuse to switch to state XenbusStateInitWait unless it's at XenbusStateClosed (see the XenbusStateInitialising case in frontend_changed), which will never be reached with your patch.
Would it be possible to call xen_vbd_free before the state change?
case XenbusStateClosed: xen_blkif_disconnect(be->blkif); xen_vbd_free(&be->blkif->vbd); xenbus_switch_state(dev, XenbusStateClosed);
I think that will break reconnection, since xen_vbd_create is only called after hotplug script execution is performed (which happens only once at device attachment), but not when DomU changes frontend state.
If you want to perform this xen_vbd_free you will also have to move the xen_vbd_create call AFAICT, to a place that's also called when reconnecting a device. Note that I could be wrong, so it might be worth a shot to try different approaches since the blkback code is quite tangled and I might miss something.
Thanks, Roger.
On Thu, Sep 06, 2018 at 06:29:32PM +0200, Roger Pau Monné wrote:
On Wed, Sep 05, 2018 at 06:28:01PM +0200, Valentin Vidic wrote:
On Wed, Sep 05, 2018 at 01:35:15PM +0200, Valentin Vidic wrote:
AFAICT, this will cause the backend to never switch to 'Closed' state until the toolstack sets online to 0, which is not good IMO.
If for example a frontend decides to close a device, the backend will stay in state 'Closing' until the toolstack actually removes the disk by setting online to 0.
This will prevent resetting blk connections, as blkback will refuse to switch to state XenbusStateInitWait unless it's at XenbusStateClosed (see the XenbusStateInitialising case in frontend_changed), which will never be reached with your patch.
Would it be possible to call xen_vbd_free before the state change?
case XenbusStateClosed: xen_blkif_disconnect(be->blkif); xen_vbd_free(&be->blkif->vbd); xenbus_switch_state(dev, XenbusStateClosed);I think that will break reconnection, since xen_vbd_create is only called after hotplug script execution is performed (which happens only once at device attachment), but not when DomU changes frontend state.
If you want to perform this xen_vbd_free you will also have to move the xen_vbd_create call AFAICT, to a place that's also called when reconnecting a device. Note that I could be wrong, so it might be worth a shot to try different approaches since the blkback code is quite tangled and I might miss something.
It seems like the Closed state is not a good point to call the remove script since the device could go back from Closed to Connected.
Maybe it would help to introduce a new final state (7 = XenbusStateFree or XenbusStateRemove) that would be set after xen_vbd_free to let the userspace know it is safe to run the remove script?
static void xen_blkif_free(struct xen_blkif *blkif) {
WARN_ON(xen_blkif_disconnect(blkif)); xen_vbd_free(&blkif->vbd); xenbus_switch_state(blkif->be->dev, XenbusStateFree); kfree(blkif->be->mode); kfree(blkif->be);
/* Make sure everything is drained before shutting down */ kmem_cache_free(xen_blkif_cachep, blkif); }
On Fri, Sep 07, 2018 at 12:19:29AM +0200, Valentin Vidic wrote:
On Thu, Sep 06, 2018 at 06:29:32PM +0200, Roger Pau Monné wrote:
On Wed, Sep 05, 2018 at 06:28:01PM +0200, Valentin Vidic wrote:
On Wed, Sep 05, 2018 at 01:35:15PM +0200, Valentin Vidic wrote:
AFAICT, this will cause the backend to never switch to 'Closed' state until the toolstack sets online to 0, which is not good IMO.
If for example a frontend decides to close a device, the backend will stay in state 'Closing' until the toolstack actually removes the disk by setting online to 0.
This will prevent resetting blk connections, as blkback will refuse to switch to state XenbusStateInitWait unless it's at XenbusStateClosed (see the XenbusStateInitialising case in frontend_changed), which will never be reached with your patch.
Would it be possible to call xen_vbd_free before the state change?
case XenbusStateClosed: xen_blkif_disconnect(be->blkif); xen_vbd_free(&be->blkif->vbd); xenbus_switch_state(dev, XenbusStateClosed);I think that will break reconnection, since xen_vbd_create is only called after hotplug script execution is performed (which happens only once at device attachment), but not when DomU changes frontend state.
If you want to perform this xen_vbd_free you will also have to move the xen_vbd_create call AFAICT, to a place that's also called when reconnecting a device. Note that I could be wrong, so it might be worth a shot to try different approaches since the blkback code is quite tangled and I might miss something.
It seems like the Closed state is not a good point to call the remove script since the device could go back from Closed to Connected.
Maybe it would help to introduce a new final state (7 = XenbusStateFree or XenbusStateRemove) that would be set after xen_vbd_free to let the userspace know it is safe to run the remove script?
I'm not sure that's a good idea, there are a lot of backends (apart from blkback), and the tools won't know whether a specific backend supports such state or not. Also the current protocol and states are shared between all the Xen PV devices, so new additions should be considered very carefully.
IMO the best options are either calling vbd_free/vbd_create at proper stages in blkback or changing the hotplug script so it waits for the device to have no open clients.
Thanks, Roger.
On Fri, Sep 07, 2018 at 09:15:30AM +0200, Roger Pau Monné wrote:
I'm not sure that's a good idea, there are a lot of backends (apart from blkback), and the tools won't know whether a specific backend supports such state or not. Also the current protocol and states are shared between all the Xen PV devices, so new additions should be considered very carefully.
Sure, I understand.
IMO the best options are either calling vbd_free/vbd_create at proper stages in blkback or changing the hotplug script so it waits for the device to have no open clients.
Changing the block-drbd script would be ideal for me too, but I don't think that piece of DRBD state is exposed at the moment.
On Fri, Sep 07, 2018 at 09:23:19AM +0200, Valentin Vidic wrote:
On Fri, Sep 07, 2018 at 09:15:30AM +0200, Roger Pau Monné wrote:
I'm not sure that's a good idea, there are a lot of backends (apart from blkback), and the tools won't know whether a specific backend supports such state or not. Also the current protocol and states are shared between all the Xen PV devices, so new additions should be considered very carefully.
Sure, I understand.
IMO the best options are either calling vbd_free/vbd_create at proper stages in blkback or changing the hotplug script so it waits for the device to have no open clients.
Changing the block-drbd script would be ideal for me too, but I don't think that piece of DRBD state is exposed at the moment.
Then I'm afraid you will have to look into the vbd_free/create fix.
Thanks, Roger.
On Fri, Sep 07, 2018 at 09:54:55AM +0200, Roger Pau Monné wrote:
Then I'm afraid you will have to look into the vbd_free/create fix.
Yes, here is a first draft for that idea, let me know if you see some problems there:
--- xenbus.c.orig 2018-09-07 12:11:57.798071485 +0200 +++ xenbus.c 2018-09-07 12:14:23.536077992 +0200 @@ -758,6 +759,7 @@ enum xenbus_state frontend_state) { struct backend_info *be = dev_get_drvdata(&dev->dev); + struct block_device *bdev; int err;
pr_debug("%s %p %s\n", __func__, dev, xenbus_strstate(frontend_state)); @@ -772,6 +774,22 @@
case XenbusStateInitialised: case XenbusStateConnected: + if (!be->blkif->vbd.bdev) { + printk("blkdev_get"); + bdev = blkdev_get_by_dev(be->blkif->vbd.pdevice, + be->blkif->vbd.readonly ? + FMODE_READ : FMODE_WRITE, NULL); + + if (IS_ERR(bdev)) { + pr_warn("frontend_changed: device %08x could not be opened\n", + be->blkif->vbd.pdevice); + break; + } + + printk("blkdev_get good"); + be->blkif->vbd.bdev = bdev; + } + /* * Ensure we connect even when two watches fire in * close succession and we miss the intermediate value @@ -808,6 +826,7 @@
case XenbusStateClosed: xen_blkif_disconnect(be->blkif); + xen_vbd_free(&be->blkif->vbd); xenbus_switch_state(dev, XenbusStateClosed); if (xenbus_dev_is_online(dev)) break;
On Fri, Sep 07, 2018 at 12:20:26PM +0200, Valentin Vidic wrote:
On Fri, Sep 07, 2018 at 09:54:55AM +0200, Roger Pau Monné wrote:
Then I'm afraid you will have to look into the vbd_free/create fix.
Yes, here is a first draft for that idea, let me know if you see some problems there:
Thanks!
--- xenbus.c.orig 2018-09-07 12:11:57.798071485 +0200 +++ xenbus.c 2018-09-07 12:14:23.536077992 +0200 @@ -758,6 +759,7 @@ enum xenbus_state frontend_state) { struct backend_info *be = dev_get_drvdata(&dev->dev);
struct block_device *bdev; int err;pr_debug("%s %p %s\n", __func__, dev, xenbus_strstate(frontend_state)); @@ -772,6 +774,22 @@ case XenbusStateInitialised: case XenbusStateConnected:
if (!be->blkif->vbd.bdev) {printk("blkdev_get");bdev = blkdev_get_by_dev(be->blkif->vbd.pdevice,be->blkif->vbd.readonly ?FMODE_READ : FMODE_WRITE, NULL);if (IS_ERR(bdev)) {pr_warn("frontend_changed: device %08x could not be opened\n",be->blkif->vbd.pdevice);break;}printk("blkdev_get good");be->blkif->vbd.bdev = bdev;}
I would prefer if you could avoid open-coding this here, and instead use xen_vbd_create or similar. I would also prefer that the call to xen_vbd_create in backend_changed was removed and we had a single call to xen_vbd_create that's used for both initial device connection and reconnection.
Also, I think this could cause issues if for some reason the frontend switches to state 'Connected' before hotplug scripts have run, in which case you would try to open an unexpected device because pdevice won't be correctly set.
Roger.
On Fri, Sep 07, 2018 at 12:43:09PM +0200, Roger Pau Monné wrote:
I would prefer if you could avoid open-coding this here, and instead use xen_vbd_create or similar. I would also prefer that the call to xen_vbd_create in backend_changed was removed and we had a single call to xen_vbd_create that's used for both initial device connection and reconnection.
Also, I think this could cause issues if for some reason the frontend switches to state 'Connected' before hotplug scripts have run, in which case you would try to open an unexpected device because pdevice won't be correctly set.
Sure, this is just to test if the idea would work and needs a lot of cleanup. Unfortunately it does not seem to help with the original problem because this case is not executed on VM shutdown:
case XenbusStateClosed: xen_blkif_disconnect(be->blkif); xen_vbd_free(&be->blkif->vbd); xenbus_switch_state(dev, XenbusStateClosed);
Instead xen_vbd_free gets run from a different code path after the remove script has already failed:
[ 337.407634] block drbd0: State change failed: Device is held open by someone [ 337.407673] block drbd0: state = { cs:Connected ro:Primary/Secondary ds:UpToDate/UpToDate r----- } [ 337.407713] block drbd0: wanted = { cs:Connected ro:Secondary/Secondary ds:UpToDate/UpToDate r----- } ... [ 340.109459] Workqueue: events xen_blkif_deferred_free [xen_blkback] [ 340.109461] 0000000000000000 ffffffff81331e54 ffff883f84d19d38 ffff883f84d19d32 [ 340.109463] ffffffffc058169e ffff883f84d19d88 ffff883f84d19d20 ffffffffc05816f7 [ 340.109465] ffff883f84d19d88 ffff883f87b5a900 ffffffff81092fea 0000000088ec3080 [ 340.109467] Call Trace: [ 340.109471] [<ffffffff81331e54>] ? dump_stack+0x5c/0x78 [ 340.109473] [<ffffffffc058169e>] ? xen_vbd_free.isra.9+0x2e/0x60 [xen_blkback] [ 340.109475] [<ffffffffc05816f7>] ? xen_blkif_deferred_free+0x27/0x70 [xen_blkback] [ 340.109477] [<ffffffff81092fea>] ? process_one_work+0x18a/0x420 [ 340.109479] [<ffffffff810932cd>] ? worker_thread+0x4d/0x490 [ 340.109480] [<ffffffff81093280>] ? process_one_work+0x420/0x420 [ 340.109482] [<ffffffff81099329>] ? kthread+0xd9/0xf0 [ 340.109484] [<ffffffff81099250>] ? kthread_park+0x60/0x60 [ 340.109486] [<ffffffff81615df7>] ? ret_from_fork+0x57/0x70
linux-stable-mirror@lists.linaro.org
-
Roger Pau Monné -
Valentin Vidic