Mailing List Archive

On 21/09/12 17:04, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> Backend drivers shouldn't transistion to CLOSED unless the frontend is
> CLOSED. If a backend does transition to CLOSED too soon then the
> frontend may not see the CLOSING state and will not properly shutdown.
>
> So, treat an unexpected backend CLOSED state the same as CLOSING.

Didn't handle the frontend block device being mounted. Updated patch here.

8<---------------------------------
xen-blkfront: handle backend CLOSED without CLOSING

Backend drivers shouldn't transistion to CLOSED unless the frontend is
CLOSED. If a backend does transition to CLOSED too soon then the
frontend may not see the CLOSING state and will not properly shutdown.

So, treat an unexpected backend CLOSED state the same as CLOSING.

If the backend is CLOSED then the frontend can't talk to it so go to
CLOSED immediately without waiting for the block device to be closed
or unmounted.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
drivers/block/xen-blkfront.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 2c2d2e5..c1f5f38 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1143,7 +1143,8 @@ static int blkfront_resume(struct xenbus_device *dev)
}

static void
-blkfront_closing(struct blkfront_info *info)
+blkfront_closing(struct blkfront_info *info,
+ enum xenbus_state backend_state)
{
struct xenbus_device *xbdev = info->xbdev;
struct block_device *bdev = NULL;
@@ -1167,7 +1168,8 @@ blkfront_closing(struct blkfront_info *info)

mutex_lock(&bdev->bd_mutex);

- if (bdev->bd_openers) {
+ /* If the backend is already CLOSED, close now. */
+ if (bdev->bd_openers && backend_state != XenbusStateClosed) {
xenbus_dev_error(xbdev, -EBUSY,
"Device in use; refusing to close");
xenbus_switch_state(xbdev, XenbusStateClosing);
@@ -1340,15 +1342,18 @@ static void blkback_changed(struct xenbus_device *dev,
case XenbusStateReconfiguring:
case XenbusStateReconfigured:
case XenbusStateUnknown:
- case XenbusStateClosed:
break;

case XenbusStateConnected:
blkfront_connect(info);
break;

+ case XenbusStateClosed:
+ if (dev->state == XenbusStateClosed)
+ break;
+ /* Missed the backend's Closing state -- fallthrough */
case XenbusStateClosing:
- blkfront_closing(info);
+ blkfront_closing(info, backend_state);
break;
}
}
--
1.7.2.5

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 25/09/12 18:53, David Vrabel wrote:
> On 21/09/12 17:04, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Backend drivers shouldn't transistion to CLOSED unless the frontend is
>> CLOSED. If a backend does transition to CLOSED too soon then the
>> frontend may not see the CLOSING state and will not properly shutdown.
>>
>> So, treat an unexpected backend CLOSED state the same as CLOSING.
>
> Didn't handle the frontend block device being mounted. Updated patch here.

Konrad, can you ack this updated patch if you're happy with it.

Thanks.

David

> 8<---------------------------------
> xen-blkfront: handle backend CLOSED without CLOSING
>
> Backend drivers shouldn't transistion to CLOSED unless the frontend is
> CLOSED. If a backend does transition to CLOSED too soon then the
> frontend may not see the CLOSING state and will not properly shutdown.
>
> So, treat an unexpected backend CLOSED state the same as CLOSING.
>
> If the backend is CLOSED then the frontend can't talk to it so go to
> CLOSED immediately without waiting for the block device to be closed
> or unmounted.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> drivers/block/xen-blkfront.c | 13 +++++++++----
> 1 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 2c2d2e5..c1f5f38 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1143,7 +1143,8 @@ static int blkfront_resume(struct xenbus_device *dev)
> }
>
> static void
> -blkfront_closing(struct blkfront_info *info)
> +blkfront_closing(struct blkfront_info *info,
> + enum xenbus_state backend_state)
> {
> struct xenbus_device *xbdev = info->xbdev;
> struct block_device *bdev = NULL;
> @@ -1167,7 +1168,8 @@ blkfront_closing(struct blkfront_info *info)
>
> mutex_lock(&bdev->bd_mutex);
>
> - if (bdev->bd_openers) {
> + /* If the backend is already CLOSED, close now. */
> + if (bdev->bd_openers && backend_state != XenbusStateClosed) {
> xenbus_dev_error(xbdev, -EBUSY,
> "Device in use; refusing to close");
> xenbus_switch_state(xbdev, XenbusStateClosing);
> @@ -1340,15 +1342,18 @@ static void blkback_changed(struct xenbus_device *dev,
> case XenbusStateReconfiguring:
> case XenbusStateReconfigured:
> case XenbusStateUnknown:
> - case XenbusStateClosed:
> break;
>
> case XenbusStateConnected:
> blkfront_connect(info);
> break;
>
> + case XenbusStateClosed:
> + if (dev->state == XenbusStateClosed)
> + break;
> + /* Missed the backend's Closing state -- fallthrough */
> case XenbusStateClosing:
> - blkfront_closing(info);
> + blkfront_closing(info, backend_state);
> break;
> }
> }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Mon, Oct 01, 2012 at 06:19:19PM +0100, David Vrabel wrote:
> On 25/09/12 18:53, David Vrabel wrote:
> > On 21/09/12 17:04, David Vrabel wrote:
> >> From: David Vrabel <david.vrabel@citrix.com>
> >>
> >> Backend drivers shouldn't transistion to CLOSED unless the frontend is
> >> CLOSED. If a backend does transition to CLOSED too soon then the
> >> frontend may not see the CLOSING state and will not properly shutdown.
> >>
> >> So, treat an unexpected backend CLOSED state the same as CLOSING.
> >
> > Didn't handle the frontend block device being mounted. Updated patch here.
>
> Konrad, can you ack this updated patch if you're happy with it.

Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Or should I just carry it in my for-jens-3.7 bug-fixes queue and ask
Jen to pull it once rc0 is out?
>
> Thanks.
>
> David
>
> > 8<---------------------------------
> > xen-blkfront: handle backend CLOSED without CLOSING
> >
> > Backend drivers shouldn't transistion to CLOSED unless the frontend is
> > CLOSED. If a backend does transition to CLOSED too soon then the
> > frontend may not see the CLOSING state and will not properly shutdown.
> >
> > So, treat an unexpected backend CLOSED state the same as CLOSING.
> >
> > If the backend is CLOSED then the frontend can't talk to it so go to
> > CLOSED immediately without waiting for the block device to be closed
> > or unmounted.
> >
> > Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> > ---
> > drivers/block/xen-blkfront.c | 13 +++++++++----
> > 1 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 2c2d2e5..c1f5f38 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -1143,7 +1143,8 @@ static int blkfront_resume(struct xenbus_device *dev)
> > }
> >
> > static void
> > -blkfront_closing(struct blkfront_info *info)
> > +blkfront_closing(struct blkfront_info *info,
> > + enum xenbus_state backend_state)
> > {
> > struct xenbus_device *xbdev = info->xbdev;
> > struct block_device *bdev = NULL;
> > @@ -1167,7 +1168,8 @@ blkfront_closing(struct blkfront_info *info)
> >
> > mutex_lock(&bdev->bd_mutex);
> >
> > - if (bdev->bd_openers) {
> > + /* If the backend is already CLOSED, close now. */
> > + if (bdev->bd_openers && backend_state != XenbusStateClosed) {
> > xenbus_dev_error(xbdev, -EBUSY,
> > "Device in use; refusing to close");
> > xenbus_switch_state(xbdev, XenbusStateClosing);
> > @@ -1340,15 +1342,18 @@ static void blkback_changed(struct xenbus_device *dev,
> > case XenbusStateReconfiguring:
> > case XenbusStateReconfigured:
> > case XenbusStateUnknown:
> > - case XenbusStateClosed:
> > break;
> >
> > case XenbusStateConnected:
> > blkfront_connect(info);
> > break;
> >
> > + case XenbusStateClosed:
> > + if (dev->state == XenbusStateClosed)
> > + break;
> > + /* Missed the backend's Closing state -- fallthrough */
> > case XenbusStateClosing:
> > - blkfront_closing(info);
> > + blkfront_closing(info, backend_state);
> > break;
> > }
> > }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

>>> On 25.09.12 at 19:53, David Vrabel <david.vrabel@citrix.com> wrote:
> @@ -1167,7 +1168,8 @@ blkfront_closing(struct blkfront_info *info)
>
> mutex_lock(&bdev->bd_mutex);
>
> - if (bdev->bd_openers) {
> + /* If the backend is already CLOSED, close now. */
> + if (bdev->bd_openers && backend_state != XenbusStateClosed) {
> xenbus_dev_error(xbdev, -EBUSY,
> "Device in use; refusing to close");
> xenbus_switch_state(xbdev, XenbusStateClosing);

This looks wrong to me on a second glance: As long as there
are users of the device, I don't think we want to go into Closed
ourselves, irrespective of the backend state.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 04/10/12 11:14, Jan Beulich wrote:
>>>> On 25.09.12 at 19:53, David Vrabel <david.vrabel@citrix.com> wrote:
>> @@ -1167,7 +1168,8 @@ blkfront_closing(struct blkfront_info *info)
>>
>> mutex_lock(&bdev->bd_mutex);
>>
>> - if (bdev->bd_openers) {
>> + /* If the backend is already CLOSED, close now. */
>> + if (bdev->bd_openers && backend_state != XenbusStateClosed) {
>> xenbus_dev_error(xbdev, -EBUSY,
>> "Device in use; refusing to close");
>> xenbus_switch_state(xbdev, XenbusStateClosing);
>
> This looks wrong to me on a second glance: As long as there
> are users of the device, I don't think we want to go into Closed
> ourselves, irrespective of the backend state.

Any users of the frontend device are screwed either way, as the backend
is gone. It seems sensible to handle this case the same as (e.g.,) a
physical unplug of a USB storage device. Removing the device and
forcing all outstanding I/O to fail immediately rather than lingering in
the rings, going nowhere.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 02/10/12 21:02, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 01, 2012 at 06:19:19PM +0100, David Vrabel wrote:
>> On 25/09/12 18:53, David Vrabel wrote:
>>> On 21/09/12 17:04, David Vrabel wrote:
>>>> From: David Vrabel <david.vrabel@citrix.com>
>>>>
>>>> Backend drivers shouldn't transistion to CLOSED unless the frontend is
>>>> CLOSED. If a backend does transition to CLOSED too soon then the
>>>> frontend may not see the CLOSING state and will not properly shutdown.
>>>>
>>>> So, treat an unexpected backend CLOSED state the same as CLOSING.
>>>
>>> Didn't handle the frontend block device being mounted. Updated patch here.
>>
>> Konrad, can you ack this updated patch if you're happy with it.
>
> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> Or should I just carry it in my for-jens-3.7 bug-fixes queue and ask
> Jen to pull it once rc0 is out?

This seems easiest, if Jan is happy with the patch.

Thanks.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

>>> On 05.10.12 at 13:42, David Vrabel <david.vrabel@citrix.com> wrote:
> On 02/10/12 21:02, Konrad Rzeszutek Wilk wrote:
>> On Mon, Oct 01, 2012 at 06:19:19PM +0100, David Vrabel wrote:
>>> On 25/09/12 18:53, David Vrabel wrote:
>>>> On 21/09/12 17:04, David Vrabel wrote:
>>>>> From: David Vrabel <david.vrabel@citrix.com>
>>>>>
>>>>> Backend drivers shouldn't transistion to CLOSED unless the frontend is
>>>>> CLOSED. If a backend does transition to CLOSED too soon then the
>>>>> frontend may not see the CLOSING state and will not properly shutdown.
>>>>>
>>>>> So, treat an unexpected backend CLOSED state the same as CLOSING.
>>>>
>>>> Didn't handle the frontend block device being mounted. Updated patch here.
>>>
>>> Konrad, can you ack this updated patch if you're happy with it.
>>
>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> Or should I just carry it in my for-jens-3.7 bug-fixes queue and ask
>> Jen to pull it once rc0 is out?
>
> This seems easiest, if Jan is happy with the patch.

I see the point of your explanation yesterday, but am still not
convinced that the early cleanup in spite of active users of the
disk can't cause any problems (like dangling pointers or NULL
dereferences).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 05/10/12 12:55, Jan Beulich wrote:
>>>> On 05.10.12 at 13:42, David Vrabel <david.vrabel@citrix.com> wrote:
>> On 02/10/12 21:02, Konrad Rzeszutek Wilk wrote:
>>> On Mon, Oct 01, 2012 at 06:19:19PM +0100, David Vrabel wrote:
>>>> On 25/09/12 18:53, David Vrabel wrote:
>>>>> On 21/09/12 17:04, David Vrabel wrote:
>>>>>> From: David Vrabel <david.vrabel@citrix.com>
>>>>>>
>>>>>> Backend drivers shouldn't transistion to CLOSED unless the frontend is
>>>>>> CLOSED. If a backend does transition to CLOSED too soon then the
>>>>>> frontend may not see the CLOSING state and will not properly shutdown.
>>>>>>
>>>>>> So, treat an unexpected backend CLOSED state the same as CLOSING.
>>>>>
>>>>> Didn't handle the frontend block device being mounted. Updated patch here.
>>>>
>>>> Konrad, can you ack this updated patch if you're happy with it.
>>>
>>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>
>>> Or should I just carry it in my for-jens-3.7 bug-fixes queue and ask
>>> Jen to pull it once rc0 is out?
>>
>> This seems easiest, if Jan is happy with the patch.
>
> I see the point of your explanation yesterday, but am still not
> convinced that the early cleanup in spite of active users of the
> disk can't cause any problems (like dangling pointers or NULL
> dereferences).

I tested it some more and it is broken. Please apply the first version
instead.

blkfront needs to cancel and complete with an error requests that are
are still on the ring after the backend has disconnected and it needs to
complete with an error all requests submitted while disconnected.
Otherwise, if there is outstanding requests or new requests then
del_gendisk() blocks waiting for the I/O to complete.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Fri, Oct 05, 2012 at 04:57:36PM +0100, David Vrabel wrote:
> On 05/10/12 12:55, Jan Beulich wrote:
> >>>> On 05.10.12 at 13:42, David Vrabel <david.vrabel@citrix.com> wrote:
> >> On 02/10/12 21:02, Konrad Rzeszutek Wilk wrote:
> >>> On Mon, Oct 01, 2012 at 06:19:19PM +0100, David Vrabel wrote:
> >>>> On 25/09/12 18:53, David Vrabel wrote:
> >>>>> On 21/09/12 17:04, David Vrabel wrote:
> >>>>>> From: David Vrabel <david.vrabel@citrix.com>
> >>>>>>
> >>>>>> Backend drivers shouldn't transistion to CLOSED unless the frontend is
> >>>>>> CLOSED. If a backend does transition to CLOSED too soon then the
> >>>>>> frontend may not see the CLOSING state and will not properly shutdown.
> >>>>>>
> >>>>>> So, treat an unexpected backend CLOSED state the same as CLOSING.
> >>>>>
> >>>>> Didn't handle the frontend block device being mounted. Updated patch here.
> >>>>
> >>>> Konrad, can you ack this updated patch if you're happy with it.
> >>>
> >>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>>
> >>> Or should I just carry it in my for-jens-3.7 bug-fixes queue and ask
> >>> Jen to pull it once rc0 is out?
> >>
> >> This seems easiest, if Jan is happy with the patch.
> >
> > I see the point of your explanation yesterday, but am still not
> > convinced that the early cleanup in spite of active users of the
> > disk can't cause any problems (like dangling pointers or NULL
> > dereferences).
>
> I tested it some more and it is broken. Please apply the first version
> instead.

OK, can we do this once more - can you repost the patches, but CC the
invidiual maintainers? The FB and KBD need to go through Florian I think?

>
> blkfront needs to cancel and complete with an error requests that are
> are still on the ring after the backend has disconnected and it needs to
> complete with an error all requests submitted while disconnected.
> Otherwise, if there is outstanding requests or new requests then
> del_gendisk() blocks waiting for the I/O to complete.
>
> David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel