On Thu, 17 May 2012 13:49:38 -0700, Roland Dreier wrote: > > > Mmmmm, I'd still like to try to avoid (another) atomic op in the fast > > path w/ hardware_lock held if at all possible.. > > atomic_read() is not an atomic op. Or more importantly it doesn't force the cacheline out of shared mode, which is the performance killer to avoid. > > Also thinking more about the case in your comment, I'm starting to > > wonder if tcm_qla2xxx_free_session() should be obtaining hardware_lock + > > clearing the s_id and loop_id lookup entries earlier than currently > > done.. > > > > That is, target_wait_for_sess_cmds() is called before clearing the > > nodeacl pointers from s_id + loop_id in tcm_qla2xxx_free_session(), > > which means that we still perform se_node_acl lookups in the case where > > the individual tcm_qla2xxx endpoint was not also being shutdown as > > well.. > > > > So that said, I think your work-around is OK to address the current BUG, > > but the s_id + loop_id clearing order in tcm_qla2xxx_free_session() > > would be having an effect here as well.. > > Yes, in fact we need to clear the session from all lookup tables > even earlier than ->free_session. We need to make sure that as > soon as target_release_session() is called, that session will no > longer be used for any new commands. > > So qla_tgt_unreg_sess() needs to clear the session from the > "by loop_id" table as well as the "by fc_id" table. Not even that is sufficient. We need to atomically drop the refcount and remove the session from all lookup tables. Call stack is: tcm_qla2xxx_free_session <-- removes from lookup tables qla_tgt_free_session_done --- <-- move to workqueue, dropping hardware_lock qla_tgt_unreg_sess tcm_qla2xxx_close_session <-- takes hardware_lock target_release_session target_put_session <-- drops refcount Since the lookup tables are protected by hardware_lock, we would have to either take the hardware_lock _before_ dropping the reference count or do atomic_dec_and_lock(). Both of which requires knowledge about the qla2xxx hardware_lock in generic target code. Jörn -- Tough times don't last, but tough people do. -- Nigerian proverb -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
