[PATCH 01/11] libiscsi: Convert to host_lock less w/ interrupts disabled internally

[PATCH 01/11] libiscsi: Convert to host_lock less w/ interrupts disabled internally

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch changes iscsi_queuecommand_lck() to a host_lock less
iscsi_queuecommand() that will internally disable interrupts using
session->lock and drop the now legacy host_lock unlock.

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/libiscsi.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index c15fde8..499f837 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1599,7 +1599,7 @@ enum {
 	FAILURE_SESSION_NOT_READY,
 };
 
-static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
+int iscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
 {
 	struct iscsi_cls_session *cls_session;
 	struct Scsi_Host *host;
@@ -1608,18 +1608,17 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
 	struct iscsi_session *session;
 	struct iscsi_conn *conn;
 	struct iscsi_task *task = NULL;
+	unsigned long irq_flags;
 
-	sc->scsi_done = done;
 	sc->result = 0;
 	sc->SCp.ptr = NULL;
 
 	host = sc->device->host;
 	ihost = shost_priv(host);
-	spin_unlock(host->host_lock);
 
 	cls_session = starget_to_session(scsi_target(sc->device));
 	session = cls_session->dd_data;
-	spin_lock(&session->lock);
+	spin_lock_irqsave(&session->lock, irq_flags);
 
 	reason = iscsi_session_chkready(cls_session);
 	if (reason) {
@@ -1705,25 +1704,23 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
 	}
 
 	session->queued_cmdsn++;
-	spin_unlock(&session->lock);
-	spin_lock(host->host_lock);
+	spin_unlock_irqrestore(&session->lock, irq_flags);
 	return 0;
 
 prepd_reject:
 	sc->scsi_done = NULL;
 	iscsi_complete_task(task, ISCSI_TASK_COMPLETED);
 reject:
-	spin_unlock(&session->lock);
+	spin_unlock_irqrestore(&session->lock, irq_flags);
 	ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
 			  sc->cmnd[0], reason);
-	spin_lock(host->host_lock);
 	return SCSI_MLQUEUE_TARGET_BUSY;
 
 prepd_fault:
 	sc->scsi_done = NULL;
 	iscsi_complete_task(task, ISCSI_TASK_COMPLETED);
 fault:
-	spin_unlock(&session->lock);
+	spin_unlock_irqrestore(&session->lock, irq_flags);
 	ISCSI_DBG_SESSION(session, "iscsi: cmd 0x%x is not queued (%d)\n",
 			  sc->cmnd[0], reason);
 	if (!scsi_bidi_cmnd(sc))
@@ -1732,12 +1729,10 @@ fault:
 		scsi_out(sc)->resid = scsi_out(sc)->length;
 		scsi_in(sc)->resid = scsi_in(sc)->length;
 	}
-	done(sc);
-	spin_lock(host->host_lock);
+	sc->scsi_done(sc);
 	return 0;
 }
 
-DEF_SCSI_QCMD(iscsi_queuecommand)
 EXPORT_SYMBOL_GPL(iscsi_queuecommand);
 
 int iscsi_change_queue_depth(struct scsi_device *sdev, int depth, int reason)
-- 
1.7.2.3

Re: [PATCH 01/11] libiscsi: Convert to host_lock less w/ interrupts disabled internally

[Mike Christie]

On 11/17/2010 04:18 PM, Nicholas A. Bellinger wrote:
>   prepd_fault:
>   	sc->scsi_done = NULL;

> -	done(sc);
> -	spin_lock(host->host_lock);
> +	sc->scsi_done(sc);
>   	return 0;

This will NULL pointer. See a couple lines above where we NULL it. 
iscsi_free_task checks if the scsi_done pointer is set and if it is it 
will call scsi_done.

It is a hack to prevent the normal completion path from calling 
scsi_done. For the case where we return SCSI_MLQUEUE_TARGET_BUSY (the 
prepd_reject case) we need something to prevent scsi_done from getting 
called.

For the return 0/prepd_fault case we can just call sc->scsi_done, but we 
have to move some code around.

I do not like how the code does the NULLing and testing. Let me work on 
this and send a tested patch.

Re: [PATCH 01/11] libiscsi: Convert to host_lock less w/ interrupts disabled internally

[Boaz Harrosh]

On 11/18/2010 03:06 AM, Mike Christie wrote:
> On 11/17/2010 04:18 PM, Nicholas A. Bellinger wrote:
>>   prepd_fault:
>>   	sc->scsi_done = NULL;
> 
>> -	done(sc);
>> -	spin_lock(host->host_lock);
>> +	sc->scsi_done(sc);
>>   	return 0;
> 
> This will NULL pointer. See a couple lines above where we NULL it. 
> iscsi_free_task checks if the scsi_done pointer is set and if it is it 
> will call scsi_done.
> 
> It is a hack to prevent the normal completion path from calling 
> scsi_done. For the case where we return SCSI_MLQUEUE_TARGET_BUSY (the 
> prepd_reject case) we need something to prevent scsi_done from getting 
> called.
> 
> For the return 0/prepd_fault case we can just call sc->scsi_done, but we 
> have to move some code around.
> 
> I do not like how the code does the NULLing and testing. Let me work on 
> this and send a tested patch.

Mike if you are on this. Do you think we need the _irqsave locking.
I always thought that the network receive is not on the HW interrupt
but on a completion thread. Could you verify?

Thanks
Boaz

Re: [PATCH 01/11] libiscsi: Convert to host_lock less w/ interrupts disabled internally

[Mike Christie]

On 11/18/2010 03:59 AM, Boaz Harrosh wrote:
> On 11/18/2010 03:06 AM, Mike Christie wrote:
>> On 11/17/2010 04:18 PM, Nicholas A. Bellinger wrote:
>>>    prepd_fault:
>>>    	sc->scsi_done = NULL;
>>
>>> -	done(sc);
>>> -	spin_lock(host->host_lock);
>>> +	sc->scsi_done(sc);
>>>    	return 0;
>>
>> This will NULL pointer. See a couple lines above where we NULL it.
>> iscsi_free_task checks if the scsi_done pointer is set and if it is it
>> will call scsi_done.
>>
>> It is a hack to prevent the normal completion path from calling
>> scsi_done. For the case where we return SCSI_MLQUEUE_TARGET_BUSY (the
>> prepd_reject case) we need something to prevent scsi_done from getting
>> called.
>>
>> For the return 0/prepd_fault case we can just call sc->scsi_done, but we
>> have to move some code around.
>>
>> I do not like how the code does the NULLing and testing. Let me work on
>> this and send a tested patch.
>
> Mike if you are on this. Do you think we need the _irqsave locking.
> I always thought that the network receive is not on the HW interrupt
> but on a completion thread. Could you verify?
>

Yeah, we do not need the irqsave locking, because for software iscsi the 
session lock is just locked in softirqs/timers and the iscsi xmit 
thread. For offload and iser (be2iscsi, bnx2i, iser and cxgb*) we have 
similar combinations (some drivers have a tasklet thrown in there too) 
and no real interrupts.

qla4xxx does not use the iscsi_queuecommand function.

Re: [PATCH 01/11] libiscsi: Convert to host_lock less w/ interrupts disabled internally

[Nicholas A. Bellinger]

<Trimming CC list>

On Thu, 2010-11-18 at 12:26 -0600, Mike Christie wrote:
> On 11/18/2010 03:59 AM, Boaz Harrosh wrote:
> > On 11/18/2010 03:06 AM, Mike Christie wrote:
> >> On 11/17/2010 04:18 PM, Nicholas A. Bellinger wrote:
> >>>    prepd_fault:
> >>>    	sc->scsi_done = NULL;
> >>
> >>> -	done(sc);
> >>> -	spin_lock(host->host_lock);
> >>> +	sc->scsi_done(sc);
> >>>    	return 0;
> >>
> >> This will NULL pointer. See a couple lines above where we NULL it.
> >> iscsi_free_task checks if the scsi_done pointer is set and if it is it
> >> will call scsi_done.
> >>
> >> It is a hack to prevent the normal completion path from calling
> >> scsi_done. For the case where we return SCSI_MLQUEUE_TARGET_BUSY (the
> >> prepd_reject case) we need something to prevent scsi_done from getting
> >> called.
> >>
> >> For the return 0/prepd_fault case we can just call sc->scsi_done, but we
> >> have to move some code around.
> >>
> >> I do not like how the code does the NULLing and testing. Let me work on
> >> this and send a tested patch.
> >
> > Mike if you are on this. Do you think we need the _irqsave locking.
> > I always thought that the network receive is not on the HW interrupt
> > but on a completion thread. Could you verify?
> >
> 
> Yeah, we do not need the irqsave locking, because for software iscsi the 
> session lock is just locked in softirqs/timers and the iscsi xmit 
> thread. For offload and iser (be2iscsi, bnx2i, iser and cxgb*) we have 
> similar combinations (some drivers have a tasklet thrown in there too) 
> and no real interrupts.
> 

Indeed, I have been meaning to ping you about this..  I had originally
assumed that the software case would be OK to drop this, but was unsure
about the HW offload case.  Thanks for the clarification here that we
can gain the extra benefit here..

I am commiting the following incremental patch to
lock_less-LLDs-for-38-v2, and will apply your forthcoming NULL
sc->scsi_done() cleanup patch on top of this for v3.

Thanks Mike!

--nab


>From 51f6d901d9b175c6a83c31eb1ffe3ac0fc0a7b5b Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Thu, 18 Nov 2010 23:31:01 +0000
Subject: [PATCH] libiscsi: Do not disable interrupts around queuecommand session->lock

This patch follows Mike Christie's recommendation that there is no hard
requirement for either Open-iSCSI software mode or existing HW offload
to disable interrupts around session->lock in iscsi_queuecommand().

Reported-by: Mike Christie <michaelc@cs.wisc.edu>
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/libiscsi.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 499f837..5535782 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1608,7 +1608,6 @@ int iscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
        struct iscsi_session *session;
        struct iscsi_conn *conn;
        struct iscsi_task *task = NULL;
-       unsigned long irq_flags;

        sc->result = 0;
        sc->SCp.ptr = NULL;
@@ -1618,7 +1617,7 @@ int iscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)

        cls_session = starget_to_session(scsi_target(sc->device));
        session = cls_session->dd_data;
-       spin_lock_irqsave(&session->lock, irq_flags);
+       spin_lock(&session->lock);

        reason = iscsi_session_chkready(cls_session);
        if (reason) {
@@ -1704,14 +1703,14 @@ int iscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
        }

        session->queued_cmdsn++;
-       spin_unlock_irqrestore(&session->lock, irq_flags);
+       spin_unlock(&session->lock);
        return 0;

 prepd_reject:
        sc->scsi_done = NULL;
        iscsi_complete_task(task, ISCSI_TASK_COMPLETED);
 reject:
-       spin_unlock_irqrestore(&session->lock, irq_flags);
+       spin_unlock(&session->lock);
        ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
                          sc->cmnd[0], reason);
        return SCSI_MLQUEUE_TARGET_BUSY;
@@ -1720,7 +1719,7 @@ prepd_fault:
        sc->scsi_done = NULL;
        iscsi_complete_task(task, ISCSI_TASK_COMPLETED);
 fault:
-       spin_unlock_irqrestore(&session->lock, irq_flags);
+       spin_unlock(&session->lock);
        ISCSI_DBG_SESSION(session, "iscsi: cmd 0x%x is not queued (%d)\n",
                          sc->cmnd[0], reason);
        if (!scsi_bidi_cmnd(sc))
-- 
1.7.2.3

[PATCH 01/11] libiscsi: Convert to host_lock less w/ interrupts disabled internally

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch changes iscsi_queuecommand_lck() to a host_lock less
iscsi_queuecommand() that will internally disable interrupts using
session->lock and drop the now legacy host_lock unlock.

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/libiscsi.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index c15fde8..8880e7e 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1599,7 +1599,7 @@ enum {
 	FAILURE_SESSION_NOT_READY,
 };
 
-static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
+int iscsi_queuecommand(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
 {
 	struct iscsi_cls_session *cls_session;
 	struct Scsi_Host *host;
@@ -1608,6 +1608,7 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
 	struct iscsi_session *session;
 	struct iscsi_conn *conn;
 	struct iscsi_task *task = NULL;
+	unsigned long irq_flags;
 
 	sc->scsi_done = done;
 	sc->result = 0;
@@ -1615,11 +1616,10 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
 
 	host = sc->device->host;
 	ihost = shost_priv(host);
-	spin_unlock(host->host_lock);
 
 	cls_session = starget_to_session(scsi_target(sc->device));
 	session = cls_session->dd_data;
-	spin_lock(&session->lock);
+	spin_lock_irqsave(&session->lock, irq_flags);
 
 	reason = iscsi_session_chkready(cls_session);
 	if (reason) {
@@ -1705,25 +1705,23 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
 	}
 
 	session->queued_cmdsn++;
-	spin_unlock(&session->lock);
-	spin_lock(host->host_lock);
+	spin_unlock_irqrestore(&session->lock, irq_flags);
 	return 0;
 
 prepd_reject:
 	sc->scsi_done = NULL;
 	iscsi_complete_task(task, ISCSI_TASK_COMPLETED);
 reject:
-	spin_unlock(&session->lock);
+	spin_unlock_irqrestore(&session->lock, irq_flags);
 	ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
 			  sc->cmnd[0], reason);
-	spin_lock(host->host_lock);
 	return SCSI_MLQUEUE_TARGET_BUSY;
 
 prepd_fault:
 	sc->scsi_done = NULL;
 	iscsi_complete_task(task, ISCSI_TASK_COMPLETED);
 fault:
-	spin_unlock(&session->lock);
+	spin_unlock_irqrestore(&session->lock, irq_flags);
 	ISCSI_DBG_SESSION(session, "iscsi: cmd 0x%x is not queued (%d)\n",
 			  sc->cmnd[0], reason);
 	if (!scsi_bidi_cmnd(sc))
@@ -1733,11 +1731,8 @@ fault:
 		scsi_in(sc)->resid = scsi_in(sc)->length;
 	}
 	done(sc);
-	spin_lock(host->host_lock);
 	return 0;
 }
-
-DEF_SCSI_QCMD(iscsi_queuecommand)
 EXPORT_SYMBOL_GPL(iscsi_queuecommand);
 
 int iscsi_change_queue_depth(struct scsi_device *sdev, int depth, int reason)
-- 
1.7.2.3