On 10/13/2015 06:18 AM, Christoph Hellwig wrote:
>> - list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
>> + if (kref_get_unless_zero(&cmd->cmd_kref))
>> + list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
>
> Can you add a comment here why we might see a zero reference count, and
> why it's safe to ignore the command in this case?
>
>> @@ -282,8 +264,14 @@ static void core_tmr_drain_state_list(
>> if (prout_cmd == cmd)
>> continue;
>>
>> + if (!kref_get_unless_zero(&cmd->cmd_kref))
>> + continue;
>> +
>
> Same here.
>
>> list_move_tail(&cmd->state_list, &drain_task_list);
>> cmd->state_active = false;
>> + cmd->transport_state |= CMD_T_ABORTED;
>> + if (tmr_nacl && tmr_nacl != cmd->se_sess->se_node_acl && tas)
>> + cmd->send_abort_response = true;
>
> And even if the old code didn't do this either, this condition is magic
> enough to warrant a comment as well.
How about addressing the above three comments via the patch below ?
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index a798189..b0e2599 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -186,6 +186,10 @@ static void core_tmr_drain_tmr_list(
if (target_check_cdb_and_preempt(preempt_and_abort_list, cmd))
continue;
+ /*
+ * The command reference counts drop to zero before the removal
+ * from sess_cmd_list occurs. Hence use kref_get_unless_zero().
+ */
if (kref_get_unless_zero(&cmd->cmd_kref))
list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
}
@@ -205,6 +209,24 @@ static void core_tmr_drain_tmr_list(
}
}
+/**
+ * core_tmr_drain_state_list() - Abort SCSI commands associated with a device.
+ *
+ * @dev: Device for which to abort outstanding SCSI commands.
+ * @prout_cmd: Pointer to the SCSI PREEMPT AND ABORT if this function is called
+ * to realize the PREEMPT AND ABORT functionality.
+ * @tmr_nacl: Initiator port through which the LUN RESET has been received.
+ * @tas: Task Aborted Status (TAS) bit from the SCSI control mode page.
+ * A quote from SPC-4, paragraph "7.5.10 Control mode page":
+ * "A task aborted status (TAS) bit set to zero specifies that
+ * aborted commands shall be terminated by the device server
+ * without any response to the application client. A TAS bit set
+ * to one specifies that commands aborted by the actions of an I_T
+ * nexus other than the I_T nexus on which the command was
+ * received shall be completed with TASK ABORTED status."
+ * @preempt_and_abort_list: For the PREEMPT AND ABORT functionality, a list
+ * with registrations that will be preempted.
+ */
static void core_tmr_drain_state_list(
struct se_device *dev,
struct se_cmd *prout_cmd,
@@ -253,6 +275,10 @@ static void core_tmr_drain_state_list(
if (prout_cmd == cmd)
continue;
+ /*
+ * The command reference counts drop to zero before the removal
+ * from sess_cmd_list occurs. Hence use kref_get_unless_zero().
+ */
if (!kref_get_unless_zero(&cmd->cmd_kref))
continue;
>> +static void transport_handle_abort(struct se_cmd *cmd)
>> +{
>> + transport_lun_remove_cmd(cmd);
>> +
>> + if (cmd->send_abort_response) {
>> + cmd->scsi_status = SAM_STAT_TASK_ABORTED;
>> + pr_debug("Setting SAM_STAT_TASK_ABORTED status for CDB: 0x%02x, ITT: 0x%08llx\n",
>> + cmd->t_task_cdb[0], cmd->tag);
>> + trace_target_cmd_complete(cmd);
>> + cmd->se_tfo->queue_status(cmd);
>> + transport_cmd_check_stop_to_fabric(cmd);
>> + } else {
>> + /*
>> + * Allow the fabric driver to unmap any resources before
>> + * releasing the descriptor via TFO->release_cmd()
>> + */
>> + cmd->se_tfo->aborted_task(cmd);
>> + /*
>> + * To do: establish a unit attention condition on the I_T
>> + * nexus associated with cmd. See also the paragraph "Aborting
>> + * commands" in SAM.
>> + */
>> + if (transport_cmd_check_stop_to_fabric(cmd) == 0)
>> + transport_put_cmd(cmd);
>> + }
>
> Why do we only have the call to transport_cmd_check_stop_to_fabric in
> one of the branches here?
Hmm ... I see a transport_cmd_check_stop_to_fabric() call in both branches.
Maybe that means that I misunderstood your comment ?
Bart.
--
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
