diff mbox series

[v2,1/8] scsi: target: Modify core_tmr_abort_task()

Message ID 20200717161212.10731-2-bstroesser@ts.fujitsu.com (mailing list archive)
State Superseded
Headers show
Series scsi: target: tcmu: Add TMR notification for tcmu | expand

Commit Message

Bodo Stroesser July 17, 2020, 4:12 p.m. UTC
This patch modifies core_tmr_abort_task() to use same looping
and locking scheme as core_tmr_drain_state_list() does.

This frees the state_list element in se_cmd for later use
by tmr notification handling.

Note: __target_check_io_state() now is called with param 0
instead of dev->dev_attrib.emulate_tas, because tas is not
relevant since we always get ABRT on same session like the
aborted command.

Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
---
 drivers/target/target_core_tmr.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Comments

Mike Christie July 26, 2020, 12:03 a.m. UTC | #1
On 7/17/20 11:12 AM, Bodo Stroesser wrote:
> This patch modifies core_tmr_abort_task() to use same looping
> and locking scheme as core_tmr_drain_state_list() does.
> 
> This frees the state_list element in se_cmd for later use
> by tmr notification handling.
> 
> Note: __target_check_io_state() now is called with param 0
> instead of dev->dev_attrib.emulate_tas, because tas is not
> relevant since we always get ABRT on same session like the
> aborted command.
> 
> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> ---
>  drivers/target/target_core_tmr.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> index 89c84d472cd7..b65d7a0a5df1 100644
> --- a/drivers/target/target_core_tmr.c
> +++ b/drivers/target/target_core_tmr.c
> @@ -116,14 +116,15 @@ void core_tmr_abort_task(
>  	struct se_tmr_req *tmr,
>  	struct se_session *se_sess)
>  {
> -	struct se_cmd *se_cmd;
> +	struct se_cmd *se_cmd, *next;
>  	unsigned long flags;
> +	bool rc;
>  	u64 ref_tag;
>  
> -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> -	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
> +	spin_lock_irqsave(&dev->execute_task_lock, flags);
> +	list_for_each_entry_safe(se_cmd, next, &dev->state_list, state_list) {
>  
> -		if (dev != se_cmd->se_dev)
> +		if (se_sess != se_cmd->se_sess)
>  			continue;
>  
>  		/* skip task management functions, including tmr->task_cmd */
> @@ -137,11 +138,16 @@ void core_tmr_abort_task(
>  		printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
>  			se_cmd->se_tfo->fabric_name, ref_tag);
>  
> -		if (!__target_check_io_state(se_cmd, se_sess,
> -					     dev->dev_attrib.emulate_tas))
> +		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);

I don't think you can use flags twice. This call seems to be overwriting the outer spin lock caller's value, and I'm seeing a lot of warnings.


> +		rc = __target_check_io_state(se_cmd, se_sess, 0);
> +		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);


Use spin_lock/spin_unlock since we know above we have disabled irqs right above. Or make the drain users work like above.

Just make it consistent so future devs don't have to wonder why we use different calls.

> +		if (!rc)
>  			continue;
>  
> -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);> +		list_del_init(&se_cmd->state_list);
> +		se_cmd->state_active = false;
> +
> +		spin_unlock_irqrestore(&dev->execute_task_lock, flags);
>  
>  		/*
>  		 * Ensure that this ABORT request is visible to the LU RESET
> @@ -159,7 +165,7 @@ void core_tmr_abort_task(
>  		atomic_long_inc(&dev->aborts_complete);
>  		return;
>  	}
> -	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +	spin_unlock_irqrestore(&dev->execute_task_lock, flags);
>  
>  	printk("ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: %lld\n",
>  			tmr->ref_task_tag);
>
Bodo Stroesser July 26, 2020, 11:35 a.m. UTC | #2
On 2020-07-26 02:03, Mike Christie wrote:
> On 7/17/20 11:12 AM, Bodo Stroesser wrote:
>> This patch modifies core_tmr_abort_task() to use same looping
>> and locking scheme as core_tmr_drain_state_list() does.
>>
>> This frees the state_list element in se_cmd for later use
>> by tmr notification handling.
>>
>> Note: __target_check_io_state() now is called with param 0
>> instead of dev->dev_attrib.emulate_tas, because tas is not
>> relevant since we always get ABRT on same session like the
>> aborted command.
>>
>> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
>> ---
>>   drivers/target/target_core_tmr.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
>> index 89c84d472cd7..b65d7a0a5df1 100644
>> --- a/drivers/target/target_core_tmr.c
>> +++ b/drivers/target/target_core_tmr.c
>> @@ -116,14 +116,15 @@ void core_tmr_abort_task(
>>   	struct se_tmr_req *tmr,
>>   	struct se_session *se_sess)
>>   {
>> -	struct se_cmd *se_cmd;
>> +	struct se_cmd *se_cmd, *next;
>>   	unsigned long flags;
>> +	bool rc;
>>   	u64 ref_tag;
>>   
>> -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>> -	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
>> +	spin_lock_irqsave(&dev->execute_task_lock, flags);
>> +	list_for_each_entry_safe(se_cmd, next, &dev->state_list, state_list) {
>>   
>> -		if (dev != se_cmd->se_dev)
>> +		if (se_sess != se_cmd->se_sess)
>>   			continue;
>>   
>>   		/* skip task management functions, including tmr->task_cmd */
>> @@ -137,11 +138,16 @@ void core_tmr_abort_task(
>>   		printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
>>   			se_cmd->se_tfo->fabric_name, ref_tag);
>>   
>> -		if (!__target_check_io_state(se_cmd, se_sess,
>> -					     dev->dev_attrib.emulate_tas))
>> +		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> 
> I don't think you can use flags twice. This call seems to be overwriting the outer spin lock caller's value, and I'm seeing a lot of warnings.

Oops. Thank you!

Bad cut and paste. I wanted to make core_tmr_abort_task similar to core_tmr_drain_state_list.
But then I moved the calls for se_sess->sess_cmd_lock inside core_tmr_abort_task instead of copying the ones from core_tmr_drain_state_list.

I'm wondering why I don't see warnings when waiting with irq blocked ...

I'll resend without _irqsave / _irqrestore.
 
> 
> 
>> +		rc = __target_check_io_state(se_cmd, se_sess, 0);
>> +		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> 
> 
> Use spin_lock/spin_unlock since we know above we have disabled irqs right above. Or make the drain users work like above.
> 
> Just make it consistent so future devs don't have to wonder why we use different calls.
> 
>> +		if (!rc)
>>   			continue;
>>   
>> -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);> +		list_del_init(&se_cmd->state_list);
>> +		se_cmd->state_active = false;
>> +
>> +		spin_unlock_irqrestore(&dev->execute_task_lock, flags);
>>   
>>   		/*
>>   		 * Ensure that this ABORT request is visible to the LU RESET
>> @@ -159,7 +165,7 @@ void core_tmr_abort_task(
>>   		atomic_long_inc(&dev->aborts_complete);
>>   		return;
>>   	}
>> -	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
>> +	spin_unlock_irqrestore(&dev->execute_task_lock, flags);
>>   
>>   	printk("ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: %lld\n",
>>   			tmr->ref_task_tag);
>>
>
diff mbox series

Patch

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 89c84d472cd7..b65d7a0a5df1 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -116,14 +116,15 @@  void core_tmr_abort_task(
 	struct se_tmr_req *tmr,
 	struct se_session *se_sess)
 {
-	struct se_cmd *se_cmd;
+	struct se_cmd *se_cmd, *next;
 	unsigned long flags;
+	bool rc;
 	u64 ref_tag;
 
-	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
-	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
+	spin_lock_irqsave(&dev->execute_task_lock, flags);
+	list_for_each_entry_safe(se_cmd, next, &dev->state_list, state_list) {
 
-		if (dev != se_cmd->se_dev)
+		if (se_sess != se_cmd->se_sess)
 			continue;
 
 		/* skip task management functions, including tmr->task_cmd */
@@ -137,11 +138,16 @@  void core_tmr_abort_task(
 		printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
 			se_cmd->se_tfo->fabric_name, ref_tag);
 
-		if (!__target_check_io_state(se_cmd, se_sess,
-					     dev->dev_attrib.emulate_tas))
+		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
+		rc = __target_check_io_state(se_cmd, se_sess, 0);
+		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		if (!rc)
 			continue;
 
-		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		list_del_init(&se_cmd->state_list);
+		se_cmd->state_active = false;
+
+		spin_unlock_irqrestore(&dev->execute_task_lock, flags);
 
 		/*
 		 * Ensure that this ABORT request is visible to the LU RESET
@@ -159,7 +165,7 @@  void core_tmr_abort_task(
 		atomic_long_inc(&dev->aborts_complete);
 		return;
 	}
-	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+	spin_unlock_irqrestore(&dev->execute_task_lock, flags);
 
 	printk("ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: %lld\n",
 			tmr->ref_task_tag);