Re: [PATCH 04/36] target: Does tmr notify on aborted command

[Bodo Stroesser <bostroesser@gmail.com>]

Hi Thinh,

On 09.07.22 01:11, Thinh Nguyen wrote:
> On 7/7/2022, Dmitry Bogdanov wrote:
>> Hi Thinh,
>>
>> On Wed, Jul 06, 2022 at 04:34:49PM -0700, Thinh Nguyen wrote:
>>> If the tmr_notify is not implemented, simply execute a generic command
>>> completion to notify the command abort.
>> Why? What are you trying to fix?
> 
> If tmr_notify() is not implemented (which most don't), then the user
> won't get notified of the command completion.

When you talk about 'user' you indeed mean the initiator, right?

The initiator _is_ notified of command completion, because TMR
completion is deferred until all aborted cmds are completed!

> 
> I was trying to directly notify the user via target_complete_cmd(). It
> may not be the right way to handle this, any advise?

Target core must defer TMR completion until backend has completed all
aborted cmds, because completion of TMR tells initiator, that processing
of aborted cmds has ended and it now can start new cmds.

I implemented the tmr_notify callback for two reasons:

1) It allows e.g. userspace backend on tcmu to create a consistent
logging not only showing scsi cmds, but the TMRs also.
Only cmds that are aborted before they reach backend processing (for
tcmu that means: before they reach tcmu's cmd ring) are not visible
for the backend.
Additionally, some userspace daemons need to know about incoming TMRs
to allow handling of special situations.

2) it allows to speed up TMR processing, if backend uses it to finish /
abort processing of aborted cmds as early as possible. In tcmu for all
cmds in the cmd ring this is up to userspace.

If you want to speed up TMR processing for other backends, you could do
that by implementing tmr_notify() in those backends. Changing the core
IMHO is the wrong way.

Bodo


> 
> Thanks,
> Thinh
> 
>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>> ---
>>>    drivers/target/target_core_tmr.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
>>> index 7a7e24069ba7..2af80d0998bf 100644
>>> --- a/drivers/target/target_core_tmr.c
>>> +++ b/drivers/target/target_core_tmr.c
>>> @@ -14,6 +14,7 @@
>>>    #include <linux/spinlock.h>
>>>    #include <linux/list.h>
>>>    #include <linux/export.h>
>>> +#include <scsi/scsi_proto.h>
>>>    
>>>    #include <target/target_core_base.h>
>>>    #include <target/target_core_backend.h>
>>> @@ -150,6 +151,9 @@ void core_tmr_abort_task(
>>>    			if (dev->transport->tmr_notify)
>>>    				dev->transport->tmr_notify(dev, TMR_ABORT_TASK,
>>>    							   &aborted_list);
>>> +			else
>>> +				target_complete_cmd(se_cmd,
>>> +						    SAM_STAT_TASK_ABORTED);
>> That is wrong and breaks a command lifecycle and command kref counting.
>> target_complete_cmd is used to be called by a backend driver.
>>>    
>>>    			list_del_init(&se_cmd->state_list);
>>>    			target_put_cmd_and_wait(se_cmd);
>