On Tue, 2010-08-31 at 17:06 -0700, Joe Eykholt wrote: > > On 8/31/10 3:53 PM, Vasu Dev wrote: > > Currently fc_queuecommand drops this lock very early on and then re-acquires > > Very early but not right away. See below.
We had really early on upon just enter for really long time until recently we moved fc_remote_port_chkready under host lock. My recent IO errors on cable pull patches led to moving fc_remote_port_chkready under host lock but as I said in patch description that I don't see those IO errors after this patch also for same cable pull since in this case perhaps lport gets out of ready state sooner then any of its rport and that should be preventing anymore new IOs getting thru fc_queuecommand on cable pull. Now with this patch's intent of no more host lock use inside fc_queuecommand If any such issues shows up later then should be fixed differently unless cannot be fixed w/o host lock. Also we have been always checking lport state w/o any lock, so same should be okay with fc_remote_port_chkready as it was until recently moved under host lock. > > > 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. > > > > 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 > > this lock on each IO, this added flag drops this lock only if LLD wants > > as fcoe.ko does here, thus this change won't affect existing LLD since > > added unlocked_qcmds flag will be zero in those cases and their host > > lock uses would remain same after this patch. > > > > I considered having this lock dropped inside fc_queuecommand using irq flags > > saved by its caller then return without re-acquiring this lock but that > > would make this lock usage asymmetric and passing saved irq flags was > > another issue with this approach, so RFC to seek any better possible fix > > for this or any issue with added unlocked_qcmds while not having > > fc_queuecommand under host_lock anymore. I'd appreciate any comments from > > Mike, Joe and folks on open-fcoe first before taking this patch to > > linux-scsi directly as suggested by Rob and Chris here. > > > > I also did cable pull test in middle of IOs with this patch and I don't see > > any issue with that after this patch and some more testing is already done > > successfully with this patch. > > > > Signed-off-by: Vasu Dev <[email protected]> > > --- > > drivers/scsi/fcoe/fcoe.c | 1 + > > drivers/scsi/libfc/fc_fcp.c | 14 +++++--------- > > drivers/scsi/scsi.c | 8 +++++++- > > include/scsi/scsi_host.h | 3 +++ > > 4 files changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c > > index 844d618..280a4df 100644 > > --- a/drivers/scsi/fcoe/fcoe.c > > +++ b/drivers/scsi/fcoe/fcoe.c > > @@ -706,6 +706,7 @@ static int fcoe_shost_config(struct fc_lport *lport, > > struct device *dev) > > lport->host->max_id = FCOE_MAX_FCP_TARGET; > > lport->host->max_channel = 0; > > lport->host->max_cmd_len = FCOE_MAX_CMD_LEN; > > + lport->host->unlocked_qcmds = 1; > > > > if (lport->vport) > > lport->host->transportt = fcoe_vport_transport_template; > > diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c > > index 43866e6..39b6bfa 100644 > > --- a/drivers/scsi/libfc/fc_fcp.c > > +++ b/drivers/scsi/libfc/fc_fcp.c > > @@ -1751,8 +1751,7 @@ static inline int fc_fcp_lport_queue_ready(struct > > fc_lport *lport) > > * @cmd: The scsi_cmnd to be executed > > * @done: The callback function to be called when the scsi_cmnd is > > complete > > * > > - * This is the i/o strategy routine, called by the SCSI layer. This routine > > - * is called with the host_lock held. > > + * This is the i/o strategy routine, called by the SCSI layer. > > */ > > int fc_queuecommand(struct scsi_cmnd *sc_cmd, void (*done)(struct > > scsi_cmnd *)) > > { > > @@ -1770,9 +1769,8 @@ int fc_queuecommand(struct scsi_cmnd *sc_cmd, void > > (*done)(struct scsi_cmnd *)) > > if (rval) { > > sc_cmd->result = rval; > > done(sc_cmd); > > - return 0; > > + return rc; > > } > > Note this isn't quite the first thing. I seem to recall that to avoid races > it's necessary > to call fc_remote_port_chkready(rport) before dropping the lock. There was a > recent patch > to move the lock after that. This is already covered in above comments. > So, maybe the caller can do the chkready for us? This is good suggestion, should be doable for all FC LLDs and that should move up some LLD code into scsi-ml as a common code, I intent to look into that separately after above comments of no immediate need for rport checking under host lock. > I think it's good to eliminate the extra lock taking, somehow. > > Maybe we could just never drop that lock in queuecommand()? I'm not sure how > hard that is. > We'd probably run into lock ordering issues with exchange locks, etc. So > maybe that's not > possible. > Yes, it is very likely that it may cause more issue beside that won't be as efficient as it would be w/o holding the host lock for entire fc_queuecommand context, since this context does lot more to finish sending SCSI cmd thru eth driver and that requires few more allocs beside few more locks grabbing, so better not to do all that under the host lock beside possibly issues with that, so as you said eliminating extra locking is better and with less extending host lock it will be even more better. Thanks for comments Vasu _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
