On 05/19/15 23:31, Nicholas A. Bellinger wrote:
> On Tue, 2015-05-19 at 16:30 +0200, Bart Van Assche wrote:
>> Instead of invoking transport_lun_remove_cmd() before
>> transport_cmd_check_stop_to_fabric(), inline that function in
>> transport_release_cmd_kref(). This makes it easier to verify
>> that LUN removal occurs before a command is released.
>>
>> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
>> ---
>
> This breaks active I/O LUN shutdown in a couple of different ways.
>
> It's expected the se_lun reference is dropped from the descriptor before
> it's handed back to fabric, because there's no guarantee that the
> descriptor will be released in a reasonable about of time to drop the
> se_lun reference after-the-fact, in order to allow LUN shutdown to
> complete.
>
> Also, se_cmd can fail early which means transport_generic_free_cmd()
> still needs to drop the outstanding reference.
Hello Nic,
If you agree I can start testing the patch below. That patch moves most
transport_lun_remove_cmd() calls into transport_cmd_check_stop_to_fabric()
instead of deferring the transport_lun_remove_cmd() call until a command
has been released.
Thanks,
Bart.
[PATCH] target: Reduce number of transport_lun_remove_cmd() callers
Instead of calling transport_lun_remove_cmd() explicitly before
each transport_cmd_check_stop_to_fabric() call, call the former
function from inside the latter. Note: testing the SCF_SE_LUN_CMD
flag test before calling transport_lun_remove_cmd() is redundant
because that function tests lun_ref_active before dropping the
LUN reference.
---
drivers/target/target_core_transport.c | 31 ++++++++++---------------------
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 0de29d8..f78ac66 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -570,6 +570,14 @@ static void target_remove_from_state_list(struct se_cmd *cmd)
spin_unlock_irqrestore(&dev->execute_task_lock, flags);
}
+static void transport_lun_remove_cmd(struct se_cmd *cmd)
+{
+ struct se_lun *lun = cmd->se_lun;
+
+ if (lun && cmpxchg(&cmd->lun_ref_active, true, false))
+ percpu_ref_put(&lun->lun_ref);
+}
+
static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
bool write_pending)
{
@@ -625,24 +633,13 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
{
- return transport_cmd_check_stop(cmd, true, false);
-}
-
-static void transport_lun_remove_cmd(struct se_cmd *cmd)
-{
- struct se_lun *lun = cmd->se_lun;
-
- if (!lun)
- return;
+ transport_lun_remove_cmd(cmd);
- if (cmpxchg(&cmd->lun_ref_active, true, false))
- percpu_ref_put(&lun->lun_ref);
+ return transport_cmd_check_stop(cmd, true, false);
}
void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
{
- 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()
@@ -1720,7 +1717,6 @@ void transport_generic_request_failure(struct se_cmd *cmd,
goto queue_full;
check_stop:
- transport_lun_remove_cmd(cmd);
if (!transport_cmd_check_stop_to_fabric(cmd))
;
return;
@@ -1972,7 +1968,6 @@ out:
transport_handle_queue_full(cmd, cmd->se_dev);
return;
}
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
}
@@ -2047,7 +2042,6 @@ static void target_complete_ok_work(struct work_struct *work)
if (ret == -EAGAIN || ret == -ENOMEM)
goto queue_full;
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return;
}
@@ -2071,7 +2065,6 @@ static void target_complete_ok_work(struct work_struct *work)
if (ret == -EAGAIN || ret == -ENOMEM)
goto queue_full;
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return;
}
@@ -2097,7 +2090,6 @@ queue_rsp:
if (ret == -EAGAIN || ret == -ENOMEM)
goto queue_full;
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return;
}
@@ -2140,7 +2132,6 @@ queue_rsp:
break;
}
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return;
@@ -3010,8 +3001,6 @@ void transport_send_task_abort(struct se_cmd *cmd)
}
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);
--
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
