* [PATCH] target: iscsi: simplify the connection closing mechanism
@ 2021-11-04 14:25 Maurizio Lombardi
2021-11-10 16:54 ` Maurizio Lombardi
0 siblings, 1 reply; 2+ messages in thread
From: Maurizio Lombardi @ 2021-11-04 14:25 UTC (permalink / raw)
To: martin.petersen
Cc: bostroesser, michael.christie, target-devel, linux-scsi, k.shelekhin
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)
Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
drivers/target/iscsi/iscsi_target.c | 31 +++++------------------
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, 8 insertions(+), 38 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 2c54c5d8412d..7df10cfcba2a 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4223,34 +4223,17 @@ int iscsit_close_connection(
spin_unlock_bh(&sess->conn_lock);
- /*
- * If connection reinstatement is being performed on this connection,
- * up the connection reinstatement semaphore that is being blocked on
- * in iscsit_cause_connection_reinstatement().
- */
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);
+ /*
+ * If connection reinstatement is being performed on this connection,
+ * up the connection reinstatement semaphore that is being blocked on
+ * in iscsit_cause_connection_reinstatement() or
+ * in iscsit_connection_reinstatement_rcfr()
+ */
+ 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;
--
2.27.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] target: iscsi: simplify the connection closing mechanism
2021-11-04 14:25 [PATCH] target: iscsi: simplify the connection closing mechanism Maurizio Lombardi
@ 2021-11-10 16:54 ` Maurizio Lombardi
0 siblings, 0 replies; 2+ messages in thread
From: Maurizio Lombardi @ 2021-11-10 16:54 UTC (permalink / raw)
To: martin.petersen
Cc: bostroesser, michael.christie, target-devel, linux-scsi, k.shelekhin
Please ignore this. I realized this could introduce a race condition.
Will send a V2 when ready.
Maurizio
On Thu, Nov 04, 2021 at 03:25:45PM +0100, 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)
>
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
> drivers/target/iscsi/iscsi_target.c | 31 +++++------------------
> 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, 8 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 2c54c5d8412d..7df10cfcba2a 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -4223,34 +4223,17 @@ int iscsit_close_connection(
>
> spin_unlock_bh(&sess->conn_lock);
>
> - /*
> - * If connection reinstatement is being performed on this connection,
> - * up the connection reinstatement semaphore that is being blocked on
> - * in iscsit_cause_connection_reinstatement().
> - */
> 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);
>
> + /*
> + * If connection reinstatement is being performed on this connection,
> + * up the connection reinstatement semaphore that is being blocked on
> + * in iscsit_cause_connection_reinstatement() or
> + * in iscsit_connection_reinstatement_rcfr()
> + */
> + 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;
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-11-10 16:55 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-04 14:25 [PATCH] target: iscsi: simplify the connection closing mechanism Maurizio Lombardi
2021-11-10 16:54 ` Maurizio Lombardi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.