| Message ID | a15b6eb1fd62e7e8bc7ad65f77cd327a2afde07e.1657149962.git.Thinh.Nguyen@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: f_tcm: Enhance UASP driver | expand |
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);
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);
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); >
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); >
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);
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 --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);
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(+)