On 01/07/2016 04:46 AM, Bart Van Assche wrote:
> On 01/06/2016 11:42 PM, Nicholas A. Bellinger wrote:
>> You've utterly ignored everything mentioned in the review from November:
>
> Hello Nic,
>
> Please note that two comments from that e-mail have already been
> addressed. The two comments about session shutdown have been addressed
> through patch "[PATCH 09/21] target: Simplify session shutdown code". I
> will also address the other comments before I repost this patch series.
Hi Nic and Christoph,
Please find below a new version of the patch that makes ABORT and LUN RESET
handling synchronous. This patch is based on the latest for-next branch of
the target code with on top Nic's patches for handling a LUN RESET for
active I/O correctly. All previous review comments should have been addressed
in this patch. Feedback is welcome.
Thanks,
Bart.
From: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
Subject: [PATCH] target: Make ABORT and LUN RESET handling synchronous
Instead of invoking target driver callback functions from the
context that handles an abort or LUN RESET task management function,
only set the abort flag from that context and perform the actual
abort handling from the context of the regular command processing
flow. This approach has the following advantages:
- The task management function code becomes much easier to read and
to verify since the number of potential race conditions against
the command processing flow is strongly reduced.
- It is no longer needed to store the command state into the command
itself since that information is no longer needed from the context
where a task management function is processed.
Notes:
- The target_remove_from_state_list() call in transport_cmd_check_stop()
has been moved up such that it is called without t_state_lock held.
This is necessary to avoid triggering lock inversion against
core_tmr_drain_state_list(), which now locks execute_task_lock and
t_state_lock in that order.
- With this patch applied the CMD_T_ABORTED flag is checked at two points
by the target core: just before local execution of a command starts
(see also target_execute_cmd()) and also just before the response is
sent (see also target_complete_cmd()).
Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
Cc: Andy Grover <agrover@xxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Sagi Grimberg <sagig@xxxxxxxxxxxx>
---
drivers/target/target_core_internal.h | 3 -
drivers/target/target_core_tmr.c | 71 +++++++-------
drivers/target/target_core_transport.c | 166 +++++++++++----------------------
include/target/target_core_base.h | 5 +-
4 files changed, 89 insertions(+), 156 deletions(-)
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index dae0750c..e810d49 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -132,7 +132,6 @@ int init_se_kmem_caches(void);
void release_se_kmem_caches(void);
u32 scsi_get_new_index(scsi_index_t);
void transport_subsystem_check_init(void);
-void transport_cmd_finish_abort(struct se_cmd *, int);
unsigned char *transport_dump_cmd_direction(struct se_cmd *);
void transport_dump_dev_state(struct se_device *, char *, int *);
void transport_dump_dev_info(struct se_device *, struct se_lun *,
@@ -141,9 +140,7 @@ void transport_dump_vpd_proto_id(struct t10_vpd *, unsigned char *, int);
int transport_dump_vpd_assoc(struct t10_vpd *, unsigned char *, int);
int transport_dump_vpd_ident_type(struct t10_vpd *, unsigned char *, int);
int transport_dump_vpd_ident(struct t10_vpd *, unsigned char *, int);
-bool target_stop_cmd(struct se_cmd *cmd, unsigned long *flags);
void transport_clear_lun_ref(struct se_lun *);
-void transport_send_task_abort(struct se_cmd *);
sense_reason_t target_cmd_size_check(struct se_cmd *cmd, unsigned int size);
void target_qf_do_work(struct work_struct *work);
bool target_check_wce(struct se_device *dev);
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 377ecb3..3166a0d 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -76,23 +76,6 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
kfree(tmr);
}
-static void core_tmr_handle_tas_abort(
- struct se_node_acl *tmr_nacl,
- struct se_cmd *cmd,
- int tas)
-{
- bool remove = true;
- /*
- * TASK ABORTED status (TAS) bit support
- */
- if ((tmr_nacl && (tmr_nacl != cmd->se_sess->se_node_acl)) && tas) {
- remove = false;
- transport_send_task_abort(cmd);
- }
-
- transport_cmd_finish_abort(cmd, remove);
-}
-
static int target_check_cdb_and_preempt(struct list_head *list,
struct se_cmd *cmd)
{
@@ -171,13 +154,13 @@ void core_tmr_abort_task(
goto out;
}
- list_del_init(&se_cmd->se_cmd_list);
+ se_cmd->send_abort_response = false;
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
- cancel_work_sync(&se_cmd->work);
- transport_wait_for_tasks(se_cmd);
+ while (!wait_for_completion_timeout(&se_cmd->finished, 180 * HZ))
+ pr_debug("ABORT TASK: still waiting for ITT %#llx\n",
+ ref_tag);
- transport_cmd_finish_abort(se_cmd, true);
target_put_sess_cmd(se_cmd);
printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
@@ -199,7 +182,6 @@ static void core_tmr_drain_tmr_list(
struct list_head *preempt_and_abort_list)
{
LIST_HEAD(drain_tmr_list);
- struct se_session *sess;
struct se_tmr_req *tmr_p, *tmr_pp;
struct se_cmd *cmd;
unsigned long flags;
@@ -267,14 +249,31 @@ static void core_tmr_drain_tmr_list(
(preempt_and_abort_list) ? "Preempt" : "", tmr_p,
tmr_p->function, tmr_p->response, cmd->t_state);
- cancel_work_sync(&cmd->work);
- transport_wait_for_tasks(cmd);
-
- transport_cmd_finish_abort(cmd, 1);
+ while (!wait_for_completion_timeout(&cmd->finished, 180 * HZ))
+ pr_debug("LUN RESET: still waiting for ITT %#llx\n",
+ cmd->tag);
target_put_sess_cmd(cmd);
}
}
+/**
+ * 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,
@@ -312,6 +311,10 @@ static void core_tmr_drain_state_list(
*/
spin_lock_irqsave(&dev->execute_task_lock, flags);
list_for_each_entry_safe(cmd, next, &dev->state_list, state_list) {
+ /* Skip task management functions. */
+ if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
+ continue;
+
/*
* For PREEMPT_AND_ABORT usage, only process commands
* with a matching reservation key.
@@ -345,6 +348,8 @@ static void core_tmr_drain_state_list(
}
list_move_tail(&cmd->state_list, &drain_task_list);
cmd->state_active = false;
+ if (tmr_nacl && tmr_nacl != cmd->se_sess->se_node_acl && tas)
+ cmd->send_abort_response = true;
}
spin_unlock_irqrestore(&dev->execute_task_lock, flags);
@@ -367,17 +372,9 @@ static void core_tmr_drain_state_list(
(cmd->transport_state & CMD_T_STOP) != 0,
(cmd->transport_state & CMD_T_SENT) != 0);
- /*
- * If the command may be queued onto a workqueue cancel it now.
- *
- * This is equivalent to removal from the execute queue in the
- * loop above, but we do it down here given that
- * cancel_work_sync may block.
- */
- cancel_work_sync(&cmd->work);
- transport_wait_for_tasks(cmd);
-
- core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
+ while (!wait_for_completion_timeout(&cmd->finished, 180 * HZ))
+ pr_debug("LUN RESET: still waiting for cmd with ITT %#llx\n",
+ cmd->tag);
target_put_sess_cmd(cmd);
}
}
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 2a2a6c8..0675b2a 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -561,10 +561,6 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
{
unsigned long flags;
- spin_lock_irqsave(&cmd->t_state_lock, flags);
- if (write_pending)
- cmd->t_state = TRANSPORT_WRITE_PENDING;
-
if (remove_from_lists) {
target_remove_from_state_list(cmd);
@@ -574,6 +570,10 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
cmd->se_lun = NULL;
}
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ if (write_pending)
+ cmd->t_state = TRANSPORT_WRITE_PENDING;
+
/*
* Determine if frontend context caller is requesting the stopping of
* this command for frontend exceptions.
@@ -584,7 +584,6 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
- complete_all(&cmd->t_transport_stop_comp);
return 1;
}
@@ -625,25 +624,6 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
percpu_ref_put(&lun->lun_ref);
}
-void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
-{
- bool ack_kref = (cmd->se_cmd_flags & SCF_ACK_KREF);
-
- if (cmd->se_cmd_flags & SCF_SE_LUN_CMD)
- transport_lun_remove_cmd(cmd);
- /*
- * Allow the fabric driver to unmap any resources before
- * releasing the descriptor via TFO->release_cmd()
- */
- if (remove)
- cmd->se_tfo->aborted_task(cmd);
-
- if (transport_cmd_check_stop_to_fabric(cmd))
- return;
- if (remove && ack_kref)
- transport_put_cmd(cmd);
-}
-
static void target_complete_failure_work(struct work_struct *work)
{
struct se_cmd *cmd = container_of(work, struct se_cmd, work);
@@ -675,14 +655,55 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd)
return cmd->sense_buffer;
}
+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_generic_free_cmd(cmd, 0);
+ }
+}
+
+static void target_abort_work(struct work_struct *work)
+{
+ struct se_cmd *cmd = container_of(work, struct se_cmd, work);
+
+ transport_handle_abort(cmd);
+}
+
+/* May be called from interrupt context so must not sleep. */
void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
{
struct se_device *dev = cmd->se_dev;
int success = scsi_status == GOOD;
unsigned long flags;
- cmd->scsi_status = scsi_status;
+ if (cmd->transport_state & CMD_T_ABORTED) {
+ INIT_WORK(&cmd->work, target_abort_work);
+ goto queue_work;
+ } else if (cmd->transport_state & (CMD_T_REQUEST_STOP | CMD_T_STOP)) {
+ return;
+ }
+ cmd->scsi_status = scsi_status;
spin_lock_irqsave(&cmd->t_state_lock, flags);
cmd->transport_state &= ~CMD_T_BUSY;
@@ -695,25 +716,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
success = 1;
}
- /*
- * See if we are waiting to complete for an exception condition.
- */
- if (cmd->transport_state & CMD_T_REQUEST_STOP) {
- spin_unlock_irqrestore(&cmd->t_state_lock, flags);
- complete(&cmd->task_stop_comp);
- return;
- }
-
- /*
- * Check for case where an explicit ABORT_TASK has been received
- * and transport_wait_for_tasks() will be waiting for completion..
- */
- if (cmd->transport_state & CMD_T_ABORTED &&
- cmd->transport_state & CMD_T_STOP) {
- spin_unlock_irqrestore(&cmd->t_state_lock, flags);
- complete_all(&cmd->t_transport_stop_comp);
- return;
- } else if (!success) {
+ if (!success) {
INIT_WORK(&cmd->work, target_complete_failure_work);
} else {
INIT_WORK(&cmd->work, target_complete_ok_work);
@@ -723,6 +726,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
cmd->transport_state |= (CMD_T_COMPLETE | CMD_T_ACTIVE);
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+queue_work:
queue_work_on(cmd->cpuid, target_completion_wq, &cmd->work);
}
EXPORT_SYMBOL(target_complete_cmd);
@@ -1200,9 +1204,8 @@ void transport_init_se_cmd(
INIT_LIST_HEAD(&cmd->se_qf_node);
INIT_LIST_HEAD(&cmd->se_cmd_list);
INIT_LIST_HEAD(&cmd->state_list);
- init_completion(&cmd->t_transport_stop_comp);
init_completion(&cmd->cmd_wait_comp);
- init_completion(&cmd->task_stop_comp);
+ init_completion(&cmd->finished);
spin_lock_init(&cmd->t_state_lock);
kref_init(&cmd->cmd_kref);
cmd->transport_state = CMD_T_DEV_ACTIVE;
@@ -1636,33 +1639,6 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
EXPORT_SYMBOL(target_submit_tmr);
/*
- * If the cmd is active, request it to be stopped and sleep until it
- * has completed.
- */
-bool target_stop_cmd(struct se_cmd *cmd, unsigned long *flags)
- __releases(&cmd->t_state_lock)
- __acquires(&cmd->t_state_lock)
-{
- bool was_active = false;
-
- if (cmd->transport_state & CMD_T_BUSY) {
- cmd->transport_state |= CMD_T_REQUEST_STOP;
- spin_unlock_irqrestore(&cmd->t_state_lock, *flags);
-
- pr_debug("cmd %p waiting to complete\n", cmd);
- wait_for_completion(&cmd->task_stop_comp);
- pr_debug("cmd %p stopped successfully\n", cmd);
-
- spin_lock_irqsave(&cmd->t_state_lock, *flags);
- cmd->transport_state &= ~CMD_T_REQUEST_STOP;
- cmd->transport_state &= ~CMD_T_BUSY;
- was_active = true;
- }
-
- return was_active;
-}
-
-/*
* Handle SAM-esque emulation for generic transport request failures.
*/
void transport_generic_request_failure(struct se_cmd *cmd,
@@ -1872,16 +1848,13 @@ void target_execute_cmd(struct se_cmd *cmd)
* Determine if frontend context caller is requesting the stopping of
* this command for frontend exceptions.
*/
- spin_lock_irq(&cmd->t_state_lock);
if (cmd->transport_state & CMD_T_STOP) {
pr_debug("%s:%d CMD_T_STOP for ITT: 0x%08llx\n",
__func__, __LINE__, cmd->tag);
-
- spin_unlock_irq(&cmd->t_state_lock);
- complete_all(&cmd->t_transport_stop_comp);
return;
}
+ spin_lock_irq(&cmd->t_state_lock);
cmd->t_state = TRANSPORT_PROCESSING;
cmd->transport_state |= CMD_T_ACTIVE|CMD_T_BUSY|CMD_T_SENT;
spin_unlock_irq(&cmd->t_state_lock);
@@ -2518,6 +2491,8 @@ static void target_release_cmd_kref(struct kref *kref)
struct se_session *se_sess = se_cmd->se_sess;
unsigned long flags;
+ complete_all(&se_cmd->finished);
+
spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
if (list_empty(&se_cmd->se_cmd_list)) {
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
@@ -2651,7 +2626,7 @@ bool transport_wait_for_tasks(struct se_cmd *cmd)
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
- wait_for_completion(&cmd->t_transport_stop_comp);
+ wait_for_completion(&cmd->finished);
spin_lock_irqsave(&cmd->t_state_lock, flags);
cmd->transport_state &= ~(CMD_T_ACTIVE | CMD_T_STOP);
@@ -2867,41 +2842,6 @@ int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
}
EXPORT_SYMBOL(transport_check_aborted_status);
-void transport_send_task_abort(struct se_cmd *cmd)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&cmd->t_state_lock, flags);
- if (cmd->se_cmd_flags & (SCF_SENT_CHECK_CONDITION)) {
- spin_unlock_irqrestore(&cmd->t_state_lock, flags);
- return;
- }
- spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-
- /*
- * If there are still expected incoming fabric WRITEs, we wait
- * until until they have completed before sending a TASK_ABORTED
- * response. This response with TASK_ABORTED status will be
- * queued back to fabric module by transport_check_aborted_status().
- */
- if (cmd->data_direction == DMA_TO_DEVICE) {
- if (cmd->se_tfo->write_pending_status(cmd) != 0) {
- cmd->transport_state |= CMD_T_ABORTED;
- cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
- return;
- }
- }
- cmd->scsi_status = SAM_STAT_TASK_ABORTED;
-
- transport_lun_remove_cmd(cmd);
-
- 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);
-}
-
static void target_tmr_work(struct work_struct *work)
{
struct se_cmd *cmd = container_of(work, struct se_cmd, work);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 47a0d6a..abcc0ad 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -446,6 +446,7 @@ struct se_cmd {
unsigned cmd_wait_set:1;
unsigned unknown_data_length:1;
bool state_active:1;
+ bool send_abort_response:1;
u64 tag; /* SAM command identifier aka task tag */
/* Delay for ALUA Active/NonOptimized state access in milliseconds */
int alua_nonop_delay;
@@ -497,6 +498,7 @@ struct se_cmd {
spinlock_t t_state_lock;
struct kref cmd_kref;
struct completion t_transport_stop_comp;
+ struct completion finished;
struct work_struct work;
@@ -513,9 +515,6 @@ struct se_cmd {
struct list_head state_list;
- /* old task stop completion, consider merging with some of the above */
- struct completion task_stop_comp;
-
/* backend private data */
void *priv;
--
2.1.4
--
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
