From 41380160f83a0e3b31dd044556b580e74fb29b3d Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Tue, 15 Nov 2022 14:41:10 +0300 Subject: [PATCH] scst_copy_mgr: Serialize scst_cm_desig_list list updates The SCST device may receive several events almost simultaneously to update its designators. Each such event calls scst_cm_update_dev(), which frees all device designators in the global list and then submits a inquiry that fills a new designators into the scst_cm_desig_list list. This is racy because submiting the inquiry is asynchronous and can be finished in another thread. scst_cm_update_dev() 1 scst_cm_update_dev() 2 ---------------------- ---------------------- [1] mutex_lock(&scst_mutex) [2] scst_cm_send_init_inquiry(dev, lun, NULL) [3] mutex_unlock(&scst_mutex) [4] mutex_lock(&scst_mutex) [5] scst_cm_dev_free_designators(dev) [6] scst_cm_init_inq_finish() [7] scst_cm_send_init_inquiry(dev, lun, NULL) [8] mutex_unlock(&scst_mutex) As a result we may get the scst_cm_desig_list list, which contains SCST device designators from several inquiries. Hence serialize scst_cm_desig_list list updates. --- scst/include/scst.h | 3 ++ scst/src/scst_copy_mgr.c | 77 +++++++++++++++++++++++++++++++++++----- scst/src/scst_lib.c | 1 + 3 files changed, 73 insertions(+), 8 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index 354e9b1e5..f83dc4be8 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -2828,6 +2828,9 @@ struct scst_device { atomic_t dev_cmd_count; #endif + /* Number of copy manager designators update requests. */ + atomic_t cm_update_req_cnt; + /* * One more than the number of commands associated with this device * and the number of SCST data structures holding a reference on this diff --git a/scst/src/scst_copy_mgr.c b/scst/src/scst_copy_mgr.c index 189f09a7c..874d0da08 100644 --- a/scst/src/scst_copy_mgr.c +++ b/scst/src/scst_copy_mgr.c @@ -2307,6 +2307,8 @@ static void scst_cm_inq_retry_fn(struct scst_cmd *cmd) return; } +static void scst_cm_update_dev_fini(struct scst_device *dev); + static void scst_cm_init_inq_finish(struct scst_cmd *cmd) { int length, page_len, off, rc; @@ -2428,6 +2430,7 @@ out_put: out_put_ref: percpu_ref_put(&dev->refcnt); + scst_cm_update_dev_fini(dev); out: TRACE_EXIT(); return; @@ -2591,6 +2594,8 @@ static int scst_cm_dev_register(struct scst_device *dev, uint64_t lun) scst_block_dev(dev); spin_unlock_bh(&dev->dev_lock); + atomic_set(&dev->cm_update_req_cnt, 1); + res = scst_cm_send_init_inquiry(dev, lun, NULL); if (res != 0) goto out_unblock; @@ -2604,6 +2609,8 @@ out_unblock: scst_unblock_dev(dev); spin_unlock_bh(&dev->dev_lock); + atomic_set(&dev->cm_update_req_cnt, 0); + scst_acg_del_lun(scst_cm_tgt->default_acg, lun, false); out_err: @@ -2656,19 +2663,15 @@ static void scst_cm_dev_unregister(struct scst_device *dev) return; } -void scst_cm_update_dev(struct scst_device *dev) +static int __scst_cm_update_dev(struct scst_device *dev) { unsigned int lun; - int rc; + int rc = 0; TRACE_ENTRY(); TRACE_MGMT_DBG("copy manager: updating device %s", dev->virt_name); - if (!scst_auto_cm_assignment || - !dev->handler->auto_cm_assignment_possible) - goto out; - mutex_lock(&scst_mutex); lun = scst_cm_get_lun(dev); @@ -2677,6 +2680,7 @@ void scst_cm_update_dev(struct scst_device *dev) * Verify that scst_unregister_virtual_device() is in progress. */ WARN_ON_ONCE(!dev->remove_completion); + rc = -EINVAL; goto out_unlock; } @@ -2693,17 +2697,74 @@ void scst_cm_update_dev(struct scst_device *dev) out_unlock: mutex_unlock(&scst_mutex); -out: TRACE_EXIT(); - return; + + return rc; out_unblock: spin_lock_bh(&dev->dev_lock); scst_unblock_dev(dev); spin_unlock_bh(&dev->dev_lock); + goto out_unlock; } +static void +scst_cm_update_dev_start(struct scst_device *dev) +{ + int update_req_cnt, rc; + + update_req_cnt = atomic_inc_return(&dev->cm_update_req_cnt); + if (update_req_cnt > 1) + return; + + rc = __scst_cm_update_dev(dev); + if (rc) + atomic_set(&dev->cm_update_req_cnt, 0); +} + +static void +scst_cm_update_dev_fini(struct scst_device *dev) +{ + int update_req_cnt, rc; + + update_req_cnt = atomic_dec_return(&dev->cm_update_req_cnt); + + WARN_ON_ONCE(update_req_cnt < 0); + + if (update_req_cnt == 0) + return; + + /* + * If we have received at least one update, we must re-update the + * designators information. We don't care about the exact number of + * updates we've received since the inquiry was submitted, as only the + * last one is indicative. So set dev->cm_update_req_cnt to 1 to avoid + * unnecessary __scst_cm_update_dev() calls. + */ + atomic_set(&dev->cm_update_req_cnt, 1); + + rc = __scst_cm_update_dev(dev); + if (rc) + atomic_set(&dev->cm_update_req_cnt, 0); +} + +void scst_cm_update_dev(struct scst_device *dev) +{ + TRACE_ENTRY(); + + if (!scst_auto_cm_assignment || + !dev->handler->auto_cm_assignment_possible) + goto out; + + scst_cm_update_dev_start(dev); + +out: + TRACE_EXIT(); + + return; +} + int scst_cm_on_dev_register(struct scst_device *dev) { int res = 0; diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 0d5c4e322..39c597f13 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -4215,6 +4215,7 @@ int scst_alloc_device(gfp_t gfp_mask, int nodeid, struct scst_device **out_dev) #ifdef CONFIG_SCST_PER_DEVICE_CMD_COUNT_LIMIT atomic_set(&dev->dev_cmd_count, 0); #endif + atomic_set(&dev->cm_update_req_cnt, 0); scst_init_mem_lim(&dev->dev_mem_lim); spin_lock_init(&dev->dev_lock); lockdep_register_key(&dev->dev_lock_key);