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.
--
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
