diff mbox series

[V2] target: iscsi: simplify the connection closing mechanism

Message ID 20211111133752.159379-1-mlombard@redhat.com (mailing list archive)
State New, archived
Headers show
Series [V2] target: iscsi: simplify the connection closing mechanism | expand

Commit Message

Maurizio Lombardi Nov. 11, 2021, 1:37 p.m. UTC
When the connection reinstatement is performed, the target driver
executes a complex scheme of complete()/wait_for_completion() that is not
really needed.

Considering that:

1) The callers of iscsit_connection_reinstatement_rcfr() and
   iscsit_cause_connection_reinstatement() hold a reference
   to the conn structure.

2) iscsit_close_connection() will sleep when calling
   iscsit_check_conn_usage_count() until the conn structure's refcount
   reaches zero.

we can optimize the driver the following way:

* The threads that must sleep until the connection is closed
  will all wait for the "conn_wait_comp" completion,
  iscsit_close_connection() will then call complete_all() to wake them up.
  No need to have multiple completion structures.

* The conn_post_wait_comp completion is not necessary and can be removed
  because iscsit_close_connection() sleeps until all the other threads
  release the conn structure.
  (see the iscsit_check_conn_usage_count() function)

V2: do not set connection_reinstatement to 1 in iscsit_close_connection(),
    leave iscsit_cause_connection_reinstatement() deal with reentrancy.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/target/iscsi/iscsi_target.c       | 27 +++--------------------
 drivers/target/iscsi/iscsi_target_erl0.c  |  6 +----
 drivers/target/iscsi/iscsi_target_login.c |  2 --
 drivers/target/iscsi/iscsi_target_util.c  |  3 ---
 include/target/iscsi/iscsi_target_core.h  |  4 ----
 5 files changed, 4 insertions(+), 38 deletions(-)

Comments

Mike Christie Dec. 7, 2021, 7:59 p.m. UTC | #1
On 11/11/21 7:37 AM, Maurizio Lombardi wrote:
> When the connection reinstatement is performed, the target driver
> executes a complex scheme of complete()/wait_for_completion() that is not
> really needed.
> 
> Considering that:
> 
> 1) The callers of iscsit_connection_reinstatement_rcfr() and
>    iscsit_cause_connection_reinstatement() hold a reference
>    to the conn structure.> > 2) iscsit_close_connection() will sleep when calling
>    iscsit_check_conn_usage_count() until the conn structure's refcount
>    reaches zero.
> 
> we can optimize the driver the following way:
> 
> * The threads that must sleep until the connection is closed
>   will all wait for the "conn_wait_comp" completion,
>   iscsit_close_connection() will then call complete_all() to wake them up.
>   No need to have multiple completion structures.
> 
> * The conn_post_wait_comp completion is not necessary and can be removed
>   because iscsit_close_connection() sleeps until all the other threads
>   release the conn structure.
>   (see the iscsit_check_conn_usage_count() function)
> 
> V2: do not set connection_reinstatement to 1 in iscsit_close_connection(),
>     leave iscsit_cause_connection_reinstatement() deal with reentrancy.
> 

What was the issue with setting it to 1?
Maurizio Lombardi Dec. 9, 2021, 10:25 a.m. UTC | #2
On Tue, Dec 07, 2021 at 01:59:08PM -0600, Mike Christie wrote:
> On 11/11/21 7:37 AM, Maurizio Lombardi wrote:
> > When the connection reinstatement is performed, the target driver
> > executes a complex scheme of complete()/wait_for_completion() that is not
> > really needed.
> > 
> > Considering that:
> > 
> > 1) The callers of iscsit_connection_reinstatement_rcfr() and
> >    iscsit_cause_connection_reinstatement() hold a reference
> >    to the conn structure.> > 2) iscsit_close_connection() will sleep when calling
> >    iscsit_check_conn_usage_count() until the conn structure's refcount
> >    reaches zero.
> > 
> > we can optimize the driver the following way:
> > 
> > * The threads that must sleep until the connection is closed
> >   will all wait for the "conn_wait_comp" completion,
> >   iscsit_close_connection() will then call complete_all() to wake them up.
> >   No need to have multiple completion structures.
> > 
> > * The conn_post_wait_comp completion is not necessary and can be removed
> >   because iscsit_close_connection() sleeps until all the other threads
> >   release the conn structure.
> >   (see the iscsit_check_conn_usage_count() function)
> > 
> > V2: do not set connection_reinstatement to 1 in iscsit_close_connection(),
> >     leave iscsit_cause_connection_reinstatement() deal with reentrancy.
> > 
> 
> What was the issue with setting it to 1?
>

At first I thought that setting it to 1 could introduce a race condition,
then I realized that it's not the case, setting it to 1 simply has no effect
and therefore the assignment can be removed.

Maurizio
diff mbox series

Patch

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 2c54c5d8412d..d558c4c064cb 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4226,31 +4226,10 @@  int iscsit_close_connection(
 	/*
 	 * If connection reinstatement is being performed on this connection,
 	 * up the connection reinstatement semaphore that is being blocked on
-	 * in iscsit_cause_connection_reinstatement().
+	 * in iscsit_cause_connection_reinstatement() or
+	 * in iscsit_connection_reinstatement_rcfr()
 	 */
-	spin_lock_bh(&conn->state_lock);
-	if (atomic_read(&conn->sleep_on_conn_wait_comp)) {
-		spin_unlock_bh(&conn->state_lock);
-		complete(&conn->conn_wait_comp);
-		wait_for_completion(&conn->conn_post_wait_comp);
-		spin_lock_bh(&conn->state_lock);
-	}
-
-	/*
-	 * If connection reinstatement is being performed on this connection
-	 * by receiving a REMOVECONNFORRECOVERY logout request, up the
-	 * connection wait rcfr semaphore that is being blocked on
-	 * an iscsit_connection_reinstatement_rcfr().
-	 */
-	if (atomic_read(&conn->connection_wait_rcfr)) {
-		spin_unlock_bh(&conn->state_lock);
-		complete(&conn->conn_wait_rcfr_comp);
-		wait_for_completion(&conn->conn_post_wait_comp);
-		spin_lock_bh(&conn->state_lock);
-	}
-	atomic_set(&conn->connection_reinstatement, 1);
-	spin_unlock_bh(&conn->state_lock);
-
+	complete_all(&conn->conn_wait_comp);
 	/*
 	 * If any other processes are accessing this connection pointer we
 	 * must wait until they have completed.
diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c
index 102c9cbf59f3..584e0a0b517d 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.c
+++ b/drivers/target/iscsi/iscsi_target_erl0.c
@@ -839,8 +839,7 @@  void iscsit_connection_reinstatement_rcfr(struct iscsi_conn *conn)
 		send_sig(SIGINT, conn->rx_thread, 1);
 
 sleep:
-	wait_for_completion(&conn->conn_wait_rcfr_comp);
-	complete(&conn->conn_post_wait_comp);
+	wait_for_completion(&conn->conn_wait_comp);
 }
 
 void iscsit_cause_connection_reinstatement(struct iscsi_conn *conn, int sleep)
@@ -871,12 +870,9 @@  void iscsit_cause_connection_reinstatement(struct iscsi_conn *conn, int sleep)
 		spin_unlock_bh(&conn->state_lock);
 		return;
 	}
-
-	atomic_set(&conn->sleep_on_conn_wait_comp, 1);
 	spin_unlock_bh(&conn->state_lock);
 
 	wait_for_completion(&conn->conn_wait_comp);
-	complete(&conn->conn_post_wait_comp);
 }
 EXPORT_SYMBOL(iscsit_cause_connection_reinstatement);
 
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 1a9c50401bdb..982c23459272 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1096,9 +1096,7 @@  static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
 	INIT_LIST_HEAD(&conn->conn_cmd_list);
 	INIT_LIST_HEAD(&conn->immed_queue_list);
 	INIT_LIST_HEAD(&conn->response_queue_list);
-	init_completion(&conn->conn_post_wait_comp);
 	init_completion(&conn->conn_wait_comp);
-	init_completion(&conn->conn_wait_rcfr_comp);
 	init_completion(&conn->conn_waiting_on_uc_comp);
 	init_completion(&conn->conn_logout_comp);
 	init_completion(&conn->rx_half_close_comp);
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index 6dd5810e2af1..d7b1f9110d49 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -824,9 +824,6 @@  struct iscsi_conn *iscsit_get_conn_from_cid_rcfr(struct iscsi_session *sess, u16
 	list_for_each_entry(conn, &sess->sess_conn_list, conn_list) {
 		if (conn->cid == cid) {
 			iscsit_inc_conn_usage_count(conn);
-			spin_lock(&conn->state_lock);
-			atomic_set(&conn->connection_wait_rcfr, 1);
-			spin_unlock(&conn->state_lock);
 			spin_unlock_bh(&sess->conn_lock);
 			return conn;
 		}
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 1eccb2ac7d02..aeb8932507c2 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -542,12 +542,8 @@  struct iscsi_conn {
 	atomic_t		connection_exit;
 	atomic_t		connection_recovery;
 	atomic_t		connection_reinstatement;
-	atomic_t		connection_wait_rcfr;
-	atomic_t		sleep_on_conn_wait_comp;
 	atomic_t		transport_failed;
-	struct completion	conn_post_wait_comp;
 	struct completion	conn_wait_comp;
-	struct completion	conn_wait_rcfr_comp;
 	struct completion	conn_waiting_on_uc_comp;
 	struct completion	conn_logout_comp;
 	struct completion	tx_half_close_comp;