On Sun, Oct 09, 2011 at 08:28:50AM +0000, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx> > > This patch converts se_cmd->transport_wait_for_tasks(se_cmd, 1) usage to use > transport_generic_free_cmd() directly in target-core and iscsi-target fabric > usage. The includes: > > *) Removal of the optional transport_generic_free_cmd() call from within > transport_generic_wait_for_tasks() > *) Usage of existing SCF_SUPPORTED_SAM_OPCODE to determine when > transport_generic_wait_for_tasks() processing may occur instead of > checking se_cmd->transport_wait_for_tasks() > *) Move transport_generic_wait_for_tasks() call ahead of core_dec_lacl_count() > and transport_lun_remove_cmd() in transport_generic_free_cmd() to follow > existing logic for iscsi-target w/ se_cmd->transport_wait_for_tasks(se_cmd, 1) > *) Removal of se_cmd->transport_wait_for_tasks() function pointer > *) Add EXPORT_SYMBOL for transport_generic_wait_for_tasks() > > For the case in iscsi_target_erl2.c:iscsit_prepare_cmds_for_realligance() > where se_cmd->transport_wait_for_tasks(se_cmd, 0) is called, this patch > adds a direct call to transport_generic_wait_for_tasks(). > > * thread context via iscsit_close_connection() once the other context > @@ -3953,16 +3952,13 @@ static void iscsit_release_commands_from_conn(struct iscsi_conn *conn) > list_del(&cmd->i_list); > spin_unlock_bh(&conn->cmd_lock); > iscsit_increment_maxcmdsn(cmd, sess); > - se_cmd = &cmd->se_cmd; > /* > * Special cases for active iSCSI TMR, and > * transport_lookup_cmd_lun() failing from > * iscsit_get_lun_for_cmd() in iscsit_handle_scsi_cmd(). > */ > - if (cmd->tmr_req && se_cmd->transport_wait_for_tasks) > - se_cmd->transport_wait_for_tasks(se_cmd, 1); > - else if (cmd->se_cmd.se_cmd_flags & SCF_SE_LUN_CMD) > - transport_release_cmd(se_cmd); > + if (cmd->tmr_req) > + transport_generic_free_cmd(&cmd->se_cmd, 0); Why does this not pass 1 to transport_generic_free_cmd? At least the previous code included the wait. > - if (!(cmd->se_cmd.se_cmd_flags & SCF_SE_LUN_CMD) || > - !(cmd->se_cmd.transport_wait_for_tasks)) > + if (!(cmd->se_cmd.se_cmd_flags & SCF_SE_LUN_CMD)) > iscsit_release_cmd(cmd); > else > - cmd->se_cmd.transport_wait_for_tasks( > - &cmd->se_cmd, 1); > + transport_generic_free_cmd(&cmd->se_cmd, 1); With the existing logic a call to transport_generic_free_cmd for a !SCF_SE_LUN_CMD cmd does little more than calling into ->release_cmd, namely: - calling core_tmr_release_req if .se_tmr_req is set - freeing an external cdb buffer any reason we don't want those here (and the other places in the iscsi code using that pattern)? > +void transport_generic_wait_for_tasks(struct se_cmd *cmd) Now that it is exported I'd rather call it target_wait_for_tasks, and add a comment explaining its usage. Preferably like the kerneldoc comments I added in my previous series. -- 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
