On 11/23/21 6:21 AM, Manish Rangankar wrote:
> When issued LUN reset under heavy i/o, we hit the qedi WARN_ON
> because of a mismatch in firmware i/o cmd cleanup request count
> and i/o cmd cleanup response count received. The mismatch is
> because of the race caused by the postfix increment of
> cmd_cleanup_cmpl.
>
> [qedi_clearsq:1295]:18: fatal error, need hard reset, cid=0x0
> WARNING: CPU: 48 PID: 110963 at drivers/scsi/qedi/qedi_fw.c:1296 qedi_clearsq+0xa5/0xd0 [qedi]
> CPU: 48 PID: 110963 Comm: kworker/u130:0 Kdump: loaded Tainted: G W
> Hardware name: HPE ProLiant DL385 Gen10/ProLiant DL385 Gen10, BIOS A40 04/15/2020
> Workqueue: iscsi_conn_cleanup iscsi_cleanup_conn_work_fn [scsi_transport_iscsi]
> RIP: 0010:qedi_clearsq+0xa5/0xd0 [qedi]
> RSP: 0018:ffffac2162c7fd98 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff975213c40ab8 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff9761bf816858 RDI: ffff9761bf816858
> RBP: ffff975247018628 R08: 000000000000522c R09: 000000000000005b
> R10: 0000000000000000 R11: ffffac2162c7fbd8 R12: ffff97522e1b2be8
> R13: 0000000000000000 R14: ffff97522e1b2800 R15: 0000000000000001
> FS: 0000000000000000(0000) GS:ffff9761bf800000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f1a34e3e1a0 CR3: 0000000108bb2000 CR4: 0000000000350ee0
> Call Trace:
> qedi_ep_disconnect+0x533/0x550 [qedi]
> ? iscsi_dbg_trace+0x63/0x80 [scsi_transport_iscsi]
> ? _cond_resched+0x15/0x30
> ? iscsi_suspend_queue+0x19/0x40 [libiscsi]
> iscsi_ep_disconnect+0xb0/0x130 [scsi_transport_iscsi]
> iscsi_cleanup_conn_work_fn+0x82/0x130 [scsi_transport_iscsi]
> process_one_work+0x1a7/0x360
> ? create_worker+0x1a0/0x1a0
> worker_thread+0x30/0x390
> ? create_worker+0x1a0/0x1a0
> kthread+0x116/0x130
> ? kthread_flush_work_fn+0x10/0x10
> ret_from_fork+0x22/0x40
> ---[ end trace 5f1441f59082235c ]---
>
> Signed-off-by: Manish Rangankar <mrangankar@xxxxxxxxxxx>
> ---
> drivers/scsi/qedi/qedi_fw.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
> index 84a4204a2cb4..2eed2c6cf424 100644
> --- a/drivers/scsi/qedi/qedi_fw.c
> +++ b/drivers/scsi/qedi/qedi_fw.c
> @@ -813,10 +813,11 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
>
> check_cleanup_reqs:
> if (qedi_conn->cmd_cleanup_req > 0) {
> - QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_TID,
> - "Freeing tid=0x%x for cid=0x%x\n",
> - cqe->itid, qedi_conn->iscsi_conn_id);
> - qedi_conn->cmd_cleanup_cmpl++;
> + ++qedi_conn->cmd_cleanup_cmpl;
> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_SCSI_TM,
> + "Freeing tid=0x%x for cid=0x%x cleanup count=%d\n",
> + cqe->itid, qedi_conn->iscsi_conn_id,
> + qedi_conn->cmd_cleanup_cmpl);
Is the issue that cmd_cleanup_cmpl's increment is not seen by
qedi_cleanup_all_io's wait_event_interruptible_timeout call when it
wakes up, and your patch fixes this by doing a pre increment?
Does doing a pre increment give you barrier like behavior and is that
why this works? I thought if wake_up ends up waking up the other thread it
does a barrier already, so it's not clear to me how changing to a
pre-increment helps.
Is doing a pre-increment a common way to handle this? It looks like we do a
post increment and wake_up* in other places. However, like in the scsi layer
we do wake_up_process and memory-barriers.txt says that always does a general
barrier, so is that why we can do a post increment there?
Does pre-increment give you barrier like behavior, and is the wake_up call
not waking up the process so we didn't get a barrier from that, and so that's
why this works?
> wake_up(&qedi_conn->wait_queue);
> } else {
> QEDI_ERR(&qedi->dbg_ctx,
>
