Re: [PATCH 10/12] scsi: iscsi: Try to avoid taking back_lock in xmit path

[Lee Duncan <lduncan@suse.com>]

On 3/7/22 16:27, Mike Christie wrote:
> We need the back lock when freeing a task, so we hold it when calling
> __iscsi_put_task from the completion path to make it easier and to avoid
> having to retake it in that path. For iscsi_put_task we just grabbed it
> while also doing the decrement on the refcount but it's only really needed
> if the refcount is zero and we free the task. This modifies iscsi_put_task
> to just take the lock when needed then has the xmit path use it. Normally
> we will then not take the back lock from the xmit path. It will only be
> rare cases where the network is so fast that we get a response right after
> we send the header/data.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/libiscsi.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index a2d0daf5bd60..cde389225059 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -490,6 +490,12 @@ bool iscsi_get_task(struct iscsi_task *task)
>   }
>   EXPORT_SYMBOL_GPL(iscsi_get_task);
>   
> +/**
> + * __iscsi_put_task - drop the refcount on a task
> + * @task: iscsi_task to drop the refcount on
> + *
> + * The back_lock must be held when calling in case it frees the task.
> + */
>   void __iscsi_put_task(struct iscsi_task *task)
>   {
>   	if (refcount_dec_and_test(&task->refcount))
> @@ -501,10 +507,11 @@ void iscsi_put_task(struct iscsi_task *task)
>   {
>   	struct iscsi_session *session = task->conn->session;
>   
> -	/* regular RX path uses back_lock */
> -	spin_lock_bh(&session->back_lock);
> -	__iscsi_put_task(task);
> -	spin_unlock_bh(&session->back_lock);
> +	if (refcount_dec_and_test(&task->refcount)) {
> +		spin_lock_bh(&session->back_lock);
> +		iscsi_free_task(task);
> +		spin_unlock_bh(&session->back_lock);
> +	}
>   }
>   EXPORT_SYMBOL_GPL(iscsi_put_task);
>   
> @@ -1453,8 +1460,6 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
>   {
>   	int rc;
>   
> -	spin_lock_bh(&conn->session->back_lock);
> -
>   	if (!conn->task) {
>   		/*
>   		 * Take a ref so we can access it after xmit_task().
> @@ -1463,7 +1468,6 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
>   		 * stopped the xmit thread. WARN on move on.
>   		 */
>   		if (!iscsi_get_task(task)) {
> -			spin_unlock_bh(&conn->session->back_lock);
>   			WARN_ON_ONCE(1);
>   			return 0;
>   		}
> @@ -1477,7 +1481,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
>   	 * case a bad target sends a cmd rsp before we have handled the task.
>   	 */
>   	if (was_requeue)
> -		__iscsi_put_task(task);
> +		iscsi_put_task(task);
>   
>   	/*
>   	 * Do this after dropping the extra ref because if this was a requeue
> @@ -1489,10 +1493,8 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
>   		 * task and get woken up again.
>   		 */
>   		conn->task = task;
> -		spin_unlock_bh(&conn->session->back_lock);
>   		return -ENODATA;
>   	}
> -	spin_unlock_bh(&conn->session->back_lock);
>   
>   	spin_unlock_bh(&conn->session->frwd_lock);
>   	rc = conn->session->tt->xmit_task(task);
> @@ -1500,10 +1502,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
>   	if (!rc) {
>   		/* done with this task */
>   		task->last_xfer = jiffies;
> -	}
> -	/* regular RX path uses back_lock */
> -	spin_lock(&conn->session->back_lock);
> -	if (rc) {
> +	} else {
>   		/*
>   		 * get an extra ref that is released next time we access it
>   		 * as conn->task above.
> @@ -1512,8 +1511,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
>   		conn->task = task;
>   	}
>   
> -	__iscsi_put_task(task);
> -	spin_unlock(&conn->session->back_lock);
> +	iscsi_put_task(task);
>   	return rc;
>   }
>   

Reviewed-by: Lee Duncan <lduncan@suse.com>