From 551eb598e5ea52996eb821f43740496a78a97b68 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 2 Jun 2017 14:21:53 -0700 Subject: scsi: Split scsi_internal_device_block() Instead of passing a "wait" argument to scsi_internal_device_block(), split this function into a function that waits and a function that doesn't wait. This will make it easier to serialize SCSI device state changes through a mutex. Signed-off-by: Bart Van Assche Reviewed-by: Hannes Reinecke Reviewed-by: Johannes Thumshirn Reviewed-by: Christoph Hellwig Cc: Sreekanth Reddy Signed-off-by: Martin K. Petersen --- include/scsi/scsi_device.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include/scsi/scsi_device.h') diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 05641aebd181..6ce6888f3c69 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -472,7 +472,7 @@ static inline int scsi_device_created(struct scsi_device *sdev) sdev->sdev_state == SDEV_CREATED_BLOCK; } -int scsi_internal_device_block(struct scsi_device *sdev, bool wait); +int scsi_internal_device_block_nowait(struct scsi_device *sdev); int scsi_internal_device_unblock(struct scsi_device *sdev, enum scsi_device_state new_state); -- cgit v1.2.3 From 43f7571be077ee4673466cbcba115427d68440e1 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 2 Jun 2017 14:21:54 -0700 Subject: scsi: Create two versions of scsi_internal_device_unblock() This will make it easier to serialize SCSI device state changes through a mutex. Signed-off-by: Bart Van Assche Reviewed-by: Hannes Reinecke Reviewed-by: Johannes Thumshirn Reviewed-by: Christoph Hellwig Cc: Sreekanth Reddy Signed-off-by: Martin K. Petersen --- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 4 ++-- drivers/scsi/scsi_lib.c | 46 +++++++++++++++++++++++++----------- include/scsi/scsi_device.h | 4 ++-- 3 files changed, 36 insertions(+), 18 deletions(-) (limited to 'include/scsi/scsi_device.h') diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c index c63bc5ccce37..22998cbd538f 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c @@ -2883,7 +2883,7 @@ _scsih_internal_device_unblock(struct scsi_device *sdev, sdev_printk(KERN_WARNING, sdev, "device_unblock and setting to running, " "handle(0x%04x)\n", sas_device_priv_data->sas_target->handle); sas_device_priv_data->block = 0; - r = scsi_internal_device_unblock(sdev, SDEV_RUNNING); + r = scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING); if (r == -EINVAL) { /* The device has been set to SDEV_RUNNING by SD layer during * device addition but the request queue is still stopped by @@ -2902,7 +2902,7 @@ _scsih_internal_device_unblock(struct scsi_device *sdev, r, sas_device_priv_data->sas_target->handle); sas_device_priv_data->block = 0; - r = scsi_internal_device_unblock(sdev, SDEV_RUNNING); + r = scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING); if (r) sdev_printk(KERN_WARNING, sdev, "retried device_unblock" " failed with return(%d) for handle(0x%04x)\n", diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index fc8f394eb854..f0eb55744513 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -3023,24 +3023,22 @@ static int scsi_internal_device_block(struct scsi_device *sdev) } /** - * scsi_internal_device_unblock - resume a device after a block request + * scsi_internal_device_unblock_nowait - resume a device after a block request * @sdev: device to resume - * @new_state: state to set devices to after unblocking + * @new_state: state to set the device to after unblocking * - * Called by scsi lld's or the midlayer to restart the device queue - * for the previously suspended scsi device. Called from interrupt or - * normal process context. + * Restart the device queue for a previously suspended SCSI device. Does not + * sleep. * - * Returns zero if successful or error if not. + * Returns zero if successful or a negative error code upon failure. * - * Notes: - * This routine transitions the device to the SDEV_RUNNING state - * or to one of the offline states (which must be a legal transition) - * allowing the midlayer to goose the queue for this device. + * Notes: + * This routine transitions the device to the SDEV_RUNNING state or to one of + * the offline states (which must be a legal transition) allowing the midlayer + * to goose the queue for this device. */ -int -scsi_internal_device_unblock(struct scsi_device *sdev, - enum scsi_device_state new_state) +int scsi_internal_device_unblock_nowait(struct scsi_device *sdev, + enum scsi_device_state new_state) { struct request_queue *q = sdev->request_queue; unsigned long flags; @@ -3072,7 +3070,27 @@ scsi_internal_device_unblock(struct scsi_device *sdev, return 0; } -EXPORT_SYMBOL_GPL(scsi_internal_device_unblock); +EXPORT_SYMBOL_GPL(scsi_internal_device_unblock_nowait); + +/** + * scsi_internal_device_unblock - resume a device after a block request + * @sdev: device to resume + * @new_state: state to set the device to after unblocking + * + * Restart the device queue for a previously suspended SCSI device. May sleep. + * + * Returns zero if successful or a negative error code upon failure. + * + * Notes: + * This routine transitions the device to the SDEV_RUNNING state or to one of + * the offline states (which must be a legal transition) allowing the midlayer + * to goose the queue for this device. + */ +static int scsi_internal_device_unblock(struct scsi_device *sdev, + enum scsi_device_state new_state) +{ + return scsi_internal_device_unblock_nowait(sdev, new_state); +} static void device_block(struct scsi_device *sdev, void *data) diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 6ce6888f3c69..5f24dae2a8e1 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -473,8 +473,8 @@ static inline int scsi_device_created(struct scsi_device *sdev) } int scsi_internal_device_block_nowait(struct scsi_device *sdev); -int scsi_internal_device_unblock(struct scsi_device *sdev, - enum scsi_device_state new_state); +int scsi_internal_device_unblock_nowait(struct scsi_device *sdev, + enum scsi_device_state new_state); /* accessor functions for the SCSI parameters */ static inline int scsi_device_sync(struct scsi_device *sdev) -- cgit v1.2.3 From 0db6ca8a5e1ea585795db3643ec7d50fc8cb1aff Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 2 Jun 2017 14:21:55 -0700 Subject: scsi: Protect SCSI device state changes with a mutex Serializing SCSI device state changes avoids that two state changes can occur concurrently, e.g. the state changes in scsi_target_block() and __scsi_remove_device(). This serialization is essential to make patch "Make __scsi_remove_device go straight from BLOCKED to DEL" work reliably. Enable this mechanism for all scsi_target_*block() callers but not for the scsi_internal_device_unblock() calls from the mpt3sas driver because that driver can call scsi_internal_device_unblock() from atomic context. Signed-off-by: Bart Van Assche Cc: Christoph Hellwig Cc: Hannes Reinecke Cc: Johannes Thumshirn Signed-off-by: Martin K. Petersen --- drivers/scsi/scsi_error.c | 8 +++++++- drivers/scsi/scsi_lib.c | 27 +++++++++++++++++++++------ drivers/scsi/scsi_scan.c | 16 +++++++++------- drivers/scsi/scsi_sysfs.c | 24 +++++++++++++++++++----- drivers/scsi/scsi_transport_srp.c | 7 ++++--- drivers/scsi/sd.c | 7 +++++-- include/scsi/scsi_device.h | 1 + 7 files changed, 66 insertions(+), 24 deletions(-) (limited to 'include/scsi/scsi_device.h') diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index ecc07dab893d..ac3196420435 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1628,11 +1628,17 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q, struct list_head *done_q) { struct scsi_cmnd *scmd, *next; + struct scsi_device *sdev; list_for_each_entry_safe(scmd, next, work_q, eh_entry) { sdev_printk(KERN_INFO, scmd->device, "Device offlined - " "not ready after error recovery\n"); - scsi_device_set_state(scmd->device, SDEV_OFFLINE); + sdev = scmd->device; + + mutex_lock(&sdev->state_mutex); + scsi_device_set_state(sdev, SDEV_OFFLINE); + mutex_unlock(&sdev->state_mutex); + scsi_eh_finish_cmd(scmd, done_q); } return; diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index f0eb55744513..4b0bac3ac6ab 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2882,7 +2882,12 @@ static void scsi_wait_for_queuecommand(struct scsi_device *sdev) int scsi_device_quiesce(struct scsi_device *sdev) { - int err = scsi_device_set_state(sdev, SDEV_QUIESCE); + int err; + + mutex_lock(&sdev->state_mutex); + err = scsi_device_set_state(sdev, SDEV_QUIESCE); + mutex_unlock(&sdev->state_mutex); + if (err) return err; @@ -2910,10 +2915,11 @@ void scsi_device_resume(struct scsi_device *sdev) * so assume the state is being managed elsewhere (for example * device deleted during suspend) */ - if (sdev->sdev_state != SDEV_QUIESCE || - scsi_device_set_state(sdev, SDEV_RUNNING)) - return; - scsi_run_queue(sdev->request_queue); + mutex_lock(&sdev->state_mutex); + if (sdev->sdev_state == SDEV_QUIESCE && + scsi_device_set_state(sdev, SDEV_RUNNING) == 0) + scsi_run_queue(sdev->request_queue); + mutex_unlock(&sdev->state_mutex); } EXPORT_SYMBOL(scsi_device_resume); @@ -3012,6 +3018,7 @@ static int scsi_internal_device_block(struct scsi_device *sdev) struct request_queue *q = sdev->request_queue; int err; + mutex_lock(&sdev->state_mutex); err = scsi_internal_device_block_nowait(sdev); if (err == 0) { if (q->mq_ops) @@ -3019,6 +3026,8 @@ static int scsi_internal_device_block(struct scsi_device *sdev) else scsi_wait_for_queuecommand(sdev); } + mutex_unlock(&sdev->state_mutex); + return err; } @@ -3089,7 +3098,13 @@ EXPORT_SYMBOL_GPL(scsi_internal_device_unblock_nowait); static int scsi_internal_device_unblock(struct scsi_device *sdev, enum scsi_device_state new_state) { - return scsi_internal_device_unblock_nowait(sdev, new_state); + int ret; + + mutex_lock(&sdev->state_mutex); + ret = scsi_internal_device_unblock_nowait(sdev, new_state); + mutex_unlock(&sdev->state_mutex); + + return ret; } static void diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 6f7128f49c30..e6de4eee97a3 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -231,6 +231,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, sdev->id = starget->id; sdev->lun = lun; sdev->channel = starget->channel; + mutex_init(&sdev->state_mutex); sdev->sdev_state = SDEV_CREATED; INIT_LIST_HEAD(&sdev->siblings); INIT_LIST_HEAD(&sdev->same_target_siblings); @@ -943,16 +944,17 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, /* set the device running here so that slave configure * may do I/O */ + mutex_lock(&sdev->state_mutex); ret = scsi_device_set_state(sdev, SDEV_RUNNING); - if (ret) { + if (ret) ret = scsi_device_set_state(sdev, SDEV_BLOCK); + mutex_unlock(&sdev->state_mutex); - if (ret) { - sdev_printk(KERN_ERR, sdev, - "in wrong state %s to complete scan\n", - scsi_device_state_name(sdev->sdev_state)); - return SCSI_SCAN_NO_RESPONSE; - } + if (ret) { + sdev_printk(KERN_ERR, sdev, + "in wrong state %s to complete scan\n", + scsi_device_state_name(sdev->sdev_state)); + return SCSI_SCAN_NO_RESPONSE; } if (*bflags & BLIST_MS_192_BYTES_FOR_3F) diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 82dfe07b1d47..a91537a3abbf 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -719,7 +719,7 @@ static ssize_t store_state_field(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { - int i; + int i, ret; struct scsi_device *sdev = to_scsi_device(dev); enum scsi_device_state state = 0; @@ -734,9 +734,11 @@ store_state_field(struct device *dev, struct device_attribute *attr, if (!state) return -EINVAL; - if (scsi_device_set_state(sdev, state)) - return -EINVAL; - return count; + mutex_lock(&sdev->state_mutex); + ret = scsi_device_set_state(sdev, state); + mutex_unlock(&sdev->state_mutex); + + return ret == 0 ? count : -EINVAL; } static ssize_t @@ -1272,6 +1274,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev) void __scsi_remove_device(struct scsi_device *sdev) { struct device *dev = &sdev->sdev_gendev; + int res; /* * This cleanup path is not reentrant and while it is impossible @@ -1282,7 +1285,15 @@ void __scsi_remove_device(struct scsi_device *sdev) return; if (sdev->is_visible) { - if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0) + /* + * If scsi_internal_target_block() is running concurrently, + * wait until it has finished before changing the device state. + */ + mutex_lock(&sdev->state_mutex); + res = scsi_device_set_state(sdev, SDEV_CANCEL); + mutex_unlock(&sdev->state_mutex); + + if (res != 0) return; bsg_unregister_queue(sdev->request_queue); @@ -1298,7 +1309,10 @@ void __scsi_remove_device(struct scsi_device *sdev) * scsi_run_queue() invocations have finished before tearing down the * device. */ + mutex_lock(&sdev->state_mutex); scsi_device_set_state(sdev, SDEV_DEL); + mutex_unlock(&sdev->state_mutex); + blk_cleanup_queue(sdev->request_queue); cancel_work_sync(&sdev->requeue_work); diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c index 3c5d89852e9f..f617021c94f7 100644 --- a/drivers/scsi/scsi_transport_srp.c +++ b/drivers/scsi/scsi_transport_srp.c @@ -554,11 +554,12 @@ int srp_reconnect_rport(struct srp_rport *rport) * invoking scsi_target_unblock() won't change the state of * these devices into running so do that explicitly. */ - spin_lock_irq(shost->host_lock); - __shost_for_each_device(sdev, shost) + shost_for_each_device(sdev, shost) { + mutex_lock(&sdev->state_mutex); if (sdev->sdev_state == SDEV_OFFLINE) sdev->sdev_state = SDEV_RUNNING; - spin_unlock_irq(shost->host_lock); + mutex_unlock(&sdev->state_mutex); + } } else if (rport->state == SRP_RPORT_RUNNING) { /* * srp_reconnect_rport() has been invoked with fast_io_fail diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index b6bb4e0ce0e3..53b6d72c214d 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1818,8 +1818,9 @@ static void sd_eh_reset(struct scsi_cmnd *scmd) static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp) { struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk); + struct scsi_device *sdev = scmd->device; - if (!scsi_device_online(scmd->device) || + if (!scsi_device_online(sdev) || !scsi_medium_access_command(scmd) || host_byte(scmd->result) != DID_TIME_OUT || eh_disp != SUCCESS) @@ -1845,7 +1846,9 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp) if (sdkp->medium_access_timed_out >= sdkp->max_medium_access_timeouts) { scmd_printk(KERN_ERR, scmd, "Medium access timeout failure. Offlining disk!\n"); - scsi_device_set_state(scmd->device, SDEV_OFFLINE); + mutex_lock(&sdev->state_mutex); + scsi_device_set_state(sdev, SDEV_OFFLINE); + mutex_unlock(&sdev->state_mutex); return SUCCESS; } diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 5f24dae2a8e1..d13bc80825b1 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -207,6 +207,7 @@ struct scsi_device { void *handler_data; unsigned char access_state; + struct mutex state_mutex; enum scsi_device_state sdev_state; unsigned long sdev_data[0]; } __attribute__((aligned(sizeof(unsigned long)))); -- cgit v1.2.3 From 6934be4f016311cf88c316dad5c75e4ccf8a7fc0 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Mon, 19 Jun 2017 14:27:24 +0200 Subject: scsi: scsi_dh_alua: remove synchronous STPG support Since 9c58b395 ("scsi: scsi_devinfo: remove synchronous ALUA for NETAPP devices") this code is unused. Signed-off-by: Christoph Hellwig Reviewed-by: Bart Van Assche Signed-off-by: Martin K. Petersen --- drivers/scsi/device_handler/scsi_dh_alua.c | 27 +++++---------------------- include/scsi/scsi_device.h | 1 - 2 files changed, 5 insertions(+), 23 deletions(-) (limited to 'include/scsi/scsi_device.h') diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c index c01b47e5b55a..0962fd544401 100644 --- a/drivers/scsi/device_handler/scsi_dh_alua.c +++ b/drivers/scsi/device_handler/scsi_dh_alua.c @@ -57,7 +57,6 @@ /* device handler flags */ #define ALUA_OPTIMIZE_STPG 0x01 #define ALUA_RTPG_EXT_HDR_UNSUPP 0x02 -#define ALUA_SYNC_STPG 0x04 /* State machine flags */ #define ALUA_PG_RUN_RTPG 0x10 #define ALUA_PG_RUN_STPG 0x20 @@ -70,7 +69,6 @@ MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than static LIST_HEAD(port_group_list); static DEFINE_SPINLOCK(port_group_lock); static struct workqueue_struct *kaluad_wq; -static struct workqueue_struct *kaluad_sync_wq; struct alua_port_group { struct kref kref; @@ -380,8 +378,6 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h, } spin_lock_irqsave(&pg->lock, flags); - if (sdev->synchronous_alua) - pg->flags |= ALUA_SYNC_STPG; if (pg_updated) list_add_rcu(&h->node, &pg->dh_list); spin_unlock_irqrestore(&pg->lock, flags); @@ -785,7 +781,6 @@ static void alua_rtpg_work(struct work_struct *work) int err = SCSI_DH_OK; struct alua_queue_data *qdata, *tmp; unsigned long flags; - struct workqueue_struct *alua_wq = kaluad_wq; spin_lock_irqsave(&pg->lock, flags); sdev = pg->rtpg_sdev; @@ -796,8 +791,6 @@ static void alua_rtpg_work(struct work_struct *work) kref_put(&pg->kref, release_port_group); return; } - if (pg->flags & ALUA_SYNC_STPG) - alua_wq = kaluad_sync_wq; pg->flags |= ALUA_PG_RUNNING; if (pg->flags & ALUA_PG_RUN_RTPG) { int state = pg->state; @@ -810,7 +803,7 @@ static void alua_rtpg_work(struct work_struct *work) pg->flags &= ~ALUA_PG_RUNNING; pg->flags |= ALUA_PG_RUN_RTPG; spin_unlock_irqrestore(&pg->lock, flags); - queue_delayed_work(alua_wq, &pg->rtpg_work, + queue_delayed_work(kaluad_wq, &pg->rtpg_work, pg->interval * HZ); return; } @@ -822,7 +815,7 @@ static void alua_rtpg_work(struct work_struct *work) pg->flags &= ~ALUA_PG_RUNNING; pg->flags |= ALUA_PG_RUN_RTPG; spin_unlock_irqrestore(&pg->lock, flags); - queue_delayed_work(alua_wq, &pg->rtpg_work, + queue_delayed_work(kaluad_wq, &pg->rtpg_work, pg->interval * HZ); return; } @@ -839,7 +832,7 @@ static void alua_rtpg_work(struct work_struct *work) pg->interval = 0; pg->flags &= ~ALUA_PG_RUNNING; spin_unlock_irqrestore(&pg->lock, flags); - queue_delayed_work(alua_wq, &pg->rtpg_work, + queue_delayed_work(kaluad_wq, &pg->rtpg_work, pg->interval * HZ); return; } @@ -874,8 +867,6 @@ static bool alua_rtpg_queue(struct alua_port_group *pg, { int start_queue = 0; unsigned long flags; - struct workqueue_struct *alua_wq = kaluad_wq; - if (WARN_ON_ONCE(!pg) || scsi_device_get(sdev)) return false; @@ -900,12 +891,10 @@ static bool alua_rtpg_queue(struct alua_port_group *pg, } } - if (pg->flags & ALUA_SYNC_STPG) - alua_wq = kaluad_sync_wq; spin_unlock_irqrestore(&pg->lock, flags); if (start_queue) { - if (queue_delayed_work(alua_wq, &pg->rtpg_work, + if (queue_delayed_work(kaluad_wq, &pg->rtpg_work, msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS))) sdev = NULL; else @@ -1166,16 +1155,11 @@ static int __init alua_init(void) /* Temporary failure, bypass */ return SCSI_DH_DEV_TEMP_BUSY; } - kaluad_sync_wq = create_workqueue("kaluad_sync"); - if (!kaluad_sync_wq) { - destroy_workqueue(kaluad_wq); - return SCSI_DH_DEV_TEMP_BUSY; - } + r = scsi_register_device_handler(&alua_dh); if (r != 0) { printk(KERN_ERR "%s: Failed to register scsi device handler", ALUA_DH_NAME); - destroy_workqueue(kaluad_sync_wq); destroy_workqueue(kaluad_wq); } return r; @@ -1184,7 +1168,6 @@ static int __init alua_init(void) static void __exit alua_exit(void) { scsi_unregister_device_handler(&alua_dh); - destroy_workqueue(kaluad_sync_wq); destroy_workqueue(kaluad_wq); } diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index d13bc80825b1..d3fb98f72a03 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -181,7 +181,6 @@ struct scsi_device { unsigned no_dif:1; /* T10 PI (DIF) should be disabled */ unsigned broken_fua:1; /* Don't set FUA bit */ unsigned lun_in_cdb:1; /* Store LUN bits in CDB[1] */ - unsigned synchronous_alua:1; /* Synchronous ALUA commands */ atomic_t disk_events_disable_depth; /* disable depth for disk events */ -- cgit v1.2.3 From f9279c968c257ee39b0d7bd2571a4d231a67bcc1 Mon Sep 17 00:00:00 2001 From: "Ewan D. Milne" Date: Tue, 27 Jun 2017 14:55:58 -0400 Subject: scsi: Add STARGET_CREATED_REMOVE state to scsi_target_state The addition of the STARGET_REMOVE state had the side effect of introducing a race condition that can cause a crash. scsi_target_reap_ref_release() checks the starget->state to see if it still in STARGET_CREATED, and if so, skips calling transport_remove_device() and device_del(), because the starget->state is only set to STARGET_RUNNING after scsi_target_add() has called device_add() and transport_add_device(). However, if an rport loss occurs while a target is being scanned, it can happen that scsi_remove_target() will be called while the starget is still in the STARGET_CREATED state. In this case, the starget->state will be set to STARGET_REMOVE, and as a result, scsi_target_reap_ref_release() will take the wrong path. The end result is a panic: [ 1255.356653] Oops: 0000 [#1] SMP [ 1255.360154] Modules linked in: x86_pkg_temp_thermal kvm_intel kvm irqbypass crc32c_intel ghash_clmulni_i [ 1255.393234] CPU: 5 PID: 149 Comm: kworker/u96:4 Tainted: G W 4.11.0+ #8 [ 1255.401879] Hardware name: Dell Inc. PowerEdge R320/08VT7V, BIOS 2.0.22 11/19/2013 [ 1255.410327] Workqueue: scsi_wq_6 fc_scsi_scan_rport [scsi_transport_fc] [ 1255.417720] task: ffff88060ca8c8c0 task.stack: ffffc900048a8000 [ 1255.424331] RIP: 0010:kernfs_find_ns+0x13/0xc0 [ 1255.429287] RSP: 0018:ffffc900048abbf0 EFLAGS: 00010246 [ 1255.435123] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 1255.443083] RDX: 0000000000000000 RSI: ffffffff8188d659 RDI: 0000000000000000 [ 1255.451043] RBP: ffffc900048abc10 R08: 0000000000000000 R09: 0000012433fe0025 [ 1255.459005] R10: 0000000025e5a4b5 R11: 0000000025e5a4b5 R12: ffffffff8188d659 [ 1255.466972] R13: 0000000000000000 R14: ffff8805f55e5088 R15: 0000000000000000 [ 1255.474931] FS: 0000000000000000(0000) GS:ffff880616b40000(0000) knlGS:0000000000000000 [ 1255.483959] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1255.490370] CR2: 0000000000000068 CR3: 0000000001c09000 CR4: 00000000000406e0 [ 1255.498332] Call Trace: [ 1255.501058] kernfs_find_and_get_ns+0x31/0x60 [ 1255.505916] sysfs_unmerge_group+0x1d/0x60 [ 1255.510498] dpm_sysfs_remove+0x22/0x60 [ 1255.514783] device_del+0xf4/0x2e0 [ 1255.518577] ? device_remove_file+0x19/0x20 [ 1255.523241] attribute_container_class_device_del+0x1a/0x20 [ 1255.529457] transport_remove_classdev+0x4e/0x60 [ 1255.534607] ? transport_add_class_device+0x40/0x40 [ 1255.540046] attribute_container_device_trigger+0xb0/0xc0 [ 1255.546069] transport_remove_device+0x15/0x20 [ 1255.551025] scsi_target_reap_ref_release+0x25/0x40 [ 1255.556467] scsi_target_reap+0x2e/0x40 [ 1255.560744] __scsi_scan_target+0xaa/0x5b0 [ 1255.565312] scsi_scan_target+0xec/0x100 [ 1255.569689] fc_scsi_scan_rport+0xb1/0xc0 [scsi_transport_fc] [ 1255.576099] process_one_work+0x14b/0x390 [ 1255.580569] worker_thread+0x4b/0x390 [ 1255.584651] kthread+0x109/0x140 [ 1255.588251] ? rescuer_thread+0x330/0x330 [ 1255.592730] ? kthread_park+0x60/0x60 [ 1255.596815] ret_from_fork+0x29/0x40 [ 1255.600801] Code: 24 08 48 83 42 40 01 5b 41 5c 5d c3 66 66 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 [ 1255.621876] RIP: kernfs_find_ns+0x13/0xc0 RSP: ffffc900048abbf0 [ 1255.628479] CR2: 0000000000000068 [ 1255.632756] ---[ end trace 34a69ba0477d036f ]--- Fix this by adding another scsi_target state STARGET_CREATED_REMOVE to distinguish this case. Fixes: f05795d3d771 ("scsi: Add intermediate STARGET_REMOVE state to scsi_target_state") Reported-by: David Jeffery Signed-off-by: Ewan D. Milne Cc: Reviewed-by: Laurence Oberman Tested-by: Laurence Oberman Reviewed-by: Johannes Thumshirn Signed-off-by: Martin K. Petersen --- drivers/scsi/scsi_scan.c | 5 +++-- drivers/scsi/scsi_sysfs.c | 8 ++++++-- include/scsi/scsi_device.h | 1 + 3 files changed, 10 insertions(+), 4 deletions(-) (limited to 'include/scsi/scsi_device.h') diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 3c4403210a1a..3832ba57151b 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -385,11 +385,12 @@ static void scsi_target_reap_ref_release(struct kref *kref) = container_of(kref, struct scsi_target, reap_ref); /* - * if we get here and the target is still in the CREATED state that + * if we get here and the target is still in a CREATED state that * means it was allocated but never made visible (because a scan * turned up no LUNs), so don't call device_del() on it. */ - if (starget->state != STARGET_CREATED) { + if ((starget->state != STARGET_CREATED) && + (starget->state != STARGET_CREATED_REMOVE)) { transport_remove_device(&starget->dev); device_del(&starget->dev); } diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index ce470f62a8ae..d6984df71f1c 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1394,11 +1394,15 @@ restart: spin_lock_irqsave(shost->host_lock, flags); list_for_each_entry(starget, &shost->__targets, siblings) { if (starget->state == STARGET_DEL || - starget->state == STARGET_REMOVE) + starget->state == STARGET_REMOVE || + starget->state == STARGET_CREATED_REMOVE) continue; if (starget->dev.parent == dev || &starget->dev == dev) { kref_get(&starget->reap_ref); - starget->state = STARGET_REMOVE; + if (starget->state == STARGET_CREATED) + starget->state = STARGET_CREATED_REMOVE; + else + starget->state = STARGET_REMOVE; spin_unlock_irqrestore(shost->host_lock, flags); __scsi_remove_target(starget); scsi_target_reap(starget); diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index d3fb98f72a03..b41ee9d8a042 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -248,6 +248,7 @@ enum scsi_target_state { STARGET_CREATED = 1, STARGET_RUNNING, STARGET_REMOVE, + STARGET_CREATED_REMOVE, STARGET_DEL, }; -- cgit v1.2.3