On Thu, 2015-10-22 at 15:57 -0700, Bart Van Assche wrote:
> This patch avoids that the following kernel crash can occur with
> later patches in this patch series:
Why this is preceding the other patches..?
>
> general protection fault: 0000 [#1] SMP
> CPU: 0 PID: 6 Comm: kworker/u8:0 Not tainted 4.3.0-rc1-debug+ #1
> Workqueue: tmr-fileio target_tmr_work [target_core_mod]
> Call Trace:
> [<ffffffff810a6915>] lock_acquire+0x65/0x90
> [<ffffffff815e740b>] _raw_spin_lock_irqsave+0x4b/0x60
> [<ffffffffa03bc7ca>] target_release_cmd_kref+0x2a/0xa0 [target_core_mod]
> [<ffffffffa03bd418>] target_put_sess_cmd+0x28/0x50 [target_core_mod]
> [<ffffffffa03bad50>] core_tmr_lun_reset+0x390/0x640 [target_core_mod]
> [<ffffffffa03bce50>] target_tmr_work+0x80/0xd0 [target_core_mod]
> [<ffffffff81070e6d>] process_one_work+0x19d/0x430
> [<ffffffff8107120f>] worker_thread+0x10f/0x460
> [<ffffffff810772ba>] kthread+0xea/0x100
> [<ffffffff815e7a2f>] ret_from_fork+0x3f/0x70
>
I assume this is with FCoE target, which as-is is not accounting for
outstanding I/Os before invoking transport_free_session().
> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> Cc: Andy Grover <agrover@xxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxx>
> Cc: Sagi Grimberg <sagig@xxxxxxxxxxxx>
> ---
> drivers/target/target_core_transport.c | 9 +++++++++
> include/target/target_core_base.h | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 7f4d3ac..4d7b1bc 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -239,6 +239,7 @@ struct se_session *transport_init_session(enum target_prot_op sup_prot_ops)
> INIT_LIST_HEAD(&se_sess->sess_cmd_list);
> INIT_LIST_HEAD(&se_sess->sess_wait_list);
> spin_lock_init(&se_sess->sess_cmd_lock);
> + init_waitqueue_head(&se_sess->cmd_list_wq);
> kref_init(&se_sess->sess_kref);
> se_sess->sup_prot_ops = sup_prot_ops;
>
> @@ -464,6 +465,12 @@ EXPORT_SYMBOL(transport_deregister_session_configfs);
>
> void transport_free_session(struct se_session *se_sess)
> {
> + spin_lock_irq(&se_sess->sess_cmd_lock);
> + wait_event_lock_irq(se_sess->cmd_list_wq,
> + list_empty(&se_sess->sess_cmd_list),
> + se_sess->sess_cmd_lock);
> + spin_unlock_irq(&se_sess->sess_cmd_lock);
> +
> if (se_sess->sess_cmd_map) {
> percpu_ida_destroy(&se_sess->sess_tag_pool);
> kvfree(se_sess->sess_cmd_map);
The reason your hitting this is because FCoE target doesn't use
target_sess_cmd_list_set_waiting() + target_wait_for_sess_cmds() during
se_session shutdown, which target_submit_cmd() + target_submit_tmr()
depends on for checking se_session->sess_tearing_down, and why
target_release_cmd_kref() does complete(&se_cmd->cmd_wait_comp);
So really, this needs to be a BUG_ON(!list_empty()) for sess_cmd_list +
sess_wait_list in transport_free_session(), and FCoE target needs to be
converted to use these primitives following tcm_qla2xxx, ib_isert, and
ib_srpt function.
Adding a hack for FCoE target is not going to cut it.
> @@ -2524,6 +2531,8 @@ static void target_release_cmd_kref(struct kref *kref)
> return;
> }
> list_del(&se_cmd->se_cmd_list);
> + if (list_empty(&se_cmd->se_cmd_list))
> + wake_up(&se_sess->cmd_list_wq);
> spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
>
> se_cmd->se_tfo->release_cmd(se_cmd);
Unnecessary fast-path wake-up for every I/O. No thanks.
--
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
