Re: crash with 2.6.22.1 crash:ll_rw_blk.c blk_remove_plug()

[walter harms <wharms@bfs.de>]

Satyam Sharma wrote:
> On 7/23/07, Jens Axboe <jens.axboe@oracle.com> wrote:
>> On Sun, Jul 22 2007, Satyam Sharma wrote:
>> > Hi Walter,
>> >
>> > Thanks for reporting this.
>> >
>> > On 7/22/07, walter harms <wharms@bfs.de> wrote:
>> >> hello all,
>> >> on my asus notebook tm620 there is a crash with 2.6.22 and 2.6.21
>> >
>> > Did this happen when you were resuming from a suspend-to-ram/disk?
>> > [ I ask because I see swsusp in the trace below, linux-pm added to
>> Cc: ]
>> >
>> >> ....
>> >> Using IPI Shortcut mode
>> >> WARNING: at block/ll_rw_blk.c:1575 blk_remove_plug()
>> >>  [<c01ac87e>] blk_remove_plug+0x36/0x5a
>> >>  [<c01ac8b6>] __generic_unplug_device+0x14/0x1f
>> >>  [<c01ad587>] __make_request+0x39b/0x49c
>> >>  [<c01abc8c>] generic_make_request+0x228/0x255
>> >>  [<c01adb54>] submit_bio+0xa5/0xac
>> >>  [<c013e233>] mempool_alloc+0x37/0xae
>> >>  [<c01314dc>] submit+0xc2/0x11d
>> >>  [<c0131585>] bio_read_page+0x24/0x27
>> >>  [<c013188b>] swsusp_check+0x4f/0xaf
>> >>  [<c012f6c2>] software_resume+0x5f/0x108
>> >>  [<c037867e>] kernel_init+0xb0/0x212
>> >>  [<c0103a16>] ret_from_fork+0x6/0x1c
>> >>  [<c03785ce>] kernel_init+0x0/0x212
>> >>  [<c03785ce>] kernel_init+0x0/0x212
>> >>  [<c010465b>] kernel_thread_helper+0x7/0x10
>> >>  =======================
>> >
>> > Surprising, that's a WARN_ON(!irqs_disabled()) but IRQs are disabled
>> > alright on that codepath. OTOH, __make_request() is heavily
>> goto-driven,
>> > uses the non-save/restore variants of spin_lock_irq, and does not even
>> > balance locks / unlocks for some error paths ... gaah.
>>
>> __make_request() must be called from process context, hence
>> spin_lock_irq() is perfectly already and the fastest way to go. And of
>> course the locking is balanced! So please save your 'gaah's for code
>> you actually took the time to try and understand.
> 
> You're right, I didn't really look at that code for long (it even
> explicitly
> comments about what's going with the locking in there!) sorry about
> that.
> 
> [ Off-topic: BTW does every call to __make_request() end up in
> blk_remove_plug()? Since you're explicitly making the assumption
> that it *must* be called from process context (and hence the use of
> the non-save/restore variants), you could consider putting a
> WARN_ON(irqs_disabled()) over there, and perhaps a WARN_ON
> (!spin_is_locked(queue_lock)) in blk_remove_plug() instead, and
> other such similar functions that currently have the !irqs_disabled
> check. This way you'd effectively cover _both_ the assertions,
> and in appropriate places -- just a suggestion. ]
> 
>> But it does look like unbalanced irq disable/enable calls. I'd guess in
>> the suspend/resume path. Obviously something more esoteric, since this
>> is the first such report for 2.6.22, so like some not-very-used driver
>> for instance.
> 
> Now that I do look at the codepath, it does seem surprising irqs were
> not disabled there. There are a bunch of calls to _other_ functions
> between the spin_lock_irq and the blk_remove_plug via
> __generic_unplug_device that would also have complained about
> !irqs_disabled.
> 
> Walter, does this happen reproducibly?
> 

yes, with 2.6.21 and 2.6.22.1

re,
 wh