| Message ID | 20170207231224.GA29711@infradead.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, 2017-02-07 at 15:12 -0800, Christoph Hellwig wrote: > And the real patch after compile fixing it is here of course: > Getting rid of the extra se_node_acl->acl_free_comp seems to make sense here.. The only potential issue is if returning from configfs syscall rmdir /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/acls/$INITIATOR/ before se_node_acl memory is released has implications when the underlying ../$WWN/$TPGT/ is removed immediately after. In any event, I'll verify this patch with the original test case and use it instead if everything looks OK. > diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h > index 9ab7090f7c83..96c38f30069d 100644 > --- a/drivers/target/target_core_internal.h > +++ b/drivers/target/target_core_internal.h > @@ -152,6 +152,7 @@ void target_qf_do_work(struct work_struct *work); > bool target_check_wce(struct se_device *dev); > bool target_check_fua(struct se_device *dev); > void __target_execute_cmd(struct se_cmd *, bool); > +void target_put_nacl(struct se_node_acl *); > > /* target_core_stat.c */ > void target_stat_setup_dev_default_groups(struct se_device *); > diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c > index d99752c6cd60..08738c87e5b0 100644 > --- a/drivers/target/target_core_tpg.c > +++ b/drivers/target/target_core_tpg.c > @@ -197,7 +197,6 @@ static struct se_node_acl *target_alloc_node_acl(struct se_portal_group *tpg, > INIT_LIST_HEAD(&acl->acl_sess_list); > INIT_HLIST_HEAD(&acl->lun_entry_hlist); > kref_init(&acl->acl_kref); > - init_completion(&acl->acl_free_comp); > spin_lock_init(&acl->nacl_sess_lock); > mutex_init(&acl->lun_entry_mutex); > atomic_set(&acl->acl_pr_ref_count, 0); > @@ -370,21 +369,6 @@ void core_tpg_del_initiator_node_acl(struct se_node_acl *acl) > target_shutdown_sessions(acl); > > target_put_nacl(acl); > - /* > - * Wait for last target_put_nacl() to complete in target_complete_nacl() > - * for active fabric session transport_deregister_session() callbacks. > - */ > - wait_for_completion(&acl->acl_free_comp); > - > - core_tpg_wait_for_nacl_pr_ref(acl); > - core_free_device_list_for_node(acl, tpg); > - > - pr_debug("%s_TPG[%hu] - Deleted ACL with TCQ Depth: %d for %s" > - " Initiator Node: %s\n", tpg->se_tpg_tfo->get_fabric_name(), > - tpg->se_tpg_tfo->tpg_get_tag(tpg), acl->queue_depth, > - tpg->se_tpg_tfo->get_fabric_name(), acl->initiatorname); > - > - kfree(acl); > } > > /* core_tpg_set_initiator_node_queue_depth(): > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 1cadc9eefa21..9ebd86a8ef41 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -453,19 +453,25 @@ ssize_t target_show_dynamic_sessions(struct se_portal_group *se_tpg, char *page) > } > EXPORT_SYMBOL(target_show_dynamic_sessions); > > -static void target_complete_nacl(struct kref *kref) > +static void target_destroy_nacl(struct kref *kref) > { > struct se_node_acl *nacl = container_of(kref, > struct se_node_acl, acl_kref); > + struct se_portal_group *se_tpg = nacl->se_tpg; > > - complete(&nacl->acl_free_comp); > + mutex_lock(&se_tpg->acl_node_mutex); > + list_del(&nacl->acl_list); > + mutex_unlock(&se_tpg->acl_node_mutex); > + > + core_tpg_wait_for_nacl_pr_ref(nacl); > + core_free_device_list_for_node(nacl, se_tpg); > + kfree(nacl); > } > > void target_put_nacl(struct se_node_acl *nacl) > { > - kref_put(&nacl->acl_kref, target_complete_nacl); > + kref_put(&nacl->acl_kref, target_destroy_nacl); > } > -EXPORT_SYMBOL(target_put_nacl); > > void transport_deregister_session_configfs(struct se_session *se_sess) > { > @@ -499,12 +505,40 @@ EXPORT_SYMBOL(transport_deregister_session_configfs); > void transport_free_session(struct se_session *se_sess) > { > struct se_node_acl *se_nacl = se_sess->se_node_acl; > + > /* > * Drop the se_node_acl->nacl_kref obtained from within > * core_tpg_get_initiator_node_acl(). > */ > if (se_nacl) { > + struct se_portal_group *se_tpg = se_nacl->se_tpg; > + const struct target_core_fabric_ops *se_tfo = se_tpg->se_tpg_tfo; > + unsigned long flags; > + bool stop = false; > + > se_sess->se_node_acl = NULL; > + > + /* > + * Also determine if we need to drop the extra ->cmd_kref if > + * it had been previously dynamically generated, and > + * the endpoint is not caching dynamic ACLs. > + */ > + mutex_lock(&se_tpg->acl_node_mutex); > + if (se_nacl->dynamic_node_acl && > + !se_tfo->tpg_check_demo_mode_cache(se_tpg)) { > + spin_lock_irqsave(&se_nacl->nacl_sess_lock, flags); > + if (list_empty(&se_nacl->acl_sess_list)) > + stop = true; > + spin_unlock_irqrestore(&se_nacl->nacl_sess_lock, flags); > + > + if (stop) > + list_del_init(&se_nacl->acl_list); > + } > + mutex_unlock(&se_tpg->acl_node_mutex); > + > + if (stop) > + target_put_nacl(se_nacl); > + > target_put_nacl(se_nacl); > } > if (se_sess->sess_cmd_map) { > @@ -518,16 +552,12 @@ EXPORT_SYMBOL(transport_free_session); > void transport_deregister_session(struct se_session *se_sess) > { > struct se_portal_group *se_tpg = se_sess->se_tpg; > - const struct target_core_fabric_ops *se_tfo; > - struct se_node_acl *se_nacl; > unsigned long flags; > - bool drop_nacl = false; > > if (!se_tpg) { > transport_free_session(se_sess); > return; > } > - se_tfo = se_tpg->se_tpg_tfo; > > spin_lock_irqsave(&se_tpg->session_lock, flags); > list_del(&se_sess->sess_list); > @@ -535,35 +565,8 @@ void transport_deregister_session(struct se_session *se_sess) > se_sess->fabric_sess_ptr = NULL; > spin_unlock_irqrestore(&se_tpg->session_lock, flags); > > - /* > - * Determine if we need to do extra work for this initiator node's > - * struct se_node_acl if it had been previously dynamically generated. > - */ > - se_nacl = se_sess->se_node_acl; > - > - mutex_lock(&se_tpg->acl_node_mutex); > - if (se_nacl && se_nacl->dynamic_node_acl) { > - if (!se_tfo->tpg_check_demo_mode_cache(se_tpg)) { > - list_del(&se_nacl->acl_list); > - drop_nacl = true; > - } > - } > - mutex_unlock(&se_tpg->acl_node_mutex); > - > - if (drop_nacl) { > - core_tpg_wait_for_nacl_pr_ref(se_nacl); > - core_free_device_list_for_node(se_nacl, se_tpg); > - se_sess->se_node_acl = NULL; > - kfree(se_nacl); > - } > pr_debug("TARGET_CORE[%s]: Deregistered fabric_sess\n", > se_tpg->se_tpg_tfo->get_fabric_name()); > - /* > - * If last kref is dropping now for an explicit NodeACL, awake sleeping > - * ->acl_free_comp caller to wakeup configfs se_node_acl->acl_group > - * removal context from within transport_free_session() code. > - */ > - > transport_free_session(se_sess); > } > EXPORT_SYMBOL(transport_deregister_session); > diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h > index 43edf82e54ff..edad452c3c25 100644 > --- a/include/target/target_core_base.h > +++ b/include/target/target_core_base.h > @@ -557,7 +557,6 @@ struct se_node_acl { > struct config_group acl_fabric_stat_group; > struct list_head acl_list; > struct list_head acl_sess_list; > - struct completion acl_free_comp; > struct kref acl_kref; > }; > > diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h > index 358041bad1da..1c417731b63d 100644 > --- a/include/target/target_core_fabric.h > +++ b/include/target/target_core_fabric.h > @@ -125,7 +125,6 @@ void transport_register_session(struct se_portal_group *, > struct se_node_acl *, struct se_session *, void *); > ssize_t target_show_dynamic_sessions(struct se_portal_group *, char *); > void transport_free_session(struct se_session *); > -void target_put_nacl(struct se_node_acl *); > void transport_deregister_session_configfs(struct se_session *); > void transport_deregister_session(struct se_session *); > > -- > 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-02-07 at 19:46 -0800, Nicholas A. Bellinger wrote: > On Tue, 2017-02-07 at 15:12 -0800, Christoph Hellwig wrote: > > And the real patch after compile fixing it is here of course: > > > > Getting rid of the extra se_node_acl->acl_free_comp seems to make sense > here.. > > The only potential issue is if returning from configfs syscall > rmdir /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/acls/$INITIATOR/ > before se_node_acl memory is released has implications when the > underlying ../$WWN/$TPGT/ is removed immediately after. > > In any event, I'll verify this patch with the original test case and use > it instead if everything looks OK. Ok, I remember why se_node_acl->acl_free_comp is needed now.. The issue is when se_node_acl is being explicitly removed from configfs while se_sessions associated with se_node_acl are still active. The se_node_acl->acl_group configfs_group_operations->drop_item() callback must block until all associated se_sessions have done their target_put_nacl() and completed se_node_acl->acl_free_comp, otherwise there is nothing preventing parent config_groups of se_node_acl from being removed while the se_sessions are still active. That said, dropping the unnecessary list_del_init() usage, but otherwise keeping the patch as-is.
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h index 9ab7090f7c83..96c38f30069d 100644 --- a/drivers/target/target_core_internal.h +++ b/drivers/target/target_core_internal.h @@ -152,6 +152,7 @@ void target_qf_do_work(struct work_struct *work); bool target_check_wce(struct se_device *dev); bool target_check_fua(struct se_device *dev); void __target_execute_cmd(struct se_cmd *, bool); +void target_put_nacl(struct se_node_acl *); /* target_core_stat.c */ void target_stat_setup_dev_default_groups(struct se_device *); diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c index d99752c6cd60..08738c87e5b0 100644 --- a/drivers/target/target_core_tpg.c +++ b/drivers/target/target_core_tpg.c @@ -197,7 +197,6 @@ static struct se_node_acl *target_alloc_node_acl(struct se_portal_group *tpg, INIT_LIST_HEAD(&acl->acl_sess_list); INIT_HLIST_HEAD(&acl->lun_entry_hlist); kref_init(&acl->acl_kref); - init_completion(&acl->acl_free_comp); spin_lock_init(&acl->nacl_sess_lock); mutex_init(&acl->lun_entry_mutex); atomic_set(&acl->acl_pr_ref_count, 0); @@ -370,21 +369,6 @@ void core_tpg_del_initiator_node_acl(struct se_node_acl *acl) target_shutdown_sessions(acl); target_put_nacl(acl); - /* - * Wait for last target_put_nacl() to complete in target_complete_nacl() - * for active fabric session transport_deregister_session() callbacks. - */ - wait_for_completion(&acl->acl_free_comp); - - core_tpg_wait_for_nacl_pr_ref(acl); - core_free_device_list_for_node(acl, tpg); - - pr_debug("%s_TPG[%hu] - Deleted ACL with TCQ Depth: %d for %s" - " Initiator Node: %s\n", tpg->se_tpg_tfo->get_fabric_name(), - tpg->se_tpg_tfo->tpg_get_tag(tpg), acl->queue_depth, - tpg->se_tpg_tfo->get_fabric_name(), acl->initiatorname); - - kfree(acl); } /* core_tpg_set_initiator_node_queue_depth(): diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 1cadc9eefa21..9ebd86a8ef41 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -453,19 +453,25 @@ ssize_t target_show_dynamic_sessions(struct se_portal_group *se_tpg, char *page) } EXPORT_SYMBOL(target_show_dynamic_sessions); -static void target_complete_nacl(struct kref *kref) +static void target_destroy_nacl(struct kref *kref) { struct se_node_acl *nacl = container_of(kref, struct se_node_acl, acl_kref); + struct se_portal_group *se_tpg = nacl->se_tpg; - complete(&nacl->acl_free_comp); + mutex_lock(&se_tpg->acl_node_mutex); + list_del(&nacl->acl_list); + mutex_unlock(&se_tpg->acl_node_mutex); + + core_tpg_wait_for_nacl_pr_ref(nacl); + core_free_device_list_for_node(nacl, se_tpg); + kfree(nacl); } void target_put_nacl(struct se_node_acl *nacl) { - kref_put(&nacl->acl_kref, target_complete_nacl); + kref_put(&nacl->acl_kref, target_destroy_nacl); } -EXPORT_SYMBOL(target_put_nacl); void transport_deregister_session_configfs(struct se_session *se_sess) { @@ -499,12 +505,40 @@ EXPORT_SYMBOL(transport_deregister_session_configfs); void transport_free_session(struct se_session *se_sess) { struct se_node_acl *se_nacl = se_sess->se_node_acl; + /* * Drop the se_node_acl->nacl_kref obtained from within * core_tpg_get_initiator_node_acl(). */ if (se_nacl) { + struct se_portal_group *se_tpg = se_nacl->se_tpg; + const struct target_core_fabric_ops *se_tfo = se_tpg->se_tpg_tfo; + unsigned long flags; + bool stop = false; + se_sess->se_node_acl = NULL; + + /* + * Also determine if we need to drop the extra ->cmd_kref if + * it had been previously dynamically generated, and + * the endpoint is not caching dynamic ACLs. + */ + mutex_lock(&se_tpg->acl_node_mutex); + if (se_nacl->dynamic_node_acl && + !se_tfo->tpg_check_demo_mode_cache(se_tpg)) { + spin_lock_irqsave(&se_nacl->nacl_sess_lock, flags); + if (list_empty(&se_nacl->acl_sess_list)) + stop = true; + spin_unlock_irqrestore(&se_nacl->nacl_sess_lock, flags); + + if (stop) + list_del_init(&se_nacl->acl_list); + } + mutex_unlock(&se_tpg->acl_node_mutex); + + if (stop) + target_put_nacl(se_nacl); + target_put_nacl(se_nacl); } if (se_sess->sess_cmd_map) { @@ -518,16 +552,12 @@ EXPORT_SYMBOL(transport_free_session); void transport_deregister_session(struct se_session *se_sess) { struct se_portal_group *se_tpg = se_sess->se_tpg; - const struct target_core_fabric_ops *se_tfo; - struct se_node_acl *se_nacl; unsigned long flags; - bool drop_nacl = false; if (!se_tpg) { transport_free_session(se_sess); return; } - se_tfo = se_tpg->se_tpg_tfo; spin_lock_irqsave(&se_tpg->session_lock, flags); list_del(&se_sess->sess_list); @@ -535,35 +565,8 @@ void transport_deregister_session(struct se_session *se_sess) se_sess->fabric_sess_ptr = NULL; spin_unlock_irqrestore(&se_tpg->session_lock, flags); - /* - * Determine if we need to do extra work for this initiator node's - * struct se_node_acl if it had been previously dynamically generated. - */ - se_nacl = se_sess->se_node_acl; - - mutex_lock(&se_tpg->acl_node_mutex); - if (se_nacl && se_nacl->dynamic_node_acl) { - if (!se_tfo->tpg_check_demo_mode_cache(se_tpg)) { - list_del(&se_nacl->acl_list); - drop_nacl = true; - } - } - mutex_unlock(&se_tpg->acl_node_mutex); - - if (drop_nacl) { - core_tpg_wait_for_nacl_pr_ref(se_nacl); - core_free_device_list_for_node(se_nacl, se_tpg); - se_sess->se_node_acl = NULL; - kfree(se_nacl); - } pr_debug("TARGET_CORE[%s]: Deregistered fabric_sess\n", se_tpg->se_tpg_tfo->get_fabric_name()); - /* - * If last kref is dropping now for an explicit NodeACL, awake sleeping - * ->acl_free_comp caller to wakeup configfs se_node_acl->acl_group - * removal context from within transport_free_session() code. - */ - transport_free_session(se_sess); } EXPORT_SYMBOL(transport_deregister_session); diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 43edf82e54ff..edad452c3c25 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -557,7 +557,6 @@ struct se_node_acl { struct config_group acl_fabric_stat_group; struct list_head acl_list; struct list_head acl_sess_list; - struct completion acl_free_comp; struct kref acl_kref; }; diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 358041bad1da..1c417731b63d 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -125,7 +125,6 @@ void transport_register_session(struct se_portal_group *, struct se_node_acl *, struct se_session *, void *); ssize_t target_show_dynamic_sessions(struct se_portal_group *, char *); void transport_free_session(struct se_session *); -void target_put_nacl(struct se_node_acl *); void transport_deregister_session_configfs(struct se_session *); void transport_deregister_session(struct se_session *);