On Sun, 2011-10-09 at 07:53 -0400, Christoph Hellwig wrote: > 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. Because the 'wait_for_tasks' check in transport_generic_free_cmd() is not in the block for !SCF_SE_LUN_CMD with this TMR case. Also, in the iscsi-target patch to remove SCF_SE_LUN_CMD abuses, the special case block of code in iscsit_release_commands_from_conn() from above has been removed all-together. > > > - 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)? > Nope, this is fine. > > +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. > Yep, was thinking about this as well but left it as is in the original patch considering the transport_generic_free_cmd() naming.. Changed to target_wait_for_tasks() and added a docbook comment. --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
