On 1/30/23 20:22, Devale, Sindhu wrote:
Subject: Re: [PATCH for-rc] IB/isert: Fix hang in iscsit_wait_for_tag
From: Mustafa Ismail <mustafa.ismail@xxxxxxxxx>
Running fio can occasionally cause a hang when sbitmap_queue_get()
fails to return a tag in iscsit_allocate_cmd() and
iscsit_wait_for_tag() is called and will never return from the
schedule(). This is because the polling thread of the CQ is suspended,
and will not poll for a SQ completion which would free up a tag.
Fix this by creating a separate CQ for the SQ so that send completions
are processed on a separate thread and are not blocked when the RQ CQ
is stalled.
Fixes: 10e9cbb6b531 ("scsi: target: Convert target drivers to use
sbitmap")
Is this the real offending commit? What prevented this from happening
before?
Maybe going to a global bitmap instead of per cpu ida makes it less likely to occur.
Going to single CQ maybe the real root cause in this commit:6f0fae3d7797("iser-target: Use single CQ for TX and RX")
Yes this is more likely.
Reviewed-by: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx>
Signed-off-by: Mustafa Ismail <mustafa.ismail@xxxxxxxxx>
Signed-off-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
---
drivers/infiniband/ulp/isert/ib_isert.c | 33 +++++++++++++++++++++++--
--------
drivers/infiniband/ulp/isert/ib_isert.h | 3 ++-
2 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
b/drivers/infiniband/ulp/isert/ib_isert.c
index 7540488..f827b91 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -109,19 +109,27 @@ static int isert_sg_tablesize_set(const char *val,
const struct kernel_param *kp
struct ib_qp_init_attr attr;
int ret, factor;
- isert_conn->cq = ib_cq_pool_get(ib_dev, cq_size, -1,
IB_POLL_WORKQUEUE);
- if (IS_ERR(isert_conn->cq)) {
- isert_err("Unable to allocate cq\n");
- ret = PTR_ERR(isert_conn->cq);
+ isert_conn->snd_cq = ib_cq_pool_get(ib_dev, cq_size, -1,
+ IB_POLL_WORKQUEUE);
+ if (IS_ERR(isert_conn->snd_cq)) {
+ isert_err("Unable to allocate send cq\n");
+ ret = PTR_ERR(isert_conn->snd_cq);
return ERR_PTR(ret);
}
+ isert_conn->rcv_cq = ib_cq_pool_get(ib_dev, cq_size, -1,
+ IB_POLL_WORKQUEUE);
+ if (IS_ERR(isert_conn->rcv_cq)) {
+ isert_err("Unable to allocate receive cq\n");
+ ret = PTR_ERR(isert_conn->rcv_cq);
+ goto create_cq_err;
+ }
Does this have any noticeable performance implications?
Initial testing seems to indicate this change causes significant performance variability specifically only with 2K Writes.
We suspect that may be due an unfortunate vector placement where the snd_cq and rcv_cq are on different numa nodes.
We can, in the patch, alter the second CQ creation to pass comp_vector to insure they are hinted to the same affinity.
Even so, still there are now two competing threads for completion
processing.
Also I wander if there are any other assumptions in the code for having a
single context processing completions...
We don't see any.
It'd be much easier if iscsi_allocate_cmd could accept a timeout to fail...
CCing target-devel and Mike.
Do you mean add a timeout to the wait or removing the call to iscsit_wait_for_tag() iscsit_allocate_cmd()?
Looking at the code, passing it TASK_RUNNING will make it fail if there
no available tag (and hence drop the received command, let the initiator
retry). But I also think that isert may need a deeper default queue
depth...
