On Sat, 2011-11-05 at 06:50 -0400, Christoph Hellwig wrote: > On Fri, Nov 04, 2011 at 02:07:09PM -0700, Nicholas A. Bellinger wrote: > > One of the two modules (tcm_qla2xxx) in lio-core.git that have been > > converted to use this does not call transport_handle_cdb(), and instead > > uses process context provided by TFO->new_cmd_map() after being queued > > up from interrupt context. Having this as an external caller atm gives > > me a little more flexibility when debugging active I/O shutdown issues > > with ib_srpt and tcm_qla2xxx, but will eventually need to be included > > into core callers as you have observed. > > We really need to get rid of new_cmd_map in its current from anyway - it's > one of those abuses of the I/O processing threads that is rather in the way > of parallelizing it. Yep, this makes sense as a v3.3 core along with qla2xxx > > > Also, I wanted this code to be 'opt-in' as iscsi-target still does a > > number of special things wrt to shutdown that are on a per iscsi_conn > > basis, for which a conversion currently to per session tracking does not > > really make sense. > > IMHO the maintainance of the list should be done uncdontionally. > There's nothing worse but data structure linkages that are different > depending on users. Actually making use of them is a different thing. <nod>, it should be eventually enabled by default as there are more fabrics that need per session command tracking than not. However I think it would still be useful to optionally disable it if the fabric really does not need it by virtue of stopping all active I/O threads during shutdown (iscsi-target / tcm_fc) or keeping per-connection lists (iscsi-target). > > > The release path in target_put_sess_cmd() was broken up into a seperate > > caller as tcm_qla2xxx currently may sleep between > > transport_generic_free_cmd() and TFO->check_release_cmd() for module > > dependent reasons, which is why this became an external function to > > start with. Also, there is a special case with ib_srpt where this is > > called individually by the srpt_abort_cmd() callback as well. > > I think this is a major design issue in the qla2xxx driver and really > shouldn't creep into the core. I'd really prefer not to add more hacks > until qla2xxx is whacked into a sane model - that is make sure target > core code is always called form user context by using workqueues inside > qla2xxx (we're getting there for the command freeing, but even the > current version is a bit half-assed), Completely agreed on this, but the bit mentioned above for tcm_qla2xxx having to wait until ->check_stop_free() has been called in the release path in not related to incoming I/O workqueue conversion.. > make sure the whole > cmd->locked_rsp crap goes away - which should be a fairly easy fallout > from calling qla_tgt_handle_cmd_for_atio from a workqueue without > qla2xxx-specific locks held, <nod>, already looking into this.. > and doing some major cleanp of > qla_tgt_sess_work_fn and it's subfunctions, that is: > > - use one struct work item per work item, instead of keeping > a list that gets run from one work item and thus kill the whole > struct qla_tgt_sess_work_param crapola and sess_work_lock mess. > - de-multiplex qla_tgt_exec_sess_work into one function per > different functionality > - audit hardware_lock usage in these, and cut them down to the > actual required critical sections > All very reasonable to cleanups for this code. We need to post an v3.3 RFC for this very soon to get the ball rolling on the changes required to existing qla2xxx LLD code, and i'll start getting these qla_target.c specific items addressed in parallel. --nab -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
