Greg KH, on 10/15/2010 12:04 AM wrote: > I am asking that ANY kobject release function call kfree to release the > memory that object is embedded in. That is how kobjects work, please > read the documentation for more details. > >> We have a simple and straightforward errors recovery semantic: if >> scst_tgt_sysfs_create() failed, then tgt_kobj returned deinited as if >> scst_tgt_sysfs_create() has never been called. This is a regular >> practice in the kernel: don't return half-initialized objects. > > True. > >> If we implement freeing tgt in scst_tgtt_release() as you requesting, we >> will need to add in the error recovery path additional recovery code to >> track and delete half-initialized tgt_kobj. Particularly, we will need >> to add additional flag to track that tgt_kobj initialized, hence needs >> deleting. Similar flag and code will be added in all similar to scst_tgt >> SCST objects. >> >> This code will be quite errors prone as you can see on the example of >> device_register() which on failure requires device_put() to be called >> (http://lkml.org/lkml/2010/9/19/93). > > That's not "error prone", it's "people don't read the provided > documentation about how to use the API". > > And yes, one could argue to make the API easier to use, and patches are > always welcome to do so. > >> (I'm not questioning device_register() implementation, there might be >> very good reasons to implement it this way (I don't know), I mean, it >> is too easy to forget to do the needed recovery of the half-created >> objects as this case demonstrating.) > > There are good reasons, but the most important one being, if you pass > off an object, as of that moment in time, you had better handle the > reference counting correctly. If you think of it this way, cleanup > logic is even simpler as that's the only rule you ever need to think > about, none of thies "half-initialized" stuff. > >> Could you confirm if I understand you correctly and need to implement >> freeing tgt in the kobject release() function, please? > > Again, yes. Please read the documentation. Here is the patch. It converts all the cases except 3: - struct scst_tgt_template and struct scst_dev_type, because they are supplied by target drivers and dev handlers correspondingly and usually static. - struct scst_session, because sometimes we need to create several sessions for the same initiator (for iSCSI sessions reinstatement, for instance) and for such cases need a logic to have different SYSFS names for them. To have this logic functioning correctly, we need to keep sessions alive longer than life of their kobject (see scst_sess_sysfs_create() and scst_free_session() for more details). I don't like this patch, because it makes the code in almost 100 lines of code bigger and worse by making objects initialization/freeing a lot more complicated and harder to audit. But, since you insist, I applied it. It isn't late to revert it, just say a word. Thanks, Vlad Signed-off-by: Vladislav Bolkhovitin <vst@xxxxxxxx> --- drivers/scst/scst_lib.c | 102 ++++++++++++++---- drivers/scst/scst_main.c | 26 +--- drivers/scst/scst_priv.h | 37 +++++- drivers/scst/scst_sysfs.c | 247 ++++++++++++++++++++++++---------------------- include/scst/scst.h | 22 +++- 5 files changed, 261 insertions(+), 173 deletions(-) diff -upr linux-2.6.35/include/scst/scst.h linux-2.6.35/include/scst/scst.h --- include/scst/scst.h +++ include/scst/scst.h @@ -1431,8 +1431,10 @@ struct scst_tgt { char *default_group_name; #endif + unsigned int tgt_kobj_initialized:1; + /* sysfs release completion */ - struct completion tgt_kobj_release_cmpl; + struct completion *tgt_kobj_release_cmpl; struct kobject tgt_kobj; /* main targets/target kobject */ struct kobject *tgt_sess_kobj; /* target/sessions/ */ @@ -2077,6 +2079,9 @@ struct scst_device { /* If set, dev is read only */ unsigned short rd_only:1; + /* If set, dev_kobj was initialized */ + unsigned short dev_kobj_initialized:1; + /**************************************************************/ /************************************************************* @@ -2217,7 +2222,7 @@ struct scst_device { enum scst_dev_type_threads_pool_type threads_pool_type; /* sysfs release completion */ - struct completion dev_kobj_release_cmpl; + struct completion *dev_kobj_release_cmpl; struct kobject dev_kobj; /* kobject for this struct */ struct kobject *dev_exp_kobj; /* exported groups */ @@ -2342,6 +2347,9 @@ struct scst_tgt_dev { /* Set if INQUIRY DATA HAS CHANGED UA is needed */ unsigned int inq_changed_ua_needed:1; + /* Set if tgt_dev_kobj was initialized */ + unsigned int tgt_dev_kobj_initialized:1; + /* * Stored Unit Attention sense and its length for possible * subsequent REQUEST SENSE. Both protected by tgt_dev_lock. @@ -2350,7 +2358,7 @@ struct scst_tgt_dev { uint8_t tgt_dev_sense[SCST_SENSE_BUFFERSIZE]; /* sysfs release completion */ - struct completion tgt_dev_kobj_release_cmpl; + struct completion *tgt_dev_kobj_release_cmpl; struct kobject tgt_dev_kobj; /* kobject for this struct */ @@ -2378,6 +2386,9 @@ struct scst_acg_dev { /* If set, the corresponding LU is read only */ unsigned int rd_only:1; + /* If set acg_dev_kobj was initialized */ + unsigned int acg_dev_kobj_initialized:1; + struct scst_acg *acg; /* parent acg */ /* List entry in dev->dev_acg_dev_list */ @@ -2390,7 +2401,7 @@ struct scst_acg_dev { struct kobject acg_dev_kobj; /* sysfs release completion */ - struct completion acg_dev_kobj_release_cmpl; + struct completion *acg_dev_kobj_release_cmpl; /* Name of the link to the corresponding LUN */ char acg_dev_link_name[20]; @@ -2431,9 +2442,10 @@ struct scst_acg { cpumask_t acg_cpu_mask; unsigned int tgt_acg:1; + unsigned int acg_kobj_initialized:1; /* sysfs release completion */ - struct completion acg_kobj_release_cmpl; + struct completion *acg_kobj_release_cmpl; /* kobject for this structure */ struct kobject acg_kobj; diff -upr linux-2.6.35/drivers/scst/scst_main.c linux-2.6.35/drivers/scst/scst_main.c --- drivers/scst/scst_main.c +++ drivers/scst/scst_main.c @@ -553,7 +553,6 @@ out: #ifndef CONFIG_SCST_PROC out_sysfs_del: mutex_unlock(&scst_mutex); - scst_tgt_sysfs_del(tgt); goto out_free_tgt; #endif @@ -640,10 +639,6 @@ again: mutex_unlock(&scst_mutex); scst_resume_activity(); -#ifndef CONFIG_SCST_PROC - scst_tgt_sysfs_del(tgt); -#endif - PRINT_INFO("Target %s for template %s unregistered successfully", tgt->tgt_name, vtt->name); @@ -862,7 +857,7 @@ static int scst_register_device(struct s #endif } - res = scst_alloc_device(GFP_KERNEL, &dev); + res = scst_alloc_dev(GFP_KERNEL, &dev); if (res != 0) goto out_unlock; @@ -925,7 +920,7 @@ out_del: list_del(&dev->dev_list_entry); out_free_dev: - scst_free_device(dev); + scst_free_dev(dev); out_unlock: mutex_unlock(&scst_mutex); @@ -973,12 +968,10 @@ static void scst_unregister_device(struc scst_resume_activity(); - scst_dev_sysfs_del(dev); - PRINT_INFO("Detached from scsi%d, channel %d, id %d, lun %d, type %d", scsidp->host->host_no, scsidp->channel, scsidp->id, scsidp->lun, scsidp->type); - scst_free_device(dev); + scst_free_dev(dev); out: @@ -1054,7 +1047,6 @@ int scst_register_virtual_device(struct { int res, rc; struct scst_device *dev, *d; - bool sysfs_del = false; @@ -1088,7 +1080,7 @@ int scst_register_virtual_device(struct goto out_resume; } - res = scst_alloc_device(GFP_KERNEL, &dev); + res = scst_alloc_dev(GFP_KERNEL, &dev); if (res != 0) goto out_unlock; @@ -1135,7 +1127,6 @@ int scst_register_virtual_device(struct if (strcmp(d->virt_name, dev_name) == 0) { PRINT_ERROR("Device %s already exists", dev_name); res = -EEXIST; - sysfs_del = true; goto out_pr_clear_dev; } } @@ -1143,7 +1134,6 @@ int scst_register_virtual_device(struct rc = scst_assign_dev_handler(dev, dev_handler); if (rc != 0) { res = rc; - sysfs_del = true; goto out_pr_clear_dev; } @@ -1171,9 +1161,7 @@ out_pr_clear_dev: out_free_dev: mutex_unlock(&scst_mutex); - if (sysfs_del) - scst_dev_sysfs_del(dev); - scst_free_device(dev); + scst_free_dev(dev); goto out_resume; out_unlock: @@ -1225,12 +1213,10 @@ void scst_unregister_virtual_device(int mutex_unlock(&scst_mutex); scst_resume_activity(); - scst_dev_sysfs_del(dev); - PRINT_INFO("Detached from virtual device %s (id %d)", dev->virt_name, dev->virt_id); - scst_free_device(dev); + scst_free_dev(dev); out: diff -upr linux-2.6.35/drivers/scst/scst_priv.h linux-2.6.35/drivers/scst/scst_priv.h --- drivers/scst/scst_priv.h +++ drivers/scst/scst_priv.h @@ -306,13 +306,16 @@ int scst_queue_retry_cmd(struct scst_cmd int scst_alloc_tgt(struct scst_tgt_template *tgtt, struct scst_tgt **tgt); void scst_free_tgt(struct scst_tgt *tgt); +void __scst_free_tgt(struct scst_tgt *tgt); -int scst_alloc_device(gfp_t gfp_mask, struct scst_device **out_dev); -void scst_free_device(struct scst_device *dev); +int scst_alloc_dev(gfp_t gfp_mask, struct scst_device **out_dev); +void scst_free_dev(struct scst_device *dev); +void __scst_free_dev(struct scst_device *dev); struct scst_acg *scst_alloc_add_acg(struct scst_tgt *tgt, const char *acg_name, bool tgt_acg); void scst_del_free_acg(struct scst_acg *acg); +void __scst_free_acg(struct scst_acg *acg); struct scst_acg *scst_tgt_find_acg(struct scst_tgt *tgt, const char *name); struct scst_acg *scst_find_acg(const struct scst_session *sess); @@ -323,6 +326,8 @@ int scst_sess_alloc_tgt_devs(struct scst void scst_sess_free_tgt_devs(struct scst_session *sess); void scst_nexus_loss(struct scst_tgt_dev *tgt_dev, bool queue_UA); +void __scst_free_tgt_dev(struct scst_tgt_dev *tgt_dev); + int scst_acg_add_lun(struct scst_acg *acg, struct kobject *parent, struct scst_device *dev, uint64_t lun, int read_only, bool gen_scst_report_luns_changed, struct scst_acg_dev **out_acg_dev); @@ -336,6 +341,8 @@ int scst_acg_remove_name(struct scst_acg void scst_del_free_acn(struct scst_acn *acn, bool reassign); struct scst_acn *scst_find_acn(struct scst_acg *acg, const char *name); +void scst_free_acg_dev(struct scst_acg_dev *acg_dev); + /* The activity supposed to be suspended and scst_mutex held */ static inline bool scst_acg_sess_is_empty(struct scst_acg *acg) { @@ -426,19 +433,24 @@ static inline int scst_sysfs_init(void) } static inline void scst_sysfs_cleanup(void) { } +static inline void scst_tgt_sysfs_del_free(struct scst_tgt *tgt) { BUG(); } + static inline int scst_devt_dev_sysfs_create(struct scst_device *dev) { return 0; } static inline void scst_devt_dev_sysfs_del(struct scst_device *dev) { } -static inline void scst_dev_sysfs_del(struct scst_device *dev) { } +static inline void scst_dev_sysfs_del_free(struct scst_device *dev) { BUG(); } static inline int scst_tgt_dev_sysfs_create(struct scst_tgt_dev *tgt_dev) { return 0; } -static inline void scst_tgt_dev_sysfs_del(struct scst_tgt_dev *tgt_dev) { } +static inline void scst_tgt_dev_sysfs_del_free(struct scst_tgt_dev *tgt_dev) +{ + BUG(); +} static inline int scst_sess_sysfs_create(struct scst_session *sess) { @@ -451,7 +463,12 @@ static inline int scst_acg_dev_sysfs_cre return 0; } -static inline void scst_acg_dev_sysfs_del(struct scst_acg_dev *acg_dev) { } +static inline void scst_acg_dev_sysfs_del_free(struct scst_acg_dev *acg_dev) +{ + BUG(); +} + +static inline void scst_acg_sysfs_del_free(struct scst_acg *acg) { BUG(); } static inline int scst_acn_sysfs_create(struct scst_acn *acn) { @@ -473,7 +490,7 @@ int scst_tgtt_sysfs_create(struct scst_t void scst_tgtt_sysfs_del(struct scst_tgt_template *tgtt); int scst_tgt_sysfs_create(struct scst_tgt *tgt); void scst_tgt_sysfs_prepare_put(struct scst_tgt *tgt); -void scst_tgt_sysfs_del(struct scst_tgt *tgt); +void scst_tgt_sysfs_del_free(struct scst_tgt *tgt); int scst_sess_sysfs_create(struct scst_session *sess); void scst_sess_sysfs_del(struct scst_session *sess); int scst_recreate_sess_luns_link(struct scst_session *sess); @@ -482,17 +499,17 @@ void scst_sgv_sysfs_del(struct sgv_pool int scst_devt_sysfs_create(struct scst_dev_type *devt); void scst_devt_sysfs_del(struct scst_dev_type *devt); int scst_dev_sysfs_create(struct scst_device *dev); -void scst_dev_sysfs_del(struct scst_device *dev); +void scst_dev_sysfs_del_free(struct scst_device *dev); int scst_tgt_dev_sysfs_create(struct scst_tgt_dev *tgt_dev); -void scst_tgt_dev_sysfs_del(struct scst_tgt_dev *tgt_dev); +void scst_tgt_dev_sysfs_del_free(struct scst_tgt_dev *tgt_dev); int scst_devt_dev_sysfs_create(struct scst_device *dev); void scst_devt_dev_sysfs_del(struct scst_device *dev); int scst_acg_sysfs_create(struct scst_tgt *tgt, struct scst_acg *acg); -void scst_acg_sysfs_del(struct scst_acg *acg); +void scst_acg_sysfs_del_free(struct scst_acg *acg); int scst_acg_dev_sysfs_create(struct scst_acg_dev *acg_dev, struct kobject *parent); -void scst_acg_dev_sysfs_del(struct scst_acg_dev *acg_dev); +void scst_acg_dev_sysfs_del_free(struct scst_acg_dev *acg_dev); int scst_acn_sysfs_create(struct scst_acn *acn); void scst_acn_sysfs_del(struct scst_acn *acn); diff -upr linux-2.6.35/drivers/scst/scst_lib.c linux-2.6.35/drivers/scst/scst_lib.c --- drivers/scst/scst_lib.c +++ drivers/scst/scst_lib.c @@ -2536,7 +2536,7 @@ out: } /* No locks */ -void scst_free_tgt(struct scst_tgt *tgt) +void __scst_free_tgt(struct scst_tgt *tgt) { @@ -2551,8 +2551,22 @@ void scst_free_tgt(struct scst_tgt *tgt) return; } +/* No locks */ +void scst_free_tgt(struct scst_tgt *tgt) +{ + if (tgt->tgt_kobj_initialized) + scst_tgt_sysfs_del_free(tgt); + else + __scst_free_tgt(tgt); + return; +} + /* Called under scst_mutex and suspended activity */ -int scst_alloc_device(gfp_t gfp_mask, struct scst_device **out_dev) +int scst_alloc_dev(gfp_t gfp_mask, struct scst_device **out_dev) { struct scst_device *dev; int res = 0; @@ -2596,7 +2610,20 @@ out: return res; } -void scst_free_device(struct scst_device *dev) +void __scst_free_dev(struct scst_device *dev) +{ + TRACE_MEM("Freeing dev %p", dev); + + kfree(dev->virt_name); + kfree(dev); +} + +/* + * Must not be called under scst_mutex, due to possible deadlock with + * sysfs ref counting in sysfs works (it is waiting for the last put, but + * the last ref counter holder is waiting for scst_mutex) + */ +void scst_free_dev(struct scst_device *dev) { @@ -2611,8 +2638,10 @@ void scst_free_device(struct scst_device scst_deinit_threads(&dev->dev_cmd_threads); - kfree(dev->virt_name); - kfree(dev); + if (dev->dev_kobj_initialized) + scst_dev_sysfs_del_free(dev); + else + __scst_free_dev(dev); return; @@ -2656,11 +2685,17 @@ out: return res; } +void scst_free_acg_dev(struct scst_acg_dev *acg_dev) +{ + TRACE_MEM("Freeing acg_dev %p", acg_dev); + kmem_cache_free(scst_acgd_cachep, acg_dev); +} + /* * The activity supposed to be suspended and scst_mutex held or the * corresponding target supposed to be stopped. */ -static void scst_del_free_acg_dev(struct scst_acg_dev *acg_dev, bool del_sysfs) +static void scst_del_free_acg_dev(struct scst_acg_dev *acg_dev) { @@ -2669,10 +2704,10 @@ static void scst_del_free_acg_dev(struct list_del(&acg_dev->acg_dev_list_entry); list_del(&acg_dev->dev_acg_dev_list_entry); - if (del_sysfs) - scst_acg_dev_sysfs_del(acg_dev); - - kmem_cache_free(scst_acgd_cachep, acg_dev); + if (acg_dev->acg_dev_kobj_initialized) + scst_acg_dev_sysfs_del_free(acg_dev); + else + scst_free_acg_dev(acg_dev); return; @@ -2688,7 +2723,6 @@ int scst_acg_add_lun(struct scst_acg *ac struct scst_tgt_dev *tgt_dev; struct scst_session *sess; LIST_HEAD(tmp_tgt_dev_list); - bool del_sysfs = true; @@ -2718,10 +2752,8 @@ int scst_acg_add_lun(struct scst_acg *ac } res = scst_acg_dev_sysfs_create(acg_dev, parent); - if (res != 0) { - del_sysfs = false; + if (res != 0) goto out_free; - } if (gen_scst_report_luns_changed) scst_report_luns_changed(acg); @@ -2742,7 +2774,7 @@ out_free: extra_tgt_dev_list_entry) { scst_free_tgt_dev(tgt_dev); } - scst_del_free_acg_dev(acg_dev, del_sysfs); + scst_del_free_acg_dev(acg_dev); goto out; } @@ -2774,7 +2806,7 @@ int scst_acg_del_lun(struct scst_acg *ac scst_free_tgt_dev(tgt_dev); } - scst_del_free_acg_dev(acg_dev, true); + scst_del_free_acg_dev(acg_dev); if (gen_scst_report_luns_changed) scst_report_luns_changed(acg); @@ -2787,6 +2819,22 @@ out: return res; } +void __scst_free_acg(struct scst_acg *acg) +{ + TRACE_MEM("Freeing acg %p", acg); + + kfree(acg->acg_name); + kfree(acg); +} + +static void scst_free_acg(struct scst_acg *acg) +{ + if (acg->acg_kobj_initialized) + scst_acg_sysfs_del_free(acg); + else + __scst_free_acg(acg); +} + /* The activity supposed to be suspended and scst_mutex held */ struct scst_acg *scst_alloc_add_acg(struct scst_tgt *tgt, const char *acg_name, bool tgt_acg) @@ -2844,7 +2892,7 @@ out_del: #endif out_free: - kfree(acg); + scst_free_acg(acg); acg = NULL; goto out; } @@ -2871,7 +2919,7 @@ void scst_del_free_acg(struct scst_acg * if (tgt_dev->acg_dev == acg_dev) scst_free_tgt_dev(tgt_dev); } - scst_del_free_acg_dev(acg_dev, true); + scst_del_free_acg_dev(acg_dev); } /* Freeing names */ @@ -2887,8 +2935,6 @@ void scst_del_free_acg(struct scst_acg * if (acg->tgt_acg) { TRACE_DBG("Removing acg %s from list", acg->acg_name); list_del(&acg->acg_list_entry); - - scst_acg_sysfs_del(acg); } else acg->tgt->default_acg = NULL; #endif @@ -2897,8 +2943,7 @@ void scst_del_free_acg(struct scst_acg * sBUG_ON(!list_empty(&acg->acg_dev_list)); sBUG_ON(!list_empty(&acg->acn_list)); - kfree(acg->acg_name); - kfree(acg); + scst_free_acg(acg); return; @@ -3440,6 +3485,12 @@ void scst_nexus_loss(struct scst_tgt_dev return; } +void __scst_free_tgt_dev(struct scst_tgt_dev *tgt_dev) +{ + TRACE_MEM("Freeing tgt_dev %p", tgt_dev); + kmem_cache_free(scst_tgtd_cachep, tgt_dev); +} + /* * scst_mutex supposed to be held, there must not be parallel activity in this * session. @@ -3456,8 +3507,6 @@ static void scst_free_tgt_dev(struct scs list_del(&tgt_dev->sess_tgt_dev_list_entry); - scst_tgt_dev_sysfs_del(tgt_dev); - if (tgt_dev->sess->tgt->tgtt->get_initiator_port_transport_id == NULL) dev->not_pr_supporting_tgt_devs_num--; @@ -3476,7 +3525,10 @@ static void scst_free_tgt_dev(struct scs sBUG_ON(!list_empty(&tgt_dev->thr_data_list)); - kmem_cache_free(scst_tgtd_cachep, tgt_dev); + if (tgt_dev->tgt_dev_kobj_initialized) + scst_tgt_dev_sysfs_del_free(tgt_dev); + else + __scst_free_tgt_dev(tgt_dev); return; diff -upr linux-2.6.35/drivers/scst/scst_sysfs.c linux-2.6.35/drivers/scst/scst_sysfs.c --- drivers/scst/scst_sysfs.c +++ drivers/scst/scst_sysfs.c @@ -922,7 +922,9 @@ static void scst_tgt_release(struct kobj tgt = container_of(kobj, struct scst_tgt, tgt_kobj); - complete_all(&tgt->tgt_kobj_release_cmpl); + complete_all(tgt->tgt_kobj_release_cmpl); + + __scst_free_tgt(tgt); return; @@ -940,7 +942,9 @@ static void scst_acg_release(struct kobj acg = container_of(kobj, struct scst_acg, acg_kobj); - complete_all(&acg->acg_kobj_release_cmpl); + complete_all(acg->acg_kobj_release_cmpl); + + __scst_free_acg(acg); return; @@ -1116,8 +1120,11 @@ static struct kobj_attribute tgt_enable_ scst_tgt_enable_show, scst_tgt_enable_store); /* - * Supposed to be called under scst_mutex. In case of error will drop, - * then reacquire it. + * Supposed to be called under scst_mutex. + * + * Upon return, including with an error, if tgt_kobj_initialized set + * scst_tgt_sysfs_del_free() must be called to free tgt instead of + * __scst_free_tgt()! */ int scst_tgt_sysfs_create(struct scst_tgt *tgt) { @@ -1126,8 +1133,6 @@ int scst_tgt_sysfs_create(struct scst_tg - init_completion(&tgt->tgt_kobj_release_cmpl); - res = kobject_init_and_add(&tgt->tgt_kobj, &tgt_ktype, &tgt->tgtt->tgtt_kobj, tgt->tgt_name); if (res != 0) { @@ -1135,6 +1140,8 @@ int scst_tgt_sysfs_create(struct scst_tg goto out; } + tgt->tgt_kobj_initialized = 1; + if ((tgt->tgtt->enable_target != NULL) && (tgt->tgtt->is_target_enabled != NULL)) { res = sysfs_create_file(&tgt->tgt_kobj, @@ -1142,7 +1149,7 @@ int scst_tgt_sysfs_create(struct scst_tg if (res != 0) { PRINT_ERROR("Can't add attr %s to sysfs", tgt_enable_attr.attr.name); - goto out_err; + goto out; } } @@ -1162,7 +1169,7 @@ int scst_tgt_sysfs_create(struct scst_tg if (res != 0) { PRINT_ERROR("Can't add attribute %s for tgt %s", scst_luns_mgmt.attr.name, tgt->tgt_name); - goto out_err; + goto out; } tgt->tgt_ini_grp_kobj = kobject_create_and_add("ini_groups", @@ -1178,7 +1185,7 @@ int scst_tgt_sysfs_create(struct scst_tg if (res != 0) { PRINT_ERROR("Can't add attribute %s for tgt %s", scst_ini_group_mgmt.attr.name, tgt->tgt_name); - goto out_err; + goto out; } res = sysfs_create_file(&tgt->tgt_kobj, @@ -1186,7 +1193,7 @@ int scst_tgt_sysfs_create(struct scst_tg if (res != 0) { PRINT_ERROR("Can't add attribute %s for tgt %s", scst_rel_tgt_id.attr.name, tgt->tgt_name); - goto out_err; + goto out; } res = sysfs_create_file(&tgt->tgt_kobj, @@ -1194,7 +1201,7 @@ int scst_tgt_sysfs_create(struct scst_tg if (res != 0) { PRINT_ERROR("Can't add attribute %s for tgt %s", scst_tgt_addr_method.attr.name, tgt->tgt_name); - goto out_err; + goto out; } res = sysfs_create_file(&tgt->tgt_kobj, @@ -1202,14 +1209,14 @@ int scst_tgt_sysfs_create(struct scst_tg if (res != 0) { PRINT_ERROR("Can't add attribute %s for tgt %s", scst_tgt_io_grouping_type.attr.name, tgt->tgt_name); - goto out_err; + goto out; } res = sysfs_create_file(&tgt->tgt_kobj, &scst_tgt_cpu_mask.attr); if (res != 0) { PRINT_ERROR("Can't add attribute %s for tgt %s", scst_tgt_cpu_mask.attr.name, tgt->tgt_name); - goto out_err; + goto out; } pattr = tgt->tgtt->tgt_attrs; @@ -1221,7 +1228,7 @@ int scst_tgt_sysfs_create(struct scst_tg if (res != 0) { PRINT_ERROR("Can't add tgt attr %s for tgt %s", (*pattr)->name, tgt->tgt_name); - goto out_err; + goto out; } pattr++; } @@ -1233,25 +1240,24 @@ out: out_nomem: res = -ENOMEM; - -out_err: - mutex_unlock(&scst_mutex); - scst_tgt_sysfs_del(tgt); - mutex_lock(&scst_mutex); goto out; } /* + * Deletes tgt from sysfs and frees it in the tgt_kobj release() + * * Must not be called under scst_mutex, due to possible deadlock with * sysfs ref counting in sysfs works (it is waiting for the last put, but * the last ref counter holder is waiting for scst_mutex) */ -void scst_tgt_sysfs_del(struct scst_tgt *tgt) +void scst_tgt_sysfs_del_free(struct scst_tgt *tgt) { - int rc; + DECLARE_COMPLETION_ONSTACK(cmpl); + tgt->tgt_kobj_release_cmpl = &cmpl; + kobject_del(tgt->tgt_sess_kobj); kobject_put(tgt->tgt_sess_kobj); @@ -1261,18 +1267,17 @@ void scst_tgt_sysfs_del(struct scst_tgt kobject_del(tgt->tgt_ini_grp_kobj); kobject_put(tgt->tgt_ini_grp_kobj); + if (atomic_read(&tgt->tgt_kobj.kref.refcount) > 1) + TRACE_MGMT_DBG("Waiting for releasing sysfs entry " + "for tgt %s (%d refs)...", tgt->tgt_name, + atomic_read(&tgt->tgt_kobj.kref.refcount)); + kobject_del(&tgt->tgt_kobj); kobject_put(&tgt->tgt_kobj); - rc = wait_for_completion_timeout(&tgt->tgt_kobj_release_cmpl, HZ); - if (rc == 0) { - PRINT_INFO("Waiting for releasing sysfs entry " - "for target %s (%d refs)...", tgt->tgt_name, - atomic_read(&tgt->tgt_kobj.kref.refcount)); - wait_for_completion(&tgt->tgt_kobj_release_cmpl); - PRINT_INFO("Done waiting for releasing sysfs " - "entry for target %s", tgt->tgt_name); - } + /* tgt can be dead here! */ + + wait_for_completion(&cmpl); return; @@ -1578,7 +1583,9 @@ static void scst_sysfs_dev_release(struc dev = container_of(kobj, struct scst_device, dev_kobj); - complete_all(&dev->dev_kobj_release_cmpl); + complete_all(dev->dev_kobj_release_cmpl); + + __scst_free_dev(dev); return; @@ -1690,8 +1697,9 @@ static struct kobj_type scst_dev_ktype = }; /* - * Must not be called under scst_mutex, because it can call - * scst_dev_sysfs_del() + * Upon return, including with an error, if dev_kobj_initialized set + * scst_dev_sysfs_del_free() must be called to free dev instead of + * __scst_free_dev()! */ int scst_dev_sysfs_create(struct scst_device *dev) { @@ -1699,8 +1707,6 @@ int scst_dev_sysfs_create(struct scst_de - init_completion(&dev->dev_kobj_release_cmpl); - res = kobject_init_and_add(&dev->dev_kobj, &scst_dev_ktype, scst_devices_kobj, dev->virt_name); if (res != 0) { @@ -1708,13 +1714,15 @@ int scst_dev_sysfs_create(struct scst_de goto out; } + dev->dev_kobj_initialized = 1; + dev->dev_exp_kobj = kobject_create_and_add("exported", &dev->dev_kobj); if (dev->dev_exp_kobj == NULL) { PRINT_ERROR("Can't create exported link for device %s", dev->virt_name); res = -ENOMEM; - goto out_del; + goto out; } if (dev->scsi_dev != NULL) { @@ -1723,7 +1731,7 @@ int scst_dev_sysfs_create(struct scst_de if (res != 0) { PRINT_ERROR("Can't create scsi_device link for dev %s", dev->virt_name); - goto out_del; + goto out; } } @@ -1734,7 +1742,7 @@ int scst_dev_sysfs_create(struct scst_de if (res != 0) { PRINT_ERROR("Can't create attr %s for dev %s", dev_dump_prs_attr.attr.name, dev->virt_name); - goto out_del; + goto out; } } #endif @@ -1742,38 +1750,37 @@ int scst_dev_sysfs_create(struct scst_de out: return res; - -out_del: - scst_dev_sysfs_del(dev); - goto out; } /* + * Deletes dev from sysfs and frees it in the dev_kobj release() + * * Must not be called under scst_mutex, due to possible deadlock with * sysfs ref counting in sysfs works (it is waiting for the last put, but * the last ref counter holder is waiting for scst_mutex) */ -void scst_dev_sysfs_del(struct scst_device *dev) +void scst_dev_sysfs_del_free(struct scst_device *dev) { - int rc; + DECLARE_COMPLETION_ONSTACK(cmpl); + dev->dev_kobj_release_cmpl = &cmpl; + kobject_del(dev->dev_exp_kobj); kobject_put(dev->dev_exp_kobj); + if (atomic_read(&dev->dev_kobj.kref.refcount) > 1) + TRACE_MGMT_DBG("Waiting for releasing sysfs entry " + "for dev %s (%d refs)...", dev->virt_name, + atomic_read(&dev->dev_kobj.kref.refcount)); + kobject_del(&dev->dev_kobj); kobject_put(&dev->dev_kobj); - rc = wait_for_completion_timeout(&dev->dev_kobj_release_cmpl, HZ); - if (rc == 0) { - PRINT_INFO("Waiting for releasing sysfs entry " - "for device %s (%d refs)...", dev->virt_name, - atomic_read(&dev->dev_kobj.kref.refcount)); - wait_for_completion(&dev->dev_kobj_release_cmpl); - PRINT_INFO("Done waiting for releasing sysfs " - "entry for device %s", dev->virt_name); - } + /* dev can be dead here! */ + + wait_for_completion(&cmpl); return; @@ -1930,7 +1937,9 @@ static void scst_sysfs_tgt_dev_release(s tgt_dev = container_of(kobj, struct scst_tgt_dev, tgt_dev_kobj); - complete_all(&tgt_dev->tgt_dev_kobj_release_cmpl); + complete_all(tgt_dev->tgt_dev_kobj_release_cmpl); + + __scst_free_tgt_dev(tgt_dev); return; @@ -1948,8 +1957,6 @@ int scst_tgt_dev_sysfs_create(struct scs - init_completion(&tgt_dev->tgt_dev_kobj_release_cmpl); - res = kobject_init_and_add(&tgt_dev->tgt_dev_kobj, &scst_tgt_dev_ktype, &tgt_dev->sess->sess_kobj, "lun%lld", (unsigned long long)tgt_dev->lun); @@ -1959,38 +1966,42 @@ int scst_tgt_dev_sysfs_create(struct scs goto out; } + tgt_dev->tgt_dev_kobj_initialized = 1; + out: return res; } /* + * Deletes tgt_dev from sysfs and frees it in the tgt_dev_kobj release() + * * Called with scst_mutex held. * * !! No sysfs works must use kobject_get() to protect tgt_dev, due to possible * !! deadlock with scst_mutex (it is waiting for the last put, but * !! the last ref counter holder is waiting for scst_mutex) */ -void scst_tgt_dev_sysfs_del(struct scst_tgt_dev *tgt_dev) +void scst_tgt_dev_sysfs_del_free(struct scst_tgt_dev *tgt_dev) { - int rc; + DECLARE_COMPLETION_ONSTACK(cmpl); - kobject_del(&tgt_dev->tgt_dev_kobj); - kobject_put(&tgt_dev->tgt_dev_kobj); + tgt_dev->tgt_dev_kobj_release_cmpl = &cmpl; - rc = wait_for_completion_timeout( - &tgt_dev->tgt_dev_kobj_release_cmpl, HZ); - if (rc == 0) { - PRINT_INFO("Waiting for releasing sysfs entry " - "for tgt_dev %lld (%d refs)...", + if (atomic_read(&tgt_dev->tgt_dev_kobj.kref.refcount) > 1) + TRACE_MGMT_DBG("Waiting for releasing sysfs entry " + "for tgt_dev LUN %lld, (%d refs)...", (unsigned long long)tgt_dev->lun, atomic_read(&tgt_dev->tgt_dev_kobj.kref.refcount)); - wait_for_completion(&tgt_dev->tgt_dev_kobj_release_cmpl); - PRINT_INFO("Done waiting for releasing sysfs entry for " - "tgt_dev %lld", (unsigned long long)tgt_dev->lun); - } + + kobject_del(&tgt_dev->tgt_dev_kobj); + kobject_put(&tgt_dev->tgt_dev_kobj); + + /* tgt_dev can be dead here! */ + + wait_for_completion(&cmpl); return; @@ -2515,7 +2526,9 @@ static void scst_acg_dev_release(struct acg_dev = container_of(kobj, struct scst_acg_dev, acg_dev_kobj); - complete_all(&acg_dev->acg_dev_kobj_release_cmpl); + complete_all(acg_dev->acg_dev_kobj_release_cmpl); + + scst_free_acg_dev(acg_dev); return; @@ -2550,41 +2563,49 @@ static struct kobj_type acg_dev_ktype = }; /* + * Deletes acg_dev from sysfs and frees it in the acg_dev_kobj release() + * * Called with scst_mutex held. * * !! No sysfs works must use kobject_get() to protect acg_dev, due to possible * !! deadlock with scst_mutex (it is waiting for the last put, but * !! the last ref counter holder is waiting for scst_mutex) */ -void scst_acg_dev_sysfs_del(struct scst_acg_dev *acg_dev) +void scst_acg_dev_sysfs_del_free(struct scst_acg_dev *acg_dev) { - int rc; + DECLARE_COMPLETION_ONSTACK(cmpl); + acg_dev->acg_dev_kobj_release_cmpl = &cmpl; + if (acg_dev->dev != NULL) { sysfs_remove_link(acg_dev->dev->dev_exp_kobj, acg_dev->acg_dev_link_name); kobject_put(&acg_dev->dev->dev_kobj); } + if (atomic_read(&acg_dev->acg_dev_kobj.kref.refcount) > 1) + TRACE_MGMT_DBG("Waiting for releasing sysfs entry " + "for acg_dev %p (%d refs)...", acg_dev, + atomic_read(&acg_dev->acg_dev_kobj.kref.refcount)); + kobject_del(&acg_dev->acg_dev_kobj); kobject_put(&acg_dev->acg_dev_kobj); - rc = wait_for_completion_timeout(&acg_dev->acg_dev_kobj_release_cmpl, HZ); - if (rc == 0) { - PRINT_INFO("Waiting for releasing sysfs entry " - "for acg_dev %p (%d refs)...", acg_dev, - atomic_read(&acg_dev->acg_dev_kobj.kref.refcount)); - wait_for_completion(&acg_dev->acg_dev_kobj_release_cmpl); - PRINT_INFO("Done waiting for releasing sysfs " - "entry for acg_dev %p", acg_dev); - } + /* acg_dev can be dead here! */ + + wait_for_completion(&cmpl); return; } +/* + * Upon return, including with an error, if acg_dev_kobj_initialized set + * scst_acg_dev_sysfs_del_free() must be called to free acg_dev instead of + * scst_free_acg_dev()! + */ int scst_acg_dev_sysfs_create(struct scst_acg_dev *acg_dev, struct kobject *parent) { @@ -2592,8 +2613,6 @@ int scst_acg_dev_sysfs_create(struct scs - init_completion(&acg_dev->acg_dev_kobj_release_cmpl); - res = kobject_init_and_add(&acg_dev->acg_dev_kobj, &acg_dev_ktype, parent, "%u", acg_dev->lun); if (res != 0) { @@ -2601,6 +2620,8 @@ int scst_acg_dev_sysfs_create(struct scs goto out; } + acg_dev->acg_dev_kobj_initialized = 1; + kobject_get(&acg_dev->dev->dev_kobj); snprintf(acg_dev->acg_dev_link_name, sizeof(acg_dev->acg_dev_link_name), @@ -2611,7 +2632,7 @@ int scst_acg_dev_sysfs_create(struct scs if (res != 0) { PRINT_ERROR("Can't create acg %s LUN link", acg_dev->acg->acg_name); - goto out_del; + goto out; } res = sysfs_create_link(&acg_dev->acg_dev_kobj, @@ -2619,15 +2640,11 @@ int scst_acg_dev_sysfs_create(struct scs if (res != 0) { PRINT_ERROR("Can't create acg %s device link", acg_dev->acg->acg_name); - goto out_del; + goto out; } out: return res; - -out_del: - scst_acg_dev_sysfs_del(acg_dev); - goto out; } static int __scst_process_luns_mgmt_store(char *buffer, @@ -3340,41 +3357,49 @@ out: } /* + * Deletes acg from sysfs and frees it in the acg_kobj release() + * * Called with scst_mutex held. * * !! No sysfs works must use kobject_get() to protect acg, due to possible * !! deadlock with scst_mutex (it is waiting for the last put, but * !! the last ref counter holder is waiting for scst_mutex) */ -void scst_acg_sysfs_del(struct scst_acg *acg) +void scst_acg_sysfs_del_free(struct scst_acg *acg) { - int rc; + DECLARE_COMPLETION_ONSTACK(cmpl); + acg->acg_kobj_release_cmpl = &cmpl; + kobject_del(acg->luns_kobj); kobject_put(acg->luns_kobj); kobject_del(acg->initiators_kobj); kobject_put(acg->initiators_kobj); + if (atomic_read(&acg->acg_kobj.kref.refcount) > 1) + TRACE_MGMT_DBG("Waiting for releasing sysfs entry " + "for acg %s (%d refs)...", acg->acg_name, + atomic_read(&acg->acg_kobj.kref.refcount)); + kobject_del(&acg->acg_kobj); kobject_put(&acg->acg_kobj); - rc = wait_for_completion_timeout(&acg->acg_kobj_release_cmpl, HZ); - if (rc == 0) { - PRINT_INFO("Waiting for releasing sysfs entry " - "for acg %s (%d refs)...", acg->acg_name, - atomic_read(&acg->acg_kobj.kref.refcount)); - wait_for_completion(&acg->acg_kobj_release_cmpl); - PRINT_INFO("Done waiting for releasing sysfs " - "entry for acg %s", acg->acg_name); - } + /* acg can be dead here! */ + + wait_for_completion(&cmpl); return; } +/* + * Upon return, including with an error, if acg_kobj_initialized set + * scst_acg_sysfs_del_free() must be called to free acg instead of + * __scst_free_acg()! + */ int scst_acg_sysfs_create(struct scst_tgt *tgt, struct scst_acg *acg) { @@ -3382,8 +3407,6 @@ int scst_acg_sysfs_create(struct scst_tg - init_completion(&acg->acg_kobj_release_cmpl); - res = kobject_init_and_add(&acg->acg_kobj, &acg_ktype, tgt->tgt_ini_grp_kobj, acg->acg_name); if (res != 0) { @@ -3391,19 +3414,21 @@ int scst_acg_sysfs_create(struct scst_tg goto out; } + acg->acg_kobj_initialized = 1; + acg->luns_kobj = kobject_create_and_add("luns", &acg->acg_kobj); if (acg->luns_kobj == NULL) { PRINT_ERROR("Can't create luns kobj for tgt %s", tgt->tgt_name); res = -ENOMEM; - goto out_del; + goto out; } res = sysfs_create_file(acg->luns_kobj, &scst_acg_luns_mgmt.attr); if (res != 0) { PRINT_ERROR("Can't add tgt attr %s for tgt %s", scst_acg_luns_mgmt.attr.name, tgt->tgt_name); - goto out_del; + goto out; } acg->initiators_kobj = kobject_create_and_add("initiators", @@ -3412,7 +3437,7 @@ int scst_acg_sysfs_create(struct scst_tg PRINT_ERROR("Can't create initiators kobj for tgt %s", tgt->tgt_name); res = -ENOMEM; - goto out_del; + goto out; } res = sysfs_create_file(acg->initiators_kobj, @@ -3420,37 +3445,33 @@ int scst_acg_sysfs_create(struct scst_tg if (res != 0) { PRINT_ERROR("Can't add tgt attr %s for tgt %s", scst_acg_ini_mgmt.attr.name, tgt->tgt_name); - goto out_del; + goto out; } res = sysfs_create_file(&acg->acg_kobj, &scst_acg_addr_method.attr); if (res != 0) { PRINT_ERROR("Can't add tgt attr %s for tgt %s", scst_acg_addr_method.attr.name, tgt->tgt_name); - goto out_del; + goto out; } res = sysfs_create_file(&acg->acg_kobj, &scst_acg_io_grouping_type.attr); if (res != 0) { PRINT_ERROR("Can't add tgt attr %s for tgt %s", scst_acg_io_grouping_type.attr.name, tgt->tgt_name); - goto out_del; + goto out; } res = sysfs_create_file(&acg->acg_kobj, &scst_acg_cpu_mask.attr); if (res != 0) { PRINT_ERROR("Can't add tgt attr %s for tgt %s", scst_acg_cpu_mask.attr.name, tgt->tgt_name); - goto out_del; + goto out; } out: return res; - -out_del: - scst_acg_sysfs_del(acg); - goto out; } static ssize_t scst_acg_addr_method_show(struct kobject *kobj, -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
