diff mbox

[1/1] iscsi: fix regression caused by session lock patch

Message ID 1447304741-6594-1-git-send-email-mchristi@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mike Christie Nov. 12, 2015, 5:05 a.m. UTC
From: Mike Christie <mchristi@redhat.com>

This patch fixes this oops report by Guilherme Piccol:

list_del corruption. prev->next should be c000000f3da2b080, but was c000000f3da2c080
------------[ cut here ]------------
WARNING: at lib/list_debug.c:59
CPU: 48 PID: 12033 Comm: fio-2.2.7 Not tainted 3.18.22-354.el7_1.pkvm3_1_0.3600.1.ppc64le #1
task: c000000f25827000 ti: c000000fff5f8000 task.ti: c000000f057e4000
NIP: c0000000004d15a4 LR: c0000000004d15a0 CTR: c0000000004bdfd0
REGS: c000000fff5faf10 TRAP: 0700   Not tainted (3.18.22-354.el7_1.pkvm3_1_0.3600.1.ppc64le)
MSR: 9000000000029033 <SF,HV,EE,ME,IR,DR,RI,LE>  CR: 28082842  XER: 20000000
CFAR: c000000000988dbc SOFTE: 1
NIP [c0000000004d15a4] __list_del_entry+0xb4/0xe0
LR [c0000000004d15a0] __list_del_entry+0xb0/0xe0
Call Trace:
[c000000fff5fb190] [c0000000004d15a0] __list_del_entry+0xb0/0xe0 (unreliable)
[c000000fff5fb1f0] [d000000010fb0c8c] iscsi_complete_task+0x8c/0x190 [libiscsi]
[c000000fff5fb270] [d000000010fb24c8] __iscsi_complete_pdu+0x508/0xaa0 [libiscsi]
[c000000fff5fb350] [d000000010fb2ab4] iscsi_complete_pdu+0x54/0xa0 [libiscsi]
[c000000fff5fb3a0] [d0000000110121ac] iscsi_tcp_hdr_recv_done+0x38c/0x1250 [libiscsi_tcp]
[c000000fff5fb480] [d000000011011a48] iscsi_tcp_recv_skb+0xd8/0x4b0 [libiscsi_tcp]
[c000000fff5fb570] [d000000011071708] iscsi_sw_tcp_recv+0x98/0x170 [iscsi_tcp]
[c000000fff5fb610] [c000000000882794] tcp_read_sock+0xe4/0x2d0
[c000000fff5fb680] [d000000011071590] iscsi_sw_tcp_data_ready+0x70/0x150 [iscsi_tcp]
[c000000fff5fb720] [c0000000008921c8] tcp_rcv_established+0x3f8/0x6f0
[c000000fff5fb780] [c00000000089d75c] tcp_v4_do_rcv+0x19c/0x420
[c000000fff5fb7e0] [c0000000008a15bc] tcp_v4_rcv+0xa6c/0xb20
[c000000fff5fb8c0] [c0000000008700f8] ip_local_deliver_finish+0x178/0x350
[c000000fff5fb910] [c000000000870424] ip_rcv_finish+0x154/0x420
[c000000fff5fb990] [c000000000816bf4] __netif_receive_skb_core+0x7a4/0x9c0
[c000000fff5fba80] [c00000000081a0fc] netif_receive_skb_internal+0x4c/0xf0
[c000000fff5fbac0] [c00000000081b17c] napi_gro_receive+0xfc/0x1a0
[c000000fff5fbb00] [d000000008373280] bnx2x_rx_int+0xb30/0x1650 [bnx2x]
[c000000fff5fbc90] [d000000008374440] bnx2x_poll+0x310/0x440 [bnx2x]
[c000000fff5fbd40] [c00000000081a8e8] net_rx_action+0x2d8/0x3e0
[c000000fff5fbe10] [c0000000000cfc44] __do_softirq+0x174/0x3a0
[c000000fff5fbf00] [c0000000000d01f8] irq_exit+0xc8/0x100
[c000000fff5fbf20] [c000000000010d40] __do_irq+0xa0/0x1c0
[c000000fff5fbf90] [c000000000024aac] call_do_irq+0x14/0x24
[c000000f057e7dd0] [c000000000010f00] do_IRQ+0xa0/0x120
[c000000f057e7e30] [c0000000000025e8] hardware_interrupt_common+0x168/0x180
Instruction dump:
0fe00000 4bffffd8 3c62ff91 386304c0 484b77ad 60000000 0fe00000 4bffffc0
 3c62ff91 38630480 484b7795 60000000 <0fe00000> 4bffffa8 3c62ff91 7d254b78
---[ end trace 005d2749e6bab12f ]---

The bug is caused by this patch:

659743b02c411075b26601725947b21df0bb29c8

which allowed the task lists to be manipulated under different locks
in the xmit and completion path.

To fix the oops this patch just reverts that patch. It also reverts
these 2 patches for regressions that were also a result of that patch:

72b9740201d5f0e24b0b8326a4949786a30ff628
35843048e7e979df3b7b9f2ad49e21797a11386b

Cc: Guilherme  Piccoli <gpiccoli@linux.vnet.ibm.com>
Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/scsi/be2iscsi/be_main.c  |  26 ++---
 drivers/scsi/bnx2i/bnx2i_hwi.c   |  46 ++++-----
 drivers/scsi/bnx2i/bnx2i_iscsi.c |  10 +-
 drivers/scsi/iscsi_tcp.c         |  22 ++--
 drivers/scsi/libiscsi.c          | 214 +++++++++++++++++----------------------
 drivers/scsi/libiscsi_tcp.c      |  28 ++---
 drivers/scsi/qla4xxx/ql4_isr.c   |   4 +-
 include/scsi/libiscsi.h          |  17 +---
 include/scsi/libiscsi_tcp.h      |   2 -
 9 files changed, 161 insertions(+), 208 deletions(-)

Comments

Sagi Grimberg Nov. 12, 2015, 12:03 p.m. UTC | #1
> The bug is caused by this patch:
>
> 659743b02c411075b26601725947b21df0bb29c8
>
> which allowed the task lists to be manipulated under different locks
> in the xmit and completion path.
>
> To fix the oops this patch just reverts that patch. It also reverts
> these 2 patches for regressions that were also a result of that patch:

Whoa now Mike, this is a major change. Can we take a step back and think
about this for a second?

My understanding is that the kfifo circular buffer design allows a
writer (e.g. the producer) and a reader (e.g. the consumer) to avoid
extra locking when accessing the kfifo buffer.

 From include/linux/kfifo.h:
/*
  * Note about locking : There is no locking required until only * one 
reader
  * and one writer is using the fifo and no kfifo_reset() will be * called
  *  kfifo_reset_out() can be safely used, until it will be only called
  * in the reader thread.
  *  For multiple writer and one reader there is only a need to lock the 
writer.
  * And vice versa for only one writer and multiple reader there is only 
a need
  * to lock the reader.
  */

The patch by Shlomo, implements the locking policy documented above:
- multiple readers: multiple threads entering queuecommand reading the
   kfifo (kfifo_out) are mutually excluded by the frwd_lock.
- multiple writers: completion contexts writing to the kfifo (kfifo_in)
   are mutually excluded by the back_lock.

Can you provide a more detailed analysis of why is this list corruption
triggered? What scenario was not honoring the locking policy?
This code has been running reliably in our labs for a long time now
(both iser and tcp).

Going back to a single lock restores the contention point between
queuecommand and complete_pdu.


P.S.
While we're on the subject, I'd like to see block tags replace the
kfifo for iscsi tasks. It's difficult because sw and offload drivers
map hosts/sessions differently. It can be done if we move the tasks to
the iscsi_host and have a reserved (unique) task tags for the session
login/logout/nop_in/nop_out. With this we won't need locks around our
tasks arrays.

Sagi.
--
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
Mike Christie Nov. 12, 2015, 8:58 p.m. UTC | #2
On 11/12/2015 06:03 AM, Sagi Grimberg wrote:
> 
>> The bug is caused by this patch:
>>
>> 659743b02c411075b26601725947b21df0bb29c8
>>
>> which allowed the task lists to be manipulated under different locks
>> in the xmit and completion path.
>>
>> To fix the oops this patch just reverts that patch. It also reverts
>> these 2 patches for regressions that were also a result of that patch:
> 
> Whoa now Mike, this is a major change. Can we take a step back and think
> about this for a second?

The issue has been on the open-iscsi list for a week! You are on the
list still right? Or is even ccd on the thread.

> 
> My understanding is that the kfifo circular buffer design allows a
> writer (e.g. the producer) and a reader (e.g. the consumer) to avoid
> extra locking when accessing the kfifo buffer.
> 

For the next feature window I am working on patch that makes the api
easier to use (the cleanup_task locking is bad as can be seen from the
bnx2i regression the patch also caused) and also uses kfifos for the
fast path.


> From include/linux/kfifo.h:
> /*
>  * Note about locking : There is no locking required until only * one
> reader
>  * and one writer is using the fifo and no kfifo_reset() will be * called
>  *  kfifo_reset_out() can be safely used, until it will be only called
>  * in the reader thread.
>  *  For multiple writer and one reader there is only a need to lock the
> writer.
>  * And vice versa for only one writer and multiple reader there is only
> a need
>  * to lock the reader.
>  */
> 
> The patch by Shlomo, implements the locking policy documented above:
> - multiple readers: multiple threads entering queuecommand reading the
>   kfifo (kfifo_out) are mutually excluded by the frwd_lock.
> - multiple writers: completion contexts writing to the kfifo (kfifo_in)
>   are mutually excluded by the back_lock.
> 
> Can you provide a more detailed analysis of why is this list corruption
> triggered? What scenario was not honoring the locking policy?

Basic locking around a linked list bug. iscsi_queuecommand adds it under
the frwd lock and iscsi_complete_task was taking it off the back_lock.


> This code has been running reliably in our labs for a long time now
> (both iser and tcp).

The patch has caused multiple regressions, did not even compile when
sent to me, and was poorly reviewed and I have not heard from you guys
in a week. Given the issues the patch has had and the current time, I do
not feel comfortable with it anymore. I want to re-review it and fix it
up when there is more time.

--
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
Chris Leech Nov. 12, 2015, 9:33 p.m. UTC | #3
On Thu, Nov 12, 2015 at 4:03 AM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
>
>> The bug is caused by this patch:
>>
>> 659743b02c411075b26601725947b21df0bb29c8
>>
>> which allowed the task lists to be manipulated under different locks
>> in the xmit and completion path.
>>
>> To fix the oops this patch just reverts that patch. It also reverts
>> these 2 patches for regressions that were also a result of that patch:
>
>
> Whoa now Mike, this is a major change. Can we take a step back and think
> about this for a second?
>
> My understanding is that the kfifo circular buffer design allows a
> writer (e.g. the producer) and a reader (e.g. the consumer) to avoid
> extra locking when accessing the kfifo buffer.

I don't think the problem is with the kfifo cmdpool, but rather the
connection mgmtqueue/cmdqueue linked lists.   iscsi_task structs are linked
in using the task->running list_head.

- Chris
--
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
Or Gerlitz Nov. 13, 2015, 3:06 p.m. UTC | #4
On Thu, Nov 12, 2015 at 10:58 PM, Mike Christie <michaelc@cs.wisc.edu> wrote:
> On 11/12/2015 06:03 AM, Sagi Grimberg wrote:
>>> The bug is caused by this patch:
>>>
>>> 659743b02c411075b26601725947b21df0bb29c8
>>>
>>> which allowed the task lists to be manipulated under different locks
>>> in the xmit and completion path.

>>> To fix the oops this patch just reverts that patch. It also reverts
>>> these 2 patches for regressions that were also a result of that patch:

>> Whoa now Mike, this is a major change. Can we take a step back and think
>> about this for a second?

> The issue has been on the open-iscsi list for a week! You are on the
> list still right? Or is even ccd on the thread.

The email you sent me a week ago also cc-ed open-iscsi hence was
routed by a rule in my mailer that made it to land in my open-iscsi
subscription folder which is don't visit much nowadays. Only when you
posted to linux-scsi we saw that and responded within few hours, to
begin with.

>> Can you provide a more detailed analysis of why is this list corruption
>> triggered? What scenario was not honoring the locking policy?

> Basic locking around a linked list bug. iscsi_queuecommand adds it under
> the frwd lock and iscsi_complete_task was taking it off the back_lock.

>> This code has been running reliably in our labs for a long time now
>> (both iser and tcp).

> The patch has caused multiple regressions, did not even compile when
> sent to me, and was poorly reviewed and I have not heard from you guys
> in a week. Given the issues the patch has had and the current time, I do
> not feel comfortable with it anymore. I want to re-review it and fix it
> up when there is more time.

Mike (Hi),

It's a complex patch that touches all the iscsi transports, and yes,
when it was send to you the 1st time, there was build error on one of
the offload transports (bad! but happens) and yes, as you pointed, one
static checker fix + one bug fix for it went upstream after this has
been merged, happens too.

What makes you say it was poorly reviewed?

And now after few years in upstream a possibly real bug was found
(happens), why rush and revert? lets see if we can fix.

Or.
--
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
Mike Christie Nov. 13, 2015, 4:51 p.m. UTC | #5
On 11/13/2015 09:06 AM, Or Gerlitz wrote:
>> The patch has caused multiple regressions, did not even compile when
>> > sent to me, and was poorly reviewed and I have not heard from you guys
>> > in a week. Given the issues the patch has had and the current time, I do
>> > not feel comfortable with it anymore. I want to re-review it and fix it
>> > up when there is more time.
> Mike (Hi),
> 
> It's a complex patch that touches all the iscsi transports, and yes,
> when it was send to you the 1st time, there was build error on one of
> the offload transports (bad! but happens) and yes, as you pointed, one
> static checker fix + one bug fix for it went upstream after this has
> been merged, happens too.

A patch should not cause this many issues.

> What makes you say it was poorly reviewed?

I just did not do a good job at looking at the patch. I should have
caught all of these issues.

- The bnx2i cleanup_task bug should have been obvious, especially for me
because I had commented about the back lock and the abort path.

- This oops, was so basic. Incorrect locking around a linked list being
accessed from 2 threads is really one of those 1st year kernel
programmer things.

- The iscsi_xmit_task static checker locking was really simple too.

- There was the issue offlist I emailed you guys about where I started
to try and fix/review it yesterday, and I found another race in the
abort and completion path that can result in a null pointer reference.

- I have not had time to fully review it again, but I think the the
reset/recovery code looks shady too.

> 
> And now after few years in upstream a possibly real bug was found
> (happens), why rush and revert? lets see if we can fix.

Send patches.
--
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
Or Gerlitz Nov. 15, 2015, 10:10 a.m. UTC | #6
On Fri, Nov 13, 2015 at 6:51 PM, Mike Christie <michaelc@cs.wisc.edu> wrote:
> On 11/13/2015 09:06 AM, Or Gerlitz wrote:
>>> The patch has caused multiple regressions, did not even compile when
>>> > sent to me, and was poorly reviewed and I have not heard from you guys
>>> > in a week. Given the issues the patch has had and the current time, I do
>>> > not feel comfortable with it anymore. I want to re-review it and fix it
>>> > up when there is more time.
>> Mike (Hi),
>>
>> It's a complex patch that touches all the iscsi transports, and yes,
>> when it was send to you the 1st time, there was build error on one of
>> the offload transports (bad! but happens) and yes, as you pointed, one
>> static checker fix + one bug fix for it went upstream after this has
>> been merged, happens too.
>
> A patch should not cause this many issues.
>
>> What makes you say it was poorly reviewed?
>
> I just did not do a good job at looking at the patch. I should have
> caught all of these issues.
>
> - The bnx2i cleanup_task bug should have been obvious, especially for me
> because I had commented about the back lock and the abort path.
>
> - This oops, was so basic. Incorrect locking around a linked list being
> accessed from 2 threads is really one of those 1st year kernel
> programmer things.

Mike, Chris

After the locking change, adding a task to any of the connection
mgmtqueue, cmdqueue, or requeue lists is under the session forward lock.

Removing tasks from any of these lists in iscsi_data_xmit is under
the session forward lock and **before** calling down to the transport
to handle the task.

The iscsi_complete_task helper was added by Mike's commit
3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races"
and is indeed typically called under the backward lock && has this section

+       if (!list_empty(&task->running))
+               list_del_init(&task->running);

which per my reading of the code never comes into play, can you comment?

Lets address this area before we move to the others claims made on the patch.

Again, the patch is around for ~18 months, since 3.15, and no deep
complaints so far, lets
not jump to conclusions.

Or.
--
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
Mike Christie Nov. 16, 2015, 5:30 p.m. UTC | #7
> On Nov 15, 2015, at 4:10 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> 
> On Fri, Nov 13, 2015 at 6:51 PM, Mike Christie <michaelc@cs.wisc.edu> wrote:
>> On 11/13/2015 09:06 AM, Or Gerlitz wrote:
>>>> The patch has caused multiple regressions, did not even compile when
>>>>> sent to me, and was poorly reviewed and I have not heard from you guys
>>>>> in a week. Given the issues the patch has had and the current time, I do
>>>>> not feel comfortable with it anymore. I want to re-review it and fix it
>>>>> up when there is more time.
>>> Mike (Hi),
>>> 
>>> It's a complex patch that touches all the iscsi transports, and yes,
>>> when it was send to you the 1st time, there was build error on one of
>>> the offload transports (bad! but happens) and yes, as you pointed, one
>>> static checker fix + one bug fix for it went upstream after this has
>>> been merged, happens too.
>> 
>> A patch should not cause this many issues.
>> 
>>> What makes you say it was poorly reviewed?
>> 
>> I just did not do a good job at looking at the patch. I should have
>> caught all of these issues.
>> 
>> - The bnx2i cleanup_task bug should have been obvious, especially for me
>> because I had commented about the back lock and the abort path.
>> 
>> - This oops, was so basic. Incorrect locking around a linked list being
>> accessed from 2 threads is really one of those 1st year kernel
>> programmer things.
> 
> Mike, Chris
> 
> After the locking change, adding a task to any of the connection
> mgmtqueue, cmdqueue, or requeue lists is under the session forward lock.
> 
> Removing tasks from any of these lists in iscsi_data_xmit is under
> the session forward lock and **before** calling down to the transport
> to handle the task.
> 
> The iscsi_complete_task helper was added by Mike's commit
> 3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races"
> and is indeed typically called under the backward lock && has this section
> 
> +       if (!list_empty(&task->running))
> +               list_del_init(&task->running);
> 
> which per my reading of the code never comes into play, can you comment?


I had sent this to Sagi and your mellanox email the other day:


> The bug occurs when a target completes a command while we are still
> processing it. If we are doing a WRITE and the iscsi_task
> is on the cmdqueue because we are handling a R2T. The target shouldn't
> send a Check Condition at this time, but some do. If that happens, then
> iscsi_queuecommand could be adding a new task to the cmdqueue, while the
> recv path is handling the CC for the task with the outsanding R2T.  The
> recv path iscsi_complete_task call sees that task it on the cmdqueue and
> deletes it from the list at the same time iscsi_queuecommand is adding a new
> task.
> 
> This should not happen per the iscsi spec. There is some wording about
> waiting to finish the sequence in progress, but targets goof this up.




--
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
Or Gerlitz Nov. 17, 2015, 4:55 p.m. UTC | #8
On Mon, Nov 16, 2015 at 7:30 PM, Michael Christie <michaelc@cs.wisc.edu> wrote:
>> On Nov 15, 2015, at 4:10 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:

>> After the locking change, adding a task to any of the connection
>> mgmtqueue, cmdqueue, or requeue lists is under the session forward lock.
>>
>> Removing tasks from any of these lists in iscsi_data_xmit is under
>> the session forward lock and **before** calling down to the transport
>> to handle the task.
>>
>> The iscsi_complete_task helper was added by Mike's commit
>> 3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races"
>> and is indeed typically called under the backward lock && has this section
>>
>> +       if (!list_empty(&task->running))
>> +               list_del_init(&task->running);
>> which per my reading of the code never comes into play, can you comment?

>> The bug occurs when a target completes a command while we are still
>> processing it. If we are doing a WRITE and the iscsi_task
>> is on the cmdqueue because we are handling a R2T. The target shouldn't
>> send a Check Condition at this time, but some do. If that happens, then
>> iscsi_queuecommand could be adding a new task to the cmdqueue, while the
>> recv path is handling the CC for the task with the outsanding R2T.  The
>> recv path iscsi_complete_task call sees that task it on the cmdqueue and
>> deletes it from the list at the same time iscsi_queuecommand is adding a new
>> task.

So we're now a bit beyond trivial bug and

>> This should not happen per the iscsi spec. There is some wording about
>> waiting to finish the sequence in progress, but targets goof this up.

we have target/s that violate the spec, this is life, but can explain
why it took us 18m to get
bug report. Can you provide few point pointers into the relevant code
pieces that one need
to look at to realize what's going on there? was this code added
before or after the patch?

Or.
--
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
Or Gerlitz Nov. 18, 2015, 11:30 a.m. UTC | #9
On Mon, Nov 16, 2015 at 7:30 PM, Michael Christie <michaelc@cs.wisc.edu> wrote:
>> On Nov 15, 2015, at 4:10 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Fri, Nov 13, 2015 at 6:51 PM, Mike Christie <michaelc@cs.wisc.edu> wrote:
>>> On 11/13/2015 09:06 AM, Or Gerlitz wrote:

>> After the locking change, adding a task to any of the connection
>> mgmtqueue, cmdqueue, or requeue lists is under the session forward lock.
>>
>> Removing tasks from any of these lists in iscsi_data_xmit is under
>> the session forward lock and **before** calling down to the transport
>> to handle the task.
>>
>> The iscsi_complete_task helper was added by Mike's commit
>> 3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races"
>> and is indeed typically called under the backward lock && has this section
>>
>> +       if (!list_empty(&task->running))
>> +               list_del_init(&task->running);
>>
>> which per my reading of the code never comes into play, can you comment?

> I had sent this to Sagi and your mellanox email the other day:

> The bug occurs when a target completes a command while we are still
> processing it. If we are doing a WRITE and the iscsi_task
> is on the cmdqueue because we are handling a R2T.

Mike,

I failed to find how an iscsi_task can be added again to the cmdqueue list,
nor how it can be added to the requeue list without the right locking, nor how
can an iscsi_task be on either of these lists when iscsi_complete_task
is invoked.

Specifically, my reading of the code says that these are the events over time:

t1. queuecommand :: we put the task on the cmdqueue list
(libiscsi.c:1741) - under fwd lock

t2. iscsi_data_xmit :: we remove the task from the cmdqueue list
(libiscsi.c:1537) - under fwd lock

when the R2T flow happens, we do the following

t3. iscsi_tcp_hdr_dissect --> iscsi_tcp_r2t_rsp --> iscsi_requeue_task ::
put the task on the requeue list -- the call to iscsi_tcp_r2t_rsp is
under the fwd lock

t4. iscsi_data_xmit :: we remove the task from the requeue list
(libiscsi.c: 1578) - under fwd lock

Do you agree to t1...t4 being what's possible for a given task? or I
missed something?

>> The target shouldn't
>> send a Check Condition at this time, but some do. If that happens, then
>> iscsi_queuecommand could be adding a new task to the cmdqueue, while the
>> recv path is handling the CC for the task with the outsanding R2T.  The
>> recv path iscsi_complete_task call sees that task it on the cmdqueue and
>> deletes it from the list at the same time iscsi_queuecommand is adding a new task.


>> This should not happen per the iscsi spec. There is some wording about
>> waiting to finish the sequence in progress, but targets goof this up.
--
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
Mike Christie Nov. 18, 2015, 6:39 p.m. UTC | #10
On 11/18/15, 5:30 AM, Or Gerlitz wrote:
> On Mon, Nov 16, 2015 at 7:30 PM, Michael Christie <michaelc@cs.wisc.edu> wrote:
>>> On Nov 15, 2015, at 4:10 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>> On Fri, Nov 13, 2015 at 6:51 PM, Mike Christie <michaelc@cs.wisc.edu> wrote:
>>>> On 11/13/2015 09:06 AM, Or Gerlitz wrote:
>
>>> After the locking change, adding a task to any of the connection
>>> mgmtqueue, cmdqueue, or requeue lists is under the session forward lock.
>>>
>>> Removing tasks from any of these lists in iscsi_data_xmit is under
>>> the session forward lock and **before** calling down to the transport
>>> to handle the task.
>>>
>>> The iscsi_complete_task helper was added by Mike's commit
>>> 3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races"
>>> and is indeed typically called under the backward lock && has this section
>>>
>>> +       if (!list_empty(&task->running))
>>> +               list_del_init(&task->running);
>>>
>>> which per my reading of the code never comes into play, can you comment?
>
>> I had sent this to Sagi and your mellanox email the other day:
>
>> The bug occurs when a target completes a command while we are still
>> processing it. If we are doing a WRITE and the iscsi_task
>> is on the cmdqueue because we are handling a R2T.
>
> Mike,
>
> I failed to find how an iscsi_task can be added again to the cmdqueue list,
> nor how it can be added to the requeue list without the right locking, nor how
> can an iscsi_task be on either of these lists when iscsi_complete_task
> is invoked.

Are you only considering normal execution or also the cc case I 
mentioned in the last mail? For normal execution we are ok.

>
> Specifically, my reading of the code says that these are the events over time:
>
> t1. queuecommand :: we put the task on the cmdqueue list
> (libiscsi.c:1741) - under fwd lock
>
> t2. iscsi_data_xmit :: we remove the task from the cmdqueue list
> (libiscsi.c:1537) - under fwd lock
>
> when the R2T flow happens, we do the following
>
> t3. iscsi_tcp_hdr_dissect --> iscsi_tcp_r2t_rsp --> iscsi_requeue_task ::
> put the task on the requeue list -- the call to iscsi_tcp_r2t_rsp is
> under the fwd lock

If the target were to send a CC at this point, we are adding the task 
under the frwd lock, but the completion path would only have the back 
lock. The list_empty, list_add and list_del calls then race and we could 
end up in a bad state right?

>
> t4. iscsi_data_xmit :: we remove the task from the requeue list
> (libiscsi.c: 1578) - under fwd lock

We could also get the bad CC at this time and end up doing 2 list_dels 
at the same time. The t4 one under the frwd lock and the cc handling 
completion one under the back lock like in t3.

>
> Do you agree to t1...t4 being what's possible for a given task? or I
> missed something?
>
>>> The target shouldn't
>>> send a Check Condition at this time, but some do. If that happens, then
>>> iscsi_queuecommand could be adding a new task to the cmdqueue, while the
>>> recv path is handling the CC for the task with the outsanding R2T.  The
>>> recv path iscsi_complete_task call sees that task it on the cmdqueue and
>>> deletes it from the list at the same time iscsi_queuecommand is adding a new task.
>
>
>>> This should not happen per the iscsi spec. There is some wording about
>>> waiting to finish the sequence in progress, but targets goof this up.
> --
> 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
>

--
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
Brian King Jan. 22, 2016, 4:50 p.m. UTC | #11
On 11/11/2015 11:05 PM, mchristi@redhat.com wrote:
> From: Mike Christie <mchristi@redhat.com>
> 
> This patch fixes this oops report by Guilherme Piccol:
> 
> list_del corruption. prev->next should be c000000f3da2b080, but was c000000f3da2c080

Hi Mike! I haven't seen any follow ups on this for a while. What is the plan for this one? 

Thanks,

Brian
Mike Christie Jan. 22, 2016, 7:11 p.m. UTC | #12
On 01/22/2016 10:50 AM, Brian King wrote:
> On 11/11/2015 11:05 PM, mchristi@redhat.com wrote:
>> From: Mike Christie <mchristi@redhat.com>
>>
>> This patch fixes this oops report by Guilherme Piccol:
>>
>> list_del corruption. prev->next should be c000000f3da2b080, but was c000000f3da2c080
> 
> Hi Mike! I haven't seen any follow ups on this for a while. What is the plan for this one? 
> 

Hey,

I ended up sending Guilherme a patch for the problem I described here.
They were not hitting that problem. Checked if we were hitting a
different regression in the abort path. Not hitting that either.

Or was still debugging it.
--
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
Chris Leech Nov. 7, 2016, 6:15 p.m. UTC | #13
Hi,

I'm kicking this old thread because I don't think this ever got
resolved.  I wish I had more info, but it seems to involve target
specific behavior that hasn't come up in our test labs.

So what can I do at this point to help resolve this?

On Sun, Nov 15, 2015 at 12:10:48PM +0200, Or Gerlitz wrote:
> Mike, Chris
> 
> After the locking change, adding a task to any of the connection
> mgmtqueue, cmdqueue, or requeue lists is under the session forward lock.
> 
> Removing tasks from any of these lists in iscsi_data_xmit is under
> the session forward lock and **before** calling down to the transport
> to handle the task.
> 
> The iscsi_complete_task helper was added by Mike's commit
> 3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races"
> and is indeed typically called under the backward lock && has this section
> 
> +       if (!list_empty(&task->running))
> +               list_del_init(&task->running);
> 
> which per my reading of the code never comes into play, can you comment?
> 
> Lets address this area before we move to the others claims made on the patch.

This bit in particular is where I see a cause for concern.  If that
list_del_init call ever races against other list operations, there's a
potential corruption.  It's presumably there for a reason, and Mike
explained a case where some targets have been known to send a check
condition at unexpected times that would hit it.

I don't like having known list locking violations hanging around, based
on an expectation that we'll never hit that path with well behaved
targets.

If we can get a fix worked up for the list locking here, can we get any
testing on it from the original reports at IBM?  That was very helpful
in testing a full reversion patch.

- Chris Leech

--
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
Guilherme G. Piccoli Nov. 7, 2016, 6:23 p.m. UTC | #14
On 11/07/2016 04:15 PM, Chris Leech wrote:
> Hi,
> 
> I'm kicking this old thread because I don't think this ever got
> resolved.  I wish I had more info, but it seems to involve target
> specific behavior that hasn't come up in our test labs.

Thanks very much for reopening this thread! We have the Or's patch
reverted in multiple distros, so the issue is not likely to happen on
customer's from IBM, but upstream kernel never saw a proper fix for this.


> 
> So what can I do at this point to help resolve this?
> 
> On Sun, Nov 15, 2015 at 12:10:48PM +0200, Or Gerlitz wrote:
>> Mike, Chris
>>
>> After the locking change, adding a task to any of the connection
>> mgmtqueue, cmdqueue, or requeue lists is under the session forward lock.
>>
>> Removing tasks from any of these lists in iscsi_data_xmit is under
>> the session forward lock and **before** calling down to the transport
>> to handle the task.
>>
>> The iscsi_complete_task helper was added by Mike's commit
>> 3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races"
>> and is indeed typically called under the backward lock && has this section
>>
>> +       if (!list_empty(&task->running))
>> +               list_del_init(&task->running);
>>
>> which per my reading of the code never comes into play, can you comment?
>>
>> Lets address this area before we move to the others claims made on the patch.
> 
> This bit in particular is where I see a cause for concern.  If that
> list_del_init call ever races against other list operations, there's a
> potential corruption.  It's presumably there for a reason, and Mike
> explained a case where some targets have been known to send a check
> condition at unexpected times that would hit it.
> 
> I don't like having known list locking violations hanging around, based
> on an expectation that we'll never hit that path with well behaved
> targets.
> 
> If we can get a fix worked up for the list locking here, can we get any
> testing on it from the original reports at IBM?  That was very helpful
> in testing a full reversion patch.

Sure! Count on us to test any patches. I guess the first step is to
reproduce on upstream right? We haven't tested specifically this
scenario for long time. Will try to reproduce on 4.9-rc4 and update here.

Cheers,



Guilherme


> 
> - Chris Leech
> 

--
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
diff mbox

Patch

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index a4a5d6d..987cd60 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -232,20 +232,20 @@  static int beiscsi_eh_abort(struct scsi_cmnd *sc)
 	cls_session = starget_to_session(scsi_target(sc->device));
 	session = cls_session->dd_data;
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (!aborted_task || !aborted_task->sc) {
 		/* we raced */
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		return SUCCESS;
 	}
 
 	aborted_io_task = aborted_task->dd_data;
 	if (!aborted_io_task->scsi_cmnd) {
 		/* raced or invalid command */
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		return SUCCESS;
 	}
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	/* Invalidate WRB Posted for this Task */
 	AMAP_SET_BITS(struct amap_iscsi_wrb, invld,
 		      aborted_io_task->pwrb_handle->pwrb,
@@ -310,9 +310,9 @@  static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
 	/* invalidate iocbs */
 	cls_session = starget_to_session(scsi_target(sc->device));
 	session = cls_session->dd_data;
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN) {
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		return FAILED;
 	}
 	conn = session->leadconn;
@@ -341,7 +341,7 @@  static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
 		num_invalidate++;
 		inv_tbl++;
 	}
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	inv_tbl = phba->inv_tbl;
 
 	nonemb_cmd.va = pci_alloc_consistent(phba->ctrl.pdev,
@@ -1137,9 +1137,9 @@  beiscsi_process_async_pdu(struct beiscsi_conn *beiscsi_conn,
 		return 1;
 	}
 
-	spin_lock_bh(&session->back_lock);
+	spin_lock_bh(&session->lock);
 	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)ppdu, pbuffer, buf_len);
-	spin_unlock_bh(&session->back_lock);
+	spin_unlock_bh(&session->lock);
 	return 0;
 }
 
@@ -1560,7 +1560,7 @@  static void hwi_complete_cmd(struct beiscsi_conn *beiscsi_conn,
 	pwrb = pwrb_handle->pwrb;
 	type = ((struct beiscsi_io_task *)task->dd_data)->wrb_type;
 
-	spin_lock_bh(&session->back_lock);
+	spin_lock_bh(&session->lock);
 	switch (type) {
 	case HWH_TYPE_IO:
 	case HWH_TYPE_IO_RD:
@@ -1599,7 +1599,7 @@  static void hwi_complete_cmd(struct beiscsi_conn *beiscsi_conn,
 		break;
 	}
 
-	spin_unlock_bh(&session->back_lock);
+	spin_unlock_bh(&session->lock);
 }
 
 static struct list_head *hwi_get_async_busy_list(struct hwi_async_pdu_context
@@ -4688,9 +4688,9 @@  beiscsi_offload_connection(struct beiscsi_conn *beiscsi_conn,
 	 * login/startup related tasks.
 	 */
 	beiscsi_conn->login_in_progress = 0;
-	spin_lock_bh(&session->back_lock);
+	spin_lock_bh(&session->lock);
 	beiscsi_cleanup_task(task);
-	spin_unlock_bh(&session->back_lock);
+	spin_unlock_bh(&session->lock);
 
 	pwrb_handle = alloc_wrb_handle(phba, beiscsi_conn->beiscsi_conn_cid);
 
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index fb072cc..9bfc3d6 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -1363,7 +1363,7 @@  int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
 	u32 datalen = 0;
 
 	resp_cqe = (struct bnx2i_cmd_response *)cqe;
-	spin_lock_bh(&session->back_lock);
+	spin_lock_bh(&session->lock);
 	task = iscsi_itt_to_task(conn,
 				 resp_cqe->itt & ISCSI_CMD_RESPONSE_INDEX);
 	if (!task)
@@ -1434,7 +1434,7 @@  done:
 	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)hdr,
 			     conn->data, datalen);
 fail:
-	spin_unlock_bh(&session->back_lock);
+	spin_unlock_bh(&session->lock);
 	return 0;
 }
 
@@ -1459,7 +1459,7 @@  static int bnx2i_process_login_resp(struct iscsi_session *session,
 	int pad_len;
 
 	login = (struct bnx2i_login_response *) cqe;
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	task = iscsi_itt_to_task(conn,
 				 login->itt & ISCSI_LOGIN_RESPONSE_INDEX);
 	if (!task)
@@ -1502,7 +1502,7 @@  static int bnx2i_process_login_resp(struct iscsi_session *session,
 		bnx2i_conn->gen_pdu.resp_buf,
 		bnx2i_conn->gen_pdu.resp_wr_ptr - bnx2i_conn->gen_pdu.resp_buf);
 done:
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 	return 0;
 }
 
@@ -1527,7 +1527,7 @@  static int bnx2i_process_text_resp(struct iscsi_session *session,
 	int pad_len;
 
 	text = (struct bnx2i_text_response *) cqe;
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	task = iscsi_itt_to_task(conn, text->itt & ISCSI_LOGIN_RESPONSE_INDEX);
 	if (!task)
 		goto done;
@@ -1563,7 +1563,7 @@  static int bnx2i_process_text_resp(struct iscsi_session *session,
 			     bnx2i_conn->gen_pdu.resp_wr_ptr -
 			     bnx2i_conn->gen_pdu.resp_buf);
 done:
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 	return 0;
 }
 
@@ -1586,7 +1586,7 @@  static int bnx2i_process_tmf_resp(struct iscsi_session *session,
 	struct iscsi_tm_rsp *resp_hdr;
 
 	tmf_cqe = (struct bnx2i_tmf_response *)cqe;
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	task = iscsi_itt_to_task(conn,
 				 tmf_cqe->itt & ISCSI_TMF_RESPONSE_INDEX);
 	if (!task)
@@ -1602,7 +1602,7 @@  static int bnx2i_process_tmf_resp(struct iscsi_session *session,
 
 	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr, NULL, 0);
 done:
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 	return 0;
 }
 
@@ -1625,7 +1625,7 @@  static int bnx2i_process_logout_resp(struct iscsi_session *session,
 	struct iscsi_logout_rsp *resp_hdr;
 
 	logout = (struct bnx2i_logout_response *) cqe;
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	task = iscsi_itt_to_task(conn,
 				 logout->itt & ISCSI_LOGOUT_RESPONSE_INDEX);
 	if (!task)
@@ -1649,7 +1649,7 @@  static int bnx2i_process_logout_resp(struct iscsi_session *session,
 
 	bnx2i_conn->ep->state = EP_STATE_LOGOUT_RESP_RCVD;
 done:
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 	return 0;
 }
 
@@ -1670,12 +1670,12 @@  static void bnx2i_process_nopin_local_cmpl(struct iscsi_session *session,
 	struct iscsi_task *task;
 
 	nop_in = (struct bnx2i_nop_in_msg *)cqe;
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	task = iscsi_itt_to_task(conn,
 				 nop_in->itt & ISCSI_NOP_IN_MSG_INDEX);
 	if (task)
 		__iscsi_put_task(task);
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 }
 
 /**
@@ -1714,7 +1714,7 @@  static int bnx2i_process_nopin_mesg(struct iscsi_session *session,
 
 	nop_in = (struct bnx2i_nop_in_msg *)cqe;
 
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	hdr = (struct iscsi_nopin *)&bnx2i_conn->gen_pdu.resp_hdr;
 	memset(hdr, 0, sizeof(struct iscsi_hdr));
 	hdr->opcode = nop_in->op_code;
@@ -1740,7 +1740,7 @@  static int bnx2i_process_nopin_mesg(struct iscsi_session *session,
 	}
 done:
 	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)hdr, NULL, 0);
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 
 	return tgt_async_nop;
 }
@@ -1773,7 +1773,7 @@  static void bnx2i_process_async_mesg(struct iscsi_session *session,
 		return;
 	}
 
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	resp_hdr = (struct iscsi_async *) &bnx2i_conn->gen_pdu.resp_hdr;
 	memset(resp_hdr, 0, sizeof(struct iscsi_hdr));
 	resp_hdr->opcode = async_cqe->op_code;
@@ -1792,7 +1792,7 @@  static void bnx2i_process_async_mesg(struct iscsi_session *session,
 
 	__iscsi_complete_pdu(bnx2i_conn->cls_conn->dd_data,
 			     (struct iscsi_hdr *)resp_hdr, NULL, 0);
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 }
 
 
@@ -1819,7 +1819,7 @@  static void bnx2i_process_reject_mesg(struct iscsi_session *session,
 	} else
 		bnx2i_unsol_pdu_adjust_rq(bnx2i_conn);
 
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	hdr = (struct iscsi_reject *) &bnx2i_conn->gen_pdu.resp_hdr;
 	memset(hdr, 0, sizeof(struct iscsi_hdr));
 	hdr->opcode = reject->op_code;
@@ -1830,7 +1830,7 @@  static void bnx2i_process_reject_mesg(struct iscsi_session *session,
 	hdr->ffffffff = cpu_to_be32(RESERVED_ITT);
 	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)hdr, conn->data,
 			     reject->data_length);
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 }
 
 /**
@@ -1850,13 +1850,13 @@  static void bnx2i_process_cmd_cleanup_resp(struct iscsi_session *session,
 	struct iscsi_task *task;
 
 	cmd_clean_rsp = (struct bnx2i_cleanup_response *)cqe;
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	task = iscsi_itt_to_task(conn,
 			cmd_clean_rsp->itt & ISCSI_CLEANUP_RESPONSE_INDEX);
 	if (!task)
 		printk(KERN_ALERT "bnx2i: cmd clean ITT %x not active\n",
 			cmd_clean_rsp->itt & ISCSI_CLEANUP_RESPONSE_INDEX);
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 	complete(&bnx2i_conn->cmd_cleanup_cmpl);
 }
 
@@ -1923,11 +1923,11 @@  static int bnx2i_queue_scsi_cmd_resp(struct iscsi_session *session,
 	int rc = 0;
 	int cpu;
 
-	spin_lock(&session->back_lock);
+	spin_lock(&session->lock);
 	task = iscsi_itt_to_task(bnx2i_conn->cls_conn->dd_data,
 				 cqe->itt & ISCSI_CMD_RESPONSE_INDEX);
 	if (!task || !task->sc) {
-		spin_unlock(&session->back_lock);
+		spin_unlock(&session->lock);
 		return -EINVAL;
 	}
 	sc = task->sc;
@@ -1937,7 +1937,7 @@  static int bnx2i_queue_scsi_cmd_resp(struct iscsi_session *session,
 	else
 		cpu = sc->request->cpu;
 
-	spin_unlock(&session->back_lock);
+	spin_unlock(&session->lock);
 
 	p = &per_cpu(bnx2i_percpu, cpu);
 	spin_lock(&p->p_work_lock);
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index 7289437..1ef5bbb 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1172,12 +1172,10 @@  static void bnx2i_cleanup_task(struct iscsi_task *task)
 	if (task->state == ISCSI_TASK_ABRT_TMF) {
 		bnx2i_send_cmd_cleanup_req(hba, task->dd_data);
 
-		spin_unlock_bh(&conn->session->back_lock);
-		spin_unlock_bh(&conn->session->frwd_lock);
+		spin_unlock_bh(&conn->session->lock);
 		wait_for_completion_timeout(&bnx2i_conn->cmd_cleanup_cmpl,
 				msecs_to_jiffies(ISCSI_CMD_CLEANUP_TIMEOUT));
-		spin_lock_bh(&conn->session->frwd_lock);
-		spin_lock_bh(&conn->session->back_lock);
+		spin_lock_bh(&conn->session->lock);
 	}
 	bnx2i_iscsi_unmap_sg_list(task->dd_data);
 }
@@ -2063,7 +2061,7 @@  int bnx2i_hw_ep_disconnect(struct bnx2i_endpoint *bnx2i_ep)
 		goto out;
 
 	if (session) {
-		spin_lock_bh(&session->frwd_lock);
+		spin_lock_bh(&session->lock);
 		if (bnx2i_ep->state != EP_STATE_TCP_FIN_RCVD) {
 			if (session->state == ISCSI_STATE_LOGGING_OUT) {
 				if (bnx2i_ep->state == EP_STATE_LOGOUT_SENT) {
@@ -2079,7 +2077,7 @@  int bnx2i_hw_ep_disconnect(struct bnx2i_endpoint *bnx2i_ep)
 		} else
 			close = 1;
 
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 	}
 
 	bnx2i_ep->state = EP_STATE_DISCONN_START;
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 0b8af18..19be12f 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -593,9 +593,9 @@  static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn)
 	iscsi_sw_tcp_conn_restore_callbacks(conn);
 	sock_put(sock->sk);
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	tcp_sw_conn->sock = NULL;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	sockfd_put(sock);
 }
 
@@ -663,10 +663,10 @@  iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session,
 	if (err)
 		goto free_socket;
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	/* bind iSCSI connection and socket */
 	tcp_sw_conn->sock = sock;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	/* setup Socket parameters */
 	sk = sock->sk;
@@ -727,9 +727,9 @@  static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn,
 	case ISCSI_PARAM_CONN_PORT:
 	case ISCSI_PARAM_CONN_ADDRESS:
 	case ISCSI_PARAM_LOCAL_PORT:
-		spin_lock_bh(&conn->session->frwd_lock);
+		spin_lock_bh(&conn->session->lock);
 		if (!tcp_sw_conn || !tcp_sw_conn->sock) {
-			spin_unlock_bh(&conn->session->frwd_lock);
+			spin_unlock_bh(&conn->session->lock);
 			return -ENOTCONN;
 		}
 		if (param == ISCSI_PARAM_LOCAL_PORT)
@@ -738,7 +738,7 @@  static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn,
 		else
 			rc = kernel_getpeername(tcp_sw_conn->sock,
 						(struct sockaddr *)&addr, &len);
-		spin_unlock_bh(&conn->session->frwd_lock);
+		spin_unlock_bh(&conn->session->lock);
 		if (rc)
 			return rc;
 
@@ -767,23 +767,23 @@  static int iscsi_sw_tcp_host_get_param(struct Scsi_Host *shost,
 		if (!session)
 			return -ENOTCONN;
 
-		spin_lock_bh(&session->frwd_lock);
+		spin_lock_bh(&session->lock);
 		conn = session->leadconn;
 		if (!conn) {
-			spin_unlock_bh(&session->frwd_lock);
+			spin_unlock_bh(&session->lock);
 			return -ENOTCONN;
 		}
 		tcp_conn = conn->dd_data;
 
 		tcp_sw_conn = tcp_conn->dd_data;
 		if (!tcp_sw_conn->sock) {
-			spin_unlock_bh(&session->frwd_lock);
+			spin_unlock_bh(&session->lock);
 			return -ENOTCONN;
 		}
 
 		rc = kernel_getsockname(tcp_sw_conn->sock,
 					(struct sockaddr *)&addr, &len);
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		if (rc)
 			return rc;
 
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 6bffd91..6ac6c20 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -477,7 +477,7 @@  static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
  * iscsi_free_task - free a task
  * @task: iscsi cmd task
  *
- * Must be called with session back_lock.
+ * Must be called with session lock.
  * This function returns the scsi command to scsi-ml or cleans
  * up mgmt tasks then returns the task to the pool.
  */
@@ -531,10 +531,9 @@  void iscsi_put_task(struct iscsi_task *task)
 {
 	struct iscsi_session *session = task->conn->session;
 
-	/* regular RX path uses back_lock */
-	spin_lock_bh(&session->back_lock);
+	spin_lock_bh(&session->lock);
 	__iscsi_put_task(task);
-	spin_unlock_bh(&session->back_lock);
+	spin_unlock_bh(&session->lock);
 }
 EXPORT_SYMBOL_GPL(iscsi_put_task);
 
@@ -543,7 +542,7 @@  EXPORT_SYMBOL_GPL(iscsi_put_task);
  * @task: iscsi cmd task
  * @state: state to complete task with
  *
- * Must be called with session back_lock.
+ * Must be called with session lock.
  */
 static void iscsi_complete_task(struct iscsi_task *task, int state)
 {
@@ -582,7 +581,7 @@  static void iscsi_complete_task(struct iscsi_task *task, int state)
  * This is used when drivers do not need or cannot perform
  * lower level pdu processing.
  *
- * Called with session back_lock
+ * Called with session lock
  */
 void iscsi_complete_scsi_task(struct iscsi_task *task,
 			      uint32_t exp_cmdsn, uint32_t max_cmdsn)
@@ -599,7 +598,7 @@  EXPORT_SYMBOL_GPL(iscsi_complete_scsi_task);
 
 
 /*
- * session back_lock must be held and if not called for a task that is
+ * session lock must be held and if not called for a task that is
  * still pending or from the xmit thread, then xmit thread must
  * be suspended.
  */
@@ -639,10 +638,7 @@  static void fail_scsi_task(struct iscsi_task *task, int err)
 		scsi_in(sc)->resid = scsi_in(sc)->length;
 	}
 
-	/* regular RX path uses back_lock */
-	spin_lock_bh(&conn->session->back_lock);
 	iscsi_complete_task(task, state);
-	spin_unlock_bh(&conn->session->back_lock);
 }
 
 static int iscsi_prep_mgmt_task(struct iscsi_conn *conn,
@@ -790,10 +786,7 @@  __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 	return task;
 
 free_task:
-	/* regular RX path uses back_lock */
-	spin_lock_bh(&session->back_lock);
 	__iscsi_put_task(task);
-	spin_unlock_bh(&session->back_lock);
 	return NULL;
 }
 
@@ -804,10 +797,10 @@  int iscsi_conn_send_pdu(struct iscsi_cls_conn *cls_conn, struct iscsi_hdr *hdr,
 	struct iscsi_session *session = conn->session;
 	int err = 0;
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (!__iscsi_conn_send_pdu(conn, hdr, data, data_size))
 		err = -EPERM;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	return err;
 }
 EXPORT_SYMBOL_GPL(iscsi_conn_send_pdu);
@@ -1071,19 +1064,14 @@  static int iscsi_handle_reject(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 		if (opcode != ISCSI_OP_NOOP_OUT)
 			return 0;
 
-		 if (rejected_pdu.itt == cpu_to_be32(ISCSI_RESERVED_TAG)) {
+		 if (rejected_pdu.itt == cpu_to_be32(ISCSI_RESERVED_TAG))
 			/*
 			 * nop-out in response to target's nop-out rejected.
 			 * Just resend.
 			 */
-			/* In RX path we are under back lock */
-			spin_unlock(&conn->session->back_lock);
-			spin_lock(&conn->session->frwd_lock);
 			iscsi_send_nopout(conn,
 					  (struct iscsi_nopin*)&rejected_pdu);
-			spin_unlock(&conn->session->frwd_lock);
-			spin_lock(&conn->session->back_lock);
-		} else {
+		else {
 			struct iscsi_task *task;
 			/*
 			 * Our nop as ping got dropped. We know the target
@@ -1119,7 +1107,7 @@  static int iscsi_handle_reject(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
  * This should be used for mgmt tasks like login and nops, or if
  * the LDD's itt space does not include the session age.
  *
- * The session back_lock must be held.
+ * The session lock must be held.
  */
 struct iscsi_task *iscsi_itt_to_task(struct iscsi_conn *conn, itt_t itt)
 {
@@ -1148,7 +1136,7 @@  EXPORT_SYMBOL_GPL(iscsi_itt_to_task);
  * @datalen: len of data buffer
  *
  * Completes pdu processing by freeing any resources allocated at
- * queuecommand or send generic. session back_lock must be held and verify
+ * queuecommand or send generic. session lock must be held and verify
  * itt must have been called.
  */
 int __iscsi_complete_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
@@ -1185,12 +1173,7 @@  int __iscsi_complete_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 			if (hdr->ttt == cpu_to_be32(ISCSI_RESERVED_TAG))
 				break;
 
-			/* In RX path we are under back lock */
-			spin_unlock(&session->back_lock);
-			spin_lock(&session->frwd_lock);
 			iscsi_send_nopout(conn, (struct iscsi_nopin*)hdr);
-			spin_unlock(&session->frwd_lock);
-			spin_lock(&session->back_lock);
 			break;
 		case ISCSI_OP_REJECT:
 			rc = iscsi_handle_reject(conn, hdr, data, datalen);
@@ -1297,9 +1280,9 @@  int iscsi_complete_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 {
 	int rc;
 
-	spin_lock(&conn->session->back_lock);
+	spin_lock(&conn->session->lock);
 	rc = __iscsi_complete_pdu(conn, hdr, data, datalen);
-	spin_unlock(&conn->session->back_lock);
+	spin_unlock(&conn->session->lock);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(iscsi_complete_pdu);
@@ -1343,7 +1326,7 @@  EXPORT_SYMBOL_GPL(iscsi_verify_itt);
  *
  * This should be used for cmd tasks.
  *
- * The session back_lock must be held.
+ * The session lock must be held.
  */
 struct iscsi_task *iscsi_itt_to_ctask(struct iscsi_conn *conn, itt_t itt)
 {
@@ -1373,15 +1356,15 @@  void iscsi_session_failure(struct iscsi_session *session,
 	struct iscsi_conn *conn;
 	struct device *dev;
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	conn = session->leadconn;
 	if (session->state == ISCSI_STATE_TERMINATE || !conn) {
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		return;
 	}
 
 	dev = get_device(&conn->cls_conn->dev);
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	if (!dev)
 	        return;
 	/*
@@ -1401,15 +1384,15 @@  void iscsi_conn_failure(struct iscsi_conn *conn, enum iscsi_err err)
 {
 	struct iscsi_session *session = conn->session;
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (session->state == ISCSI_STATE_FAILED) {
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		return;
 	}
 
 	if (conn->stop_stage == 0)
 		session->state = ISCSI_STATE_FAILED;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
 	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx);
@@ -1443,18 +1426,15 @@  static int iscsi_xmit_task(struct iscsi_conn *conn)
 		return -ENODATA;
 
 	__iscsi_get_task(task);
-	spin_unlock_bh(&conn->session->frwd_lock);
+	spin_unlock_bh(&conn->session->lock);
 	rc = conn->session->tt->xmit_task(task);
-	spin_lock_bh(&conn->session->frwd_lock);
+	spin_lock_bh(&conn->session->lock);
 	if (!rc) {
 		/* done with this task */
 		task->last_xfer = jiffies;
 		conn->task = NULL;
 	}
-	/* regular RX path uses back_lock */
-	spin_lock(&conn->session->back_lock);
 	__iscsi_put_task(task);
-	spin_unlock(&conn->session->back_lock);
 	return rc;
 }
 
@@ -1463,7 +1443,7 @@  static int iscsi_xmit_task(struct iscsi_conn *conn)
  * @task: task to requeue
  *
  * LLDs that need to run a task from the session workqueue should call
- * this. The session frwd_lock must be held. This should only be called
+ * this. The session lock must be held. This should only be called
  * by software drivers.
  */
 void iscsi_requeue_task(struct iscsi_task *task)
@@ -1494,10 +1474,10 @@  static int iscsi_data_xmit(struct iscsi_conn *conn)
 	struct iscsi_task *task;
 	int rc = 0;
 
-	spin_lock_bh(&conn->session->frwd_lock);
+	spin_lock_bh(&conn->session->lock);
 	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
 		ISCSI_DBG_SESSION(conn->session, "Tx suspended!\n");
-		spin_unlock_bh(&conn->session->frwd_lock);
+		spin_unlock_bh(&conn->session->lock);
 		return -ENODATA;
 	}
 
@@ -1518,10 +1498,7 @@  check_mgmt:
 					 struct iscsi_task, running);
 		list_del_init(&conn->task->running);
 		if (iscsi_prep_mgmt_task(conn, conn->task)) {
-			/* regular RX path uses back_lock */
-			spin_lock_bh(&conn->session->back_lock);
 			__iscsi_put_task(conn->task);
-			spin_unlock_bh(&conn->session->back_lock);
 			conn->task = NULL;
 			continue;
 		}
@@ -1583,11 +1560,11 @@  check_mgmt:
 		if (!list_empty(&conn->mgmtqueue))
 			goto check_mgmt;
 	}
-	spin_unlock_bh(&conn->session->frwd_lock);
+	spin_unlock_bh(&conn->session->lock);
 	return -ENODATA;
 
 done:
-	spin_unlock_bh(&conn->session->frwd_lock);
+	spin_unlock_bh(&conn->session->lock);
 	return rc;
 }
 
@@ -1657,7 +1634,7 @@  int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc)
 
 	cls_session = starget_to_session(scsi_target(sc->device));
 	session = cls_session->dd_data;
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 
 	reason = iscsi_session_chkready(cls_session);
 	if (reason) {
@@ -1743,13 +1720,13 @@  int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc)
 	}
 
 	session->queued_cmdsn++;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	return 0;
 
 prepd_reject:
 	iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
 reject:
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
 			  sc->cmnd[0], reason);
 	return SCSI_MLQUEUE_TARGET_BUSY;
@@ -1757,7 +1734,7 @@  reject:
 prepd_fault:
 	iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
 fault:
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	ISCSI_DBG_SESSION(session, "iscsi: cmd 0x%x is not queued (%d)\n",
 			  sc->cmnd[0], reason);
 	if (!scsi_bidi_cmnd(sc))
@@ -1786,14 +1763,14 @@  static void iscsi_tmf_timedout(unsigned long data)
 	struct iscsi_conn *conn = (struct iscsi_conn *)data;
 	struct iscsi_session *session = conn->session;
 
-	spin_lock(&session->frwd_lock);
+	spin_lock(&session->lock);
 	if (conn->tmf_state == TMF_QUEUED) {
 		conn->tmf_state = TMF_TIMEDOUT;
 		ISCSI_DBG_EH(session, "tmf timedout\n");
 		/* unblock eh_abort() */
 		wake_up(&conn->ehwait);
 	}
-	spin_unlock(&session->frwd_lock);
+	spin_unlock(&session->lock);
 }
 
 static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
@@ -1806,10 +1783,10 @@  static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
 	task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)hdr,
 				      NULL, 0);
 	if (!task) {
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		iscsi_conn_printk(KERN_ERR, conn, "Could not send TMF.\n");
 		iscsi_conn_failure(conn, ISCSI_ERR_CONN_FAILED);
-		spin_lock_bh(&session->frwd_lock);
+		spin_lock_bh(&session->lock);
 		return -EPERM;
 	}
 	conn->tmfcmd_pdus_cnt++;
@@ -1819,7 +1796,7 @@  static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
 	add_timer(&conn->tmf_timer);
 	ISCSI_DBG_EH(session, "tmf set timeout\n");
 
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	mutex_unlock(&session->eh_mutex);
 
 	/*
@@ -1838,7 +1815,7 @@  static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
 	del_timer_sync(&conn->tmf_timer);
 
 	mutex_lock(&session->eh_mutex);
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	/* if the session drops it will clean up the task */
 	if (age != session->age ||
 	    session->state != ISCSI_STATE_LOGGED_IN)
@@ -1874,7 +1851,7 @@  static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
  * iscsi_suspend_queue - suspend iscsi_queuecommand
  * @conn: iscsi conn to stop queueing IO on
  *
- * This grabs the session frwd_lock to make sure no one is in
+ * This grabs the session lock to make sure no one is in
  * xmit_task/queuecommand, and then sets suspend to prevent
  * new commands from being queued. This only needs to be called
  * by offload drivers that need to sync a path like ep disconnect
@@ -1883,9 +1860,9 @@  static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
  */
 void iscsi_suspend_queue(struct iscsi_conn *conn)
 {
-	spin_lock_bh(&conn->session->frwd_lock);
+	spin_lock_bh(&conn->session->lock);
 	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
-	spin_unlock_bh(&conn->session->frwd_lock);
+	spin_unlock_bh(&conn->session->lock);
 }
 EXPORT_SYMBOL_GPL(iscsi_suspend_queue);
 
@@ -1944,7 +1921,7 @@  static enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 
 	ISCSI_DBG_EH(session, "scsi cmd %p timedout\n", sc);
 
-	spin_lock(&session->frwd_lock);
+	spin_lock(&session->lock);
 	task = (struct iscsi_task *)sc->SCp.ptr;
 	if (!task) {
 		/*
@@ -2058,7 +2035,7 @@  static enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 done:
 	if (task)
 		task->last_timeout = jiffies;
-	spin_unlock(&session->frwd_lock);
+	spin_unlock(&session->lock);
 	ISCSI_DBG_EH(session, "return %s\n", rc == BLK_EH_RESET_TIMER ?
 		     "timer reset" : "nh");
 	return rc;
@@ -2070,7 +2047,7 @@  static void iscsi_check_transport_timeouts(unsigned long data)
 	struct iscsi_session *session = conn->session;
 	unsigned long recv_timeout, next_timeout = 0, last_recv;
 
-	spin_lock(&session->frwd_lock);
+	spin_lock(&session->lock);
 	if (session->state != ISCSI_STATE_LOGGED_IN)
 		goto done;
 
@@ -2087,7 +2064,7 @@  static void iscsi_check_transport_timeouts(unsigned long data)
 				  "last ping %lu, now %lu\n",
 				  conn->ping_timeout, conn->recv_timeout,
 				  last_recv, conn->last_ping, jiffies);
-		spin_unlock(&session->frwd_lock);
+		spin_unlock(&session->lock);
 		iscsi_conn_failure(conn, ISCSI_ERR_NOP_TIMEDOUT);
 		return;
 	}
@@ -2105,7 +2082,7 @@  static void iscsi_check_transport_timeouts(unsigned long data)
 	ISCSI_DBG_CONN(conn, "Setting next tmo %lu\n", next_timeout);
 	mod_timer(&conn->transport_timer, next_timeout);
 done:
-	spin_unlock(&session->frwd_lock);
+	spin_unlock(&session->lock);
 }
 
 static void iscsi_prep_abort_task_pdu(struct iscsi_task *task,
@@ -2135,7 +2112,7 @@  int iscsi_eh_abort(struct scsi_cmnd *sc)
 	ISCSI_DBG_EH(session, "aborting sc %p\n", sc);
 
 	mutex_lock(&session->eh_mutex);
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	/*
 	 * if session was ISCSI_STATE_IN_RECOVERY then we may not have
 	 * got the command.
@@ -2143,7 +2120,7 @@  int iscsi_eh_abort(struct scsi_cmnd *sc)
 	if (!sc->SCp.ptr) {
 		ISCSI_DBG_EH(session, "sc never reached iscsi layer or "
 				      "it completed.\n");
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		mutex_unlock(&session->eh_mutex);
 		return SUCCESS;
 	}
@@ -2154,7 +2131,7 @@  int iscsi_eh_abort(struct scsi_cmnd *sc)
 	 */
 	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN ||
 	    sc->SCp.phase != session->age) {
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		mutex_unlock(&session->eh_mutex);
 		ISCSI_DBG_EH(session, "failing abort due to dropped "
 				  "session.\n");
@@ -2195,7 +2172,7 @@  int iscsi_eh_abort(struct scsi_cmnd *sc)
 
 	switch (conn->tmf_state) {
 	case TMF_SUCCESS:
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		/*
 		 * stop tx side incase the target had sent a abort rsp but
 		 * the initiator was still writing out data.
@@ -2206,15 +2183,15 @@  int iscsi_eh_abort(struct scsi_cmnd *sc)
 		 * good and have never sent us a successful tmf response
 		 * then sent more data for the cmd.
 		 */
-		spin_lock_bh(&session->frwd_lock);
+		spin_lock_bh(&session->lock);
 		fail_scsi_task(task, DID_ABORT);
 		conn->tmf_state = TMF_INITIAL;
 		memset(hdr, 0, sizeof(*hdr));
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		iscsi_start_tx(conn);
 		goto success_unlocked;
 	case TMF_TIMEDOUT:
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
 		goto failed_unlocked;
 	case TMF_NOT_FOUND:
@@ -2233,7 +2210,7 @@  int iscsi_eh_abort(struct scsi_cmnd *sc)
 	}
 
 success:
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 success_unlocked:
 	ISCSI_DBG_EH(session, "abort success [sc %p itt 0x%x]\n",
 		     sc, task->itt);
@@ -2241,7 +2218,7 @@  success_unlocked:
 	return SUCCESS;
 
 failed:
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 failed_unlocked:
 	ISCSI_DBG_EH(session, "abort failed [sc %p itt 0x%x]\n", sc,
 		     task ? task->itt : 0);
@@ -2275,7 +2252,7 @@  int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 		     sc->device->lun);
 
 	mutex_lock(&session->eh_mutex);
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	/*
 	 * Just check if we are not logged in. We cannot check for
 	 * the phase because the reset could come from a ioctl.
@@ -2302,7 +2279,7 @@  int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 	case TMF_SUCCESS:
 		break;
 	case TMF_TIMEDOUT:
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
 		goto done;
 	default:
@@ -2311,21 +2288,21 @@  int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 	}
 
 	rc = SUCCESS;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	iscsi_suspend_tx(conn);
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	memset(hdr, 0, sizeof(*hdr));
 	fail_scsi_tasks(conn, sc->device->lun, DID_ERROR);
 	conn->tmf_state = TMF_INITIAL;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	iscsi_start_tx(conn);
 	goto done;
 
 unlock:
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 done:
 	ISCSI_DBG_EH(session, "dev reset result = %s\n",
 		     rc == SUCCESS ? "SUCCESS" : "FAILED");
@@ -2338,13 +2315,13 @@  void iscsi_session_recovery_timedout(struct iscsi_cls_session *cls_session)
 {
 	struct iscsi_session *session = cls_session->dd_data;
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (session->state != ISCSI_STATE_LOGGED_IN) {
 		session->state = ISCSI_STATE_RECOVERY_FAILED;
 		if (session->leadconn)
 			wake_up(&session->leadconn->ehwait);
 	}
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 }
 EXPORT_SYMBOL_GPL(iscsi_session_recovery_timedout);
 
@@ -2366,19 +2343,19 @@  int iscsi_eh_session_reset(struct scsi_cmnd *sc)
 	conn = session->leadconn;
 
 	mutex_lock(&session->eh_mutex);
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (session->state == ISCSI_STATE_TERMINATE) {
 failed:
 		ISCSI_DBG_EH(session,
 			     "failing session reset: Could not log back into "
 			     "%s, %s [age %d]\n", session->targetname,
 			     conn->persistent_address, session->age);
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		mutex_unlock(&session->eh_mutex);
 		return FAILED;
 	}
 
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	mutex_unlock(&session->eh_mutex);
 	/*
 	 * we drop the lock here but the leadconn cannot be destoyed while
@@ -2395,14 +2372,14 @@  failed:
 		flush_signals(current);
 
 	mutex_lock(&session->eh_mutex);
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (session->state == ISCSI_STATE_LOGGED_IN) {
 		ISCSI_DBG_EH(session,
 			     "session reset succeeded for %s,%s\n",
 			     session->targetname, conn->persistent_address);
 	} else
 		goto failed;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	mutex_unlock(&session->eh_mutex);
 	return SUCCESS;
 }
@@ -2438,7 +2415,7 @@  int iscsi_eh_target_reset(struct scsi_cmnd *sc)
 		     session->targetname);
 
 	mutex_lock(&session->eh_mutex);
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	/*
 	 * Just check if we are not logged in. We cannot check for
 	 * the phase because the reset could come from a ioctl.
@@ -2465,7 +2442,7 @@  int iscsi_eh_target_reset(struct scsi_cmnd *sc)
 	case TMF_SUCCESS:
 		break;
 	case TMF_TIMEDOUT:
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
 		goto done;
 	default:
@@ -2474,21 +2451,21 @@  int iscsi_eh_target_reset(struct scsi_cmnd *sc)
 	}
 
 	rc = SUCCESS;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	iscsi_suspend_tx(conn);
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	memset(hdr, 0, sizeof(*hdr));
 	fail_scsi_tasks(conn, -1, DID_ERROR);
 	conn->tmf_state = TMF_INITIAL;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	iscsi_start_tx(conn);
 	goto done;
 
 unlock:
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 done:
 	ISCSI_DBG_EH(session, "tgt %s reset result = %s\n", session->targetname,
 		     rc == SUCCESS ? "SUCCESS" : "FAILED");
@@ -2786,10 +2763,8 @@  iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
 	session->max_r2t = 1;
 	session->tt = iscsit;
 	session->dd_data = cls_session->dd_data + sizeof(*session);
-
 	mutex_init(&session->eh_mutex);
-	spin_lock_init(&session->frwd_lock);
-	spin_lock_init(&session->back_lock);
+	spin_lock_init(&session->lock);
 
 	/* initialize SCSI PDU commands pool */
 	if (iscsi_pool_init(&session->cmdpool, session->cmds_max,
@@ -2903,14 +2878,14 @@  iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
 	INIT_WORK(&conn->xmitwork, iscsi_xmitworker);
 
 	/* allocate login_task used for the login/text sequences */
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (!kfifo_out(&session->cmdpool.queue,
                          (void*)&conn->login_task,
 			 sizeof(void*))) {
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		goto login_task_alloc_fail;
 	}
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	data = (char *) __get_free_pages(GFP_KERNEL,
 					 get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
@@ -2947,7 +2922,7 @@  void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
 	del_timer_sync(&conn->transport_timer);
 
 	mutex_lock(&session->eh_mutex);
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	conn->c_stage = ISCSI_CONN_CLEANUP_WAIT;
 	if (session->leadconn == conn) {
 		/*
@@ -2956,24 +2931,21 @@  void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
 		session->state = ISCSI_STATE_TERMINATE;
 		wake_up(&conn->ehwait);
 	}
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	/* flush queued up work because we free the connection below */
 	iscsi_suspend_tx(conn);
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	free_pages((unsigned long) conn->data,
 		   get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
 	kfree(conn->persistent_address);
 	kfree(conn->local_ipaddr);
-	/* regular RX path uses back_lock */
-	spin_lock_bh(&session->back_lock);
 	kfifo_in(&session->cmdpool.queue, (void*)&conn->login_task,
 		    sizeof(void*));
-	spin_unlock_bh(&session->back_lock);
 	if (session->leadconn == conn)
 		session->leadconn = NULL;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	mutex_unlock(&session->eh_mutex);
 
 	iscsi_destroy_conn(cls_conn);
@@ -3011,7 +2983,7 @@  int iscsi_conn_start(struct iscsi_cls_conn *cls_conn)
 		conn->ping_timeout = 5;
 	}
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	conn->c_stage = ISCSI_CONN_STARTED;
 	session->state = ISCSI_STATE_LOGGED_IN;
 	session->queued_cmdsn = session->cmdsn;
@@ -3040,7 +3012,7 @@  int iscsi_conn_start(struct iscsi_cls_conn *cls_conn)
 	default:
 		break;
 	}
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	iscsi_unblock_session(session->cls_session);
 	wake_up(&conn->ehwait);
@@ -3079,9 +3051,9 @@  static void iscsi_start_session_recovery(struct iscsi_session *session,
 	int old_stop_stage;
 
 	mutex_lock(&session->eh_mutex);
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (conn->stop_stage == STOP_CONN_TERM) {
-		spin_unlock_bh(&session->frwd_lock);
+		spin_unlock_bh(&session->lock);
 		mutex_unlock(&session->eh_mutex);
 		return;
 	}
@@ -3098,14 +3070,14 @@  static void iscsi_start_session_recovery(struct iscsi_session *session,
 
 	old_stop_stage = conn->stop_stage;
 	conn->stop_stage = flag;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	del_timer_sync(&conn->transport_timer);
 	iscsi_suspend_tx(conn);
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	conn->c_stage = ISCSI_CONN_STOPPED;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	/*
 	 * for connection level recovery we should not calculate
@@ -3126,11 +3098,11 @@  static void iscsi_start_session_recovery(struct iscsi_session *session,
 	/*
 	 * flush queues.
 	 */
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	fail_scsi_tasks(conn, -1, DID_TRANSPORT_DISRUPTED);
 	fail_mgmt_tasks(session, conn);
 	memset(&conn->tmhdr, 0, sizeof(conn->tmhdr));
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 	mutex_unlock(&session->eh_mutex);
 }
 
@@ -3157,10 +3129,10 @@  int iscsi_conn_bind(struct iscsi_cls_session *cls_session,
 	struct iscsi_session *session = cls_session->dd_data;
 	struct iscsi_conn *conn = cls_conn->dd_data;
 
-	spin_lock_bh(&session->frwd_lock);
+	spin_lock_bh(&session->lock);
 	if (is_leading)
 		session->leadconn = conn;
-	spin_unlock_bh(&session->frwd_lock);
+	spin_unlock_bh(&session->lock);
 
 	/*
 	 * Unblock xmitworker(), Login Phase will pass through.
diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index 60cb6dc..2f738dd 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -446,7 +446,7 @@  iscsi_tcp_data_recv_prep(struct iscsi_tcp_conn *tcp_conn)
  * iscsi_tcp_cleanup_task - free tcp_task resources
  * @task: iscsi task
  *
- * must be called with session back_lock
+ * must be called with session lock
  */
 void iscsi_tcp_cleanup_task(struct iscsi_task *task)
 {
@@ -457,7 +457,6 @@  void iscsi_tcp_cleanup_task(struct iscsi_task *task)
 	if (!task->sc)
 		return;
 
-	spin_lock_bh(&tcp_task->queue2pool);
 	/* flush task's r2t queues */
 	while (kfifo_out(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*))) {
 		kfifo_in(&tcp_task->r2tpool.queue, (void*)&r2t,
@@ -471,7 +470,6 @@  void iscsi_tcp_cleanup_task(struct iscsi_task *task)
 			    sizeof(void*));
 		tcp_task->r2t = NULL;
 	}
-	spin_unlock_bh(&tcp_task->queue2pool);
 }
 EXPORT_SYMBOL_GPL(iscsi_tcp_cleanup_task);
 
@@ -579,13 +577,11 @@  static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task)
 		return ISCSI_ERR_DATALEN;
 	}
 
-	spin_lock(&tcp_task->pool2queue);
 	rc = kfifo_out(&tcp_task->r2tpool.queue, (void *)&r2t, sizeof(void *));
 	if (!rc) {
 		iscsi_conn_printk(KERN_ERR, conn, "Could not allocate R2T. "
 				  "Target has sent more R2Ts than it "
 				  "negotiated for or driver has leaked.\n");
-		spin_unlock(&tcp_task->pool2queue);
 		return ISCSI_ERR_PROTO;
 	}
 
@@ -600,7 +596,6 @@  static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task)
 	tcp_task->exp_datasn = r2tsn + 1;
 	kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*));
 	conn->r2t_pdus_cnt++;
-	spin_unlock(&tcp_task->pool2queue);
 
 	iscsi_requeue_task(task);
 	return 0;
@@ -673,14 +668,14 @@  iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 
 	switch(opcode) {
 	case ISCSI_OP_SCSI_DATA_IN:
-		spin_lock(&conn->session->back_lock);
+		spin_lock(&conn->session->lock);
 		task = iscsi_itt_to_ctask(conn, hdr->itt);
 		if (!task)
 			rc = ISCSI_ERR_BAD_ITT;
 		else
 			rc = iscsi_tcp_data_in(conn, task);
 		if (rc) {
-			spin_unlock(&conn->session->back_lock);
+			spin_unlock(&conn->session->lock);
 			break;
 		}
 
@@ -713,11 +708,11 @@  iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 						   tcp_conn->in.datalen,
 						   iscsi_tcp_process_data_in,
 						   rx_hash);
-			spin_unlock(&conn->session->back_lock);
+			spin_unlock(&conn->session->lock);
 			return rc;
 		}
 		rc = __iscsi_complete_pdu(conn, hdr, NULL, 0);
-		spin_unlock(&conn->session->back_lock);
+		spin_unlock(&conn->session->lock);
 		break;
 	case ISCSI_OP_SCSI_CMD_RSP:
 		if (tcp_conn->in.datalen) {
@@ -727,20 +722,18 @@  iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 		rc = iscsi_complete_pdu(conn, hdr, NULL, 0);
 		break;
 	case ISCSI_OP_R2T:
-		spin_lock(&conn->session->back_lock);
+		spin_lock(&conn->session->lock);
 		task = iscsi_itt_to_ctask(conn, hdr->itt);
-		spin_unlock(&conn->session->back_lock);
 		if (!task)
 			rc = ISCSI_ERR_BAD_ITT;
 		else if (ahslen)
 			rc = ISCSI_ERR_AHSLEN;
 		else if (task->sc->sc_data_direction == DMA_TO_DEVICE) {
 			task->last_xfer = jiffies;
-			spin_lock(&conn->session->frwd_lock);
 			rc = iscsi_tcp_r2t_rsp(conn, task);
-			spin_unlock(&conn->session->frwd_lock);
 		} else
 			rc = ISCSI_ERR_PROTO;
+		spin_unlock(&conn->session->lock);
 		break;
 	case ISCSI_OP_LOGIN_RSP:
 	case ISCSI_OP_TEXT_RSP:
@@ -988,13 +981,14 @@  EXPORT_SYMBOL_GPL(iscsi_tcp_task_init);
 
 static struct iscsi_r2t_info *iscsi_tcp_get_curr_r2t(struct iscsi_task *task)
 {
+	struct iscsi_session *session = task->conn->session;
 	struct iscsi_tcp_task *tcp_task = task->dd_data;
 	struct iscsi_r2t_info *r2t = NULL;
 
 	if (iscsi_task_has_unsol_data(task))
 		r2t = &task->unsol_r2t;
 	else {
-		spin_lock_bh(&tcp_task->queue2pool);
+		spin_lock_bh(&session->lock);
 		if (tcp_task->r2t) {
 			r2t = tcp_task->r2t;
 			/* Continue with this R2T? */
@@ -1016,7 +1010,7 @@  static struct iscsi_r2t_info *iscsi_tcp_get_curr_r2t(struct iscsi_task *task)
 			else
 				r2t = tcp_task->r2t;
 		}
-		spin_unlock_bh(&tcp_task->queue2pool);
+		spin_unlock_bh(&session->lock);
 	}
 
 	return r2t;
@@ -1146,8 +1140,6 @@  int iscsi_tcp_r2tpool_alloc(struct iscsi_session *session)
 			iscsi_pool_free(&tcp_task->r2tpool);
 			goto r2t_alloc_fail;
 		}
-		spin_lock_init(&tcp_task->pool2queue);
-		spin_lock_init(&tcp_task->queue2pool);
 	}
 
 	return 0;
diff --git a/drivers/scsi/qla4xxx/ql4_isr.c b/drivers/scsi/qla4xxx/ql4_isr.c
index 4f9c0f2..79b278b 100644
--- a/drivers/scsi/qla4xxx/ql4_isr.c
+++ b/drivers/scsi/qla4xxx/ql4_isr.c
@@ -385,9 +385,9 @@  static void qla4xxx_passthru_status_entry(struct scsi_qla_host *ha,
 
 	cls_conn = ddb_entry->conn;
 	conn = cls_conn->dd_data;
-	spin_lock(&conn->session->back_lock);
+	spin_lock(&conn->session->lock);
 	task = iscsi_itt_to_task(conn, itt);
-	spin_unlock(&conn->session->back_lock);
+	spin_unlock(&conn->session->lock);
 
 	if (task == NULL) {
 		ql4_printk(KERN_ERR, ha, "%s: Task is NULL\n", __func__);
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 4d1c46a..52d858c 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -331,19 +331,12 @@  struct iscsi_session {
 	struct iscsi_transport	*tt;
 	struct Scsi_Host	*host;
 	struct iscsi_conn	*leadconn;	/* leading connection */
-	/* Between the forward and the backward locks exists a strict locking
-	 * hierarchy. The mutual exclusion zone protected by the forward lock
-	 * can enclose the mutual exclusion zone protected by the backward lock
-	 * but not vice versa.
-	 */
-	spinlock_t		frwd_lock;	/* protects session state, *
-						 * cmdsn, queued_cmdsn     *
+	spinlock_t		lock;		/* protects session state, *
+						 * sequence numbers,       *
 						 * session resources:      *
-						 * - cmdpool kfifo_out ,   *
-						 * - mgmtpool,		   */
-	spinlock_t		back_lock;	/* protects cmdsn_exp      *
-						 * cmdsn_max,              *
-						 * cmdpool kfifo_in        */
+						 * - cmdpool,		   *
+						 * - mgmtpool,		   *
+						 * - r2tpool		   */
 	int			state;		/* session state           */
 	int			age;		/* counts session re-opens */
 
diff --git a/include/scsi/libiscsi_tcp.h b/include/scsi/libiscsi_tcp.h
index 2a7aa75..215469a 100644
--- a/include/scsi/libiscsi_tcp.h
+++ b/include/scsi/libiscsi_tcp.h
@@ -83,8 +83,6 @@  struct iscsi_tcp_task {
 	struct iscsi_pool	r2tpool;
 	struct kfifo		r2tqueue;
 	void			*dd_data;
-	spinlock_t		pool2queue;
-	spinlock_t		queue2pool;
 };
 
 enum {