[PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[Jeff Garzik]

This is in #upstream-fixes...  Each libata command is now two lock-ops
less expensive... for free!  Should be nice for multi-core SSD users etc.

(not a pull request, Linus, just an FYI)


commit 02ab5c49284c91901cad783921e816297c92ebf8
Author: Jeff Garzik <jeff@garzik.org>
Date:   Wed Nov 17 01:19:11 2010 -0500

    [libata] kill unlock+relock cycle in SCSI queuecommand hook
    
    Now that SCSI's ->queuecommand hook is unlocked, ATA no longer needs
    to unlock the SCSI host_lock prior to work, and re-lock the host_lock
    once our work is complete.
    
    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

 drivers/ata/libata-scsi.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 19835d3..d72340e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3166,8 +3166,8 @@ static inline int __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
 
 /**
  *	ata_scsi_queuecmd - Issue SCSI cdb to libata-managed device
+ *	@shost: SCSI host destination for given command
  *	@cmd: SCSI command to be sent
- *	@done: Completion function, called when command is complete
  *
  *	In some cases, this function translates SCSI commands into
  *	ATA taskfiles, and queues the taskfiles to be sent to
@@ -3183,36 +3183,41 @@ static inline int __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
  *	Return value from __ata_scsi_queuecmd() if @cmd can be queued,
  *	0 otherwise.
  */
-static int ata_scsi_queuecmd_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
+int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 {
 	struct ata_port *ap;
 	struct ata_device *dev;
 	struct scsi_device *scsidev = cmd->device;
-	struct Scsi_Host *shost = scsidev->host;
 	int rc = 0;
+	unsigned long irq_flags;
 
-	ap = ata_shost_to_port(shost);
+	local_irq_save(irq_flags);
 
+	spin_lock(shost->host_lock);
+	scsi_cmd_get_serial(shost, cmd);
 	spin_unlock(shost->host_lock);
+
+	ap = ata_shost_to_port(shost);
+
 	spin_lock(ap->lock);
 
 	ata_scsi_dump_cdb(ap, cmd);
 
 	dev = ata_scsi_find_dev(ap, scsidev);
 	if (likely(dev))
-		rc = __ata_scsi_queuecmd(cmd, done, dev);
+		rc = __ata_scsi_queuecmd(cmd, cmd->scsi_done, dev);
 	else {
 		cmd->result = (DID_BAD_TARGET << 16);
-		done(cmd);
+		cmd->scsi_done(cmd);
 	}
 
 	spin_unlock(ap->lock);
-	spin_lock(shost->host_lock);
+
+	local_irq_restore(irq_flags);
+
 	return rc;
 }
 
-DEF_SCSI_QCMD(ata_scsi_queuecmd)
-
 /**
  *	ata_scsi_simulate - simulate SCSI command on ATA device
  *	@dev: the target device

Re: [PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[Linus Torvalds]

On Tue, Nov 16, 2010 at 10:29 PM, Jeff Garzik <jeff@garzik.org> wrote:
>
> +       spin_lock(shost->host_lock);
> +       scsi_cmd_get_serial(shost, cmd);
>        spin_unlock(shost->host_lock);

This is just sad.

How important is that serial number? So important that we need to do a
spinlock over it here? And it _must_ be per-shost?

Because if you made it per-ap, you could easily just move that logic
down to after you get the ap lock.

Just add a "unsigned int serial_number" into the ata_port struct, and
do the same (trivial) logic for getting a non-zero serial number
there:

  n = ap->serial_number;
  if (!++n)
    n = 1
  ap->serial_number = n;
  cmd->serial_number = n;

or something. And then you'd _really_ not need to touch that totally
pointless host lock at all.

               Linus

Re: [PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[Jeff Garzik]

On 11/17/2010 01:44 AM, Linus Torvalds wrote:
> On Tue, Nov 16, 2010 at 10:29 PM, Jeff Garzik<jeff@garzik.org>  wrote:
>>
>> +       spin_lock(shost->host_lock);
>> +       scsi_cmd_get_serial(shost, cmd);
>>         spin_unlock(shost->host_lock);
>
> This is just sad.
>
> How important is that serial number? So important that we need to do a
> spinlock over it here? And it _must_ be per-shost?

Quite unimportant.  Quoting James from 
http://marc.info/?l=linux-kernel&m=128949079704323&w=2
> There are only a few drivers left that actually make use of a serial
> number.  Of those, the only modern ones are qla4, lpfc, mpt2sas and
> megaraid.
>
> So the next logical step seems to be eliminate the overloading of the
> serial number zero value, which removes the last mid-layer use (dpt_i2o
> seems to abuse this unnecessarily as well), then the serial number code
> can be pushed down into the queuecommand routines of only those drivers
> that actually use it.  None of the modern ones seems to have a
> legitimate use, so I think their uses can mostly be eliminated.  Thus,
> we might be able to get away with a simple queuecommand push down and
> never bother with atomics for this (since it's unlikely the legacy users
> would convert away from a lock wrapping their queuecommand routines).


Looking solely at the SCSI code (ie. ignoring LLD code), it seems like 
the magic number zero for serial_number is signaling a boolean condition 
"are we an EH command?"

EH tests this at the very beginning of the abort/reset/explode error 
handling sequence, presumably to avoid recursive EH invocations 
(scsi_try_to_abort_cmd).

So maybe an EH expert (Tejun?) can correct me here, but I think we may 
be able to completely the lock/get-serial/unlock sequence from libata, 
as long as scsi_init_cmd_errh() reliably sets an "I am an EH command" flag.

Would be nice if true...

	Jeff

Re: [PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[Jeff Garzik]

On 11/17/2010 03:08 AM, Jeff Garzik wrote:
> So maybe an EH expert (Tejun?) can correct me here, but I think we may
> be able to completely the lock/get-serial/unlock sequence from libata,
		^^^ er, completely remove

Re: [PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[Tejun Heo]

Hello, Jeff, Linus.

On 11/17/2010 09:08 AM, Jeff Garzik wrote:
> Looking solely at the SCSI code (ie. ignoring LLD code), it seems
> like the magic number zero for serial_number is signaling a boolean
> condition "are we an EH command?"
> 
> EH tests this at the very beginning of the abort/reset/explode error
> handling sequence, presumably to avoid recursive EH invocations
> (scsi_try_to_abort_cmd).
>
> So maybe an EH expert (Tejun?) can correct me here, but I think we
> may be able to completely the lock/get-serial/unlock sequence from
> libata, as long as scsi_init_cmd_errh() reliably sets an "I am an EH
> command" flag.
> 
> Would be nice if true...

Yeah, it's actually nice (for once).  libata doesn't use or care about
scmd->serial_number at all.  The SCSI EH path you mentioned above is
not applicable as libata implements its eh_strategy_handler and SCSI
only calls scsi_try_to_abort_cmd() for the default EH handler,
scsi_unjam_host().

We'll need to test a bit to make sure everything is okay but I'm
fairly certain removing it won't break anything fundamental.  If
something breaks at all, it would be some silly easy-to-fix thing.

Thanks.

-- 
tejun

Re: [PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[Jeff Garzik]

On 11/17/2010 05:01 AM, Tejun Heo wrote:
> Hello, Jeff, Linus.
>
> On 11/17/2010 09:08 AM, Jeff Garzik wrote:
>> Looking solely at the SCSI code (ie. ignoring LLD code), it seems
>> like the magic number zero for serial_number is signaling a boolean
>> condition "are we an EH command?"
>>
>> EH tests this at the very beginning of the abort/reset/explode error
>> handling sequence, presumably to avoid recursive EH invocations
>> (scsi_try_to_abort_cmd).
>>
>> So maybe an EH expert (Tejun?) can correct me here, but I think we
>> may be able to completely the lock/get-serial/unlock sequence from
>> libata, as long as scsi_init_cmd_errh() reliably sets an "I am an EH
>> command" flag.
>>
>> Would be nice if true...
>
> Yeah, it's actually nice (for once).  libata doesn't use or care about
> scmd->serial_number at all.  The SCSI EH path you mentioned above is
> not applicable as libata implements its eh_strategy_handler and SCSI
> only calls scsi_try_to_abort_cmd() for the default EH handler,
> scsi_unjam_host().
>
> We'll need to test a bit to make sure everything is okay but I'm
> fairly certain removing it won't break anything fundamental.  If
> something breaks at all, it would be some silly easy-to-fix thing.

It would be surprising if there is breakage, because serial_number is 
only tested in two places in the generic kernel:

scsi_cmd_get_serial() -- where it simply avoids the zero value -- and 
scsi_try_to_abort_cmd().

	Jeff

Re: [PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[James Bottomley]

On Wed, 2010-11-17 at 03:08 -0500, Jeff Garzik wrote:
> On 11/17/2010 01:44 AM, Linus Torvalds wrote:
> > On Tue, Nov 16, 2010 at 10:29 PM, Jeff Garzik<jeff@garzik.org>  wrote:
> >>
> >> +       spin_lock(shost->host_lock);
> >> +       scsi_cmd_get_serial(shost, cmd);
> >>         spin_unlock(shost->host_lock);
> >
> > This is just sad.
> >
> > How important is that serial number? So important that we need to do a
> > spinlock over it here? And it _must_ be per-shost?
> 
> Quite unimportant.  Quoting James from 
> http://marc.info/?l=linux-kernel&m=128949079704323&w=2
> > There are only a few drivers left that actually make use of a serial
> > number.  Of those, the only modern ones are qla4, lpfc, mpt2sas and
> > megaraid.
> >
> > So the next logical step seems to be eliminate the overloading of the
> > serial number zero value, which removes the last mid-layer use (dpt_i2o
> > seems to abuse this unnecessarily as well), then the serial number code
> > can be pushed down into the queuecommand routines of only those drivers
> > that actually use it.  None of the modern ones seems to have a
> > legitimate use, so I think their uses can mostly be eliminated.  Thus,
> > we might be able to get away with a simple queuecommand push down and
> > never bother with atomics for this (since it's unlikely the legacy users
> > would convert away from a lock wrapping their queuecommand routines).
> 
> 
> Looking solely at the SCSI code (ie. ignoring LLD code), it seems like 
> the magic number zero for serial_number is signaling a boolean condition 
> "are we an EH command?"
> 
> EH tests this at the very beginning of the abort/reset/explode error 
> handling sequence, presumably to avoid recursive EH invocations 
> (scsi_try_to_abort_cmd).
> 
> So maybe an EH expert (Tejun?) can correct me here, but I think we may 
> be able to completely the lock/get-serial/unlock sequence from libata, 
> as long as scsi_init_cmd_errh() reliably sets an "I am an EH command" flag.
> 
> Would be nice if true...

Right ... I couldn't persuade anyone else to do it, so I'll probably end
up coding it.  It looks like the serial number zero check might be
bogus.  If so I'll remove it, then push the serial number acquisition
down only into the locked queuecommand of only those drivers that
actually use it (which were listed in the quoted email, and which might
have a big impetus to remove it if the use is trivial).  Then we can
begin unwinding the locking.

James

Re: [PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[Christoph Hellwig]

> Right ... I couldn't persuade anyone else to do it, so I'll probably end
> up coding it.  It looks like the serial number zero check might be
> bogus.  If so I'll remove it, then push the serial number acquisition
> down only into the locked queuecommand of only those drivers that
> actually use it (which were listed in the quoted email, and which might
> have a big impetus to remove it if the use is trivial).  Then we can
> begin unwinding the locking.

In dc395x, eata_pio, in2000, lpfc, megaraid_legacy, megaraid_mbox,
megaraid_sas, mesh, ncr53c8xx, qla1280, qla4xxx, wd33c93 and i2o_scsi
use it in printks only, and all this can be safely removed.

Except for EH that only leaves dpt_i2o, eata, mpt2sas, u14-34f, mptscsi
as non-trivial users.  If you sort out the EH side I'll volunteer to
kill the printks and add private serial numbers to the five drivers
mentioned above.

Re: [PATCH] libata: remove unlock+relock cycle in ata_scsi_queuecmd

[James Bottomley]

On Wed, 2010-11-17 at 10:47 -0500, Christoph Hellwig wrote:
> > Right ... I couldn't persuade anyone else to do it, so I'll probably end
> > up coding it.  It looks like the serial number zero check might be
> > bogus.  If so I'll remove it, then push the serial number acquisition
> > down only into the locked queuecommand of only those drivers that
> > actually use it (which were listed in the quoted email, and which might
> > have a big impetus to remove it if the use is trivial).  Then we can
> > begin unwinding the locking.
> 
> In dc395x, eata_pio, in2000, lpfc, megaraid_legacy, megaraid_mbox,
> megaraid_sas, mesh, ncr53c8xx, qla1280, qla4xxx, wd33c93 and i2o_scsi
> use it in printks only, and all this can be safely removed.
> 
> Except for EH that only leaves dpt_i2o, eata, mpt2sas, u14-34f, mptscsi
> as non-trivial users.  If you sort out the EH side I'll volunteer to
> kill the printks and add private serial numbers to the five drivers
> mentioned above.

You're on (you were cc'd on the patch) ... I was planning just to move
the scsi_cmnd_get_serial() call into the locked queuecommand routine,
but a more permanent solution might be better.

James