From: Mike Christie <michael.christie@oracle.com>
To: lduncan@suse.com, cleech@redhat.com, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org, jejb@linux.ibm.com
Cc: Mike Christie <michael.christie@oracle.com>, Wu Bo <wubo40@huawei.com>
Subject: [PATCH 6/9] scsi: iscsi: Remove unneeded task state check
Date: Thu, 16 Jun 2022 17:45:54 -0500 [thread overview]
Message-ID: <20220616224557.115234-7-michael.christie@oracle.com> (raw)
In-Reply-To: <20220616224557.115234-1-michael.christie@oracle.com>
Commit 5923d64b7ab6 ("scsi: libiscsi: Drop taskqueuelock") added an extra
task->state because for commit 6f8830f5bbab ("scsi: libiscsi: add lock
around task lists to fix list corruption regression") we didn't know why we
ended up with cmds on the list and thought it might have been a bad target
sending a response while we were still sending the cmd. We were never able
to get a target to send us a response early, because it turns out the bug
was just a race in libiscsi/libiscsi_tcp where:
1. iscsi_tcp_r2t_rsp() queues a r2t to tcp_task->r2tqueue.
2. iscsi_tcp_task_xmit() runs iscsi_tcp_get_curr_r2t() and sees we have a
r2t. It dequeues it and iscsi_tcp_task_xmit() starts to process it.
3. iscsi_tcp_r2t_rsp() runs iscsi_requeue_task() and puts the task on the
requeue list.
4. iscsi_tcp_task_xmit() sends the data for r2t. This is the final chunk
of data, so the cmd is done.
5. target sends the response.
6. On a different CPU from #3, iscsi_complete_task() processes the
response. Since there was no common lock for the list, the lists/tasks
pointers are not fully in sync, so could end up with list corruption.
Since it was just a race on our side, remove the extra check and fix up the
comments.
Reviewed-by: Wu Bo <wubo40@huawei.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 44283014c4eb..1d646f02d516 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -567,16 +567,19 @@ static bool cleanup_queued_task(struct iscsi_task *task)
struct iscsi_conn *conn = task->conn;
bool early_complete = false;
- /* Bad target might have completed task while it was still running */
+ /*
+ * We might have raced where we handled a R2T early and got a response
+ * but have not yet taken the task off the requeue list, then a TMF or
+ * recovery happened and so we can still see it here.
+ */
if (task->state == ISCSI_TASK_COMPLETED)
early_complete = true;
if (!list_empty(&task->running)) {
list_del_init(&task->running);
/*
- * If it's on a list but still running, this could be from
- * a bad target sending a rsp early, cleanup from a TMF, or
- * session recovery.
+ * If it's on a list but still running this could be cleanup
+ * from a TMF or session recovery.
*/
if (task->state == ISCSI_TASK_RUNNING ||
task->state == ISCSI_TASK_COMPLETED)
@@ -1485,7 +1488,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
}
/* regular RX path uses back_lock */
spin_lock(&conn->session->back_lock);
- if (rc && task->state == ISCSI_TASK_RUNNING) {
+ if (rc) {
/*
* get an extra ref that is released next time we access it
* as conn->task above.
--
2.25.1
next prev parent reply other threads:[~2022-06-16 22:46 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-16 22:45 [PATCH 0/9] iscsi features for 5.20 Mike Christie
2022-06-16 22:45 ` [PATCH 1/9] scsi: iscsi: Rename iscsi_conn_queue_work() Mike Christie
2022-06-16 22:45 ` [PATCH 2/9] scsi: iscsi: Add recv workqueue helpers Mike Christie
2022-06-16 22:45 ` [PATCH 3/9] scsi: iscsi: Run recv path from workqueue Mike Christie
2022-06-16 22:45 ` [PATCH 4/9] scsi: iscsi_tcp: Tell net when there's more data Mike Christie
2022-06-16 22:45 ` [PATCH 5/9] scsi: iscsi_tcp: Drop target_alloc use Mike Christie
2022-06-16 22:45 ` Mike Christie [this message]
2022-06-16 22:45 ` [PATCH 7/9] scsi: iscsi: Remove iscsi_get_task back_lock requirement Mike Christie
2022-06-16 22:45 ` [PATCH 8/9] scsi: iscsi: Try to avoid taking back_lock in xmit path Mike Christie
2022-06-16 22:45 ` [PATCH 9/9] scsi: libiscsi: Improve conn_send_pdu API Mike Christie
2022-06-22 1:20 ` [PATCH 0/9] iscsi features for 5.20 Martin K. Petersen
2022-06-28 3:24 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220616224557.115234-7-michael.christie@oracle.com \
--to=michael.christie@oracle.com \
--cc=cleech@redhat.com \
--cc=jejb@linux.ibm.com \
--cc=lduncan@suse.com \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=wubo40@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).