[GIT] blkfront warning and crash fixes.

[GIT] blkfront warning and crash fixes.

[Ian Campbell]

Hi Jeremy,

The following changes since commit 384a55c0ac12eabac9531ff3a433598d32a42a15:
  Jeremy Fitzhardinge (1):
        Merge branch 'xen-tip/core' into xen-tip/master

are available in the git repository at:

  git://xenbits.xensource.com/people/ianc/linux-2.6.git for-jeremy/blkfront

Ian Campbell (2):
      xen/blkfront: fix warning when deleting gendisk on unplug/shutdown.
      xen/blkfront: allow xenbus state transition to Closing->Closed when not Connected.

 drivers/block/xen-blkfront.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

[PATCH] xen/blkfront: fix warning when deleting gendisk on unplug/shutdown.

[Ian Campbell]

Currently blkfront gives a warning when hot unplugging due to calling
del_gendisk() with interrupts disabled (due to blkif_io_lock).

WARNING: at kernel/softirq.c:124 local_bh_enable+0x36/0x84()
Modules linked in: xenfs xen_netfront ext3 jbd mbcache xen_blkfront
Pid: 13, comm: xenwatch Not tainted 2.6.29-xs5.5.0.13 #3
Call Trace:
 [<c012611c>] warn_slowpath+0x80/0xb6
 [<c0104cf1>] xen_sched_clock+0x16/0x63
 [<c0104710>] xen_force_evtchn_callback+0xc/0x10
 [<c0104e32>] check_events+0x8/0xe
 [<c0104d9b>] xen_restore_fl_direct_end+0x0/0x1
 [<c0103749>] xen_mc_flush+0x10a/0x13f
 [<c0105bd2>] __switch_to+0x114/0x14e
 [<c011d92b>] dequeue_task+0x62/0x70
 [<c0123b6f>] finish_task_switch+0x2b/0x84
 [<c0299877>] schedule+0x66d/0x6e7
 [<c0104710>] xen_force_evtchn_callback+0xc/0x10
 [<c0104710>] xen_force_evtchn_callback+0xc/0x10
 [<c012a642>] local_bh_enable+0x36/0x84
 [<c022f9a7>] sk_filter+0x57/0x5c
 [<c0233dae>] netlink_broadcast+0x1d5/0x315
 [<c01c6371>] kobject_uevent_env+0x28d/0x331
 [<c01e7ead>] device_del+0x10f/0x120
 [<c01e7ec6>] device_unregister+0x8/0x10
 [<c015f86d>] bdi_unregister+0x2d/0x39
 [<c01bf6f4>] unlink_gendisk+0x23/0x3e
 [<c01ac946>] del_gendisk+0x7b/0xe7
 [<d0828c19>] blkfront_closing+0x28/0x6e [xen_blkfront]
 [<d082900c>] backend_changed+0x3ad/0x41d [xen_blkfront]

We can fix this by calling del_gendisk() later in blkfront_closing, after
releasing blkif_io_lock. Since the queue is stopped during the interrupts
disabled phase I don't think there is any danger of an event occuring between
releasing the blkif_io_lock and deleting the disk.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
---
 drivers/block/xen-blkfront.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 8f90508..aa0c94b 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -934,8 +934,6 @@ static void blkfront_closing(struct xenbus_device *dev)
 
 	spin_lock_irqsave(&blkif_io_lock, flags);
 
-	del_gendisk(info->gd);
-
 	/* No more blkif_request(). */
 	blk_stop_queue(info->rq);
 
@@ -949,6 +947,8 @@ static void blkfront_closing(struct xenbus_device *dev)
 	blk_cleanup_queue(info->rq);
 	info->rq = NULL;
 
+	del_gendisk(info->gd);
+
  out:
 	xenbus_frontend_closed(dev);
 }
-- 
1.5.6.5

[PATCH] xen/blkfront: allow xenbus state transition to Closing->Closed when not Connected.

[Ian Campbell]

This situation can occur when attempting to attach a block device whose backend
is an empty physical CD-ROM driver. The backend in this case will go directly
from the Initialising state to Closing->Closed. Previously this would result in
a NULL pointer deref on info->gd (xenbus_dev_fatal does not return as a1a15ac5
seems to expect)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
---
 drivers/block/xen-blkfront.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index aa0c94b..a6cbf7b 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -977,8 +977,10 @@ static void backend_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateClosing:
-		if (info->gd == NULL)
-			xenbus_dev_fatal(dev, -ENODEV, "gd is NULL");
+		if (info->gd == NULL) {
+			xenbus_frontend_closed(dev);
+			break;
+		}
 		bd = bdget_disk(info->gd, 0);
 		if (bd == NULL)
 			xenbus_dev_fatal(dev, -ENODEV, "bdget failed");
-- 
1.5.6.5

Re: [PATCH] xen/blkfront: allow xenbus state transition to Closing->Closed when not Connected.

[Jeremy Fitzhardinge]

Ian Campbell wrote:
> This situation can occur when attempting to attach a block device whose backend
> is an empty physical CD-ROM driver. The backend in this case will go directly
> from the Initialising state to Closing->Closed. Previously this would result in
> a NULL pointer deref on info->gd (xenbus_dev_fatal does not return as a1a15ac5
> seems to expect)
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
>   

Jens, does this look OK to you?

Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

Thanks,
    J
> ---
>  drivers/block/xen-blkfront.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index aa0c94b..a6cbf7b 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -977,8 +977,10 @@ static void backend_changed(struct xenbus_device *dev,
>  		break;
>  
>  	case XenbusStateClosing:
> -		if (info->gd == NULL)
> -			xenbus_dev_fatal(dev, -ENODEV, "gd is NULL");
> +		if (info->gd == NULL) {
> +			xenbus_frontend_closed(dev);
> +			break;
> +		}
>  		bd = bdget_disk(info->gd, 0);
>  		if (bd == NULL)
>  			xenbus_dev_fatal(dev, -ENODEV, "bdget failed");
>   

Re: [PATCH] xen/blkfront: allow xenbus state transition to Closing->Closed when not Connected.

[Jens Axboe]

On Mon, May 18 2009, Jeremy Fitzhardinge wrote:
> Ian Campbell wrote:
>> This situation can occur when attempting to attach a block device whose backend
>> is an empty physical CD-ROM driver. The backend in this case will go directly
>> from the Initialising state to Closing->Closed. Previously this would result in
>> a NULL pointer deref on info->gd (xenbus_dev_fatal does not return as a1a15ac5
>> seems to expect)
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
>>   
>
> Jens, does this look OK to you?
>
> Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

Looks fine, shall I add it for 2.6.30? Does it need a stable backport?

>>  drivers/block/xen-blkfront.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>> index aa0c94b..a6cbf7b 100644
>> --- a/drivers/block/xen-blkfront.c
>> +++ b/drivers/block/xen-blkfront.c
>> @@ -977,8 +977,10 @@ static void backend_changed(struct xenbus_device *dev,
>>  		break;
>>   	case XenbusStateClosing:
>> -		if (info->gd == NULL)
>> -			xenbus_dev_fatal(dev, -ENODEV, "gd is NULL");
>> +		if (info->gd == NULL) {
>> +			xenbus_frontend_closed(dev);
>> +			break;
>> +		}
>>  		bd = bdget_disk(info->gd, 0);
>>  		if (bd == NULL)
>>  			xenbus_dev_fatal(dev, -ENODEV, "bdget failed");
>>   
>

-- 
Jens Axboe

Re: [PATCH] xen/blkfront: allow xenbus state transition to Closing->Closed when not Connected.

[Jeremy Fitzhardinge]

Jens Axboe wrote:
> On Mon, May 18 2009, Jeremy Fitzhardinge wrote:
>   
>> Ian Campbell wrote:
>>     
>>> This situation can occur when attempting to attach a block device whose backend
>>> is an empty physical CD-ROM driver. The backend in this case will go directly
>>> from the Initialising state to Closing->Closed. Previously this would result in
>>> a NULL pointer deref on info->gd (xenbus_dev_fatal does not return as a1a15ac5
>>> seems to expect)
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
>>>   
>>>       
>> Jens, does this look OK to you?
>>
>> Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>     
>
> Looks fine, shall I add it for 2.6.30? Does it need a stable backport?
>   

Yes please, and for .29.  As far as I know there's been no change to 
blkfront, so it should apply as-is.

There's another patch to go with it that I'm just having a closer look 
at.  I'll probably send it your way shortly.

Thanks,
    J

Re: [PATCH] xen/blkfront: fix warning when deleting gendisk on unplug/shutdown.

[Jeremy Fitzhardinge]

Ian Campbell wrote:
> Currently blkfront gives a warning when hot unplugging due to calling
> del_gendisk() with interrupts disabled (due to blkif_io_lock).
>
> WARNING: at kernel/softirq.c:124 local_bh_enable+0x36/0x84()
> Modules linked in: xenfs xen_netfront ext3 jbd mbcache xen_blkfront
> Pid: 13, comm: xenwatch Not tainted 2.6.29-xs5.5.0.13 #3
> Call Trace:
>  [<c012611c>] warn_slowpath+0x80/0xb6
>  [<c0104cf1>] xen_sched_clock+0x16/0x63
>  [<c0104710>] xen_force_evtchn_callback+0xc/0x10
>  [<c0104e32>] check_events+0x8/0xe
>  [<c0104d9b>] xen_restore_fl_direct_end+0x0/0x1
>  [<c0103749>] xen_mc_flush+0x10a/0x13f
>  [<c0105bd2>] __switch_to+0x114/0x14e
>  [<c011d92b>] dequeue_task+0x62/0x70
>  [<c0123b6f>] finish_task_switch+0x2b/0x84
>  [<c0299877>] schedule+0x66d/0x6e7
>  [<c0104710>] xen_force_evtchn_callback+0xc/0x10
>  [<c0104710>] xen_force_evtchn_callback+0xc/0x10
>  [<c012a642>] local_bh_enable+0x36/0x84
>  [<c022f9a7>] sk_filter+0x57/0x5c
>  [<c0233dae>] netlink_broadcast+0x1d5/0x315
>  [<c01c6371>] kobject_uevent_env+0x28d/0x331
>  [<c01e7ead>] device_del+0x10f/0x120
>  [<c01e7ec6>] device_unregister+0x8/0x10
>  [<c015f86d>] bdi_unregister+0x2d/0x39
>  [<c01bf6f4>] unlink_gendisk+0x23/0x3e
>  [<c01ac946>] del_gendisk+0x7b/0xe7
>  [<d0828c19>] blkfront_closing+0x28/0x6e [xen_blkfront]
>  [<d082900c>] backend_changed+0x3ad/0x41d [xen_blkfront]
>
> We can fix this by calling del_gendisk() later in blkfront_closing, after
> releasing blkif_io_lock. Since the queue is stopped during the interrupts
> disabled phase I don't think there is any danger of an event occuring between
> releasing the blkif_io_lock and deleting the disk.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
>   

Looks OK to me.  Alex Nixon came up with more or less the same patch to 
replace his first failed attempt to fix this.  It looks much better than 
mucking around with interrupts states.

Jens?  How does it look to you?

Thanks,
    J

> ---
>  drivers/block/xen-blkfront.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 8f90508..aa0c94b 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -934,8 +934,6 @@ static void blkfront_closing(struct xenbus_device *dev)
>  
>  	spin_lock_irqsave(&blkif_io_lock, flags);
>  
> -	del_gendisk(info->gd);
> -
>  	/* No more blkif_request(). */
>  	blk_stop_queue(info->rq);
>  
> @@ -949,6 +947,8 @@ static void blkfront_closing(struct xenbus_device *dev)
>  	blk_cleanup_queue(info->rq);
>  	info->rq = NULL;
>  
> +	del_gendisk(info->gd);
> +
>   out:
>  	xenbus_frontend_closed(dev);
>  }
>   

Re: [PATCH] xen/blkfront: fix warning when deleting gendisk on unplug/shutdown.

[Jens Axboe]

On Mon, May 18 2009, Jeremy Fitzhardinge wrote:
> Ian Campbell wrote:
>> Currently blkfront gives a warning when hot unplugging due to calling
>> del_gendisk() with interrupts disabled (due to blkif_io_lock).
>>
>> WARNING: at kernel/softirq.c:124 local_bh_enable+0x36/0x84()
>> Modules linked in: xenfs xen_netfront ext3 jbd mbcache xen_blkfront
>> Pid: 13, comm: xenwatch Not tainted 2.6.29-xs5.5.0.13 #3
>> Call Trace:
>>  [<c012611c>] warn_slowpath+0x80/0xb6
>>  [<c0104cf1>] xen_sched_clock+0x16/0x63
>>  [<c0104710>] xen_force_evtchn_callback+0xc/0x10
>>  [<c0104e32>] check_events+0x8/0xe
>>  [<c0104d9b>] xen_restore_fl_direct_end+0x0/0x1
>>  [<c0103749>] xen_mc_flush+0x10a/0x13f
>>  [<c0105bd2>] __switch_to+0x114/0x14e
>>  [<c011d92b>] dequeue_task+0x62/0x70
>>  [<c0123b6f>] finish_task_switch+0x2b/0x84
>>  [<c0299877>] schedule+0x66d/0x6e7
>>  [<c0104710>] xen_force_evtchn_callback+0xc/0x10
>>  [<c0104710>] xen_force_evtchn_callback+0xc/0x10
>>  [<c012a642>] local_bh_enable+0x36/0x84
>>  [<c022f9a7>] sk_filter+0x57/0x5c
>>  [<c0233dae>] netlink_broadcast+0x1d5/0x315
>>  [<c01c6371>] kobject_uevent_env+0x28d/0x331
>>  [<c01e7ead>] device_del+0x10f/0x120
>>  [<c01e7ec6>] device_unregister+0x8/0x10
>>  [<c015f86d>] bdi_unregister+0x2d/0x39
>>  [<c01bf6f4>] unlink_gendisk+0x23/0x3e
>>  [<c01ac946>] del_gendisk+0x7b/0xe7
>>  [<d0828c19>] blkfront_closing+0x28/0x6e [xen_blkfront]
>>  [<d082900c>] backend_changed+0x3ad/0x41d [xen_blkfront]
>>
>> We can fix this by calling del_gendisk() later in blkfront_closing, after
>> releasing blkif_io_lock. Since the queue is stopped during the interrupts
>> disabled phase I don't think there is any danger of an event occuring between
>> releasing the blkif_io_lock and deleting the disk.
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
>>   
>
> Looks OK to me.  Alex Nixon came up with more or less the same patch to  
> replace his first failed attempt to fix this.  It looks much better than  
> mucking around with interrupts states.
>
> Jens?  How does it look to you?

Yes, I have applied this one as well.

-- 
Jens Axboe