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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On 7/11/2022, Bodo Stroesser wrote:
> 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?
>

yes.

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

This is where I misunderstood.

>
>>
>> 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.

Ok.

>
> 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.

I see.

>
> 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.
>
>

Thanks for the clarifications!
Thinh


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