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

[Bodo Stroesser <bstroesser@ts.fujitsu.com>]

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);
>>
>