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