| Message ID | 20170125233646.2243-10-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 01/26/2017 12:36 AM, Bart Van Assche wrote: > target_submit_tmr() only supports the I_T_L nexus for SCSI abort > and other task management functions. Make it possible for target > drivers to specify I_T nexus for SCSI abort by passing the > TARGET_SCF_IGNORE_TMR_LUN flag to target_submit_tmr(). > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Himanshu Madhani <himanshu.madhani@cavium.com> > Cc: Giridhar Malavali <giridhar.malavali@cavium.com> > --- > drivers/target/target_core_tmr.c | 16 +++++++++++++++- > drivers/target/target_core_transport.c | 23 +++++++++++++++++------ > include/target/target_core_base.h | 1 + > 3 files changed, 33 insertions(+), 7 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
> - queue_work(cmd->se_dev->tmr_wq, &cmd->work); > + if (cmd->se_dev) > + queue_work(cmd->se_dev->tmr_wq, &cmd->work); > + else > + schedule_work(&cmd->work); This looks suspicious. se_dev->tmr_wq has the WQ_MEM_RECLAIM flag set, so it always has a rescuer thread available, while the generic system work queue is just a best effort. Additional a unbound wq with max_active of guarantees strict execution ordering. I don't even know if the code relies on this fact, but changing it should require an explanation of why that change is fine. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-01-26 at 09:31 -0800, Christoph Hellwig wrote: > > - queue_work(cmd->se_dev->tmr_wq, &cmd->work); > > + if (cmd->se_dev) > > + queue_work(cmd->se_dev->tmr_wq, &cmd->work); > > + else > > + schedule_work(&cmd->work); > > This looks suspicious. se_dev->tmr_wq has the WQ_MEM_RECLAIM flag set, so > it always has a rescuer thread available, while the generic system work > queue is just a best effort. Additional a unbound wq with max_active of > guarantees strict execution ordering. I don't even know if the code > relies on this fact, but changing it should require an explanation of why > that change is fine. Hello Christoph, Maybe I overlooked something but I haven't found any clause in SAM-5 that specifies in which order task management functions should be executed. But I will add a comment in the code that explains this. Regarding WQ_MEM_RECLAIM: that flag guarantees that there is at least one execution context regardless of memory pressure. So enabling that flag is essential for any workqueue that executes work inside the memory reclaim path. The only way the above code can be called from inside a block device implementation is if it gets called by the tcm_loop driver. However, I'm not sure anyone is using the tcm_loop driver for another purpose than testing the LIO core and backend drivers? Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 30, 2017 at 09:42:50PM +0000, Bart Van Assche wrote: > Maybe I overlooked something but I haven't found any clause in SAM-5 that > specifies in which order task management functions should be executed. But > I will add a comment in the code that explains this. It's just my unexplained change detector that went off. However if we don't need the separate workqueue to start with it would be a good preparational patch to remove it. The workqueue was added by me in 2012 and replaced a kthread - the commit log mentions it uses a singlethreaded workqueue to keep the semantics exactly the same as before. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-01-31 at 15:44 +0100, hch@lst.de wrote: > On Mon, Jan 30, 2017 at 09:42:50PM +0000, Bart Van Assche wrote: > > Maybe I overlooked something but I haven't found any clause in SAM-5 that > > specifies in which order task management functions should be executed. But > > I will add a comment in the code that explains this. > > It's just my unexplained change detector that went off. However if > we don't need the separate workqueue to start with it would be a good > preparational patch to remove it. > > The workqueue was added by me in 2012 and replaced a kthread - the > commit log mentions it uses a singlethreaded workqueue to keep the > semantics exactly the same as before. Hello Christoph, The SCSI target ALUA code uses tmr_wq to ensure that ALUA transitions are executed in the same order as the core_alua_do_transition_tg_pt() calls that triggered these transitions. So I'm not sure we can get rid of tmr_wq without rewriting the ALUA code. But I can modify the TMR code such that it always uses schedule_work() and hence doesn't need tmr_wq anymore. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 31, 2017 at 06:51:02PM +0000, Bart Van Assche wrote: > Hello Christoph, > > The SCSI target ALUA code uses tmr_wq to ensure that ALUA transitions > are executed in the same order as the core_alua_do_transition_tg_pt() > calls that triggered these transitions. So I'm not sure we can get rid > of tmr_wq without rewriting the ALUA code. But I can modify the TMR > code such that it always uses schedule_work() and hence doesn't need > tmr_wq anymore. Switching the TMR code to use schedule_work and then renaming tmr_wq sounds fine to me. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/30/2017 03:42 PM, Bart Van Assche wrote: > On Thu, 2017-01-26 at 09:31 -0800, Christoph Hellwig wrote: >>> - queue_work(cmd->se_dev->tmr_wq, &cmd->work); >>> + if (cmd->se_dev) >>> + queue_work(cmd->se_dev->tmr_wq, &cmd->work); >>> + else >>> + schedule_work(&cmd->work); >> >> This looks suspicious. se_dev->tmr_wq has the WQ_MEM_RECLAIM flag set, so >> it always has a rescuer thread available, while the generic system work >> queue is just a best effort. Additional a unbound wq with max_active of >> guarantees strict execution ordering. I don't even know if the code >> relies on this fact, but changing it should require an explanation of why >> that change is fine. > > Hello Christoph, > > Maybe I overlooked something but I haven't found any clause in SAM-5 that > specifies in which order task management functions should be executed. But > I will add a comment in the code that explains this. Does it depend on the transport? I am not sure if it is the same thing, but 6.3.3 in SAM says a LU RESET would need to terminate all other TMFs that were executing. And for iscsi, a LU RESET and ABORT TASK SET with a cmd sequence number N is supposed to terminate tasks and TMFs with cmd sequence numbers N - 1, and iscsi also says that if a TMF has terminated a task/TMF then the target cannot send responses for it. So I think we have execute them in order or make it look like they did. The drain call does though right? -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-01-31 at 14:46 -0600, Mike Christie wrote: > On 01/30/2017 03:42 PM, Bart Van Assche wrote: > > Maybe I overlooked something but I haven't found any clause in SAM-5 that > > specifies in which order task management functions should be executed. But > > I will add a comment in the code that explains this. > > Does it depend on the transport? > > I am not sure if it is the same thing, but 6.3.3 in SAM says a LU RESET > would need to terminate all other TMFs that were executing. And for > iscsi, a LU RESET and ABORT TASK SET with a cmd sequence number N is > supposed to terminate tasks and TMFs with cmd sequence numbers N - 1, > and iscsi also says that if a TMF has terminated a task/TMF then the > target cannot send responses for it. > > So I think we have execute them in order or make it look like they did. > The drain call does though right? I'm not sure the drain call guarantees this for TMFs that affect multiple logical units since today there is one WQ per logical unit. Since SCSI transport layers that require that the transport layer delivers TMFs in the same order as submitted by the initiator I think this means that we need one WQ per I_T nexus for TMFs instead of one per device to be able to implement TARGET_SCF_IGNORE_TMR_LUN. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/31/17 2:46 PM, Mike Christie wrote: > On 01/30/2017 03:42 PM, Bart Van Assche wrote: >> On Thu, 2017-01-26 at 09:31 -0800, Christoph Hellwig wrote: >>>> - queue_work(cmd->se_dev->tmr_wq, &cmd->work); >>>> + if (cmd->se_dev) >>>> + queue_work(cmd->se_dev->tmr_wq, &cmd->work); >>>> + else >>>> + schedule_work(&cmd->work); >>> This looks suspicious. se_dev->tmr_wq has the WQ_MEM_RECLAIM flag set, so >>> it always has a rescuer thread available, while the generic system work >>> queue is just a best effort. Additional a unbound wq with max_active of >>> guarantees strict execution ordering. I don't even know if the code >>> relies on this fact, but changing it should require an explanation of why >>> that change is fine. >> Hello Christoph, >> >> Maybe I overlooked something but I haven't found any clause in SAM-5 that >> specifies in which order task management functions should be executed. But >> I will add a comment in the code that explains this. > > Does it depend on the transport? > > I am not sure if it is the same thing, but 6.3.3 in SAM says a LU RESET > would need to terminate all other TMFs that were executing. And for > iscsi, a LU RESET and ABORT TASK SET with a cmd sequence number N is > supposed to terminate tasks and TMFs with cmd sequence numbers N - 1, > and iscsi also says that if a TMF has terminated a task/TMF then the > target cannot send responses for it. > > So I think we have execute them in order or make it look like they did. > The drain call does though right? > -- > To unsubscribe from this list: send the line "unsubscribe target-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Most of the cases you cite specify interactions between a TMF and commands, not other TMFs (except for the LU Reset case). More specifically, section 4.4.3 in SAM says "The order in which task management requests are processed is not specified by the SCSI architecture model. The SCSI architecture model does not require in-order delivery of task management requests or processing by the task manager in the order received." -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c index 311dc3c2f1dc..367799b4dde1 100644 --- a/drivers/target/target_core_tmr.c +++ b/drivers/target/target_core_tmr.c @@ -149,6 +149,13 @@ static bool __target_check_io_state(struct se_cmd *se_cmd, return kref_get_unless_zero(&se_cmd->cmd_kref); } +/** + * core_tmr_abort_task - abort a SCSI command + * @dev: LUN specified in task management function or NULL if no LUN has been + * specified. + * @tmr: Task management function. + * @se_sess: Session a.k.a. I_T nexus. + */ void core_tmr_abort_task( struct se_device *dev, struct se_tmr_req *tmr, @@ -161,7 +168,7 @@ void core_tmr_abort_task( spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) { - if (dev != se_cmd->se_dev) + if (dev && dev != se_cmd->se_dev) continue; /* skip task management functions, including tmr->task_cmd */ @@ -178,6 +185,13 @@ void core_tmr_abort_task( if (!__target_check_io_state(se_cmd, se_sess, 0)) continue; + if (!tmr->tmr_dev && + transport_lookup_tmr_lun(tmr->task_cmd, + se_cmd->orig_fe_lun) < 0) { + target_put_sess_cmd(se_cmd); + continue; + } + list_del_init(&se_cmd->se_cmd_list); spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 1cadc9eefa21..9b1dced4534a 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1584,18 +1584,18 @@ static void target_complete_tmr_failure(struct work_struct *work) } /** - * target_submit_tmr - lookup unpacked lun and submit uninitialized se_cmd - * for TMR CDBs + * target_submit_tmr - submit a SCSI task management function to the target core * * @se_cmd: command descriptor to submit * @se_sess: associated se_sess for endpoint * @sense: pointer to SCSI sense buffer - * @unpacked_lun: unpacked LUN to reference for struct se_lun + * @unpacked_lun: LUN the TMR applies to. Ignored if TARGET_SCF_IGNORE_TMR_LUN + * has been set in @flags. * @fabric_context: fabric context for TMR req * @tm_type: Type of TM request * @gfp: gfp type for caller * @tag: referenced task tag for TMR_ABORT_TASK - * @flags: submit cmd flags + * @flags: submit cmd flags (TARGET_SCF_*). * * Callable from all contexts. **/ @@ -1631,7 +1631,14 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess, return ret; } - ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun); + if (flags & TARGET_SCF_IGNORE_TMR_LUN) { + WARN_ON_ONCE(tm_type != TMR_ABORT_TASK); + se_cmd->se_lun = NULL; + se_cmd->se_dev = NULL; + se_cmd->se_tmr_req->tmr_dev = NULL; + } else { + ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun); + } if (ret) { /* * For callback during failure handling, push this work off @@ -1641,6 +1648,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess, schedule_work(&se_cmd->work); return 0; } + WARN_ON_ONCE(tm_type != TMR_ABORT_TASK && !se_cmd->se_dev); transport_generic_handle_tmr(se_cmd); return 0; } @@ -3129,7 +3137,10 @@ int transport_generic_handle_tmr( spin_unlock_irqrestore(&cmd->t_state_lock, flags); INIT_WORK(&cmd->work, target_tmr_work); - queue_work(cmd->se_dev->tmr_wq, &cmd->work); + if (cmd->se_dev) + queue_work(cmd->se_dev->tmr_wq, &cmd->work); + else + schedule_work(&cmd->work); return 0; } EXPORT_SYMBOL(transport_generic_handle_tmr); diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index fc52207e8076..b953ad635929 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -186,6 +186,7 @@ enum target_sc_flags_table { TARGET_SCF_ACK_KREF = 0x02, TARGET_SCF_UNKNOWN_SIZE = 0x04, TARGET_SCF_USE_CPUID = 0x08, + TARGET_SCF_IGNORE_TMR_LUN = 0x10, }; /* fabric independent task management function values */
target_submit_tmr() only supports the I_T_L nexus for SCSI abort and other task management functions. Make it possible for target drivers to specify I_T nexus for SCSI abort by passing the TARGET_SCF_IGNORE_TMR_LUN flag to target_submit_tmr(). Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Himanshu Madhani <himanshu.madhani@cavium.com> Cc: Giridhar Malavali <giridhar.malavali@cavium.com> --- drivers/target/target_core_tmr.c | 16 +++++++++++++++- drivers/target/target_core_transport.c | 23 +++++++++++++++++------ include/target/target_core_base.h | 1 + 3 files changed, 33 insertions(+), 7 deletions(-)