| Message ID | 5cfc7eb8-c59d-4b7a-3dee-99e17d72f251@suse.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Oct 04, 2016 at 11:11:18AM +0200, Hannes Reinecke wrote: > Hmm. Looking at the code it looks as we might miss some calls to > 'complete'. Can you try with the attached patch? That only looks slightly better than the original. What this really needs is a waitqueue and and waitevent on sess->ncon. Although that will need a bit more refactoring around that code. There also are a few more ovbious issues around it, e.g. iscsit_close_connection needs to use atomic_dec_and_test on sess->nconn instead of having separate atomic_dec and atomic_read calls, and a lot of the 0 or 1 atomic_ts in this code should be replaced with atomic bitops. Btw, there also was a fix from Lee in this area that added a missing wakeup, make sure your tree already has that. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Do you want me to try this patch or wait for some of the suggestions Christoph brought up to be Incorporated? ---------------- Robert LeBlanc PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1 On Tue, Oct 4, 2016 at 5:46 AM, Christoph Hellwig <hch@infradead.org> wrote: > On Tue, Oct 04, 2016 at 11:11:18AM +0200, Hannes Reinecke wrote: >> Hmm. Looking at the code it looks as we might miss some calls to >> 'complete'. Can you try with the attached patch? > > That only looks slightly better than the original. What this really > needs is a waitqueue and and waitevent on sess->ncon. Although > that will need a bit more refactoring around that code. There also > are a few more ovbious issues around it, e.g. iscsit_close_connection > needs to use atomic_dec_and_test on sess->nconn instead of having > separate atomic_dec and atomic_read calls, and a lot of the 0 or 1 > atomic_ts in this code should be replaced with atomic bitops. > > Btw, there also was a fix from Lee in this area that added a missing > wakeup, make sure your tree already has that. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
We are not able to identify the patch that you mentioned from Lee, can you give us a commit or a link to the patch? Thanks, ---------------- Robert LeBlanc PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1 On Tue, Oct 4, 2016 at 5:46 AM, Christoph Hellwig <hch@infradead.org> wrote: > On Tue, Oct 04, 2016 at 11:11:18AM +0200, Hannes Reinecke wrote: >> Hmm. Looking at the code it looks as we might miss some calls to >> 'complete'. Can you try with the attached patch? > > That only looks slightly better than the original. What this really > needs is a waitqueue and and waitevent on sess->ncon. Although > that will need a bit more refactoring around that code. There also > are a few more ovbious issues around it, e.g. iscsit_close_connection > needs to use atomic_dec_and_test on sess->nconn instead of having > separate atomic_dec and atomic_read calls, and a lot of the 0 or 1 > atomic_ts in this code should be replaced with atomic bitops. > > Btw, there also was a fix from Lee in this area that added a missing > wakeup, make sure your tree already has that. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Robert, I actually got the name wrong, the patch wasn't from Lee, but from Zhu, another SuSE engineer. This is the one: http://www.spinics.net/lists/target-devel/msg13463.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks, we will apply that too. We'd like to get this stable. We'll report back on what we find with these patches. ---------------- Robert LeBlanc PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1 On Wed, Oct 5, 2016 at 12:03 PM, Christoph Hellwig <hch@infradead.org> wrote: > Hi Robert, > > I actually got the name wrong, the patch wasn't from Lee, but from Zhu, > another SuSE engineer. This is the one: > > http://www.spinics.net/lists/target-devel/msg13463.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From d481d8c27df8c09ea3798ce4a7217a26c3533161 Mon Sep 17 00:00:00 2001 From: Hannes Reinecke <hare@suse.de> Date: Tue, 4 Oct 2016 11:05:46 +0200 Subject: [PATCH] iscsi_target: sanitze sess_wait_on_completion When closing a session we only should set 'sess_wait_on_completion' if we are actually calling wait_for_completion(). And we should indeed call 'complete' in these cases, too. And add some WARN_ON() if we mess up with calculating the number of completions, too. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/target/iscsi/iscsi_target.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 39b928c..313724c 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -4287,6 +4287,7 @@ int iscsit_close_connection( if (!atomic_read(&sess->session_reinstatement) && atomic_read(&sess->session_fall_back_to_erl0)) { spin_unlock_bh(&sess->conn_lock); + WARN_ON(atomic_read(&sess->sleep_on_sess_wait_comp)); iscsit_close_session(sess); return 0; @@ -4557,7 +4558,6 @@ int iscsit_free_session(struct iscsi_session *sess) int is_last; spin_lock_bh(&sess->conn_lock); - atomic_set(&sess->sleep_on_sess_wait_comp, 1); list_for_each_entry_safe(conn, conn_tmp, &sess->sess_conn_list, conn_list) { @@ -4585,7 +4585,10 @@ int iscsit_free_session(struct iscsi_session *sess) if (atomic_read(&sess->nconn)) { spin_unlock_bh(&sess->conn_lock); + atomic_inc(&sess->sleep_on_sess_wait_comp); wait_for_completion(&sess->session_wait_comp); + atomic_dec(&sess->sleep_on_sess_wait_comp); + WARN_ON(atomic_read(&sess->sleep_on_sess_wait_comp)); } else spin_unlock_bh(&sess->conn_lock); @@ -4603,8 +4606,6 @@ void iscsit_stop_session( int is_last; spin_lock_bh(&sess->conn_lock); - if (session_sleep) - atomic_set(&sess->sleep_on_sess_wait_comp, 1); if (connection_sleep) { list_for_each_entry_safe(conn, conn_tmp, &sess->sess_conn_list, @@ -4636,7 +4637,10 @@ void iscsit_stop_session( if (session_sleep && atomic_read(&sess->nconn)) { spin_unlock_bh(&sess->conn_lock); + atomic_inc(&sess->sleep_on_sess_wait_comp); wait_for_completion(&sess->session_wait_comp); + atomic_dec(&sess->sleep_on_sess_wait_comp); + WARN_ON(atomic_read(&sess->sleep_on_sess_wait_comp); } else spin_unlock_bh(&sess->conn_lock); } -- 2.6.6