On Thu, 2015-05-21 at 20:51 +0200, Bart Van Assche wrote: > On 05/21/15 19:56, Nicholas A. Bellinger wrote: > > I'd prefer you ask the list first before perusing large changes that > > might be based on incorrect assumptions. > > Hello Nic, > > Thanks for proposing that I ask you more about the LIO code. I have a > few questions about what I noticed while I was working on this patch series: > * Why is it that transport_lookup_cmd_lun() sets both SCF_SE_LUN_CMD and > lun_ref_active ? How about dropping the SCF_SE_LUN_CMD flag and keeping > only lun_ref_active ? lun_ref_active is cmpxchg'd immediately after the response hand-off to the fabric for the normal fast-path case. SCF_SE_LUN_CMD is used in transport_generic_free_cmd() to determine if a pre transport_lookup_cmd_lun() failure has occurred. So no, they can't be combined. > * Why is it that transport_lookup_cmd_lun() sets lun_ref_active but > transport_lookup_tmr_lun() not ? Shouldn't removal of a LUN be deferred > until both regular commands and task management functions have finished ? Yes. > * Only three of the many callers of the transport_generic_free_cmd() > function set the second argument "wait_for_tasks" to true. Since the > function transport_wait_for_tasks() is already exported, has it already > been considered to drop that second argument from > transport_generic_free_cmd() and to call transport_wait_for_tasks() > explicitly where needed ? Given transport_wait_for_tasks() is called for two different cases in transport_generic_free_cmd(), pushing this out to the fabrics and expecting them to handle both cases just complicates those cases further. > * transport_wait_for_tasks() sets the CMD_T_STOP flag. I think the iSCSI > initiator needs that functionality. However, the tcm_loop driver also > invokes transport_wait_for_tasks() (indirectly, via > transport_generic_free_cmd()). Is it needed that from the tcm_loop > driver the CMD_T_STOP flag is also set while waiting until a task > management function finishes or does the tcm_loop driver perhaps assume > that the CMD_T_STOP flag does not have any effect on task management > functions ? > All fabrics use CMD_T_STOP via transport_wait_for_tasks() to shutdown a currently active se_cmd. CMD_T_STOP is checked in various locations, and will complete_all(&cmd->t_transport_stop_comp) to stop the in-flight descriptor. The same logic is also used by ABORT_TASK logic for all fabrics, including tcm_loop. For LUN_RESET, CMD_T_REQUEST_STOP is used via target_stop_cmd(). --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
