Re: [PATCH 5/5] nvmet: fcloop: use irqsave spinlocks
From: Sagi Grimberg
Date: Thu May 31 2018 - 05:37:26 EST
Lockdep complains about inconsistent hardirq states when using
nvme-fcloop. So use the irqsave variant of spin_locks for the
nvmefc_fcp_req's reqlock.
Is this because of the nvmet bdev can trigger rx path in interrupt
context? Don't understand exactly what is violated.
Here's the lockdep report:
[ 11.138417] ================================
[ 11.139085] WARNING: inconsistent lock state
[ 11.139207] nvmet: creating controller 2 for subsystem blktests-subsystem-1 for NQN nqn.2014-08.org.nvmexpress:uuid:046394e7-bff7-4403-9502-1816800efaa0.
[ 11.139727] 4.17.0-rc5+ #883 Not tainted
[ 11.139732] --------------------------------
[ 11.144441] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
[ 11.145341] swapper/8/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
[ 11.146108] (ptrval) (&(&tfcp_req->reqlock)->rlock){?.+.}, at: fcloop_fcp_op+0x20/0x1d0 [nvme_fcloop]
[ 11.148633] {HARDIRQ-ON-W} state was registered at:
[ 11.149380] _raw_spin_lock+0x34/0x70
[ 11.149940] fcloop_fcp_recv_work+0x17/0x90 [nvme_fcloop]
[ 11.150746] process_one_work+0x1d8/0x5c0
[ 11.151346] worker_thread+0x45/0x3e0
[ 11.151886] kthread+0x101/0x140
[ 11.152370] ret_from_fork+0x3a/0x50
[ 11.152903] irq event stamp: 36666
[ 11.153402] hardirqs last enabled at (36663): [<ffffffffafc601c3>] default_idle+0x13/0x180
[ 11.154601] hardirqs last disabled at (36664): [<ffffffffafe00964>] interrupt_entry+0xc4/0xe0
[ 11.155817] softirqs last enabled at (36666): [<ffffffffaf267409>] irq_enter+0x59/0x60
[ 11.156952] softirqs last disabled at (36665): [<ffffffffaf2673ee>] irq_enter+0x3e/0x60
[ 11.158092]
[ 11.158092] other info that might help us debug this:
[ 11.159436] Possible unsafe locking scenario:
[ 11.159436]
[ 11.160299] CPU0
[ 11.160663] ----
[ 11.161026] lock(&(&tfcp_req->reqlock)->rlock);
[ 11.161709] <Interrupt>
[ 11.162091] lock(&(&tfcp_req->reqlock)->rlock);
[ 11.163148]
[ 11.163148] *** DEADLOCK ***
[ 11.163148]
[ 11.164007] no locks held by swapper/8/0.
[ 11.164596]
[ 11.164596] stack backtrace:
[ 11.165238] CPU: 8 PID: 0 Comm: swapper/8 Not tainted 4.17.0-rc5+ #883
[ 11.166180] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
[ 11.167673] Call Trace:
[ 11.168037] <IRQ>
[ 11.168349] dump_stack+0x78/0xb3
[ 11.168864] print_usage_bug+0x1ed/0x1fe
[ 11.169440] ? check_usage_backwards+0x110/0x110
[ 11.170111] mark_lock+0x263/0x2a0
[ 11.170995] __lock_acquire+0x675/0x1870
[ 11.171865] ? __lock_acquire+0x48d/0x1870
[ 11.172460] lock_acquire+0xd4/0x220
[ 11.172981] ? fcloop_fcp_op+0x20/0x1d0 [nvme_fcloop]
[ 11.173709] _raw_spin_lock+0x34/0x70
[ 11.174243] ? fcloop_fcp_op+0x20/0x1d0 [nvme_fcloop]
[ 11.174978] fcloop_fcp_op+0x20/0x1d0 [nvme_fcloop]
[ 11.175673] nvmet_fc_transfer_fcp_data+0x9b/0x130 [nvmet_fc]
[ 11.176507] nvmet_req_complete+0x10/0x110 [nvmet]
[ 11.177210] nvmet_bio_done+0x23/0x40 [nvmet]
[ 11.177837] blk_update_request+0xab/0x3b0
[ 11.178434] blk_mq_end_request+0x13/0x60
[ 11.179033] flush_smp_call_function_queue+0x58/0x150
[ 11.179755] smp_call_function_single_interrupt+0x49/0x260
[ 11.180531] call_function_single_interrupt+0xf/0x20
[ 11.181236] </IRQ>
[ 11.181542] RIP: 0010:native_safe_halt+0x2/0x10
[ 11.182186] RSP: 0018:ffffa481006cbec0 EFLAGS: 00000206 ORIG_RAX: ffffffffffffff04
[ 11.183265] RAX: ffff9f54f8b86200 RBX: ffff9f54f8b86200 RCX: 0000000000000000
[ 11.184264] RDX: ffff9f54f8b86200 RSI: 0000000000000001 RDI: ffff9f54f8b86200
[ 11.185270] RBP: 0000000000000008 R08: 0000000000000000 R09: 0000000000000000
[ 11.186271] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
[ 11.187280] R13: 0000000000000000 R14: ffff9f54f8b86200 R15: ffff9f54f8b86200
[ 11.188280] default_idle+0x18/0x180
[ 11.188806] do_idle+0x176/0x260
[ 11.189273] cpu_startup_entry+0x5a/0x60
[ 11.189832] start_secondary+0x18f/0x1b0
Signed-off-by: Johannes Thumshirn <jthumshirn@xxxxxxx>
Cc: James Smart <james.smart@xxxxxxxxxxxx>
---
drivers/nvme/target/fcloop.c | 52 ++++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 23 deletions(-)
diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index d2209c60f95f..f4a3700201c1 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -406,9 +406,10 @@ fcloop_fcp_recv_work(struct work_struct *work)
container_of(work, struct fcloop_fcpreq, fcp_rcv_work);
struct nvmefc_fcp_req *fcpreq = tfcp_req->fcpreq;
int ret = 0;
+ unsigned long flags;
bool aborted = false;
- spin_lock(&tfcp_req->reqlock);
+ spin_lock_irqsave(&tfcp_req->reqlock, flags);
switch (tfcp_req->inistate) {
case INI_IO_START:
tfcp_req->inistate = INI_IO_ACTIVE;
@@ -417,11 +418,11 @@ fcloop_fcp_recv_work(struct work_struct *work)
aborted = true;
break;
default:
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
WARN_ON(1);
return;
}
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
if (unlikely(aborted))
ret = -ECANCELED;
@@ -442,8 +443,9 @@ fcloop_fcp_abort_recv_work(struct work_struct *work)
container_of(work, struct fcloop_fcpreq, abort_rcv_work);
struct nvmefc_fcp_req *fcpreq;
bool completed = false;
+ unsigned long flags;
- spin_lock(&tfcp_req->reqlock);
+ spin_lock_irqsave(&tfcp_req->reqlock, flags);
fcpreq = tfcp_req->fcpreq;
switch (tfcp_req->inistate) {
case INI_IO_ABORTED:
@@ -452,11 +454,11 @@ fcloop_fcp_abort_recv_work(struct work_struct *work)
completed = true;
break;
default:
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
WARN_ON(1);
return;
}
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
if (unlikely(completed)) {
/* remove reference taken in original abort downcall */
@@ -468,9 +470,9 @@ fcloop_fcp_abort_recv_work(struct work_struct *work)
nvmet_fc_rcv_fcp_abort(tfcp_req->tport->targetport,
&tfcp_req->tgt_fcp_req);
- spin_lock(&tfcp_req->reqlock);
+ spin_lock_irqsave(&tfcp_req->reqlock, flags);
tfcp_req->fcpreq = NULL;
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
fcloop_call_host_done(fcpreq, tfcp_req, -ECANCELED);
/* call_host_done releases reference for abort downcall */
@@ -486,11 +488,12 @@ fcloop_tgt_fcprqst_done_work(struct work_struct *work)
struct fcloop_fcpreq *tfcp_req =
container_of(work, struct fcloop_fcpreq, tio_done_work);
struct nvmefc_fcp_req *fcpreq;
+ unsigned long flags;
- spin_lock(&tfcp_req->reqlock);
+ spin_lock_irqsave(&tfcp_req->reqlock, flags);
fcpreq = tfcp_req->fcpreq;
tfcp_req->inistate = INI_IO_COMPLETED;
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
fcloop_call_host_done(fcpreq, tfcp_req, tfcp_req->status);
}
@@ -594,13 +597,14 @@ fcloop_fcp_op(struct nvmet_fc_target_port *tgtport,
u32 rsplen = 0, xfrlen = 0;
int fcp_err = 0, active, aborted;
u8 op = tgt_fcpreq->op;
+ unsigned long flags;
- spin_lock(&tfcp_req->reqlock);
+ spin_lock_irqsave(&tfcp_req->reqlock, flags);
fcpreq = tfcp_req->fcpreq;
active = tfcp_req->active;
aborted = tfcp_req->aborted;
tfcp_req->active = true;
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
if (unlikely(active))
/* illegal - call while i/o active */
@@ -608,9 +612,9 @@ fcloop_fcp_op(struct nvmet_fc_target_port *tgtport,
if (unlikely(aborted)) {
/* target transport has aborted i/o prior */
- spin_lock(&tfcp_req->reqlock);
+ spin_lock_irqsave(&tfcp_req->reqlock, flags);
tfcp_req->active = false;
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
tgt_fcpreq->transferred_length = 0;
tgt_fcpreq->fcp_error = -ECANCELED;
tgt_fcpreq->done(tgt_fcpreq);
@@ -666,9 +670,9 @@ fcloop_fcp_op(struct nvmet_fc_target_port *tgtport,
break;
}
- spin_lock(&tfcp_req->reqlock);
+ spin_lock_irqsave(&tfcp_req->reqlock, flags);
tfcp_req->active = false;
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
tgt_fcpreq->transferred_length = xfrlen;
tgt_fcpreq->fcp_error = fcp_err;
@@ -682,15 +686,16 @@ fcloop_tgt_fcp_abort(struct nvmet_fc_target_port *tgtport,
struct nvmefc_tgt_fcp_req *tgt_fcpreq)
{
struct fcloop_fcpreq *tfcp_req = tgt_fcp_req_to_fcpreq(tgt_fcpreq);
+ unsigned long flags;
/*
* mark aborted only in case there were 2 threads in transport
* (one doing io, other doing abort) and only kills ops posted
* after the abort request
*/
- spin_lock(&tfcp_req->reqlock);
+ spin_lock_irqsave(&tfcp_req->reqlock, flags);
tfcp_req->aborted = true;
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
tfcp_req->status = NVME_SC_INTERNAL;
@@ -726,19 +731,20 @@ fcloop_fcp_abort(struct nvme_fc_local_port *localport,
struct fcloop_ini_fcpreq *inireq = fcpreq->private;
struct fcloop_fcpreq *tfcp_req;
bool abortio = true;
+ unsigned long flags;
- spin_lock(&inireq->inilock);
+ spin_lock_irqsave(&inireq->inilock, flags);
tfcp_req = inireq->tfcp_req;
if (tfcp_req)
fcloop_tfcp_req_get(tfcp_req);
- spin_unlock(&inireq->inilock);
+ spin_unlock_irqrestore(&inireq->inilock, flags);
if (!tfcp_req)
/* abort has already been called */
return;
/* break initiator/target relationship for io */
- spin_lock(&tfcp_req->reqlock);
+ spin_lock_irqsave(&tfcp_req->reqlock, flags);
switch (tfcp_req->inistate) {
case INI_IO_START:
case INI_IO_ACTIVE:
@@ -748,11 +754,11 @@ fcloop_fcp_abort(struct nvme_fc_local_port *localport,
abortio = false;
break;
default:
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
WARN_ON(1);
return;
}
- spin_unlock(&tfcp_req->reqlock);
+ spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
if (abortio)
/* leave the reference while the work item is scheduled */