From 0979cd90e458ca7434e7a58c2087a64a5f60637c Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 21 Jul 2019 01:47:11 +0000 Subject: [PATCH 1/6] scst: Make a comment more clear git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8461 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/include/scst.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index e274f5953..2079e8142 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -3120,7 +3120,7 @@ struct scst_tgt_dev { int max_sg_cnt; /************************************************************* - ** Tgt_dev's flags + ** Flags that control the behavior of a tgt_dev. *************************************************************/ /* Set if tgt_dev is read only (to save extra dereferences) */ From 81729121b0ea8ffc487b26c364c2dbbdb75e8409 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 21 Jul 2019 01:47:49 +0000 Subject: [PATCH 2/6] scst: Verify a locking assumption at runtime git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8462 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_lib.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 9c59e2352..7f839498c 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -5280,6 +5280,8 @@ void scst_tgt_dev_stop_threads(struct scst_tgt_dev *tgt_dev) TRACE_ENTRY(); + lockdep_assert_held(&scst_mutex); + if (tgt_dev->dev->threads_num < 0) goto out_deinit; From 51599b3c44c61afcaf13526f62307b9679bea379 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 21 Jul 2019 01:48:21 +0000 Subject: [PATCH 3/6] scst: Move a synchronize_rcu() call from scst_free_tgt_dev() to its callers This patch does not change any functionality. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8463 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_lib.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 7f839498c..5240729f3 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -2961,6 +2961,7 @@ retry_add: TRACE_MGMT_DBG("Replacing LUN %lld", (long long)tgt_dev->lun); scst_del_tgt_dev(tgt_dev); + synchronize_rcu(); scst_free_tgt_dev(tgt_dev); inq_changed_ua_needed = 1; break; @@ -3005,6 +3006,7 @@ next: luns_changed = true; something_freed = true; scst_del_tgt_dev(tgt_dev); + synchronize_rcu(); scst_free_tgt_dev(tgt_dev); } } @@ -4545,6 +4547,8 @@ out_on_del: scst_cm_on_del_lun(acg_dev, false); out_free: + /* To do: verify whether this synchronize_rcu() call is necessary. */ + synchronize_rcu(); list_for_each_entry_safe(tgt_dev, tt, &tmp_tgt_dev_list, extra_tgt_dev_list_entry) { scst_free_tgt_dev(tgt_dev); @@ -4645,6 +4649,7 @@ int scst_acg_del_lun(struct scst_acg *acg, uint64_t lun, mutex_unlock(&scst_mutex); scst_wait_for_tgt_devs(&tgt_dev_list); + synchronize_rcu(); mutex_lock(&scst_mutex); @@ -4694,6 +4699,7 @@ int scst_acg_repl_lun(struct scst_acg *acg, struct kobject *parent, mutex_unlock(&scst_mutex); scst_wait_for_tgt_devs(&tgt_dev_list); + synchronize_rcu(); mutex_lock(&scst_mutex); list_for_each_entry_safe(tgt_dev, tt, &tgt_dev_list, @@ -4840,6 +4846,7 @@ static void scst_free_acg(struct scst_acg *acg) scst_del_tgt_dev(tgt_dev); mutex_unlock(&sess->tgt_dev_list_mutex); + synchronize_rcu(); scst_free_tgt_dev(tgt_dev); } } @@ -5547,7 +5554,12 @@ static void scst_del_tgt_dev(struct scst_tgt_dev *tgt_dev) scst_tgt_dev_sysfs_del(tgt_dev); } -/* The caller must ensure that tgt_dev is not on sess_tgt_dev_list */ +/* + * The caller must ensure that tgt_dev is not on sess_tgt_dev_list. The caller + * is also responsible for calling synchronize_rcu() before freeing a tgt_dev + * if another thread could still be accessing @tgt_dev from inside an RCU + * read-side critical section. + */ static void scst_free_tgt_dev(struct scst_tgt_dev *tgt_dev) { struct scst_tgt_template *tgtt = tgt_dev->sess->tgt->tgtt; @@ -5560,8 +5572,6 @@ static void scst_free_tgt_dev(struct scst_tgt_dev *tgt_dev) #endif WARN_ON_ONCE(atomic_read(&tgt_dev->tgt_dev_cmd_count) != 0); - synchronize_rcu(); - if (tgtt->get_initiator_port_transport_id == NULL) dev->not_pr_supporting_tgt_devs_num--; @@ -5627,6 +5637,7 @@ void scst_sess_free_tgt_devs(struct scst_session *sess) list_for_each_entry_safe(tgt_dev, t, head, sess_tgt_dev_list_entry) { scst_del_tgt_dev(tgt_dev); + synchronize_rcu(); scst_free_tgt_dev(tgt_dev); } INIT_LIST_HEAD(head); From c9b43e0245f113e38ab476b44edde8bf7129099d Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 21 Jul 2019 01:49:03 +0000 Subject: [PATCH 4/6] scst: Fix an error path in scst_tgt_dev_setup_threads() git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8464 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_lib.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 5240729f3..20a7ffe8c 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -5229,10 +5229,8 @@ int scst_tgt_dev_setup_threads(struct scst_tgt_dev *tgt_dev) res = scst_add_threads(tgt_dev->active_cmd_threads, NULL, tgt_dev, dev->threads_num + tgtt->threads_num); - if (res != 0) { - /* Let's clear here, because no threads could be run */ - tgt_dev->active_cmd_threads->io_context = NULL; - } + if (res != 0) + scst_deinit_threads(&tgt_dev->tgt_dev_cmd_threads); break; } case SCST_THREADS_POOL_SHARED: @@ -5254,6 +5252,8 @@ int scst_tgt_dev_setup_threads(struct scst_tgt_dev *tgt_dev) out: if (res == 0) tm_dbg_init_tgt_dev(tgt_dev); + else + tgt_dev->active_cmd_threads->io_context = NULL; TRACE_EXIT_RES(res); return res; From 5b0f9ef0b935ed862dd7c2f633364c566c0a8ded Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 21 Jul 2019 01:49:38 +0000 Subject: [PATCH 5/6] scst: Increase tgt_dev_cmd_count by one This patch does not change any functionality. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8465 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/include/scst.h | 4 ++-- scst/src/scst_lib.c | 4 +++- scst/src/scst_pres.c | 2 +- scst/src/scst_sysfs.c | 4 +++- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index 2079e8142..ecc4e6361 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -3156,7 +3156,7 @@ struct scst_tgt_dev { */ int tgt_dev_dif_guard_format; - /* How many cmds alive on this dev in this session */ + /* One more than the number of commands associated with this tgt_dev. */ atomic_t tgt_dev_cmd_count ____cacheline_aligned_in_smp; /* ALUA command filter */ @@ -4776,7 +4776,7 @@ static inline int scst_cmd_get_block_size(struct scst_cmd *cmd) static inline unsigned int scst_get_active_cmd_count(struct scst_cmd *cmd) { if (likely(cmd->tgt_dev != NULL)) - return atomic_read(&cmd->tgt_dev->tgt_dev_cmd_count); + return atomic_read(&cmd->tgt_dev->tgt_dev_cmd_count) - 1; else return (unsigned int)-1; } diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 20a7ffe8c..abf9c1db3 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -5373,7 +5373,7 @@ static int scst_alloc_add_tgt_dev(struct scst_session *sess, atomic_set(&tgt_dev->tgt_dev_dif_guard_failed_dev, 0); tgt_dev->sess = sess; - atomic_set(&tgt_dev->tgt_dev_cmd_count, 0); + atomic_set(&tgt_dev->tgt_dev_cmd_count, 1); if (acg_dev->acg->acg_black_hole_type != SCST_ACG_BLACK_HOLE_NONE) set_bit(SCST_TGT_DEV_BLACK_HOLE, &tgt_dev->tgt_dev_flags); else @@ -5552,6 +5552,8 @@ static void scst_del_tgt_dev(struct scst_tgt_dev *tgt_dev) list_del_rcu(&tgt_dev->sess_tgt_dev_list_entry); scst_tgt_dev_sysfs_del(tgt_dev); + + atomic_dec(&tgt_dev->tgt_dev_cmd_count); } /* diff --git a/scst/src/scst_pres.c b/scst/src/scst_pres.c index b3022c644..68446a6b3 100644 --- a/scst/src/scst_pres.c +++ b/scst/src/scst_pres.c @@ -607,7 +607,7 @@ static void scst_pr_abort_reg(struct scst_device *dev, TRACE_PR("Aborting %d commands for %s/%d (reg %p, key 0x%016llx, " "tgt_dev %p, sess %p)", - atomic_read(®->tgt_dev->tgt_dev_cmd_count), + atomic_read(®->tgt_dev->tgt_dev_cmd_count) - 1, debug_transport_id_to_initiator_name(reg->transport_id), reg->rel_tgt_id, reg, be64_to_cpu(reg->key), reg->tgt_dev, sess); diff --git a/scst/src/scst_sysfs.c b/scst/src/scst_sysfs.c index 33710e4b5..65a1aa9f6 100644 --- a/scst/src/scst_sysfs.c +++ b/scst/src/scst_sysfs.c @@ -4152,7 +4152,8 @@ static ssize_t scst_tgt_dev_active_commands_show(struct kobject *kobj, tgt_dev = container_of(kobj, struct scst_tgt_dev, tgt_dev_kobj); - pos = sprintf(buf, "%d\n", atomic_read(&tgt_dev->tgt_dev_cmd_count)); + pos = sprintf(buf, "%d\n", + atomic_read(&tgt_dev->tgt_dev_cmd_count) - 1); return pos; } @@ -4486,6 +4487,7 @@ static int scst_sysfs_sess_get_active_commands(struct scst_session *sess) list_for_each_entry_rcu(tgt_dev, head, sess_tgt_dev_list_entry) { active_cmds += atomic_read(&tgt_dev->tgt_dev_cmd_count); + active_cmds--; } } rcu_read_unlock(); From cf2167c2b95e5cbc701f7a5a5b30515eb278a02b Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 21 Jul 2019 01:52:03 +0000 Subject: [PATCH 6/6] scst: Free removed LUNs asynchronously Instead of waiting until all commands associated with a LUN have finished, do not wait and free LUN data structures once all associated commands have finished. This commit should fix https://sourceforge.net/p/scst/tickets/23/. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8466 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/include/scst.h | 11 ++++++ scst/src/scst_lib.c | 93 +++++++++++++++++++++----------------------- scst/src/scst_priv.h | 1 + scst/src/scst_targ.c | 5 ++- 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index ecc4e6361..adb64540f 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -3104,6 +3104,10 @@ struct scst_tgt_dev { /* List entry in sess->sess_tgt_dev_list */ struct list_head sess_tgt_dev_list_entry; + struct rcu_head rcu; + struct work_struct free_work; + atomic_t *a; + struct scst_device *dev; /* to save extra dereferences */ uint64_t lun; /* to save extra dereferences */ @@ -3259,6 +3263,13 @@ struct scst_acg_dev { /* sysfs release completion */ struct completion *acg_dev_kobj_release_cmpl; + /* + * Number of deleted tgt_devs associated with this acg_dev. Set if an + * acg_dev is no longer visible and will be freed as soon as all + * associated tgt_dev instances have been freed. + */ + int nr_deleted_tgt_devs; + /* Name of the link to the corresponding LUN */ char acg_dev_link_name[20]; }; diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index abf9c1db3..f5a967440 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -4557,10 +4557,37 @@ out_free: goto out; } +static void scst_tgt_dev_free_workfn(struct work_struct *work) +{ + struct scst_tgt_dev *tgt_dev = container_of(work, typeof(*tgt_dev), + free_work); + struct scst_acg_dev *acg_dev = tgt_dev->acg_dev; + struct scst_device *dev = tgt_dev->dev; + atomic_t *a = tgt_dev->a; + + mutex_lock(&scst_mutex); + scst_free_tgt_dev(tgt_dev); + WARN_ON_ONCE(acg_dev->nr_deleted_tgt_devs <= 0); + if (--acg_dev->nr_deleted_tgt_devs == 0) + scst_free_acg_dev(acg_dev); + mutex_unlock(&scst_mutex); + + percpu_ref_put(&dev->refcnt); + scst_put(a); +} + +void scst_free_tgt_dev_rcu(struct rcu_head *rcu) +{ + struct scst_tgt_dev *tgt_dev = container_of(rcu, typeof(*tgt_dev), rcu); + + tgt_dev->a = scst_get(); + percpu_ref_get(&tgt_dev->dev->refcnt); + WARN_ON_ONCE(!schedule_work(&tgt_dev->free_work)); +} + /* Delete a LUN without generating a unit attention. */ static struct scst_acg_dev *__scst_acg_del_lun(struct scst_acg *acg, uint64_t lun, - struct list_head *tgt_dev_list, bool *report_luns_changed) { struct scst_acg_dev *acg_dev = NULL, *a; @@ -4569,8 +4596,6 @@ static struct scst_acg_dev *__scst_acg_del_lun(struct scst_acg *acg, lockdep_assert_held(&scst_mutex); - INIT_LIST_HEAD(tgt_dev_list); - list_for_each_entry(a, &acg->acg_dev_list, acg_dev_list_entry) { if (a->lun == lun) { acg_dev = a; @@ -4583,17 +4608,17 @@ static struct scst_acg_dev *__scst_acg_del_lun(struct scst_acg *acg, *report_luns_changed = scst_cm_on_del_lun(acg_dev, *report_luns_changed); + acg_dev->nr_deleted_tgt_devs = 1; + list_for_each_entry_safe(tgt_dev, tt, &acg_dev->dev->dev_tgt_dev_list, dev_tgt_dev_list_entry) { if (tgt_dev->acg_dev == acg_dev) { sess = tgt_dev->sess; mutex_lock(&sess->tgt_dev_list_mutex); + acg_dev->nr_deleted_tgt_devs++; scst_del_tgt_dev(tgt_dev); mutex_unlock(&sess->tgt_dev_list_mutex); - - list_add_tail(&tgt_dev->extra_tgt_dev_list_entry, - tgt_dev_list); } } @@ -4606,37 +4631,17 @@ out: return acg_dev; } -static int scst_tgt_devs_cmds(struct list_head *tgt_dev_list) -{ - struct scst_tgt_dev *tgt_dev; - int res = 0; - - list_for_each_entry(tgt_dev, tgt_dev_list, extra_tgt_dev_list_entry) - res += atomic_read(&tgt_dev->tgt_dev_cmd_count); - - return res; -} - -static void scst_wait_for_tgt_devs(struct list_head *tgt_dev_list) -{ - while (scst_tgt_devs_cmds(tgt_dev_list) > 0) - mdelay(100); -} - int scst_acg_del_lun(struct scst_acg *acg, uint64_t lun, bool gen_report_luns_changed) { int res = 0; struct scst_acg_dev *acg_dev; - struct scst_tgt_dev *tgt_dev, *tt; - struct list_head tgt_dev_list; TRACE_ENTRY(); lockdep_assert_held(&scst_mutex); - acg_dev = __scst_acg_del_lun(acg, lun, &tgt_dev_list, - &gen_report_luns_changed); + acg_dev = __scst_acg_del_lun(acg, lun, &gen_report_luns_changed); if (acg_dev == NULL) { PRINT_ERROR("Device is not found in group %s", acg->acg_name); res = -EINVAL; @@ -4648,16 +4653,11 @@ int scst_acg_del_lun(struct scst_acg *acg, uint64_t lun, mutex_unlock(&scst_mutex); - scst_wait_for_tgt_devs(&tgt_dev_list); synchronize_rcu(); mutex_lock(&scst_mutex); - - list_for_each_entry_safe(tgt_dev, tt, &tgt_dev_list, - extra_tgt_dev_list_entry) { - scst_free_tgt_dev(tgt_dev); - } - scst_free_acg_dev(acg_dev); + if (--acg_dev->nr_deleted_tgt_devs == 0) + scst_free_acg_dev(acg_dev); out: TRACE_EXIT_RES(res); @@ -4671,13 +4671,12 @@ int scst_acg_repl_lun(struct scst_acg *acg, struct kobject *parent, { struct scst_acg_dev *acg_dev; bool del_gen_ua = false; - struct scst_tgt_dev *tgt_dev, *tt; - struct list_head tgt_dev_list; + struct scst_tgt_dev *tgt_dev; int res = -EINVAL; lockdep_assert_held(&scst_mutex); - acg_dev = __scst_acg_del_lun(acg, lun, &tgt_dev_list, &del_gen_ua); + acg_dev = __scst_acg_del_lun(acg, lun, &del_gen_ua); if (!acg_dev) flags |= SCST_ADD_LUN_GEN_UA; res = scst_acg_add_lun(acg, parent, dev, lun, flags, NULL); @@ -4698,15 +4697,10 @@ int scst_acg_repl_lun(struct scst_acg *acg, struct kobject *parent, } mutex_unlock(&scst_mutex); - scst_wait_for_tgt_devs(&tgt_dev_list); synchronize_rcu(); mutex_lock(&scst_mutex); - list_for_each_entry_safe(tgt_dev, tt, &tgt_dev_list, - extra_tgt_dev_list_entry) { - scst_free_tgt_dev(tgt_dev); - } - if (acg_dev) + if (acg_dev && --acg_dev->nr_deleted_tgt_devs == 0) scst_free_acg_dev(acg_dev); out: @@ -5289,9 +5283,6 @@ void scst_tgt_dev_stop_threads(struct scst_tgt_dev *tgt_dev) lockdep_assert_held(&scst_mutex); - if (tgt_dev->dev->threads_num < 0) - goto out_deinit; - if (tgt_dev->active_cmd_threads == &scst_main_cmd_threads) { /* Global async threads */ kref_put(&tgt_dev->aic_keeper->aic_keeper_kref, @@ -5308,7 +5299,6 @@ void scst_tgt_dev_stop_threads(struct scst_tgt_dev *tgt_dev) scst_deinit_threads(&tgt_dev->tgt_dev_cmd_threads); } /* else no threads (not yet initialized, e.g.) */ -out_deinit: tm_dbg_deinit_tgt_dev(tgt_dev); tgt_dev->active_cmd_threads = NULL; @@ -5346,6 +5336,8 @@ static int scst_alloc_add_tgt_dev(struct scst_session *sess, } INIT_LIST_HEAD(&tgt_dev->sess_tgt_dev_list_entry); + init_rcu_head(&tgt_dev->rcu); + INIT_WORK(&tgt_dev->free_work, scst_tgt_dev_free_workfn); tgt_dev->dev = dev; tgt_dev->lun = acg_dev->lun; tgt_dev->acg_dev = acg_dev; @@ -5510,6 +5502,8 @@ out_dec_free: out_free_ua: scst_free_all_UA(tgt_dev); + destroy_rcu_head(&tgt_dev->rcu); + kmem_cache_free(scst_tgtd_cachep, tgt_dev); goto out; } @@ -5553,7 +5547,8 @@ static void scst_del_tgt_dev(struct scst_tgt_dev *tgt_dev) scst_tgt_dev_sysfs_del(tgt_dev); - atomic_dec(&tgt_dev->tgt_dev_cmd_count); + if (atomic_dec_return(&tgt_dev->tgt_dev_cmd_count) == 0) + call_rcu(&tgt_dev->rcu, scst_free_tgt_dev_rcu); } /* @@ -5590,6 +5585,8 @@ static void scst_free_tgt_dev(struct scst_tgt_dev *tgt_dev) scst_tgt_dev_stop_threads(tgt_dev); + destroy_rcu_head(&tgt_dev->rcu); + kmem_cache_free(scst_tgtd_cachep, tgt_dev); percpu_ref_put(&dev->refcnt); diff --git a/scst/src/scst_priv.h b/scst/src/scst_priv.h index e0d378e0e..84788dd89 100644 --- a/scst/src/scst_priv.h +++ b/scst/src/scst_priv.h @@ -382,6 +382,7 @@ void scst_check_reassign_sessions(void); int scst_sess_alloc_tgt_devs(struct scst_session *sess); void scst_sess_free_tgt_devs(struct scst_session *sess); struct scst_tgt_dev *scst_lookup_tgt_dev(struct scst_session *sess, u64 lun); +void scst_free_tgt_dev_rcu(struct rcu_head *rcu); void scst_nexus_loss(struct scst_tgt_dev *tgt_dev, bool queue_UA); #define SCST_ADD_LUN_READ_ONLY 1 diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index 7f1d830c8..a0918ae71 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -4504,7 +4504,8 @@ static int scst_pre_xmit_response1(struct scst_cmd *cmd) * latency, so we should decrement them after cmd completed. */ smp_mb__before_atomic_dec(); - atomic_dec(&cmd->tgt_dev->tgt_dev_cmd_count); + if (atomic_dec_return(&cmd->tgt_dev->tgt_dev_cmd_count) == 0) + call_rcu(&cmd->tgt_dev->rcu, scst_free_tgt_dev_rcu); percpu_ref_put(&cmd->dev->refcnt); #ifdef CONFIG_SCST_PER_DEVICE_CMD_COUNT_LIMIT atomic_dec(&cmd->dev->dev_cmd_count); @@ -5142,7 +5143,7 @@ static int __scst_init_cmd(struct scst_cmd *cmd) scst_set_cmd_state(cmd, SCST_CMD_STATE_PARSE); - cnt = atomic_inc_return(&tgt_dev->tgt_dev_cmd_count); + cnt = atomic_inc_return(&tgt_dev->tgt_dev_cmd_count) - 1; if (unlikely(cnt > dev->max_tgt_dev_commands)) { TRACE(TRACE_FLOW_CONTROL, "Too many pending commands (%d) in "