On 11/15/2015 04:28 PM, Nicholas A. Bellinger wrote:
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..?
To ensure that anyone who runs a bisect does not run into the crash
mentioned below.
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().
That's correct.
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.
Thanks for the feedback. I will drop this patch.
Bart.
--
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
