headers
Nicholas A. Bellinger | 1 Sep 2010 01:56

Re: [Open-FCoE] [RFC PATCH] scsi, fcoe, libfc: drop scsi host_lock use from fc_queuecommand

On Tue, 2010-08-31 at 15:53 -0700, Vasu Dev wrote:
> Currently fc_queuecommand drops this lock very early on and then re-acquires
> this lock just before return, this re-acquired lock gets dropped immediately
> by its fast path caller scsi_dispatch_cmd, this re-acquire and then immediate
> drop on each IO hits performance especially with small size IOs on multi-core
> systems, this hit is significant about 25% with 512 bytes size IOs.
> 
> This lock is not needed in fc_queuecommand and calling fc_queuecommand
> without this lock held removes performance hit due to above described
> re-acquire and immediately dropping this lock on each IO.

Hi Vasu,

Very interesting numbers with small blocksizes and your patch.  I recall
trying to make a similar optimization patch myself in early 2.6.0 for
Linux/iSCSI initiators which still obtained struct Scsi_Host->host_lock
before calling struct scsi_host_template->queuecommand() in
drivers/scsi/scsi.c:scsi_dispatch_cmd(), but instead simply did not
attempt disable hard interrupts with spin_lock_irqsave().  I actually
run into some deadlocks way back then, and ended up giving up on the
patch and have not pursued the idea further since..

Anyways the idea is an interesting one for discussion, and it's
interesting to finally see the numbers on this.

I have comment on your patch below..

> 
> So this patch adds unlocked_qcmds flag to drop host_lock before
> calling only fc_queuecommand and no need to re-acquire and then drop
(Continue reading)

Permalink | Reply | 12 comments |
headers
Nicholas A. Bellinger | 1 Sep 2010 02:16

Re: [Open-FCoE] [RFC PATCH] scsi, fcoe, libfc: drop scsi host_lock use from fc_queuecommand

On Tue, 2010-08-31 at 16:56 -0700, Nicholas A. Bellinger wrote:
> On Tue, 2010-08-31 at 15:53 -0700, Vasu Dev wrote:
> > Currently fc_queuecommand drops this lock very early on and then re-acquires
> > this lock just before return, this re-acquired lock gets dropped immediately
> > by its fast path caller scsi_dispatch_cmd, this re-acquire and then immediate
> > drop on each IO hits performance especially with small size IOs on multi-core
> > systems, this hit is significant about 25% with 512 bytes size IOs.
> > 
> > This lock is not needed in fc_queuecommand and calling fc_queuecommand
> > without this lock held removes performance hit due to above described
> > re-acquire and immediately dropping this lock on each IO.
> 
> Hi Vasu,
> 
> Very interesting numbers with small blocksizes and your patch.  I recall
> trying to make a similar optimization patch myself in early 2.6.0 for
> Linux/iSCSI initiators which still obtained struct Scsi_Host->host_lock
> before calling struct scsi_host_template->queuecommand() in
> drivers/scsi/scsi.c:scsi_dispatch_cmd(), but instead simply did not
> attempt disable hard interrupts with spin_lock_irqsave().  I actually
> run into some deadlocks way back then, and ended up giving up on the
> patch and have not pursued the idea further since..
> 
> Anyways the idea is an interesting one for discussion, and it's
> interesting to finally see the numbers on this.
> 
> I have comment on your patch below..
> 
> > 
> > So this patch adds unlocked_qcmds flag to drop host_lock before
(Continue reading)

Permalink | Reply | 0 comments |
headers
James Smart | 1 Sep 2010 04:27
Favicon

[PATCH] scsi_transport_fc: fix blocked bsg request when fc object deleted

When an rport is "blocked" and a bsg request is received, the bsg request gets
placed on the queue but the queue stalls. If the fc object is then deleted - the
bsg queue never restarts and keeps the reference on the object, and stops the
overall teardown.

This patch restarts the bsg queue on teardown and drains any pending requests,
allowing the teardown to succeed.

-- james s

 Signed-off-by: Carl Lajeunesse <carl.lajeunesse <at> emulex.com>
 Signed-off-by: James Smart <james.smart <at> emulex.com>

 ---

 scsi_transport_fc.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff -upNr a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
--- a/drivers/scsi/scsi_transport_fc.c	2010-08-06 12:23:59.000000000 -0400
+++ b/drivers/scsi/scsi_transport_fc.c	2010-08-31 22:14:12.000000000 -0400
 <at>  <at>  -4044,11 +4044,54  <at>  <at>  fc_bsg_rportadd(struct Scsi_Host *shost,
 /**
  * fc_bsg_remove - Deletes the bsg hooks on fchosts/rports
  *  <at> q:	the request_queue that is to be torn down.
+ *
+ * Notes:
+ *   Before unregistering the queue empty any requests that are blocked
+ *
+ *
(Continue reading)

Permalink | Reply | 0 comments |
headers
Mike Christie | 1 Sep 2010 06:17
Picon
Favicon

Re: [Open-FCoE] [RFC PATCH] scsi, fcoe, libfc: drop scsi host_lock use from fc_queuecommand

On 08/31/2010 06:56 PM, Nicholas A. Bellinger wrote:
>> +	if (host->unlocked_qcmds)
>> +		spin_unlock_irqrestore(host->host_lock, flags);
>> +
>>   	if (unlikely(host->shost_state == SHOST_DEL)) {
>>   		cmd->result = (DID_NO_CONNECT<<  16);
>>   		scsi_done(cmd);
>
> I don't think it's safe to call scsi_done() for the SHOST_DEL case here
> with host->unlocked_qcmds=1 w/o holding host_lock, nor would it be safe
> for the SCSI LLD itself using host->unlocked_qcmds=1 to call the
> (*scsi_done)() being passed into sht->queuecommand() without
> host->host_lock being held by either the SCSI ML or the SCSI LLD.

The host state should be checked under the host lock, but I do not think 
it needs to be held with calling scsi_done. scsi_done just queues up the 
request to be completed in the block softirq, and the block layer 
protects against something like the command getting completed by 
multiple code paths/threads.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 10 comments |
headers
Desai, Kashyap | 1 Sep 2010 12:09

RE: [PATCH 3/3] mpt2sas: Bump driver version 06.100.00.03

James,

Any update on my patch set submission?

Thanks,
Kashyap

> -----Original Message-----
> From: linux-scsi-owner <at> vger.kernel.org [mailto:linux-scsi-
> owner <at> vger.kernel.org] On Behalf Of Kashyap, Desai
> Sent: Wednesday, August 04, 2010 2:08 PM
> To: linux-scsi <at> vger.kernel.org
> Cc: James.Bottomley <at> HansenPartnership.com; Moore, Eric; Prakash, Sathya
> Subject: [PATCH 3/3] mpt2sas: Bump driver version 06.100.00.03
> 
> Bump version to 06.100.00.03
> 
> Signed-off-by: Kashyap Desai <kashyap.desai <at> lsi.com>
> ---
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
> b/drivers/scsi/mpt2sas/mpt2sas_base.h
> index 3f390de..0b7e810 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_base.h
> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
>  <at>  <at>  -69,11 +69,11  <at>  <at> 
>  #define MPT2SAS_DRIVER_NAME		"mpt2sas"
>  #define MPT2SAS_AUTHOR	"LSI Corporation <DL-MPTFusionLinux <at> lsi.com>"
>  #define MPT2SAS_DESCRIPTION	"LSI MPT Fusion SAS 2.0 Device Driver"
> -#define MPT2SAS_DRIVER_VERSION		"06.100.00.00"
> +#define MPT2SAS_DRIVER_VERSION		"06.100.00.03"
(Continue reading)

Permalink | Reply | 0 comments |
headers
Nicholas A. Bellinger | 1 Sep 2010 22:10

RE: [Open-FCoE] [RFC PATCH] scsi, fcoe, libfc: drop scsi host_lock use from fc_queuecommand

On Wed, 2010-09-01 at 00:57 -0700, Zou, Yi wrote:
> > 
> > On 08/31/2010 06:56 PM, Nicholas A. Bellinger wrote:
> > >> +	if (host->unlocked_qcmds)
> > >> +		spin_unlock_irqrestore(host->host_lock, flags);
> > >> +
> > >>   	if (unlikely(host->shost_state == SHOST_DEL)) {
> > >>   		cmd->result = (DID_NO_CONNECT<<  16);
> > >>   		scsi_done(cmd);
> > >
> > > I don't think it's safe to call scsi_done() for the SHOST_DEL case here
> > > with host->unlocked_qcmds=1 w/o holding host_lock, nor would it be safe
> > > for the SCSI LLD itself using host->unlocked_qcmds=1 to call the
> > > (*scsi_done)() being passed into sht->queuecommand() without
> > > host->host_lock being held by either the SCSI ML or the SCSI LLD.
> > 
> > The host state should be checked under the host lock, but I do not think
> > it needs to be held with calling scsi_done. scsi_done just queues up the
> > request to be completed in the block softirq, and the block layer
> > protects against something like the command getting completed by
> > multiple code paths/threads.
> 
> It looks safe to me to call scsi_done() w/o host_lock held,

Hmmmm, this indeed this appears to be safe now..  For some reason I had
it in my head (and in TCM_Loop virtual SCSI LLD code as well) that
host_lock needed to be held while calling struct scsi_cmnd->scsi_done().

I assume this is some old age relic from the BLK days in the SCSI
completion path, and the subsequent conversion.  I still see a couple of
(Continue reading)

Permalink | Reply | 8 comments |
headers
Nicholas A. Bellinger | 1 Sep 2010 23:23

[PATCH] tcm_loop: Drop host_lock around struct scsi_cmnd->scsi_done()

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

Greetings all,

This patch updates tcm_loop_queue_data_in() and tcm_loop_queue_status()
to no longer hold struct Scsi_Host->host_lock while calling the
struct scsi_cmnd->scsi_done() callback, in order queue the *sc off into
the block softirq.  This optimization is safely allowed for SCSI LLDs on
modern v2.6 kernels, and is done by the majority of mainline SCSI LLDs.

Thanks to Vasu Dev to the clarification on this item!

Signed-off-by: Nicholas A. Bellinger <nab <at> linux-iscsi.org>
---
 drivers/target/tcm_loop/tcm_loop_fabric.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/target/tcm_loop/tcm_loop_fabric.c b/drivers/target/tcm_loop/tcm_loop_fabric.c
index 588730a..ef23f07 100644
--- a/drivers/target/tcm_loop/tcm_loop_fabric.c
+++ b/drivers/target/tcm_loop/tcm_loop_fabric.c
 <at>  <at>  -379,7 +379,6  <at>  <at>  int tcm_loop_queue_data_in(struct se_cmd *se_cmd)
 	struct tcm_loop_cmd *tl_cmd =
 			(struct tcm_loop_cmd *)se_cmd->se_fabric_cmd_ptr;
 	struct scsi_cmnd *sc = tl_cmd->sc;
-	unsigned long flags;

 	TL_CDB_DEBUG( "tcm_loop_queue_data_in() called for scsi_cmnd: %p"
 			" cdb: 0x%02x\n", sc, sc->cmnd[0]);
 <at>  <at>  -393,10 +392,7  <at>  <at>  int tcm_loop_queue_data_in(struct se_cmd *se_cmd)
(Continue reading)

Permalink | Reply | 0 comments |
headers
Nicholas A. Bellinger | 1 Sep 2010 23:38

RE: [Open-FCoE] [RFC PATCH] scsi, fcoe, libfc: drop scsi host_lock use from fc_queuecommand

On Wed, 2010-09-01 at 14:06 -0700, Vasu Dev wrote:
> On Wed, 2010-09-01 at 13:10 -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2010-09-01 at 00:57 -0700, Zou, Yi wrote:
> > > > 
> > > > On 08/31/2010 06:56 PM, Nicholas A. Bellinger wrote:
> > > > >> +	if (host->unlocked_qcmds)
> > > > >> +		spin_unlock_irqrestore(host->host_lock, flags);
> > > > >> +
> > > > >>   	if (unlikely(host->shost_state == SHOST_DEL)) {
> > > > >>   		cmd->result = (DID_NO_CONNECT<<  16);
> > > > >>   		scsi_done(cmd);
> > > > >
> > > > > I don't think it's safe to call scsi_done() for the SHOST_DEL case here
> > > > > with host->unlocked_qcmds=1 w/o holding host_lock, nor would it be safe
> > > > > for the SCSI LLD itself using host->unlocked_qcmds=1 to call the
> > > > > (*scsi_done)() being passed into sht->queuecommand() without
> > > > > host->host_lock being held by either the SCSI ML or the SCSI LLD.
> > > > 
> > > > The host state should be checked under the host lock, but I do not think
> > > > it needs to be held with calling scsi_done. scsi_done just queues up the
> > > > request to be completed in the block softirq, and the block layer
> > > > protects against something like the command getting completed by
> > > > multiple code paths/threads.
> > > 
> > > It looks safe to me to call scsi_done() w/o host_lock held,
> > 
> > Hmmmm, this indeed this appears to be safe now..  For some reason I had
> > it in my head (and in TCM_Loop virtual SCSI LLD code as well) that
> > host_lock needed to be held while calling struct scsi_cmnd->scsi_done().
> >
(Continue reading)

Permalink | Reply | 3 comments |
headers
Mike Christie | 2 Sep 2010 01:38
Picon
Favicon

Re: [Open-FCoE] [RFC PATCH] scsi, fcoe, libfc: drop scsi host_lock use from fc_queuecommand

On 09/01/2010 05:45 PM, Chris Leech wrote:
> On Wed, Sep 01, 2010 at 02:06:26PM -0700, Vasu Dev wrote:
>>>> It looks safe to me to call scsi_done() w/o host_lock held,
>>>
>>> Hmmmm, this indeed this appears to be safe now..  For some reason I had
>>> it in my head (and in TCM_Loop virtual SCSI LLD code as well) that
>>> host_lock needed to be held while calling struct scsi_cmnd->scsi_done().
>>>
>>> I assume this is some old age relic from the BLK days in the SCSI
>>> completion path, and the subsequent conversion.  I still see a couple of
>>> ancient drivers in drivers/scsi/ that are still doing this, but I
>>> believe I stand corrected in that (all..?) of the modern in-use
>>> drivers/scsi code is indeed *not* holding host_lock while calling struct
>>> scsi_cmnd->scsi_done()..
>>>
>>
>> fcoe/libfc moved to scsi_done w/o holding scsi host_lock a while ago
>> around dec, 09 and it was done after discussion with Mathew and Chris
>> Leech from fcoe side at that time, they may have more to comment on
>> this.
>
> There's not a whole lot to comment on.  Matthew Wilcox was helping me
> look for opportunities to reduce our host_lock use, and said he didn't
> think it was needed around scsi_done anymore.  It held up under testing,
> so I submitted a patch.
>

The host_lock was not actually there for any scsi_done stuff. It was 
probably lazy programming that it was held there. For that code, the 
host_lock was held in fc_queuecommand for the rport check and for the
(Continue reading)

Permalink | Reply | 1 comment |
headers
Mike Christie | 2 Sep 2010 03:37
Picon
Favicon

Re: [Open-FCoE] [RFC PATCH] scsi, fcoe, libfc: drop scsi host_lock use from fc_queuecommand

On 09/01/2010 06:38 PM, Mike Christie wrote:
> wondering if there could be a problem where one thread completes the IO
> and sets those fields to NULL, but another thread could be completing it
> too and it would see a scsi_cmnd that is not released and reallocated by

There should not be a not in there. So it should be and it would see a 
scsi_cmnd that is released and reallocated by the other thread.

> the other thread. So for example the fc_eh_abort code still grabs the
> host_lock when calling CMD_SP and taking a ref and checking that the fsp
> is not null.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |

Gmane