| Message ID | 20170208222507.25715-8-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 02/08/2017 11:24 PM, Bart Van Assche wrote: > Several SCSI transport protocols require that SCSI task management > functions are processed in the order in which these have been > submitted by the initiator system. Hence introduce a workqueue > per session for TMF processing. Do not specify WQ_MEM_RECLAIM since > only the tcm_loop driver can queue TMF work from inside the memory > allocation path and since the tcm_loop driver is only used to debug > the SCSI target code. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Hannes Reinecke <hare@suse.com> > Cc: David Disseldorp <ddiss@suse.de> > --- > drivers/target/target_core_transport.c | 11 ++++++++++- > include/target/target_core_base.h | 1 + > 2 files changed, 11 insertions(+), 1 deletion(-) > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
On Wed, 2017-02-08 at 14:24 -0800, Bart Van Assche wrote: > Several SCSI transport protocols require that SCSI task management > functions are processed in the order in which these have been > submitted by the initiator system. Hence introduce a workqueue > per session for TMF processing. Do not specify WQ_MEM_RECLAIM since > only the tcm_loop driver can queue TMF work from inside the memory > allocation path and since the tcm_loop driver is only used to debug > the SCSI target code. This change effects exiting code negatively in two different ways. Comments below. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Hannes Reinecke <hare@suse.com> > Cc: David Disseldorp <ddiss@suse.de> > --- > drivers/target/target_core_transport.c | 11 ++++++++++- > include/target/target_core_base.h | 1 + > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 1cadc9eefa21..084a7fbfc72e 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -241,6 +241,13 @@ struct se_session *transport_init_session(enum target_prot_op sup_prot_ops) > spin_lock_init(&se_sess->sess_cmd_lock); > se_sess->sup_prot_ops = sup_prot_ops; > > + se_sess->tmf_wq = alloc_workqueue("tmf-%p", WQ_UNBOUND, 1, se_sess); > + if (!se_sess->tmf_wq) { > + pr_err("%s: workqueue allocation failed\n", __func__); > + kfree(se_sess); > + return ERR_PTR(-ENOMEM); > + } > + First, alloc_workqueue() and destroy_workqueue use a global wq_pool_mutex when updating the global workqueue list. Which means session creation and session deletion for target-core effectively has a global synchronization point across all endpoints and all fabric drivers. My use-case for scale-out with LIO requires to run at least 1000 unique /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/ endpoints per node that must be able to operate independently of one another, with a least one active session per endpoint. So from that perspective, this change to add alloc_workqueue() + destroy_workqueue() in the session creation + deletion paths is a non-starter for scale-out. > return se_sess; > } > EXPORT_SYMBOL(transport_init_session); > @@ -507,6 +514,8 @@ void transport_free_session(struct se_session *se_sess) > se_sess->se_node_acl = NULL; > target_put_nacl(se_nacl); > } > + if (se_sess->tmf_wq) > + destroy_workqueue(se_sess->tmf_wq); > if (se_sess->sess_cmd_map) { > percpu_ida_destroy(&se_sess->sess_tag_pool); > kvfree(se_sess->sess_cmd_map); > @@ -3129,7 +3138,7 @@ 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); > + queue_work(cmd->se_sess->tmf_wq, &cmd->work); > return 0; > } The concern for LUN_RESET is multiple kthreads draining per se_device TMRs and se_cmds from se_device->state_list in target_tmr_work() can now happen in parallel in multiple kthreads (for the same device) within the per se_session workqueue. That is a rather fundamental change to the way existing TMR code works, so without an actual bug this change addresses, or use case where it's necessary I'm dropping this patch for now. -- 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-02-09 at 01:34 -0800, Nicholas A. Bellinger wrote: > So from that perspective, this change to add alloc_workqueue() + > destroy_workqueue() in the session creation + deletion paths is a > non-starter for scale-out. OK, I'll drop this patch. 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
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 1cadc9eefa21..084a7fbfc72e 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -241,6 +241,13 @@ struct se_session *transport_init_session(enum target_prot_op sup_prot_ops) spin_lock_init(&se_sess->sess_cmd_lock); se_sess->sup_prot_ops = sup_prot_ops; + se_sess->tmf_wq = alloc_workqueue("tmf-%p", WQ_UNBOUND, 1, se_sess); + if (!se_sess->tmf_wq) { + pr_err("%s: workqueue allocation failed\n", __func__); + kfree(se_sess); + return ERR_PTR(-ENOMEM); + } + return se_sess; } EXPORT_SYMBOL(transport_init_session); @@ -507,6 +514,8 @@ void transport_free_session(struct se_session *se_sess) se_sess->se_node_acl = NULL; target_put_nacl(se_nacl); } + if (se_sess->tmf_wq) + destroy_workqueue(se_sess->tmf_wq); if (se_sess->sess_cmd_map) { percpu_ida_destroy(&se_sess->sess_tag_pool); kvfree(se_sess->sess_cmd_map); @@ -3129,7 +3138,7 @@ 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); + queue_work(cmd->se_sess->tmf_wq, &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 c9aada11a30c..f6e5fb22f338 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -606,6 +606,7 @@ struct se_session { spinlock_t sess_cmd_lock; void *sess_cmd_map; struct percpu_ida sess_tag_pool; + struct workqueue_struct *tmf_wq; }; struct se_device;
Several SCSI transport protocols require that SCSI task management functions are processed in the order in which these have been submitted by the initiator system. Hence introduce a workqueue per session for TMF processing. Do not specify WQ_MEM_RECLAIM since only the tcm_loop driver can queue TMF work from inside the memory allocation path and since the tcm_loop driver is only used to debug the SCSI target code. Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Hannes Reinecke <hare@suse.com> Cc: David Disseldorp <ddiss@suse.de> --- drivers/target/target_core_transport.c | 11 ++++++++++- include/target/target_core_base.h | 1 + 2 files changed, 11 insertions(+), 1 deletion(-)