diff mbox

[1/3] iscsi-target: Convert iscsi_thread_set usage to kthread.h

Message ID 1426918564-22581-2-git-send-email-nab@daterainc.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nicholas A. Bellinger March 21, 2015, 6:16 a.m. UTC
From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts iscsi-target code to use modern kthread.h API
callers for creating RX/TX threads for each new iscsi_conn descriptor,
and releasing associated RX/TX threads during connection shutdown.

This is done using iscsit_start_kthreads() -> kthread_run() to start
new kthreads from within iscsi_post_login_handler(), and invoking
kthread_stop() from existing iscsit_close_connection() code.

Also, convert iscsit_logout_post_handler_closesession() code to use
cmpxchg when determing when iscsit_cause_connection_reinstatement()
needs to sleep waiting for completion.

Reported-by: Sagi Grimberg <sagig@mellanox.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Slava Shwartsman <valyushash@gmail.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c       | 106 +++++++++++++-----------------
 drivers/target/iscsi/iscsi_target_erl0.c  |  13 ++--
 drivers/target/iscsi/iscsi_target_login.c |  59 +++++++++++++++--
 include/target/iscsi/iscsi_target_core.h  |   7 ++
 4 files changed, 116 insertions(+), 69 deletions(-)

Comments

Sagi Grimberg March 23, 2015, 12:21 p.m. UTC | #1
On 3/21/2015 8:16 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch converts iscsi-target code to use modern kthread.h API
> callers for creating RX/TX threads for each new iscsi_conn descriptor,
> and releasing associated RX/TX threads during connection shutdown.
>
> This is done using iscsit_start_kthreads() -> kthread_run() to start
> new kthreads from within iscsi_post_login_handler(), and invoking
> kthread_stop() from existing iscsit_close_connection() code.
>
> Also, convert iscsit_logout_post_handler_closesession() code to use
> cmpxchg when determing when iscsit_cause_connection_reinstatement()
> needs to sleep waiting for completion.

I'll run some tests with this.

two comments below.

>
> Reported-by: Sagi Grimberg <sagig@mellanox.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Slava Shwartsman <valyushash@gmail.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/iscsi/iscsi_target.c       | 106 +++++++++++++-----------------
>   drivers/target/iscsi/iscsi_target_erl0.c  |  13 ++--
>   drivers/target/iscsi/iscsi_target_login.c |  59 +++++++++++++++--
>   include/target/iscsi/iscsi_target_core.h  |   7 ++
>   4 files changed, 116 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 2accb6e..cb97b59 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -537,7 +537,7 @@ static struct iscsit_transport iscsi_target_transport = {
>
>   static int __init iscsi_target_init_module(void)
>   {
> -	int ret = 0;
> +	int ret = 0, size;
>
>   	pr_debug("iSCSI-Target "ISCSIT_VERSION"\n");
>
> @@ -546,6 +546,7 @@ static int __init iscsi_target_init_module(void)
>   		pr_err("Unable to allocate memory for iscsit_global\n");
>   		return -1;
>   	}
> +	spin_lock_init(&iscsit_global->ts_bitmap_lock);
>   	mutex_init(&auth_id_lock);
>   	spin_lock_init(&sess_idr_lock);
>   	idr_init(&tiqn_idr);
> @@ -555,15 +556,11 @@ static int __init iscsi_target_init_module(void)
>   	if (ret < 0)
>   		goto out;
>
> -	ret = iscsi_thread_set_init();
> -	if (ret < 0)
> +	size = BITS_TO_LONGS(ISCSIT_BITMAP_BITS) * sizeof(long);
> +	iscsit_global->ts_bitmap = vzalloc(size);
> +	if (!iscsit_global->ts_bitmap) {
> +		pr_err("Unable to allocate iscsit_global->ts_bitmap\n");
>   		goto configfs_out;
> -
> -	if (iscsi_allocate_thread_sets(TARGET_THREAD_SET_COUNT) !=
> -			TARGET_THREAD_SET_COUNT) {
> -		pr_err("iscsi_allocate_thread_sets() returned"
> -			" unexpected value!\n");
> -		goto ts_out1;
>   	}
>
>   	lio_qr_cache = kmem_cache_create("lio_qr_cache",
> @@ -572,7 +569,7 @@ static int __init iscsi_target_init_module(void)
>   	if (!lio_qr_cache) {
>   		pr_err("nable to kmem_cache_create() for"
>   				" lio_qr_cache\n");
> -		goto ts_out2;
> +		goto bitmap_out;
>   	}
>
>   	lio_dr_cache = kmem_cache_create("lio_dr_cache",
> @@ -617,10 +614,8 @@ dr_out:
>   	kmem_cache_destroy(lio_dr_cache);
>   qr_out:
>   	kmem_cache_destroy(lio_qr_cache);
> -ts_out2:
> -	iscsi_deallocate_thread_sets();
> -ts_out1:
> -	iscsi_thread_set_free();
> +bitmap_out:
> +	vfree(iscsit_global->ts_bitmap);
>   configfs_out:
>   	iscsi_target_deregister_configfs();
>   out:
> @@ -630,8 +625,6 @@ out:
>
>   static void __exit iscsi_target_cleanup_module(void)
>   {
> -	iscsi_deallocate_thread_sets();
> -	iscsi_thread_set_free();
>   	iscsit_release_discovery_tpg();
>   	iscsit_unregister_transport(&iscsi_target_transport);
>   	kmem_cache_destroy(lio_qr_cache);
> @@ -641,6 +634,7 @@ static void __exit iscsi_target_cleanup_module(void)
>
>   	iscsi_target_deregister_configfs();
>
> +	vfree(iscsit_global->ts_bitmap);
>   	kfree(iscsit_global);
>   }
>
> @@ -3710,17 +3704,16 @@ static int iscsit_send_reject(
>
>   void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
>   {
> -	struct iscsi_thread_set *ts = conn->thread_set;
>   	int ord, cpu;
>   	/*
> -	 * thread_id is assigned from iscsit_global->ts_bitmap from
> -	 * within iscsi_thread_set.c:iscsi_allocate_thread_sets()
> +	 * bitmap_id is assigned from iscsit_global->ts_bitmap from
> +	 * within iscsit_start_kthreads()
>   	 *
> -	 * Here we use thread_id to determine which CPU that this
> -	 * iSCSI connection's iscsi_thread_set will be scheduled to
> +	 * Here we use bitmap_id to determine which CPU that this
> +	 * iSCSI connection's RX/TX threads will be scheduled to
>   	 * execute upon.
>   	 */
> -	ord = ts->thread_id % cpumask_weight(cpu_online_mask);
> +	ord = conn->bitmap_id % cpumask_weight(cpu_online_mask);
>   	for_each_online_cpu(cpu) {
>   		if (ord-- == 0) {
>   			cpumask_set_cpu(cpu, conn->conn_cpumask);
> @@ -3909,7 +3902,7 @@ check_rsp_state:
>   	switch (state) {
>   	case ISTATE_SEND_LOGOUTRSP:
>   		if (!iscsit_logout_post_handler(cmd, conn))
> -			goto restart;
> +			return -ECONNRESET;
>   		/* fall through */
>   	case ISTATE_SEND_STATUS:
>   	case ISTATE_SEND_ASYNCMSG:
> @@ -3937,8 +3930,6 @@ check_rsp_state:
>
>   err:
>   	return -1;
> -restart:
> -	return -EAGAIN;
>   }
>
>   static int iscsit_handle_response_queue(struct iscsi_conn *conn)
> @@ -3965,21 +3956,13 @@ static int iscsit_handle_response_queue(struct iscsi_conn *conn)
>   int iscsi_target_tx_thread(void *arg)
>   {
>   	int ret = 0;
> -	struct iscsi_conn *conn;
> -	struct iscsi_thread_set *ts = arg;
> +	struct iscsi_conn *conn = arg;
>   	/*
>   	 * Allow ourselves to be interrupted by SIGINT so that a
>   	 * connection recovery / failure event can be triggered externally.
>   	 */
>   	allow_signal(SIGINT);
>
> -restart:
> -	conn = iscsi_tx_thread_pre_handler(ts);
> -	if (!conn)
> -		goto out;
> -
> -	ret = 0;
> -
>   	while (!kthread_should_stop()) {
>   		/*
>   		 * Ensure that both TX and RX per connection kthreads
> @@ -3988,11 +3971,9 @@ restart:
>   		iscsit_thread_check_cpumask(conn, current, 1);
>
>   		wait_event_interruptible(conn->queues_wq,
> -					 !iscsit_conn_all_queues_empty(conn) ||
> -					 ts->status == ISCSI_THREAD_SET_RESET);
> +					 !iscsit_conn_all_queues_empty(conn));
>
> -		if ((ts->status == ISCSI_THREAD_SET_RESET) ||
> -		     signal_pending(current))
> +		if (signal_pending(current))
>   			goto transport_err;
>
>   get_immediate:
> @@ -4003,15 +3984,14 @@ get_immediate:
>   		ret = iscsit_handle_response_queue(conn);
>   		if (ret == 1)
>   			goto get_immediate;
> -		else if (ret == -EAGAIN)
> -			goto restart;
> +		else if (ret == -ECONNRESET)
> +			goto out;
>   		else if (ret < 0)
>   			goto transport_err;
>   	}
>
>   transport_err:
>   	iscsit_take_action_for_connection_exit(conn);
> -	goto restart;
>   out:
>   	return 0;
>   }
> @@ -4106,8 +4086,7 @@ int iscsi_target_rx_thread(void *arg)
>   	int ret;
>   	u8 buffer[ISCSI_HDR_LEN], opcode;
>   	u32 checksum = 0, digest = 0;
> -	struct iscsi_conn *conn = NULL;
> -	struct iscsi_thread_set *ts = arg;
> +	struct iscsi_conn *conn = arg;
>   	struct kvec iov;
>   	/*
>   	 * Allow ourselves to be interrupted by SIGINT so that a
> @@ -4115,11 +4094,6 @@ int iscsi_target_rx_thread(void *arg)
>   	 */
>   	allow_signal(SIGINT);
>
> -restart:
> -	conn = iscsi_rx_thread_pre_handler(ts);
> -	if (!conn)
> -		goto out;
> -
>   	if (conn->conn_transport->transport_type == ISCSI_INFINIBAND) {
>   		struct completion comp;
>   		int rc;
> @@ -4129,7 +4103,7 @@ restart:
>   		if (rc < 0)
>   			goto transport_err;
>
> -		goto out;
> +		goto transport_err;
>   	}
>
>   	while (!kthread_should_stop()) {
> @@ -4205,8 +4179,6 @@ transport_err:
>   	if (!signal_pending(current))
>   		atomic_set(&conn->transport_failed, 1);
>   	iscsit_take_action_for_connection_exit(conn);
> -	goto restart;
> -out:
>   	return 0;
>   }
>
> @@ -4268,7 +4240,26 @@ int iscsit_close_connection(
>   	if (conn->conn_transport->transport_type == ISCSI_TCP)
>   		complete(&conn->conn_logout_comp);
>
> -	iscsi_release_thread_set(conn);
> +	if (!strcmp(current->comm, ISCSI_RX_THREAD_NAME)) {
> +		if (conn->tx_thread &&
> +		    cmpxchg(&conn->tx_thread_active, true, false)) {
> +			printk("close_conn signal tx_thread\n");

This print needs to drop (or convert to pr_debug)

> +			send_sig(SIGINT, conn->tx_thread, 1);
> +			kthread_stop(conn->tx_thread);
> +		}
> +	} else if (!strcmp(current->comm, ISCSI_TX_THREAD_NAME)) {
> +		if (conn->rx_thread &&
> +		    cmpxchg(&conn->rx_thread_active, true, false)) {
> +			printk("close_conn signal rx_thread\n");

This print needs to drop (or convert to pr_debug)

> +			send_sig(SIGINT, conn->rx_thread, 1);
> +			kthread_stop(conn->rx_thread);
> +		}
> +	}
> +
> +	spin_lock(&iscsit_global->ts_bitmap_lock);
> +	bitmap_release_region(iscsit_global->ts_bitmap, conn->bitmap_id,
> +			      get_order(1));
> +	spin_unlock(&iscsit_global->ts_bitmap_lock);
>
>   	iscsit_stop_timers_for_cmds(conn);
>   	iscsit_stop_nopin_response_timer(conn);
> @@ -4546,15 +4537,13 @@ static void iscsit_logout_post_handler_closesession(
>   	struct iscsi_conn *conn)
>   {
>   	struct iscsi_session *sess = conn->sess;
> -
> -	iscsi_set_thread_clear(conn, ISCSI_CLEAR_TX_THREAD);
> -	iscsi_set_thread_set_signal(conn, ISCSI_SIGNAL_TX_THREAD);
> +	int sleep = cmpxchg(&conn->tx_thread_active, true, false);
>
>   	atomic_set(&conn->conn_logout_remove, 0);
>   	complete(&conn->conn_logout_comp);
>
>   	iscsit_dec_conn_usage_count(conn);
> -	iscsit_stop_session(sess, 1, 1);
> +	iscsit_stop_session(sess, sleep, sleep);
>   	iscsit_dec_session_usage_count(sess);
>   	target_put_session(sess->se_sess);
>   }
> @@ -4562,13 +4551,12 @@ static void iscsit_logout_post_handler_closesession(
>   static void iscsit_logout_post_handler_samecid(
>   	struct iscsi_conn *conn)
>   {
> -	iscsi_set_thread_clear(conn, ISCSI_CLEAR_TX_THREAD);
> -	iscsi_set_thread_set_signal(conn, ISCSI_SIGNAL_TX_THREAD);
> +	int sleep = cmpxchg(&conn->tx_thread_active, true, false);
>
>   	atomic_set(&conn->conn_logout_remove, 0);
>   	complete(&conn->conn_logout_comp);
>
> -	iscsit_cause_connection_reinstatement(conn, 1);
> +	iscsit_cause_connection_reinstatement(conn, sleep);
>   	iscsit_dec_conn_usage_count(conn);
>   }
>
> diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c
> index bdd8731..e008ed2 100644
> --- a/drivers/target/iscsi/iscsi_target_erl0.c
> +++ b/drivers/target/iscsi/iscsi_target_erl0.c
> @@ -860,7 +860,10 @@ void iscsit_connection_reinstatement_rcfr(struct iscsi_conn *conn)
>   	}
>   	spin_unlock_bh(&conn->state_lock);
>
> -	iscsi_thread_set_force_reinstatement(conn);
> +	if (conn->tx_thread && conn->tx_thread_active)
> +		send_sig(SIGINT, conn->tx_thread, 1);
> +	if (conn->rx_thread && conn->rx_thread_active)
> +		send_sig(SIGINT, conn->rx_thread, 1);
>
>   sleep:
>   	wait_for_completion(&conn->conn_wait_rcfr_comp);
> @@ -885,10 +888,10 @@ void iscsit_cause_connection_reinstatement(struct iscsi_conn *conn, int sleep)
>   		return;
>   	}
>
> -	if (iscsi_thread_set_force_reinstatement(conn) < 0) {
> -		spin_unlock_bh(&conn->state_lock);
> -		return;
> -	}
> +	if (conn->tx_thread && conn->tx_thread_active)
> +		send_sig(SIGINT, conn->tx_thread, 1);
> +	if (conn->rx_thread && conn->rx_thread_active)
> +		send_sig(SIGINT, conn->rx_thread, 1);
>
>   	atomic_set(&conn->connection_reinstatement, 1);
>   	if (!sleep) {
> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index 153fb66..345f073 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -699,6 +699,51 @@ static void iscsi_post_login_start_timers(struct iscsi_conn *conn)
>   		iscsit_start_nopin_timer(conn);
>   }
>
> +int iscsit_start_kthreads(struct iscsi_conn *conn)
> +{
> +	int ret = 0;
> +
> +	spin_lock(&iscsit_global->ts_bitmap_lock);
> +	conn->bitmap_id = bitmap_find_free_region(iscsit_global->ts_bitmap,
> +					ISCSIT_BITMAP_BITS, get_order(1));
> +	spin_unlock(&iscsit_global->ts_bitmap_lock);
> +
> +	if (conn->bitmap_id < 0) {
> +		pr_err("bitmap_find_free_region() failed for"
> +		       " iscsit_start_kthreads()\n");
> +		return -ENOMEM;
> +	}
> +
> +	conn->tx_thread = kthread_run(iscsi_target_tx_thread, conn,
> +				      "%s", ISCSI_TX_THREAD_NAME);
> +	if (IS_ERR(conn->tx_thread)) {
> +		pr_err("Unable to start iscsi_target_tx_thread\n");
> +		ret = PTR_ERR(conn->tx_thread);
> +		goto out_bitmap;
> +	}
> +	conn->tx_thread_active = true;
> +
> +	conn->rx_thread = kthread_run(iscsi_target_rx_thread, conn,
> +				      "%s", ISCSI_RX_THREAD_NAME);
> +	if (IS_ERR(conn->rx_thread)) {
> +		pr_err("Unable to start iscsi_target_rx_thread\n");
> +		ret = PTR_ERR(conn->rx_thread);
> +		goto out_tx;
> +	}
> +	conn->rx_thread_active = true;
> +
> +	return 0;
> +out_tx:
> +	kthread_stop(conn->tx_thread);
> +	conn->tx_thread_active = false;
> +out_bitmap:
> +	spin_lock(&iscsit_global->ts_bitmap_lock);
> +	bitmap_release_region(iscsit_global->ts_bitmap, conn->bitmap_id,
> +			      get_order(1));
> +	spin_unlock(&iscsit_global->ts_bitmap_lock);
> +	return ret;
> +}
> +
>   int iscsi_post_login_handler(
>   	struct iscsi_np *np,
>   	struct iscsi_conn *conn,
> @@ -709,7 +754,7 @@ int iscsi_post_login_handler(
>   	struct se_session *se_sess = sess->se_sess;
>   	struct iscsi_portal_group *tpg = sess->tpg;
>   	struct se_portal_group *se_tpg = &tpg->tpg_se_tpg;
> -	struct iscsi_thread_set *ts;
> +	int rc;
>
>   	iscsit_inc_conn_usage_count(conn);
>
> @@ -724,7 +769,6 @@ int iscsi_post_login_handler(
>   	/*
>   	 * SCSI Initiator -> SCSI Target Port Mapping
>   	 */
> -	ts = iscsi_get_thread_set();
>   	if (!zero_tsih) {
>   		iscsi_set_session_parameters(sess->sess_ops,
>   				conn->param_list, 0);
> @@ -751,9 +795,11 @@ int iscsi_post_login_handler(
>   			sess->sess_ops->InitiatorName);
>   		spin_unlock_bh(&sess->conn_lock);
>
> -		iscsi_post_login_start_timers(conn);
> +		rc = iscsit_start_kthreads(conn);
> +		if (rc)
> +			return rc;
>
> -		iscsi_activate_thread_set(conn, ts);
> +		iscsi_post_login_start_timers(conn);
>   		/*
>   		 * Determine CPU mask to ensure connection's RX and TX kthreads
>   		 * are scheduled on the same CPU.
> @@ -810,8 +856,11 @@ int iscsi_post_login_handler(
>   		" iSCSI Target Portal Group: %hu\n", tpg->nsessions, tpg->tpgt);
>   	spin_unlock_bh(&se_tpg->session_lock);
>
> +	rc = iscsit_start_kthreads(conn);
> +	if (rc)
> +		return rc;
> +
>   	iscsi_post_login_start_timers(conn);
> -	iscsi_activate_thread_set(conn, ts);
>   	/*
>   	 * Determine CPU mask to ensure connection's RX and TX kthreads
>   	 * are scheduled on the same CPU.
> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> index d3583d3..dd0f3ab 100644
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -602,6 +602,11 @@ struct iscsi_conn {
>   	struct iscsi_session	*sess;
>   	/* Pointer to thread_set in use for this conn's threads */
>   	struct iscsi_thread_set	*thread_set;
> +	int			bitmap_id;
> +	int			rx_thread_active;
> +	struct task_struct	*rx_thread;
> +	int			tx_thread_active;
> +	struct task_struct	*tx_thread;
>   	/* list_head for session connection list */
>   	struct list_head	conn_list;
>   } ____cacheline_aligned;
> @@ -871,10 +876,12 @@ struct iscsit_global {
>   	/* Unique identifier used for the authentication daemon */
>   	u32			auth_id;
>   	u32			inactive_ts;
> +#define ISCSIT_BITMAP_BITS	262144
>   	/* Thread Set bitmap count */
>   	int			ts_bitmap_count;
>   	/* Thread Set bitmap pointer */
>   	unsigned long		*ts_bitmap;
> +	spinlock_t		ts_bitmap_lock;
>   	/* Used for iSCSI discovery session authentication */
>   	struct iscsi_node_acl	discovery_acl;
>   	struct iscsi_portal_group	*discovery_tpg;
>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sagi Grimberg March 24, 2015, 4:37 p.m. UTC | #2
On 3/23/2015 2:21 PM, Sagi Grimberg wrote:
> On 3/21/2015 8:16 AM, Nicholas A. Bellinger wrote:
>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>
>> This patch converts iscsi-target code to use modern kthread.h API
>> callers for creating RX/TX threads for each new iscsi_conn descriptor,
>> and releasing associated RX/TX threads during connection shutdown.
>>
>> This is done using iscsit_start_kthreads() -> kthread_run() to start
>> new kthreads from within iscsi_post_login_handler(), and invoking
>> kthread_stop() from existing iscsit_close_connection() code.
>>
>> Also, convert iscsit_logout_post_handler_closesession() code to use
>> cmpxchg when determing when iscsit_cause_connection_reinstatement()
>> needs to sleep waiting for completion.
>
> I'll run some tests with this.

Hi Nic,

I can report that this series (minus patch 3/3 and along with some
patches I'll send out soon) seems to resolve the list corruption issue
I've seen.

Overall the patches look good to me so feel free to include
patches 1+2 with:

Tested-by: Sagi Grimberg <sagig@mellanox.com>

Thanks!
Sagi.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas A. Bellinger March 26, 2015, 6:45 a.m. UTC | #3
On Tue, 2015-03-24 at 18:37 +0200, Sagi Grimberg wrote:
> On 3/23/2015 2:21 PM, Sagi Grimberg wrote:
> > On 3/21/2015 8:16 AM, Nicholas A. Bellinger wrote:
> >> From: Nicholas Bellinger <nab@linux-iscsi.org>
> >>
> >> This patch converts iscsi-target code to use modern kthread.h API
> >> callers for creating RX/TX threads for each new iscsi_conn descriptor,
> >> and releasing associated RX/TX threads during connection shutdown.
> >>
> >> This is done using iscsit_start_kthreads() -> kthread_run() to start
> >> new kthreads from within iscsi_post_login_handler(), and invoking
> >> kthread_stop() from existing iscsit_close_connection() code.
> >>
> >> Also, convert iscsit_logout_post_handler_closesession() code to use
> >> cmpxchg when determing when iscsit_cause_connection_reinstatement()
> >> needs to sleep waiting for completion.
> >
> > I'll run some tests with this.
> 
> Hi Nic,
> 
> I can report that this series (minus patch 3/3 and along with some
> patches I'll send out soon) seems to resolve the list corruption issue
> I've seen.
> 
> Overall the patches look good to me so feel free to include
> patches 1+2 with:
> 
> Tested-by: Sagi Grimberg <sagig@mellanox.com>
> 

Great.  The first two are queued in for-next, and for the moment #1 has
a v3.10+ stable tag as well..

Please let me know if you run into any regressions with these.

Thanks alot Sagi!

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 2accb6e..cb97b59 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -537,7 +537,7 @@  static struct iscsit_transport iscsi_target_transport = {
 
 static int __init iscsi_target_init_module(void)
 {
-	int ret = 0;
+	int ret = 0, size;
 
 	pr_debug("iSCSI-Target "ISCSIT_VERSION"\n");
 
@@ -546,6 +546,7 @@  static int __init iscsi_target_init_module(void)
 		pr_err("Unable to allocate memory for iscsit_global\n");
 		return -1;
 	}
+	spin_lock_init(&iscsit_global->ts_bitmap_lock);
 	mutex_init(&auth_id_lock);
 	spin_lock_init(&sess_idr_lock);
 	idr_init(&tiqn_idr);
@@ -555,15 +556,11 @@  static int __init iscsi_target_init_module(void)
 	if (ret < 0)
 		goto out;
 
-	ret = iscsi_thread_set_init();
-	if (ret < 0)
+	size = BITS_TO_LONGS(ISCSIT_BITMAP_BITS) * sizeof(long);
+	iscsit_global->ts_bitmap = vzalloc(size);
+	if (!iscsit_global->ts_bitmap) {
+		pr_err("Unable to allocate iscsit_global->ts_bitmap\n");
 		goto configfs_out;
-
-	if (iscsi_allocate_thread_sets(TARGET_THREAD_SET_COUNT) !=
-			TARGET_THREAD_SET_COUNT) {
-		pr_err("iscsi_allocate_thread_sets() returned"
-			" unexpected value!\n");
-		goto ts_out1;
 	}
 
 	lio_qr_cache = kmem_cache_create("lio_qr_cache",
@@ -572,7 +569,7 @@  static int __init iscsi_target_init_module(void)
 	if (!lio_qr_cache) {
 		pr_err("nable to kmem_cache_create() for"
 				" lio_qr_cache\n");
-		goto ts_out2;
+		goto bitmap_out;
 	}
 
 	lio_dr_cache = kmem_cache_create("lio_dr_cache",
@@ -617,10 +614,8 @@  dr_out:
 	kmem_cache_destroy(lio_dr_cache);
 qr_out:
 	kmem_cache_destroy(lio_qr_cache);
-ts_out2:
-	iscsi_deallocate_thread_sets();
-ts_out1:
-	iscsi_thread_set_free();
+bitmap_out:
+	vfree(iscsit_global->ts_bitmap);
 configfs_out:
 	iscsi_target_deregister_configfs();
 out:
@@ -630,8 +625,6 @@  out:
 
 static void __exit iscsi_target_cleanup_module(void)
 {
-	iscsi_deallocate_thread_sets();
-	iscsi_thread_set_free();
 	iscsit_release_discovery_tpg();
 	iscsit_unregister_transport(&iscsi_target_transport);
 	kmem_cache_destroy(lio_qr_cache);
@@ -641,6 +634,7 @@  static void __exit iscsi_target_cleanup_module(void)
 
 	iscsi_target_deregister_configfs();
 
+	vfree(iscsit_global->ts_bitmap);
 	kfree(iscsit_global);
 }
 
@@ -3710,17 +3704,16 @@  static int iscsit_send_reject(
 
 void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
 {
-	struct iscsi_thread_set *ts = conn->thread_set;
 	int ord, cpu;
 	/*
-	 * thread_id is assigned from iscsit_global->ts_bitmap from
-	 * within iscsi_thread_set.c:iscsi_allocate_thread_sets()
+	 * bitmap_id is assigned from iscsit_global->ts_bitmap from
+	 * within iscsit_start_kthreads()
 	 *
-	 * Here we use thread_id to determine which CPU that this
-	 * iSCSI connection's iscsi_thread_set will be scheduled to
+	 * Here we use bitmap_id to determine which CPU that this
+	 * iSCSI connection's RX/TX threads will be scheduled to
 	 * execute upon.
 	 */
-	ord = ts->thread_id % cpumask_weight(cpu_online_mask);
+	ord = conn->bitmap_id % cpumask_weight(cpu_online_mask);
 	for_each_online_cpu(cpu) {
 		if (ord-- == 0) {
 			cpumask_set_cpu(cpu, conn->conn_cpumask);
@@ -3909,7 +3902,7 @@  check_rsp_state:
 	switch (state) {
 	case ISTATE_SEND_LOGOUTRSP:
 		if (!iscsit_logout_post_handler(cmd, conn))
-			goto restart;
+			return -ECONNRESET;
 		/* fall through */
 	case ISTATE_SEND_STATUS:
 	case ISTATE_SEND_ASYNCMSG:
@@ -3937,8 +3930,6 @@  check_rsp_state:
 
 err:
 	return -1;
-restart:
-	return -EAGAIN;
 }
 
 static int iscsit_handle_response_queue(struct iscsi_conn *conn)
@@ -3965,21 +3956,13 @@  static int iscsit_handle_response_queue(struct iscsi_conn *conn)
 int iscsi_target_tx_thread(void *arg)
 {
 	int ret = 0;
-	struct iscsi_conn *conn;
-	struct iscsi_thread_set *ts = arg;
+	struct iscsi_conn *conn = arg;
 	/*
 	 * Allow ourselves to be interrupted by SIGINT so that a
 	 * connection recovery / failure event can be triggered externally.
 	 */
 	allow_signal(SIGINT);
 
-restart:
-	conn = iscsi_tx_thread_pre_handler(ts);
-	if (!conn)
-		goto out;
-
-	ret = 0;
-
 	while (!kthread_should_stop()) {
 		/*
 		 * Ensure that both TX and RX per connection kthreads
@@ -3988,11 +3971,9 @@  restart:
 		iscsit_thread_check_cpumask(conn, current, 1);
 
 		wait_event_interruptible(conn->queues_wq,
-					 !iscsit_conn_all_queues_empty(conn) ||
-					 ts->status == ISCSI_THREAD_SET_RESET);
+					 !iscsit_conn_all_queues_empty(conn));
 
-		if ((ts->status == ISCSI_THREAD_SET_RESET) ||
-		     signal_pending(current))
+		if (signal_pending(current))
 			goto transport_err;
 
 get_immediate:
@@ -4003,15 +3984,14 @@  get_immediate:
 		ret = iscsit_handle_response_queue(conn);
 		if (ret == 1)
 			goto get_immediate;
-		else if (ret == -EAGAIN)
-			goto restart;
+		else if (ret == -ECONNRESET)
+			goto out;
 		else if (ret < 0)
 			goto transport_err;
 	}
 
 transport_err:
 	iscsit_take_action_for_connection_exit(conn);
-	goto restart;
 out:
 	return 0;
 }
@@ -4106,8 +4086,7 @@  int iscsi_target_rx_thread(void *arg)
 	int ret;
 	u8 buffer[ISCSI_HDR_LEN], opcode;
 	u32 checksum = 0, digest = 0;
-	struct iscsi_conn *conn = NULL;
-	struct iscsi_thread_set *ts = arg;
+	struct iscsi_conn *conn = arg;
 	struct kvec iov;
 	/*
 	 * Allow ourselves to be interrupted by SIGINT so that a
@@ -4115,11 +4094,6 @@  int iscsi_target_rx_thread(void *arg)
 	 */
 	allow_signal(SIGINT);
 
-restart:
-	conn = iscsi_rx_thread_pre_handler(ts);
-	if (!conn)
-		goto out;
-
 	if (conn->conn_transport->transport_type == ISCSI_INFINIBAND) {
 		struct completion comp;
 		int rc;
@@ -4129,7 +4103,7 @@  restart:
 		if (rc < 0)
 			goto transport_err;
 
-		goto out;
+		goto transport_err;
 	}
 
 	while (!kthread_should_stop()) {
@@ -4205,8 +4179,6 @@  transport_err:
 	if (!signal_pending(current))
 		atomic_set(&conn->transport_failed, 1);
 	iscsit_take_action_for_connection_exit(conn);
-	goto restart;
-out:
 	return 0;
 }
 
@@ -4268,7 +4240,26 @@  int iscsit_close_connection(
 	if (conn->conn_transport->transport_type == ISCSI_TCP)
 		complete(&conn->conn_logout_comp);
 
-	iscsi_release_thread_set(conn);
+	if (!strcmp(current->comm, ISCSI_RX_THREAD_NAME)) {
+		if (conn->tx_thread &&
+		    cmpxchg(&conn->tx_thread_active, true, false)) {
+			printk("close_conn signal tx_thread\n");
+			send_sig(SIGINT, conn->tx_thread, 1);
+			kthread_stop(conn->tx_thread);
+		}
+	} else if (!strcmp(current->comm, ISCSI_TX_THREAD_NAME)) {
+		if (conn->rx_thread &&
+		    cmpxchg(&conn->rx_thread_active, true, false)) {
+			printk("close_conn signal rx_thread\n");
+			send_sig(SIGINT, conn->rx_thread, 1);
+			kthread_stop(conn->rx_thread);
+		}
+	}
+
+	spin_lock(&iscsit_global->ts_bitmap_lock);
+	bitmap_release_region(iscsit_global->ts_bitmap, conn->bitmap_id,
+			      get_order(1));
+	spin_unlock(&iscsit_global->ts_bitmap_lock);
 
 	iscsit_stop_timers_for_cmds(conn);
 	iscsit_stop_nopin_response_timer(conn);
@@ -4546,15 +4537,13 @@  static void iscsit_logout_post_handler_closesession(
 	struct iscsi_conn *conn)
 {
 	struct iscsi_session *sess = conn->sess;
-
-	iscsi_set_thread_clear(conn, ISCSI_CLEAR_TX_THREAD);
-	iscsi_set_thread_set_signal(conn, ISCSI_SIGNAL_TX_THREAD);
+	int sleep = cmpxchg(&conn->tx_thread_active, true, false);
 
 	atomic_set(&conn->conn_logout_remove, 0);
 	complete(&conn->conn_logout_comp);
 
 	iscsit_dec_conn_usage_count(conn);
-	iscsit_stop_session(sess, 1, 1);
+	iscsit_stop_session(sess, sleep, sleep);
 	iscsit_dec_session_usage_count(sess);
 	target_put_session(sess->se_sess);
 }
@@ -4562,13 +4551,12 @@  static void iscsit_logout_post_handler_closesession(
 static void iscsit_logout_post_handler_samecid(
 	struct iscsi_conn *conn)
 {
-	iscsi_set_thread_clear(conn, ISCSI_CLEAR_TX_THREAD);
-	iscsi_set_thread_set_signal(conn, ISCSI_SIGNAL_TX_THREAD);
+	int sleep = cmpxchg(&conn->tx_thread_active, true, false);
 
 	atomic_set(&conn->conn_logout_remove, 0);
 	complete(&conn->conn_logout_comp);
 
-	iscsit_cause_connection_reinstatement(conn, 1);
+	iscsit_cause_connection_reinstatement(conn, sleep);
 	iscsit_dec_conn_usage_count(conn);
 }
 
diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c
index bdd8731..e008ed2 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.c
+++ b/drivers/target/iscsi/iscsi_target_erl0.c
@@ -860,7 +860,10 @@  void iscsit_connection_reinstatement_rcfr(struct iscsi_conn *conn)
 	}
 	spin_unlock_bh(&conn->state_lock);
 
-	iscsi_thread_set_force_reinstatement(conn);
+	if (conn->tx_thread && conn->tx_thread_active)
+		send_sig(SIGINT, conn->tx_thread, 1);
+	if (conn->rx_thread && conn->rx_thread_active)
+		send_sig(SIGINT, conn->rx_thread, 1);
 
 sleep:
 	wait_for_completion(&conn->conn_wait_rcfr_comp);
@@ -885,10 +888,10 @@  void iscsit_cause_connection_reinstatement(struct iscsi_conn *conn, int sleep)
 		return;
 	}
 
-	if (iscsi_thread_set_force_reinstatement(conn) < 0) {
-		spin_unlock_bh(&conn->state_lock);
-		return;
-	}
+	if (conn->tx_thread && conn->tx_thread_active)
+		send_sig(SIGINT, conn->tx_thread, 1);
+	if (conn->rx_thread && conn->rx_thread_active)
+		send_sig(SIGINT, conn->rx_thread, 1);
 
 	atomic_set(&conn->connection_reinstatement, 1);
 	if (!sleep) {
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 153fb66..345f073 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -699,6 +699,51 @@  static void iscsi_post_login_start_timers(struct iscsi_conn *conn)
 		iscsit_start_nopin_timer(conn);
 }
 
+int iscsit_start_kthreads(struct iscsi_conn *conn)
+{
+	int ret = 0;
+
+	spin_lock(&iscsit_global->ts_bitmap_lock);
+	conn->bitmap_id = bitmap_find_free_region(iscsit_global->ts_bitmap,
+					ISCSIT_BITMAP_BITS, get_order(1));
+	spin_unlock(&iscsit_global->ts_bitmap_lock);
+
+	if (conn->bitmap_id < 0) {
+		pr_err("bitmap_find_free_region() failed for"
+		       " iscsit_start_kthreads()\n");
+		return -ENOMEM;
+	}
+
+	conn->tx_thread = kthread_run(iscsi_target_tx_thread, conn,
+				      "%s", ISCSI_TX_THREAD_NAME);
+	if (IS_ERR(conn->tx_thread)) {
+		pr_err("Unable to start iscsi_target_tx_thread\n");
+		ret = PTR_ERR(conn->tx_thread);
+		goto out_bitmap;
+	}
+	conn->tx_thread_active = true;
+
+	conn->rx_thread = kthread_run(iscsi_target_rx_thread, conn,
+				      "%s", ISCSI_RX_THREAD_NAME);
+	if (IS_ERR(conn->rx_thread)) {
+		pr_err("Unable to start iscsi_target_rx_thread\n");
+		ret = PTR_ERR(conn->rx_thread);
+		goto out_tx;
+	}
+	conn->rx_thread_active = true;
+
+	return 0;
+out_tx:
+	kthread_stop(conn->tx_thread);
+	conn->tx_thread_active = false;
+out_bitmap:
+	spin_lock(&iscsit_global->ts_bitmap_lock);
+	bitmap_release_region(iscsit_global->ts_bitmap, conn->bitmap_id,
+			      get_order(1));
+	spin_unlock(&iscsit_global->ts_bitmap_lock);
+	return ret;
+}
+
 int iscsi_post_login_handler(
 	struct iscsi_np *np,
 	struct iscsi_conn *conn,
@@ -709,7 +754,7 @@  int iscsi_post_login_handler(
 	struct se_session *se_sess = sess->se_sess;
 	struct iscsi_portal_group *tpg = sess->tpg;
 	struct se_portal_group *se_tpg = &tpg->tpg_se_tpg;
-	struct iscsi_thread_set *ts;
+	int rc;
 
 	iscsit_inc_conn_usage_count(conn);
 
@@ -724,7 +769,6 @@  int iscsi_post_login_handler(
 	/*
 	 * SCSI Initiator -> SCSI Target Port Mapping
 	 */
-	ts = iscsi_get_thread_set();
 	if (!zero_tsih) {
 		iscsi_set_session_parameters(sess->sess_ops,
 				conn->param_list, 0);
@@ -751,9 +795,11 @@  int iscsi_post_login_handler(
 			sess->sess_ops->InitiatorName);
 		spin_unlock_bh(&sess->conn_lock);
 
-		iscsi_post_login_start_timers(conn);
+		rc = iscsit_start_kthreads(conn);
+		if (rc)
+			return rc;
 
-		iscsi_activate_thread_set(conn, ts);
+		iscsi_post_login_start_timers(conn);
 		/*
 		 * Determine CPU mask to ensure connection's RX and TX kthreads
 		 * are scheduled on the same CPU.
@@ -810,8 +856,11 @@  int iscsi_post_login_handler(
 		" iSCSI Target Portal Group: %hu\n", tpg->nsessions, tpg->tpgt);
 	spin_unlock_bh(&se_tpg->session_lock);
 
+	rc = iscsit_start_kthreads(conn);
+	if (rc)
+		return rc;
+
 	iscsi_post_login_start_timers(conn);
-	iscsi_activate_thread_set(conn, ts);
 	/*
 	 * Determine CPU mask to ensure connection's RX and TX kthreads
 	 * are scheduled on the same CPU.
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index d3583d3..dd0f3ab 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -602,6 +602,11 @@  struct iscsi_conn {
 	struct iscsi_session	*sess;
 	/* Pointer to thread_set in use for this conn's threads */
 	struct iscsi_thread_set	*thread_set;
+	int			bitmap_id;
+	int			rx_thread_active;
+	struct task_struct	*rx_thread;
+	int			tx_thread_active;
+	struct task_struct	*tx_thread;
 	/* list_head for session connection list */
 	struct list_head	conn_list;
 } ____cacheline_aligned;
@@ -871,10 +876,12 @@  struct iscsit_global {
 	/* Unique identifier used for the authentication daemon */
 	u32			auth_id;
 	u32			inactive_ts;
+#define ISCSIT_BITMAP_BITS	262144
 	/* Thread Set bitmap count */
 	int			ts_bitmap_count;
 	/* Thread Set bitmap pointer */
 	unsigned long		*ts_bitmap;
+	spinlock_t		ts_bitmap_lock;
 	/* Used for iSCSI discovery session authentication */
 	struct iscsi_node_acl	discovery_acl;
 	struct iscsi_portal_group	*discovery_tpg;