On Mon, 2013-05-13 at 16:30 -0400, Joern Engel wrote:
> The second parameter was always 0, leading to effectively dead code. It
> called list_del() and se_cmd->se_tfo->release_cmd(), and had to set a
> flag to prevent target_release_cmd_kref() from doing the same.
Look again. The call to transport_wait_for_tasks() was dead code, but
the wait_for_completion(&se_cmd->cmd_wait_comp) most certainly is not.
> But most
> of all, it iterated the list without taking se_sess->sess_cmd_lock,
> leading to races against ABORT and LUN_RESET.
>
Ugh. You'll recall that target_wait_for_sess_cmds() originally did not
have to take the lock because the list was spliced into
sess_wait_list.
When Roland removed sess_wait_list in commit 1c7b13fe, no one re-added
the lock here.
> Since the whole point of the function is to wait for the list to drain,
> and potentially print a bit of debug information in case that never
> happens, I've replaced the wait_for_completion() with 100ms sleep. The
> only callpath that would get delayed by this is rmmod, afaics, so I
> didn't want the overhead of a waitqueue.
>
This seems totally wrong..
> Signed-off-by: Joern Engel <jo...@logfs.org>
> ---
> drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +-
> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 2 +-
> drivers/target/target_core_transport.c | 64
> +++++++++-----------------------
> include/target/target_core_base.h | 2 -
> include/target/target_core_fabric.h | 2 +-
> 5 files changed, 20 insertions(+), 52 deletions(-)
>
<SNIP>
> diff --git a/drivers/target/target_core_transport.c
> b/drivers/target/target_core_transport.c
> index 0d46276..5b6dbf9 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1043,7 +1043,6 @@ void transport_init_se_cmd(
> init_completion(&cmd->transport_lun_fe_stop_comp);
> init_completion(&cmd->transport_lun_stop_comp);
> init_completion(&cmd->t_transport_stop_comp);
> - init_completion(&cmd->cmd_wait_comp);
> init_completion(&cmd->task_stop_comp);
> spin_lock_init(&cmd->t_state_lock);
> cmd->transport_state = CMD_T_DEV_ACTIVE;
> @@ -2219,11 +2218,6 @@ static void target_release_cmd_kref(struct kref *kref)
> se_cmd->se_tfo->release_cmd(se_cmd);
> return;
> }
> - if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
> - spin_unlock(&se_sess->sess_cmd_lock);
> - complete(&se_cmd->cmd_wait_comp);
> - return;
> - }
> list_del(&se_cmd->se_cmd_list);
> spin_unlock(&se_sess->sess_cmd_lock);
>
> @@ -2241,68 +2235,44 @@ int target_put_sess_cmd(struct se_session *se_sess,
> struct se_cmd *se_cmd)
> }
> EXPORT_SYMBOL(target_put_sess_cmd);
>
> -/* target_sess_cmd_list_set_waiting - Flag all commands in
> - * sess_cmd_list to complete cmd_wait_comp. Set
> +/* target_sess_cmd_list_set_waiting - Set
> * sess_tearing_down so no more commands are queued.
> * @se_sess: session to flag
> */
> void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
> {
> - struct se_cmd *se_cmd;
> unsigned long flags;
>
> spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> -
> WARN_ON(se_sess->sess_tearing_down);
> se_sess->sess_tearing_down = 1;
> -
> - list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list)
> - se_cmd->cmd_wait_set = 1;
> -
> spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> }
> EXPORT_SYMBOL(target_sess_cmd_list_set_waiting);
>
> /* target_wait_for_sess_cmds - Wait for outstanding descriptors
> * @se_sess: session to wait for active I/O
> - * @wait_for_tasks: Make extra transport_wait_for_tasks call
> */
> -void target_wait_for_sess_cmds(
> - struct se_session *se_sess,
> - int wait_for_tasks)
> +void target_wait_for_sess_cmds(struct se_session *se_sess)
> {
> - struct se_cmd *se_cmd, *tmp_cmd;
> - bool rc = false;
> -
> - list_for_each_entry_safe(se_cmd, tmp_cmd,
> - &se_sess->sess_cmd_list, se_cmd_list) {
> - list_del(&se_cmd->se_cmd_list);
> -
> - pr_debug("Waiting for se_cmd: %p t_state: %d, fabric state:"
> - " %d\n", se_cmd, se_cmd->t_state,
> - se_cmd->se_tfo->get_cmd_state(se_cmd));
> -
> - if (wait_for_tasks) {
> - pr_debug("Calling transport_wait_for_tasks se_cmd: %p
> t_state: %d,"
> - " fabric state: %d\n", se_cmd, se_cmd->t_state,
> - se_cmd->se_tfo->get_cmd_state(se_cmd));
> -
> - rc = transport_wait_for_tasks(se_cmd);
> -
> - pr_debug("After transport_wait_for_tasks se_cmd: %p
> t_state: %d,"
> - " fabric state: %d\n", se_cmd, se_cmd->t_state,
> - se_cmd->se_tfo->get_cmd_state(se_cmd));
> - }
> + struct se_cmd *se_cmd, *last_cmd = NULL;
> + unsigned long flags;
>
> - if (!rc) {
> - wait_for_completion(&se_cmd->cmd_wait_comp);
> - pr_debug("After cmd_wait_comp: se_cmd: %p t_state: %d"
> - " fabric state: %d\n", se_cmd, se_cmd->t_state,
> - se_cmd->se_tfo->get_cmd_state(se_cmd));
> + spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> + while (!list_empty(&se_sess->sess_cmd_list)) {
> + se_cmd = list_entry(se_sess->sess_cmd_list.next, struct se_cmd,
> + se_cmd_list);
> + if (se_cmd != last_cmd) { /* print this only once per command */
> + pr_debug("Waiting for se_cmd: %p t_state: %d, fabric
> state: %d\n",
> + se_cmd, se_cmd->t_state,
> + se_cmd->se_tfo->get_cmd_state(se_cmd));
> + last_cmd = se_cmd;
> }
> -
> - se_cmd->se_tfo->release_cmd(se_cmd);
> + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> + msleep_interruptible(100);
> + spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> }
> + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> }
So what happens when the backend se_cmd I/O does not complete before the
msleep finishes..?
It seems totally wrong to drop the initial cmd_wait_set =1 assignment,
target_release_cmd_kref() completion for cmd_wait_comp, and wait on
cmd_wait_comp to allow se_cmd to finish processing here.
Who cares about waitqueue overhead in a shutdown slow path when the
replacement doesn't address long outstanding commands..?
--nab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
