On Wed, 2017-11-15 at 07:06 -0800, Chad Dupuis wrote: > If a cleanup task is not responded to while we are in > bnx2fc_abts_cleanup, it > will hang the SCSI error handler since we use wait_for_completion > instead of > wait_for_completion_timeout. So, use wait_for_completion_timeout so > that we > don't hang the SCSI error handler thread forever. > > Fixes the call trace: > > [183373.131468] INFO: task scsi_eh_16:110146 blocked for more than > 120 seconds. > [183373.131469] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" > disables this message. > [183373.131470] scsi_eh_16 D ffff88103f2fca14 0 > 110146 2 0x00000080 > [183373.131472] ffff880855e77cb0 0000000000000046 ffff881050654e70 > ffff880855e77fd8 > [183373.131474] ffff880855e77fd8 ffff880855e77fd8 ffff881050654e70 > ffff88103f2fcb48 > [183373.131475] ffff88103f2fcb50 7fffffffffffffff ffff881050654e70 > ffff88103f2fca14 > [183373.131477] Call Trace: > [183373.131479] [<ffffffff8168b579>] schedule+0x29/0x70 > [183373.131481] [<ffffffff81688fc9>] schedule_timeout+0x239/0x2d0 > [183373.131486] [<ffffffff8142821e>] ? __dev_printk+0x3e/0x90 > [183373.131487] [<ffffffff814282cd>] ? dev_printk+0x5d/0x80 > [183373.131490] [<ffffffff8168b956>] wait_for_completion+0x116/0x170 > [183373.131492] [<ffffffff810c4ec0>] ? wake_up_state+0x20/0x20 > [183373.131494] [<ffffffffa048c234>] bnx2fc_abts_cleanup+0x3d/0x62 > [bnx2fc] > [183373.131497] [<ffffffffa0483a80>] bnx2fc_eh_abort+0x470/0x580 > [bnx2fc] > [183373.131500] [<ffffffff814570af>] scsi_error_handler+0x59f/0x8b0 > [183373.131501] [<ffffffff81456b10>] ? scsi_eh_get_sense+0x250/0x250 > [183373.131503] [<ffffffff810b052f>] kthread+0xcf/0xe0 > [183373.131505] [<ffffffff810b0460>] ? > kthread_create_on_node+0x140/0x140 > [183373.131507] [<ffffffff81696418>] ret_from_fork+0x58/0x90 > [183373.131509] [<ffffffff810b0460>] ? > kthread_create_on_node+0x140/0x140 > > Signed-off-by: Chad Dupuis <[email protected]> > --- > drivers/scsi/bnx2fc/bnx2fc_io.c | 40 > ++++++++++++++++++++++++++++++++-------- > 1 file changed, 32 insertions(+), 8 deletions(-) > > diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c > b/drivers/scsi/bnx2fc/bnx2fc_io.c > index 5b6153f23f01..2cbb5be98ecb 100644 > --- a/drivers/scsi/bnx2fc/bnx2fc_io.c > +++ b/drivers/scsi/bnx2fc/bnx2fc_io.c > @@ -1084,24 +1084,35 @@ static int bnx2fc_abts_cleanup(struct > bnx2fc_cmd *io_req) > { > struct bnx2fc_rport *tgt = io_req->tgt; > int rc = SUCCESS; > + unsigned int time_left; > > io_req->wait_for_comp = 1; > bnx2fc_initiate_cleanup(io_req); > > spin_unlock_bh(&tgt->tgt_lock); > > - wait_for_completion(&io_req->tm_done); > - > + /* > + * Can't wait forever on cleanup response lest we let the > SCSI error > + * handler wait forever > + */ > + time_left = wait_for_completion_timeout(&io_req->tm_done, > + BNX2FC_FW_TIMEOUT); > io_req->wait_for_comp = 0; > + if (!time_left) > + BNX2FC_IO_DBG(io_req, "%s(): Wait for cleanup timed > out.\n", > + __func__); > + > /* > - * release the reference taken in eh_abort to allow the > - * target to re-login after flushing IOs > + * Release reference held by SCSI command the cleanup > completion > + * hits the BNX2FC_CLEANUP case in bnx2fc_process_cq_compl() > and > + * thus the SCSI command is not returnedi by > bnx2fc_scsi_done(). > */ > kref_put(&io_req->refcount, bnx2fc_cmd_release); > > spin_lock_bh(&tgt->tgt_lock); > return rc; > } > + > /** > * bnx2fc_eh_abort - eh_abort_handler api to abort an outstanding > * SCSI command > @@ -1118,6 +1129,7 @@ int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd) > struct fc_lport *lport; > struct bnx2fc_rport *tgt; > int rc; > + unsigned int time_left; > > rc = fc_block_scsi_eh(sc_cmd); > if (rc) > @@ -1194,6 +1206,11 @@ int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd) > if (cancel_delayed_work(&io_req->timeout_work)) > kref_put(&io_req->refcount, > bnx2fc_cmd_release); /* drop timer > hold */ > + /* > + * We don't want to hold off the upper layer timer > so simply > + * cleanup the command and return that I/O was > successfully > + * aborted. > + */ > rc = bnx2fc_abts_cleanup(io_req); > /* This only occurs when an task abort was requested > while ABTS > is in progress. Setting the IO_CLEANUP flag will > skip the > @@ -1201,7 +1218,7 @@ int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd) > was a result from the ABTS request rather than > the CLEANUP > request */ > set_bit(BNX2FC_FLAG_IO_CLEANUP, &io_req- > >req_flags); > - goto out; > + goto done; > } > > /* Cancel the current timer running on this io_req */ > @@ -1221,7 +1238,11 @@ int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd) > } > spin_unlock_bh(&tgt->tgt_lock); > > - wait_for_completion(&io_req->tm_done); > + /* Wait 2 * RA_TOV + 1 to be sure timeout function hasn't > fired */ > + time_left = wait_for_completion_timeout(&io_req->tm_done, > + (2 * rp->r_a_tov + 1) * HZ); > + if (time_left) > + BNX2FC_IO_DBG(io_req, "Timed out in eh_abort waiting > for tm_done"); > > spin_lock_bh(&tgt->tgt_lock); > io_req->wait_for_comp = 0; > @@ -1233,8 +1254,12 @@ int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd) > /* Let the scsi-ml try to recover this command */ > printk(KERN_ERR PFX "abort failed, xid = 0x%x\n", > io_req->xid); > + /* > + * Cleanup firmware residuals before returning > control back > + * to SCSI ML. > + */ > rc = bnx2fc_abts_cleanup(io_req); > - goto out; > + goto done; > } else { > /* > * We come here even when there was a race condition > @@ -1249,7 +1274,6 @@ int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd) > done: > /* release the reference taken in eh_abort */ > kref_put(&io_req->refcount, bnx2fc_cmd_release); > -out: > spin_unlock_bh(&tgt->tgt_lock); > return rc; > }
We experienced this at a major customer and provided this patch after working with Chad and backported to RHEL as a test kernel. Its been stable while running a test kernel and the changes look good ate least to me. Reviewed-by: Laurence Oberman <[email protected]> Tested-by: Laurence Oberman <[email protected]> Thanks Laurence
