diff mbox series

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

Message ID a15b6eb1fd62e7e8bc7ad65f77cd327a2afde07e.1657149962.git.Thinh.Nguyen@synopsys.com
State New
Headers show
Series usb: gadget: f_tcm: Enhance UASP driver | expand

Commit Message

Thinh Nguyen July 6, 2022, 11:34 p.m. UTC
If the tmr_notify is not implemented, simply execute a generic command
completion to notify the command abort.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/target/target_core_tmr.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Dmitry Bogdanov July 7, 2022, 12:56 p.m. UTC | #1
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?
> 
> 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);
Thinh Nguyen July 8, 2022, 11:11 p.m. UTC | #2
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.

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

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);
Dmitry Bogdanov July 11, 2022, 9:44 a.m. UTC | #3
On Fri, Jul 08, 2022 at 11:11:37PM +0000, 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.
tmr_notify is for transport drivers (iblock/pscsi/user) - transport of
IOs to the real storage device. Not for trasport of incoming SCSI
messages - that is a frontend driver in TCM terms.
So, USB frontend driver has nothing to do with transport->tmr_notify().
> 
> I was trying to directly notify the user via target_complete_cmd(). It
> may not be the right way to handle this, any advise?
Frontend drivers are notified of the aborted task twice:
1. The incoming TMF in frontend driver; usually a frontend driver do not
 do anything here, just pass TMF to TCM Core.
2. TCM Core makrs the command as "to be aborted". 
  cmd->transport_state |= CMD_T_ABORTED;
2. TCM Core checks that command is to be aborted when IO is not started
yet or IO is completed:
 * target_execute_cmd(start of handling SCSI cmd),
 * target_compete_cmd (backend device completes IO), 
 * transport_generic_request_failure  (some generic request to send a
   failure response)
  And calls target_handle_abort() which calls
cmd->se_tfo->aborted_task(cmd) to notify frontend driver that it will
not be asked to send response to the command and it may do some cleanup
if needed.

There are two possible continuous processes in a cmd lifecycle:
1. Data IN (several responses to initiator)
 TCM Core receives a data from transport (backstore device) and passes
it to frontend driver. Frontend driver is responsible to send it to the
initiator. Probably, it may check that cmd is aborted to break sending,
but nobody do that.
2. Data OUT (several requests from initiators)
 Data from DataOUT is collected by frontend driver to pass it to TCM
Core in target_submit_cmd. TCM Core will abort the cmd at that moment.

There is no interface in TCM Core to notify Frontend driver to stop
those continuous processes. Probably, because of differences in fronted
protocol standards.
For example, iSCSI tunes that behaviour by some negotiatable session
parameter. Current kernel iSCSI driver does not support that parameter.

> 
> 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);
>
Bodo Stroesser July 11, 2022, 10:31 a.m. UTC | #4
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);
>
Thinh Nguyen July 12, 2022, 2:57 a.m. UTC | #5
On 7/11/2022, Dmitry Bogdanov wrote:
> On Fri, Jul 08, 2022 at 11:11:37PM +0000, 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.
> tmr_notify is for transport drivers (iblock/pscsi/user) - transport of
> IOs to the real storage device. Not for trasport of incoming SCSI
> messages - that is a frontend driver in TCM terms.
> So, USB frontend driver has nothing to do with transport->tmr_notify().

I see. Thanks clarifying the terminology here.

>> I was trying to directly notify the user via target_complete_cmd(). It
>> may not be the right way to handle this, any advise?
> Frontend drivers are notified of the aborted task twice:
> 1. The incoming TMF in frontend driver; usually a frontend driver do not
>   do anything here, just pass TMF to TCM Core.
> 2. TCM Core makrs the command as "to be aborted".
>    cmd->transport_state |= CMD_T_ABORTED;
> 2. TCM Core checks that command is to be aborted when IO is not started
> yet or IO is completed:
>   * target_execute_cmd(start of handling SCSI cmd),
>   * target_compete_cmd (backend device completes IO),
>   * transport_generic_request_failure  (some generic request to send a
>     failure response)
>    And calls target_handle_abort() which calls
> cmd->se_tfo->aborted_task(cmd) to notify frontend driver that it will
> not be asked to send response to the command and it may do some cleanup
> if needed.
>
> There are two possible continuous processes in a cmd lifecycle:
> 1. Data IN (several responses to initiator)
>   TCM Core receives a data from transport (backstore device) and passes
> it to frontend driver. Frontend driver is responsible to send it to the
> initiator. Probably, it may check that cmd is aborted to break sending,
> but nobody do that.
> 2. Data OUT (several requests from initiators)
>   Data from DataOUT is collected by frontend driver to pass it to TCM
> Core in target_submit_cmd. TCM Core will abort the cmd at that moment.
>
> There is no interface in TCM Core to notify Frontend driver to stop
> those continuous processes. Probably, because of differences in fronted
> protocol standards.
> For example, iSCSI tunes that behaviour by some negotiatable session
> parameter. Current kernel iSCSI driver does not support that parameter.

Thank you for the detail explanation. Really appreciate it.

BR,
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);
Thinh Nguyen July 12, 2022, 3:09 a.m. UTC | #6
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);
>>
diff mbox series

Patch

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);
 
 			list_del_init(&se_cmd->state_list);
 			target_put_cmd_and_wait(se_cmd);