On Tue, Oct 13, 2015 at 10:01:45AM -0700, Bart Van Assche wrote:
> 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().
> + */
I don't think this comment adds much value. The way I understand the
code (and I haven't had much time to go through it lately) the only way
for the refcount to be zero is because the command is in the process
of being completed. So maybe something like this:
/*
* Don't try to move a command without a zero refount,
* it has been completed already as will be removed from
* sess_cmd_list ASAP; no need to abort it.
*/
>
> +/**
> + * 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.
> + */
This looks fine.
> >> + 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 ?
I miswrote the comment, sorry. I mean the call to transport_put_cmd.
--
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
