On Mon, 2011-10-10 at 17:06 -0700, Nicholas A. Bellinger wrote:
> 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:
>
Actually, I was not exactly correct here. This original patch does not
fix a bug, but does introduce a new one where TMRs are not hitting the
transport_wait_for_tasks() path in transport_generic_free_cmd(). Sorry
for the confusion.
Here is the follow-up patch I am including in lio-core-2.6.git to
address this breakage.
--nab
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index ff9a0be..ef65a02 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -4243,9 +4243,12 @@ EXPORT_SYMBOL(transport_release_cmd);
void transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
{
- if (!(cmd->se_cmd_flags & SCF_SE_LUN_CMD))
+ if (!(cmd->se_cmd_flags & SCF_SE_LUN_CMD)) {
+ if (wait_for_tasks && cmd->se_tmr_req)
+ transport_wait_for_tasks(cmd);
+
transport_release_cmd(cmd);
- else {
+ } else {
if (wait_for_tasks)
transport_wait_for_tasks(cmd);
@@ -4450,7 +4453,7 @@ void transport_wait_for_tasks(struct se_cmd *cmd)
* Only perform a possible wait_for_tasks if SCF_SUPPORTED_SAM_OPCODE
* has been set in transport_set_supported_SAM_opcode().
*/
- if (!(cmd->se_cmd_flags & SCF_SUPPORTED_SAM_OPCODE)) {
+ if (!(cmd->se_cmd_flags & SCF_SUPPORTED_SAM_OPCODE) && !cmd->se_tmr_req) {
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
return;
}
--
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
