Re: spinlock recursion in scsi_end_request() (kernel 2.6.24)

Re: spinlock recursion in scsi_end_request() (kernel 2.6.24)

[Tejun Heo]

Hello,

On 05/20/2010 01:33 PM, Prashant wrote:
> I have a question related to code which is almost same in the
> current kernel.  I don't know whether this is the right mailing list
> for the following question.

linux-scsi would probably fit better (cc'd).

> When a sata drive is unplugged, its corresponding sdev's state is set
> to SDEV_OFFLINE. Now if IO requests are still comming on the same device,
> They will be killed by calling scsi_kill_request().
> 
> 1) scsi_kill_request does following things:
>     i) Unlock request queue
>     ii) Increment host_busy count
>     iii) Lock request queue
>     iv) Calls __scsi_done()
> 
> 2) __scsi_done() does following things:
>      i) set request completion data
>      ii) Calls blk_completion_request()
> 
> 3) blk_completion_request() does following things:
>      i) Adds request->donelist to blk_cpu_done softirq queue
>         and raise the softirq (which is scsi_softirq_done)
> 
> 4) next sequence is:
>     scsi_softirq_done >> scsi_finish_command >> scsi_device_unbusy()
> 
> 5) scsi_device_unbusy() again locks the request_queue. This is the place where
>     we can get into the spinlock recursion.
> 
>     Is this correct? Please correct me if something is wrong.

Raising softirq defers the work to another context and grabbing the
same lock from softirq handler doesn't constitute a recursive locking.
Please try to reproduce the problem on recent kernel w/ lockdep
enabled.

Thanks.

-- 
tejun

Re: spinlock recursion in scsi_end_request() (kernel 2.6.24)

[James Bottomley]

On Thu, 2010-05-20 at 17:05 +0200, Tejun Heo wrote:
> Hello,
> 
> On 05/20/2010 01:33 PM, Prashant wrote:
> > I have a question related to code which is almost same in the
> > current kernel.  I don't know whether this is the right mailing list
> > for the following question.
> 
> linux-scsi would probably fit better (cc'd).
> 
> > When a sata drive is unplugged, its corresponding sdev's state is set
> > to SDEV_OFFLINE. Now if IO requests are still comming on the same device,
> > They will be killed by calling scsi_kill_request().
> > 
> > 1) scsi_kill_request does following things:
> >     i) Unlock request queue
> >     ii) Increment host_busy count
> >     iii) Lock request queue
> >     iv) Calls __scsi_done()
> > 
> > 2) __scsi_done() does following things:
> >      i) set request completion data
> >      ii) Calls blk_completion_request()
> > 
> > 3) blk_completion_request() does following things:
> >      i) Adds request->donelist to blk_cpu_done softirq queue
> >         and raise the softirq (which is scsi_softirq_done)
> > 
> > 4) next sequence is:
> >     scsi_softirq_done >> scsi_finish_command >> scsi_device_unbusy()
> > 
> > 5) scsi_device_unbusy() again locks the request_queue. This is the place where
> >     we can get into the spinlock recursion.
> > 
> >     Is this correct? Please correct me if something is wrong.
> 
> Raising softirq defers the work to another context and grabbing the
> same lock from softirq handler doesn't constitute a recursive locking.
> Please try to reproduce the problem on recent kernel w/ lockdep
> enabled.

Just to confirm what Tejun says: the design of the cmd -> done (i.e.
scsi_done) going through the block sofirq handler is specifically so it
can be called either locked or unlocked, so this can never be a
recursion.

James

Re: spinlock recursion in scsi_end_request() (kernel 2.6.24)

[Prashant]

On Thu, May 20, 2010 at 8:59 PM, James Bottomley
<James.Bottomley@suse.de> wrote:
> On Thu, 2010-05-20 at 17:05 +0200, Tejun Heo wrote:
>> Hello,
>>
>> On 05/20/2010 01:33 PM, Prashant wrote:
>> > I have a question related to code which is almost same in the
>> > current kernel.  I don't know whether this is the right mailing list
>> > for the following question.
>>
>> linux-scsi would probably fit better (cc'd).
>>
>> > When a sata drive is unplugged, its corresponding sdev's state is set
>> > to SDEV_OFFLINE. Now if IO requests are still comming on the same device,
>> > They will be killed by calling scsi_kill_request().
>> >
>> > 1) scsi_kill_request does following things:
>> >     i) Unlock request queue
>> >     ii) Increment host_busy count
>> >     iii) Lock request queue
>> >     iv) Calls __scsi_done()
>> >
>> > 2) __scsi_done() does following things:
>> >      i) set request completion data
>> >      ii) Calls blk_completion_request()
>> >
>> > 3) blk_completion_request() does following things:
>> >      i) Adds request->donelist to blk_cpu_done softirq queue
>> >         and raise the softirq (which is scsi_softirq_done)
>> >
>> > 4) next sequence is:
>> >     scsi_softirq_done >> scsi_finish_command >> scsi_device_unbusy()
>> >
>> > 5) scsi_device_unbusy() again locks the request_queue. This is the place where
>> >     we can get into the spinlock recursion.
>> >
>> >     Is this correct? Please correct me if something is wrong.
>>
>> Raising softirq defers the work to another context and grabbing the
>> same lock from softirq handler doesn't constitute a recursive locking.
>> Please try to reproduce the problem on recent kernel w/ lockdep
>> enabled.
>
> Just to confirm what Tejun says: the design of the cmd -> done (i.e.
> scsi_done) going through the block sofirq handler is specifically so it
> can be called either locked or unlocked, so this can never be a
> recursion.
>
> James
>
Thanks for the explanation. I will let you know if this problem is reproduced
with the latest kernel.

Thanks,
Prashant
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html