On Mon, 2011-10-10 at 07:35 -0400, Christoph Hellwig wrote:
> On Sun, Oct 09, 2011 at 10:32:23PM -0700, Nicholas A. Bellinger wrote:
> > > > 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.
>
> Let's beack get to the original case in the current tree:
> (cut down a bit):
>
> if (!(cmd->se_cmd.se_cmd_flags & SCF_SE_LUN_CMD)) {
> ...
> if (cmd->tmr_req && se_cmd->transport_wait_for_tasks)
> se_cmd->transport_wait_for_tasks(se_cmd, 1, 1);
> else if (cmd->se_cmd.se_cmd_flags & SCF_SE_LUN_CMD)
> transport_release_cmd(se_cmd);
> else
> iscsit_release_cmd(cmd);
> ...
> continue;
> }
> ...
> if (se_cmd->transport_wait_for_tasks)
> se_cmd->transport_wait_for_tasks(se_cmd, 1, 1);
> }
>
> given that all surrounding code is the same, and we double
> check SCF_SE_LUN_CMD, it could be simplified down to:
>
>
> if ((cmd->se_cmd.se_cmd_flags & SCF_SE_LUN_CMD) ||
> (cmd->tmr_req && se_cmd->transport_wait_for_tasks))
> se_cmd->transport_wait_for_tasks(se_cmd, 1, 1);
> else
> iscsit_release_cmd(cmd);
>
>
> Now transport_generic_wait_for_tasks has always special cased
> the cmd->se_tmr_req to go ahead, even if generally doesn't for the
> !SCF_SE_LUN_CMD.
>
> Now you know the code a lot better than me and I'm fine if you tell
> me the original code was wrong and needs to changed for this and that
> reason, but I'd really prefer to have this done in a patch just fixing
> the bug and explaining why instead of hiding it in a large conversion.
>
There is no bug being fixed here. Both the conversion in this patch,
along with the above mentioned follow up patch to iscsi-target to remove
SCF_SE_LUN_CMD abuses end up following the exact same codepaths for all
cases into transport_generic_free_cmd().
So like I said, the change your focusing here does not change what's
actually happening in transport_wait_for_tasks() for TMRs:
@@ -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);
else
iscsit_release_cmd(cmd);
and the follow-up patch drops iscsit_release_commands_from_conn()
special cases in favor of a direct iscsit_free_cmd() call (from your
comments) based on individual iscsi_opcode here:
iscsi-target: Remove SCF_SE_LUN_CMD flag abuses
http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=0be67f2ed8f577d2c72d917928394c5885fa9134
@@ -3947,30 +3939,13 @@ static void iscsit_release_commands_from_conn(struct iscsi_conn *conn)
*/
spin_lock_bh(&conn->cmd_lock);
list_for_each_entry_safe(cmd, cmd_tmp, &conn->conn_cmd_list, i_list) {
- if (!(cmd->se_cmd.se_cmd_flags & SCF_SE_LUN_CMD)) {
-
- list_del(&cmd->i_list);
- spin_unlock_bh(&conn->cmd_lock);
- iscsit_increment_maxcmdsn(cmd, sess);
- /*
- * 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)
- transport_generic_free_cmd(&cmd->se_cmd, 0);
- else
- iscsit_release_cmd(cmd);
- spin_lock_bh(&conn->cmd_lock);
- continue;
- }
list_del(&cmd->i_list);
spin_unlock_bh(&conn->cmd_lock);
iscsit_increment_maxcmdsn(cmd, sess);
- transport_generic_free_cmd(&cmd->se_cmd, 1);
+ iscsit_free_cmd(cmd);
spin_lock_bh(&conn->cmd_lock);
}
--
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
