[PATCH v2] scsi_sysfs: fix hang when removing scsi device

[PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Israel Rukshin]

The bug reproduce when unloading srp module with one port down.
sd_shutdown() hangs when __scsi_remove_device() get scsi_device with
state SDEV_OFFLINE or SDEV_TRANSPORT_OFFLINE.
It hangs because sd_shutdown() is trying to send sync cache command
when the device is offline but with SDEV_CANCEL status.
The status was changed to SDEV_CANCEL by __scsi_remove_device()
before it calls to device_del().

The block layer timeout mechanism doesn't cause the SYNCHRONIZE CACHE
command to fail after the timeout expired because the request timer
wasn't started.
blk_peek_request() that is called from scsi_request_fn() didn't return
this request and therefore the request timer didn't start.

This commit doesn't accept new commands if the original state was offline.

The bug was revealed after commit cff549 ("scsi: proper state checking
and module refcount handling in scsi_device_get").
After this commit scsi_device_get() returns error if the device state
is SDEV_CANCEL.
This eventually leads SRP fast I/O failure timeout handler not to clean
the sync cache command because scsi_target_unblock() skip the canceled device.
If this timeout handler is set to infinity then the hang remains forever
also before commit cff549.

sysrq: SysRq : sysrq: Show Blocked State
task PC stack pid father
kworker/2:0 D ffff88046fa95c00 0 21178 2 0x00000000
Workqueue: srp_remove srp_remove_work [ib_srp]
Call Trace:
[<ffffffff815dd985>] schedule+0x35/0x80
[<ffffffff815e02c7>] schedule_timeout+0x237/0x2d0
[<ffffffff815dcf46>] io_schedule_timeout+0xa6/0x110
[<ffffffff815de2f3>] wait_for_completion_io+0xa3/0x110
[<ffffffff812e66ff>] blk_execute_rq+0xdf/0x120
[<ffffffffa00135de>] scsi_execute+0xce/0x150 [scsi_mod]
[<ffffffffa001548f>] scsi_execute_req_flags+0x8f/0xf0 [scsi_mod]
[<ffffffffa0154849>] sd_sync_cache+0xa9/0x190 [sd_mod]
[<ffffffffa0154c3a>] sd_shutdown+0x6a/0x100 [sd_mod]
[<ffffffffa0154d34>] sd_remove+0x64/0xc0 [sd_mod]
[<ffffffff8144d3fd>] __device_release_driver+0x8d/0x120
[<ffffffff8144d4ae>] device_release_driver+0x1e/0x30
[<ffffffff8144c239>] bus_remove_device+0xf9/0x170
[<ffffffff81448bc7>] device_del+0x127/0x240
[<ffffffffa001c0f1>] __scsi_remove_device+0xc1/0xd0 [scsi_mod]
[<ffffffffa001a5d7>] scsi_forget_host+0x57/0x60 [scsi_mod]
[<ffffffffa000e3d2>] scsi_remove_host+0x72/0x110 [scsi_mod]
[<ffffffffa06f95ab>] srp_remove_work+0x8b/0x200 [ib_srp]
...

Signed-off-by: Israel Rukshin <israelr@mellanox.com>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
---

Changes from v1:
 - add extra description to commit message and to the comment.
 - refer to the commit that originally introduced this hang.

---
 drivers/scsi/scsi_sysfs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 82dfe07..8a977f5 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1282,6 +1282,8 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		return;
 
 	if (sdev->is_visible) {
+		enum scsi_device_state oldstate = sdev->sdev_state;
+
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
 			return;
 
@@ -1289,6 +1291,17 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		device_unregister(&sdev->sdev_dev);
 		transport_remove_device(dev);
 		scsi_dh_remove_device(sdev);
+
+		/*
+		 * Fail new requests if the old state was offline.
+		 * This avoids from sd_shutdown() to hang.
+		 * The SYNCHRONIZE CACHE request timer will never start
+		 * in that case.
+		 */
+		if (oldstate == SDEV_TRANSPORT_OFFLINE ||
+		    oldstate == SDEV_OFFLINE)
+			blk_set_queue_dying(sdev->request_queue);
+
 		device_del(dev);
 	} else
 		put_device(&sdev->sdev_dev);
-- 
2.4.3

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

On Thu, 2017-03-09 at 18:37 +0200, Israel Rukshin wrote:
> The bug reproduce when unloading srp module with one port down.
> sd_shutdown() hangs when __scsi_remove_device() get scsi_device with
> state SDEV_OFFLINE or SDEV_TRANSPORT_OFFLINE.
> It hangs because sd_shutdown() is trying to send sync cache command
> when the device is offline but with SDEV_CANCEL status.
> The status was changed to SDEV_CANCEL by __scsi_remove_device()
> before it calls to device_del().
> 
> The block layer timeout mechanism doesn't cause the SYNCHRONIZE CACHE
> command to fail after the timeout expired because the request timer
> wasn't started.
> blk_peek_request() that is called from scsi_request_fn() didn't return
> this request and therefore the request timer didn't start.
> 
> This commit doesn't accept new commands if the original state was offline.
> 
> The bug was revealed after commit cff549 ("scsi: proper state checking
> and module refcount handling in scsi_device_get").
> After this commit scsi_device_get() returns error if the device state
> is SDEV_CANCEL.
> This eventually leads SRP fast I/O failure timeout handler not to clean
> the sync cache command because scsi_target_unblock() skip the canceled device.
> If this timeout handler is set to infinity then the hang remains forever
> also before commit cff549.

How could blk_peek_request() not return a request that has not yet been
started? How could a patch that changes scsi_device_get() affect I/O since
scsi_device_get() is not called from the I/O path? Anyway, could you try to
reproduce the hang with the patch below applied and see whether the output
produced by this patch helps to determine what is going on?

Thanks,

Bart.

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba2286652ff6..855548ff4c4d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3018,8 +3018,10 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 		else
 			sdev->sdev_state = SDEV_CREATED;
 	} else if (sdev->sdev_state != SDEV_CANCEL &&
-		 sdev->sdev_state != SDEV_OFFLINE)
+		 sdev->sdev_state != SDEV_OFFLINE) {
+		WARN_ONCE(true, "sdev state = %d\n", sdev->sdev_state);
 		return -EINVAL;
+	}
 
 	if (q->mq_ops) {
 		blk_mq_start_stopped_hw_queues(q, false);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 82dfe07b1d47..35aa6b37e199 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1289,6 +1289,13 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		device_unregister(&sdev->sdev_dev);
 		transport_remove_device(dev);
 		scsi_dh_remove_device(sdev);
+
+		WARN_ON_ONCE(blk_queue_stopped(sdev->request_queue));
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: device_busy = %d device_blocked = %d\n",
+			    __func__, atomic_read(&sdev->device_busy),
+			    atomic_read(&sdev->device_blocked));
+
 		device_del(dev);
 	} else
 		put_device(&sdev->sdev_dev);
-- 
2.12.0

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Israel Rukshin]

Hi Bart,

scsi_device_get() affect I/O because scsi_target_unblock() use it and calls to blk_start_queue().
terminate_rport_io() is called after scsi_target_unblock() and completes all the commands
including the SYNCHRONIZE CACHE command.

I applied your patch and you can see that QUEUE_FLAG_STOPPED is on.

[  342.485087] sd 7:0:0:0: Device offlined - not ready after error recovery
[  342.505738] scsi host10: ib_srp: Path record query failed
[  342.512023] sd 10:0:0:0: Device offlined - not ready after error recovery
[  342.589265] sd 7:0:0:0: __scsi_remove_device: device_busy = 0 device_blocked = 0
[  342.624110] sd 7:0:0:0: [sdc] Synchronizing SCSI cache
[  342.630263] sd 7:0:0:0: [sdc] Synchronize Cache(10) failed: Result: hostbyte=DID_TRANSPORT_FAILFAST driverbyte=DRIVER_OK
[  342.649504] scsi 7:0:0:0: alua: Detached
[  342.769099] ------------[ cut here ]------------
[  342.769107] WARNING: CPU: 10 PID: 317 at drivers/scsi/scsi_sysfs.c:1293 __scsi_remove_device+0x131/0x140
[  342.769108] Modules linked in: nfsv3 ib_srp(-) dm_service_time scsi_transport_srp ib_uverbs ib_umad ib_ipoib ib_cm mlx4_ib ib_core rpcsec_gss_krb5 nfsv4 dns_resolver nfs netconsole fscache dm_mirror dm_region_hash dm_log sb_edac edac_core x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_simd joydev input_leds glue_helper ipmi_si iTCO_wdt pcspkr cryptd mei_me iTCO_vendor_support ipmi_devintf sg lpc_ich ipmi_msghandler mei i2c_i801 shpchp mfd_core ioatdma nfsd auth_rpcgss dm_multipath nfs_acl dm_mod lockd grace sunrpc ip_tables ext4 jbd2 mbcache sd_mod mgag200 drm_kms_helper syscopyarea isci sysfillrect sysimgblt ahci igb libsas fb_sys_fops libahci ttm scsi_transport_sas ptp pps_core crc32c_int
 el dca drm
[  342.769152]  i2c_algo_bit libata mlx4_core fjes [last unloaded: ib_srp]
[  342.769157] CPU: 10 PID: 317 Comm: kworker/10:1 Not tainted 4.11.0-rc1+ #97
[  342.769157] Hardware name: Supermicro X9DRFR/X9DRFR, BIOS 1.0a 09/11/2012
[  342.769163] Workqueue: srp_remove srp_remove_work [ib_srp]
[  342.769165] Call Trace:
[  342.769173]  dump_stack+0x63/0x90
[  342.769176]  __warn+0xcb/0xf0
[  342.769178]  warn_slowpath_null+0x1d/0x20
[  342.769180]  __scsi_remove_device+0x131/0x140
[  342.769182]  scsi_forget_host+0x60/0x70
[  342.769186]  scsi_remove_host+0x77/0x110
[  342.769189]  srp_remove_work+0x90/0x230 [ib_srp]
[  342.769192]  process_one_work+0x177/0x430
[  342.769193]  worker_thread+0x4e/0x4b0
[  342.769195]  kthread+0x101/0x140
[  342.769197]  ? process_one_work+0x430/0x430
[  342.769198]  ? kthread_create_on_node+0x60/0x60
[  342.769201]  ret_from_fork+0x2c/0x40
[  342.769202] ---[ end trace 1eef46ba7887fee3 ]---
[  342.769210] sd 10:0:0:0: __scsi_remove_device: device_busy = 0 device_blocked = 0
[  343.020039] sd 10:0:0:0: [sde] Synchronizing SCSI cache
[  352.717659] scsi host10: ib_srp: Got failed path rec status -110

Israel.


On 3/9/2017 9:36 PM, Bart Van Assche wrote:
> On Thu, 2017-03-09 at 18:37 +0200, Israel Rukshin wrote:
>> The bug reproduce when unloading srp module with one port down.
>> sd_shutdown() hangs when __scsi_remove_device() get scsi_device with
>> state SDEV_OFFLINE or SDEV_TRANSPORT_OFFLINE.
>> It hangs because sd_shutdown() is trying to send sync cache command
>> when the device is offline but with SDEV_CANCEL status.
>> The status was changed to SDEV_CANCEL by __scsi_remove_device()
>> before it calls to device_del().
>>
>> The block layer timeout mechanism doesn't cause the SYNCHRONIZE CACHE
>> command to fail after the timeout expired because the request timer
>> wasn't started.
>> blk_peek_request() that is called from scsi_request_fn() didn't return
>> this request and therefore the request timer didn't start.
>>
>> This commit doesn't accept new commands if the original state was offline.
>>
>> The bug was revealed after commit cff549 ("scsi: proper state checking
>> and module refcount handling in scsi_device_get").
>> After this commit scsi_device_get() returns error if the device state
>> is SDEV_CANCEL.
>> This eventually leads SRP fast I/O failure timeout handler not to clean
>> the sync cache command because scsi_target_unblock() skip the canceled device.
>> If this timeout handler is set to infinity then the hang remains forever
>> also before commit cff549.
> How could blk_peek_request() not return a request that has not yet been
> started? How could a patch that changes scsi_device_get() affect I/O since
> scsi_device_get() is not called from the I/O path? Anyway, could you try to
> reproduce the hang with the patch below applied and see whether the output
> produced by this patch helps to determine what is going on?
>
> Thanks,
>
> Bart.
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ba2286652ff6..855548ff4c4d 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3018,8 +3018,10 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
>   		else
>   			sdev->sdev_state = SDEV_CREATED;
>   	} else if (sdev->sdev_state != SDEV_CANCEL &&
> -		 sdev->sdev_state != SDEV_OFFLINE)
> +		 sdev->sdev_state != SDEV_OFFLINE) {
> +		WARN_ONCE(true, "sdev state = %d\n", sdev->sdev_state);
>   		return -EINVAL;
> +	}
>   
>   	if (q->mq_ops) {
>   		blk_mq_start_stopped_hw_queues(q, false);
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 82dfe07b1d47..35aa6b37e199 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1289,6 +1289,13 @@ void __scsi_remove_device(struct scsi_device *sdev)
>   		device_unregister(&sdev->sdev_dev);
>   		transport_remove_device(dev);
>   		scsi_dh_remove_device(sdev);
> +
> +		WARN_ON_ONCE(blk_queue_stopped(sdev->request_queue));
> +		sdev_printk(KERN_INFO, sdev,
> +			    "%s: device_busy = %d device_blocked = %d\n",
> +			    __func__, atomic_read(&sdev->device_busy),
> +			    atomic_read(&sdev->device_blocked));
> +
>   		device_del(dev);
>   	} else
>   		put_device(&sdev->sdev_dev);
> -- 
> 2.12.0

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

On Sun, 2017-03-12 at 12:26 +0200, Israel Rukshin wrote:
> scsi_device_get() affects I/O because scsi_target_unblock() use it and calls to blk_start_queue().
> terminate_rport_io() is called after scsi_target_unblock() and completes all the commands
> including the SYNCHRONIZE CACHE command.
> 
> I applied your patch and you can see that QUEUE_FLAG_STOPPED is on.

Hello Israel,

Thanks, that's very interesting feedback. Sorry but I prefer to fix
scsi_target_unblock() instead of changing __scsi_remove_device(). Would it
be possible to verify whether the five attached patches fix the issue you
had reported?

Thanks,

Bart.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-that-scsi_device_put-triggers-a-use-after-free.patch --]
[-- Type: text/x-patch; name="0001-Avoid-that-scsi_device_put-triggers-a-use-after-free.patch", Size: 2528 bytes --]

From 3c5882c9cd88265ff81f6321edc3caf2a2981f55 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Fri, 3 Mar 2017 11:13:39 -0800
Subject: [PATCH 1/5] Avoid that scsi_device_put() triggers a use-after-free

Avoid that the following crash occurs, a crash that is easy to
trigger with kernel v4.11-rc1 but that I only saw sporadically
in the past:

general protection fault: 0000 [#1] SMP
RIP: 0010:scsi_device_put+0xb/0x30
Call Trace:
 scsi_disk_put+0x2d/0x40
 sd_release+0x3d/0xb0
 __blkdev_put+0x29e/0x360
 blkdev_put+0x49/0x170
 dm_put_table_device+0x58/0xc0 [dm_mod]
 dm_put_device+0x70/0xc0 [dm_mod]
 free_priority_group+0x92/0xc0 [dm_multipath]
 free_multipath+0x70/0xc0 [dm_multipath]
 multipath_dtr+0x19/0x20 [dm_multipath]
 dm_table_destroy+0x67/0x120 [dm_mod]
 dev_suspend+0xde/0x240 [dm_mod]
 ctl_ioctl+0x1f5/0x520 [dm_mod]
 dm_ctl_ioctl+0xe/0x20 [dm_mod]
 do_vfs_ioctl+0x8f/0x700
 SyS_ioctl+0x3c/0x70
 entry_SYSCALL_64_fastpath+0x18/0xad

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/scsi.c        | 2 +-
 drivers/scsi/scsi_scan.c   | 1 +
 include/scsi/scsi_device.h | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 7bfbcfa7af40..b3bb49d06943 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -602,7 +602,7 @@ EXPORT_SYMBOL(scsi_device_get);
  */
 void scsi_device_put(struct scsi_device *sdev)
 {
-	module_put(sdev->host->hostt->module);
+	module_put(sdev->hostt->module);
 	put_device(&sdev->sdev_gendev);
 }
 EXPORT_SYMBOL(scsi_device_put);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6f7128f49c30..7134487abbb1 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -227,6 +227,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	sdev->model = scsi_null_device_strs;
 	sdev->rev = scsi_null_device_strs;
 	sdev->host = shost;
+	sdev->hostt = shost->hostt;
 	sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
 	sdev->id = starget->id;
 	sdev->lun = lun;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 6f22b39f1b0c..cda620ed5922 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -82,6 +82,7 @@ struct scsi_event {
 
 struct scsi_device {
 	struct Scsi_Host *host;
+	struct scsi_host_template *hostt;
 	struct request_queue *request_queue;
 
 	/* the next two are protected by the host->host_lock */
-- 
2.12.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Change-sdev-host-hostt-into-sdev-hostt.patch --]
[-- Type: text/x-patch; name="0002-Change-sdev-host-hostt-into-sdev-hostt.patch", Size: 10690 bytes --]

From ec349df5c6b0336c746c109895d717b2be8151cb Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Fri, 3 Mar 2017 11:05:13 -0800
Subject: [PATCH 2/5] Change sdev->host->hostt into sdev->hostt

Now that the host template pointer is cached in the SCSI device
structure, use that pointer instead of going around through the
SCSI host. This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/osst.c       |  7 +++----
 drivers/scsi/raid_class.c |  2 +-
 drivers/scsi/scsi.c       |  2 +-
 drivers/scsi/scsi_error.c | 10 +++++-----
 drivers/scsi/scsi_ioctl.c |  4 ++--
 drivers/scsi/scsi_lib.c   |  2 +-
 drivers/scsi/scsi_scan.c  |  4 ++--
 drivers/scsi/scsi_sysfs.c | 16 ++++++++--------
 drivers/scsi/sd.c         |  8 ++++----
 drivers/scsi/sg.c         | 14 +++++---------
 drivers/scsi/st.c         |  5 ++---
 11 files changed, 34 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index c47f4b349bac..410a5865eae3 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -5285,11 +5285,10 @@ static long osst_compat_ioctl(struct file * file, unsigned int cmd_in, unsigned
 	struct osst_tape *STp = file->private_data;
 	struct scsi_device *sdev = STp->device;
 	int ret = -ENOIOCTLCMD;
-	if (sdev->host->hostt->compat_ioctl) {
 
-		ret = sdev->host->hostt->compat_ioctl(sdev, cmd_in, (void __user *)arg);
-
-	}
+	if (sdev->hostt->compat_ioctl)
+		ret = sdev->hostt->compat_ioctl(sdev, cmd_in,
+						(void __user *)arg);
 	return ret;
 }
 #endif
diff --git a/drivers/scsi/raid_class.c b/drivers/scsi/raid_class.c
index 2c146b44d95f..ccde6e1d8dd5 100644
--- a/drivers/scsi/raid_class.c
+++ b/drivers/scsi/raid_class.c
@@ -67,7 +67,7 @@ static int raid_match(struct attribute_container *cont, struct device *dev)
 	if (scsi_is_sdev_device(dev)) {
 		struct scsi_device *sdev = to_scsi_device(dev);
 
-		if (i->f->cookie != sdev->host->hostt)
+		if (i->f->cookie != sdev->hostt)
 			return 0;
 
 		return i->f->is_raid(dev);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b3bb49d06943..59c05384cef1 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -581,7 +581,7 @@ int scsi_device_get(struct scsi_device *sdev)
 		goto fail;
 	if (!get_device(&sdev->sdev_gendev))
 		goto fail;
-	if (!try_module_get(sdev->host->hostt->module))
+	if (!try_module_get(sdev->hostt->module))
 		goto fail_put_device;
 	return 0;
 
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index f2cafae150bc..1160aa0c7b58 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -131,7 +131,7 @@ scmd_eh_abort_handler(struct work_struct *work)
 		SCSI_LOG_ERROR_RECOVERY(3,
 			scmd_printk(KERN_INFO, scmd,
 				    "aborting command\n"));
-		rtn = scsi_try_to_abort_cmd(sdev->host->hostt, scmd);
+		rtn = scsi_try_to_abort_cmd(sdev->hostt, scmd);
 		if (rtn == SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
 			if (scsi_host_eh_past_deadline(sdev->host)) {
@@ -604,7 +604,7 @@ EXPORT_SYMBOL_GPL(scsi_check_sense);
 
 static void scsi_handle_queue_ramp_up(struct scsi_device *sdev)
 {
-	struct scsi_host_template *sht = sdev->host->hostt;
+	struct scsi_host_template *sht = sdev->hostt;
 	struct scsi_device *tmp_sdev;
 
 	if (!sht->track_queue_depth ||
@@ -636,7 +636,7 @@ static void scsi_handle_queue_ramp_up(struct scsi_device *sdev)
 
 static void scsi_handle_queue_full(struct scsi_device *sdev)
 {
-	struct scsi_host_template *sht = sdev->host->hostt;
+	struct scsi_host_template *sht = sdev->hostt;
 	struct scsi_device *tmp_sdev;
 
 	if (!sht->track_queue_depth)
@@ -851,7 +851,7 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
 static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
 {
 	int rtn;
-	struct scsi_host_template *hostt = scmd->device->host->hostt;
+	struct scsi_host_template *hostt = scmd->device->hostt;
 
 	if (!hostt->eh_device_reset_handler)
 		return FAILED;
@@ -890,7 +890,7 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
 
 static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
 {
-	if (scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
+	if (scsi_try_to_abort_cmd(scmd->device->hostt, scmd) != SUCCESS)
 		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
 			if (scsi_try_target_reset(scmd) != SUCCESS)
 				if (scsi_try_bus_reset(scmd) != SUCCESS)
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index b6bf3f29a12a..4ecb5d48b65b 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -264,8 +264,8 @@ int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
 	case SG_SCSI_RESET:
 		return scsi_ioctl_reset(sdev, arg);
 	default:
-		if (sdev->host->hostt->ioctl)
-			return sdev->host->hostt->ioctl(sdev, cmd, arg);
+		if (sdev->hostt->ioctl)
+			return sdev->hostt->ioctl(sdev, cmd, arg);
 	}
 	return -EINVAL;
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba2286652ff6..e8176708cc28 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1141,7 +1141,7 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 
 	/* zero out the cmd, except for the embedded scsi_request */
 	memset((char *)cmd + sizeof(cmd->req), 0,
-		sizeof(*cmd) - sizeof(cmd->req) + dev->host->hostt->cmd_size);
+	       sizeof(*cmd) - sizeof(cmd->req) + dev->hostt->cmd_size);
 
 	cmd->device = dev;
 	cmd->sense_buffer = buf;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 7134487abbb1..aa20bfe8bdc2 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -980,8 +980,8 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 
 	transport_configure_device(&sdev->sdev_gendev);
 
-	if (sdev->host->hostt->slave_configure) {
-		ret = sdev->host->hostt->slave_configure(sdev);
+	if (sdev->hostt->slave_configure) {
+		ret = sdev->hostt->slave_configure(sdev);
 		if (ret) {
 			/*
 			 * if LLDD reports slave not present, don't clutter
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 82dfe07b1d47..4a580657c6f6 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -912,7 +912,7 @@ sdev_store_queue_depth(struct device *dev, struct device_attribute *attr,
 {
 	int depth, retval;
 	struct scsi_device *sdev = to_scsi_device(dev);
-	struct scsi_host_template *sht = sdev->host->hostt;
+	struct scsi_host_template *sht = sdev->hostt;
 
 	if (!sht->change_queue_depth)
 		return -EINVAL;
@@ -1080,11 +1080,11 @@ static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
 
 
 	if (attr == &dev_attr_queue_depth.attr &&
-	    !sdev->host->hostt->change_queue_depth)
+	    !sdev->hostt->change_queue_depth)
 		return S_IRUGO;
 
 	if (attr == &dev_attr_queue_ramp_up_period.attr &&
-	    !sdev->host->hostt->change_queue_depth)
+	    !sdev->hostt->change_queue_depth)
 		return 0;
 
 #ifdef CONFIG_SCSI_DH
@@ -1256,10 +1256,10 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 			    "Failed to register bsg queue, errno=%d\n", error);
 
 	/* add additional host specific attributes */
-	if (sdev->host->hostt->sdev_attrs) {
-		for (i = 0; sdev->host->hostt->sdev_attrs[i]; i++) {
+	if (sdev->hostt->sdev_attrs) {
+		for (i = 0; sdev->hostt->sdev_attrs[i]; i++) {
 			error = device_create_file(&sdev->sdev_gendev,
-					sdev->host->hostt->sdev_attrs[i]);
+					sdev->hostt->sdev_attrs[i]);
 			if (error)
 				return error;
 		}
@@ -1302,8 +1302,8 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);
 
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
+	if (sdev->hostt->slave_destroy)
+		sdev->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);
 
 	/*
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d277e8620e3e..376cf43febf1 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1571,9 +1571,9 @@ static int sd_compat_ioctl(struct block_device *bdev, fmode_t mode,
 	/* 
 	 * Let the static ioctl translation table take care of it.
 	 */
-	if (!sdev->host->hostt->compat_ioctl)
+	if (!sdev->hostt->compat_ioctl)
 		return -ENOIOCTLCMD; 
-	return sdev->host->hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
+	return sdev->hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
 }
 #endif
 
@@ -2968,8 +2968,8 @@ static void sd_unlock_native_capacity(struct gendisk *disk)
 {
 	struct scsi_device *sdev = scsi_disk(disk)->device;
 
-	if (sdev->host->hostt->unlock_native_capacity)
-		sdev->host->hostt->unlock_native_capacity(sdev);
+	if (sdev->hostt->unlock_native_capacity)
+		sdev->hostt->unlock_native_capacity(sdev);
 }
 
 /**
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 29b86505f796..84b3cdcc4934 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1053,7 +1053,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 	case SG_EMULATED_HOST:
 		if (atomic_read(&sdp->detaching))
 			return -ENODEV;
-		return put_user(sdp->device->host->hostt->emulated, ip);
+		return put_user(sdp->device->hostt->emulated, ip);
 	case SCSI_IOCTL_SEND_COMMAND:
 		if (atomic_read(&sdp->detaching))
 			return -ENODEV;
@@ -1120,14 +1120,10 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 		return -ENXIO;
 
 	sdev = sdp->device;
-	if (sdev->host->hostt->compat_ioctl) { 
-		int ret;
+	if (sdev->hostt->compat_ioctl)
+		return sdev->hostt->compat_ioctl(sdev, cmd_in,
+						 (void __user *)arg);
 
-		ret = sdev->host->hostt->compat_ioctl(sdev, cmd_in, (void __user *)arg);
-
-		return ret;
-	}
-	
 	return -ENOIOCTLCMD;
 }
 #endif
@@ -2689,7 +2685,7 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 				   scsidp->host->host_no,
 				   scsidp->channel, scsidp->id,
 				   scsidp->lun,
-				   scsidp->host->hostt->emulated);
+				   scsidp->hostt->emulated);
 		}
 		seq_printf(s, " sg_tablesize=%d excl=%d open_cnt=%d\n",
 			   sdp->sg_tablesize, sdp->exclude, sdp->open_cnt);
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index e5ef78a6848e..949473ae8392 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -3862,11 +3862,10 @@ static long st_compat_ioctl(struct file *file, unsigned int cmd, unsigned long a
 	struct scsi_tape *STp = file->private_data;
 	struct scsi_device *sdev = STp->device;
 	int ret = -ENOIOCTLCMD;
-	if (sdev->host->hostt->compat_ioctl) { 
 
-		ret = sdev->host->hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
+	if (sdev->hostt->compat_ioctl)
+		ret = sdev->hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
 
-	}
 	return ret;
 }
 #endif
-- 
2.12.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Warn-if-__scsi_remove_device-is-called-for-a-stopped.patch --]
[-- Type: text/x-patch; name="0003-Warn-if-__scsi_remove_device-is-called-for-a-stopped.patch", Size: 2247 bytes --]

From ca5ab7097947667e7d032728cfdb609b315ee76d Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Mon, 13 Mar 2017 10:06:13 -0700
Subject: [PATCH 3/5] Warn if __scsi_remove_device() is called for a stopped
 queue

Calling __scsi_remove_device() for a stopped queue is a bug because
the device_del() call can trigger I/O and will trigger e.g. the
following hang:

Call Trace:
[<ffffffff815dd985>] schedule+0x35/0x80
[<ffffffff815e02c7>] schedule_timeout+0x237/0x2d0
[<ffffffff815dcf46>] io_schedule_timeout+0xa6/0x110
[<ffffffff815de2f3>] wait_for_completion_io+0xa3/0x110
[<ffffffff812e66ff>] blk_execute_rq+0xdf/0x120
[<ffffffffa00135de>] scsi_execute+0xce/0x150 [scsi_mod]
[<ffffffffa001548f>] scsi_execute_req_flags+0x8f/0xf0 [scsi_mod]
[<ffffffffa0154849>] sd_sync_cache+0xa9/0x190 [sd_mod]
[<ffffffffa0154c3a>] sd_shutdown+0x6a/0x100 [sd_mod]
[<ffffffffa0154d34>] sd_remove+0x64/0xc0 [sd_mod]
[<ffffffff8144d3fd>] __device_release_driver+0x8d/0x120
[<ffffffff8144d4ae>] device_release_driver+0x1e/0x30
[<ffffffff8144c239>] bus_remove_device+0xf9/0x170
[<ffffffff81448bc7>] device_del+0x127/0x240
[<ffffffffa001c0f1>] __scsi_remove_device+0xc1/0xd0 [scsi_mod]
[<ffffffffa001a5d7>] scsi_forget_host+0x57/0x60 [scsi_mod]
[<ffffffffa000e3d2>] scsi_remove_host+0x72/0x110 [scsi_mod]
[<ffffffffa06f95ab>] srp_remove_work+0x8b/0x200 [ib_srp]

Reported-by: Israel Rukshin <israelr@mellanox.com>
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/scsi_sysfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 4a580657c6f6..30b0ffcac575 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1274,6 +1274,12 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	struct device *dev = &sdev->sdev_gendev;
 
 	/*
+	 * Calling __scsi_remove_device() for a stopped queue is a bug because
+	 * the device_del() call can trigger I/O. See also sd_remove().
+	 */
+	WARN_ON_ONCE(blk_queue_stopped(sdev->request_queue));
+
+	/*
 	 * This cleanup path is not reentrant and while it is impossible
 	 * to get a new reference with scsi_device_get() someone can still
 	 * hold a previously acquired one.
-- 
2.12.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Ensure-that-scsi_internal_device_unblock-does-not-sl.patch --]
[-- Type: text/x-patch; name="0004-Ensure-that-scsi_internal_device_unblock-does-not-sl.patch", Size: 1125 bytes --]

From 7a83374d9f721a290fc1ed73ac5500e6a37e081a Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Mon, 13 Mar 2017 10:46:42 -0700
Subject: [PATCH 4/5] Ensure that scsi_internal_device_unblock() does not sleep

The header above scsi_internal_device_unblock() mentions that this
function may be called from interrupt context. Hence ensure that
this function does not sleep.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/scsi_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e8176708cc28..90931d3f2255 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3022,10 +3022,10 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 		return -EINVAL;
 
 	if (q->mq_ops) {
-		blk_mq_start_stopped_hw_queues(q, false);
+		blk_mq_start_stopped_hw_queues(q, true);
 	} else {
 		spin_lock_irqsave(q->queue_lock, flags);
-		blk_start_queue(q);
+		blk_start_queue_async(q);
 		spin_unlock_irqrestore(q->queue_lock, flags);
 	}
 
-- 
2.12.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Ensure-that-scsi_target_unblock-examines-all-devices.patch --]
[-- Type: text/x-patch; name="0005-Ensure-that-scsi_target_unblock-examines-all-devices.patch", Size: 2962 bytes --]

From 1c6137450d6b815145d6b52481053eeb035c3f4a Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Mon, 13 Mar 2017 10:46:08 -0700
Subject: [PATCH 5/5] Ensure that scsi_target_unblock() examines all devices

Since scsi_target_unblock() uses starget_for_each_device(), since
starget_for_each_device() uses scsi_device_get(), since
scsi_device_get() fails after unloading of the LLD kernel module
has been started scsi_target_unblock() may skip devices that were
affected by scsi_target_block(). Ensure that scsi_target_block()
examines all SCSI devices. This patch avoids that unloading the
ib_srp kernel module can trigger the following hang:

Call Trace:
 schedule+0x35/0x80
 schedule_timeout+0x237/0x2d0
 io_schedule_timeout+0xa6/0x110
 wait_for_completion_io+0xa3/0x110
 blk_execute_rq+0xdf/0x120
 scsi_execute+0xce/0x150 [scsi_mod]
 scsi_execute_req_flags+0x8f/0xf0 [scsi_mod]
 sd_sync_cache+0xa9/0x190 [sd_mod]
 sd_shutdown+0x6a/0x100 [sd_mod]
 sd_remove+0x64/0xc0 [sd_mod]
 __device_release_driver+0x8d/0x120
 device_release_driver+0x1e/0x30
 bus_remove_device+0xf9/0x170
 device_del+0x127/0x240
 __scsi_remove_device+0xc1/0xd0 [scsi_mod]
 scsi_forget_host+0x57/0x60 [scsi_mod]
 scsi_remove_host+0x72/0x110 [scsi_mod]
 srp_remove_work+0x8b/0x200 [ib_srp]

Reported-by: Israel Rukshin <israelr@mellanox.com>
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/scsi_lib.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 90931d3f2255..5b57634b25fd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3074,14 +3074,30 @@ target_unblock(struct device *dev, void *data)
 	return 0;
 }
 
+/**
+ * scsi_target_unblock() - unblock all devices associated with a SCSI target
+ * @dev: Either a pointer to the dev member of struct scsi_target or a pointer
+ *	to the shost_gendev member of struct Scsi_Host.
+ * @new_state: New SCSI device state.
+ *
+ * Note: do not use scsi_get_device() nor any of the macros that uses this
+ * function from inside scsi_target_block() because otherwise this function
+ * won't have any effect when called while the SCSI LLD is being unloaded.
+ */
 void
 scsi_target_unblock(struct device *dev, enum scsi_device_state new_state)
 {
-	if (scsi_is_target_device(dev))
-		starget_for_each_device(to_scsi_target(dev), &new_state,
-					device_unblock);
-	else
+	if (scsi_is_target_device(dev)) {
+		struct scsi_target *starget = to_scsi_target(dev);
+		struct Scsi_Host *shost = dev_to_shost(dev->parent);
+		unsigned long flags;
+
+		spin_lock_irqsave(shost->host_lock, flags);
+		__starget_for_each_device(starget, &new_state, device_unblock);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+	} else {
 		device_for_each_child(dev, &new_state, target_unblock);
+	}
 }
 EXPORT_SYMBOL_GPL(scsi_target_unblock);
 
-- 
2.12.0

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[James Bottomley]

On Mon, 2017-03-13 at 18:49 +0000, Bart Van Assche wrote:
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 7bfbcfa7af40..b3bb49d06943 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -602,7 +602,7 @@ EXPORT_SYMBOL(scsi_device_get);
>   */
>  void scsi_device_put(struct scsi_device *sdev)
>  {
> -       module_put(sdev->host->hostt->module);
> +       module_put(sdev->hostt->module);
>         put_device(&sdev->sdev_gendev);
>  }
>  EXPORT_SYMBOL(scsi_device_put);
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6f7128f49c30..7134487abbb1 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -227,6 +227,7 @@ static struct scsi_device *scsi_alloc_sdev(struct
> scsi_target *starget,
>         sdev->model = scsi_null_device_strs;
>         sdev->rev = scsi_null_device_strs;
>         sdev->host = shost;
> +       sdev->hostt = shost->hostt;
>         sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
>         sdev->id = starget->id;
>         sdev->lun = lun;
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 6f22b39f1b0c..cda620ed5922 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -82,6 +82,7 @@ struct scsi_event {
>  
>  struct scsi_device {
>         struct Scsi_Host *host;
> +       struct scsi_host_template *hostt;
>         struct request_queue *request_queue;
>  

The apparent assumption behind this patch is that sdev->host can be
freed but the sdev will still exist?  That shouldn't be correct: the
rule for struct devices is that the child always holds the parent and
the host is parented (albeit not necessarily directly) to the sdev, so
it looks like something has gone wrong if the host had been freed
before the sdev.

James

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

On Mon, 2017-03-13 at 12:23 -0700, James Bottomley wrote:
> On Mon, 2017-03-13 at 18:49 +0000, Bart Van Assche wrote:
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index 7bfbcfa7af40..b3bb49d06943 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -602,7 +602,7 @@ EXPORT_SYMBOL(scsi_device_get);
> >   */
> >  void scsi_device_put(struct scsi_device *sdev)
> >  {
> > -       module_put(sdev->host->hostt->module);
> > +       module_put(sdev->hostt->module);
> >         put_device(&sdev->sdev_gendev);
> >  }
> >  EXPORT_SYMBOL(scsi_device_put);
> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > index 6f7128f49c30..7134487abbb1 100644
> > --- a/drivers/scsi/scsi_scan.c
> > +++ b/drivers/scsi/scsi_scan.c
> > @@ -227,6 +227,7 @@ static struct scsi_device *scsi_alloc_sdev(struct
> > scsi_target *starget,
> >         sdev->model = scsi_null_device_strs;
> >         sdev->rev = scsi_null_device_strs;
> >         sdev->host = shost;
> > +       sdev->hostt = shost->hostt;
> >         sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
> >         sdev->id = starget->id;
> >         sdev->lun = lun;
> > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > index 6f22b39f1b0c..cda620ed5922 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -82,6 +82,7 @@ struct scsi_event {
> >  
> >  struct scsi_device {
> >         struct Scsi_Host *host;
> > +       struct scsi_host_template *hostt;
> >         struct request_queue *request_queue;
> >  
> 
> The apparent assumption behind this patch is that sdev->host can be
> freed but the sdev will still exist?  That shouldn't be correct: the
> rule for struct devices is that the child always holds the parent and
> the host is parented (albeit not necessarily directly) to the sdev, so
> it looks like something has gone wrong if the host had been freed
> before the sdev.

Hello James,

scsi_remove_host() decreases the sdev reference count but does not wait
until the sdev release work has finished. This is why the SCSI host can
already have disappeared before the last scsi_device_put() call occurs.

Bart.

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[James Bottomley]

On Mon, 2017-03-13 at 20:33 +0000, Bart Van Assche wrote:
> On Mon, 2017-03-13 at 12:23 -0700, James Bottomley wrote:
> > On Mon, 2017-03-13 at 18:49 +0000, Bart Van Assche wrote:
> > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > > index 7bfbcfa7af40..b3bb49d06943 100644
> > > --- a/drivers/scsi/scsi.c
> > > +++ b/drivers/scsi/scsi.c
> > > @@ -602,7 +602,7 @@ EXPORT_SYMBOL(scsi_device_get);
> > >   */
> > >  void scsi_device_put(struct scsi_device *sdev)
> > >  {
> > > -       module_put(sdev->host->hostt->module);
> > > +       module_put(sdev->hostt->module);
> > >         put_device(&sdev->sdev_gendev);
> > >  }
> > >  EXPORT_SYMBOL(scsi_device_put);
> > > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > > index 6f7128f49c30..7134487abbb1 100644
> > > --- a/drivers/scsi/scsi_scan.c
> > > +++ b/drivers/scsi/scsi_scan.c
> > > @@ -227,6 +227,7 @@ static struct scsi_device
> > > *scsi_alloc_sdev(struct
> > > scsi_target *starget,
> > >         sdev->model = scsi_null_device_strs;
> > >         sdev->rev = scsi_null_device_strs;
> > >         sdev->host = shost;
> > > +       sdev->hostt = shost->hostt;
> > >         sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
> > >         sdev->id = starget->id;
> > >         sdev->lun = lun;
> > > diff --git a/include/scsi/scsi_device.h
> > > b/include/scsi/scsi_device.h
> > > index 6f22b39f1b0c..cda620ed5922 100644
> > > --- a/include/scsi/scsi_device.h
> > > +++ b/include/scsi/scsi_device.h
> > > @@ -82,6 +82,7 @@ struct scsi_event {
> > >  
> > >  struct scsi_device {
> > >         struct Scsi_Host *host;
> > > +       struct scsi_host_template *hostt;
> > >         struct request_queue *request_queue;
> > >  
> > 
> > The apparent assumption behind this patch is that sdev->host can be
> > freed but the sdev will still exist?  That shouldn't be correct:
> > the
> > rule for struct devices is that the child always holds the parent
> > and
> > the host is parented (albeit not necessarily directly) to the sdev,
> > so
> > it looks like something has gone wrong if the host had been freed
> > before the sdev.
> 
> Hello James,
> 
> scsi_remove_host() decreases the sdev reference count but does not 
> wait until the sdev release work has finished. This is why the SCSI
> host can already have disappeared before the last scsi_device_put()
> call occurs.

This is true, but I don't see how it can cause the host to be freed
before the sdev.  The memory for struct Scsi_Host is freed in the
shost_gendev release routine, which should be pinned by the parent
traversal from sdev.  So it should not be possible for
 scsi_host_dev_release() to be called before
 scsi_device_dev_release_usercontext() becase the latter has the final
put of the parent device.

James

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

On Mon, 2017-03-13 at 14:55 -0700, James Bottomley wrote:
> This is true, but I don't see how it can cause the host to be freed
> before the sdev.  The memory for struct Scsi_Host is freed in the
> shost_gendev release routine, which should be pinned by the parent
> traversal from sdev.  So it should not be possible for
>  scsi_host_dev_release() to be called before
>  scsi_device_dev_release_usercontext() becase the latter has the final
> put of the parent device.

Hello James,

I will run a bisect to see whether that provides more information about
what caused the change in the reference counting behavior.

Israel, since you did not hit the reference counting issue in your tests,
can you repeat your test with patches 3, 4 and 5 applied?

Thanks,

Bart.

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Israel Rukshin]

Hello Bart,

I applied and tested patches 3, 4 and 5.
I am sorry to tell you that it didn't work.
I saw the warning you added and the hang at device_del().

[  333.696462] ------------[ cut here ]------------
[  333.696471] WARNING: CPU: 11 PID: 321 at 
drivers/scsi/scsi_sysfs.c:1280 __scsi_remove_device+0x106/0x110
[  333.696472] Modules linked in: nfsv3 ib_srp(-) dm_service_time 
scsi_transport_srp ib_uverbs ib_umad ib_ipoib ib_cm mlx4_ib ib_core 
rpcsec_gss_krb5 nfsv4 dns_resolver nfs netconsole fscache dm_mirror 
dm_region_hash dm_log sb_edac edac_core x86_pkg_temp_thermal 
intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul 
crc32_pclmul ghash_clmulni_intel iTCO_wdt pcbc iTCO_vendor_support 
aesni_intel mei_me crypto_simd joydev input_leds ipmi_si ioatdma mei 
glue_helper lpc_ich pcspkr cryptd ipmi_devintf shpchp ipmi_msghandler sg 
i2c_i801 mfd_core dm_multipath dm_mod nfsd binfmt_misc auth_rpcgss 
nfs_acl lockd grace sunrpc ip_tables ext4 jbd2 mbcache sd_mod mgag200 
drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops isci igb 
ttm ahci libsas ptp libahci scsi_transport_sas pps_core dca crc32c_intel
[  333.696511]  drm i2c_algo_bit libata mlx4_core fjes [last unloaded: 
ib_srp]
[  333.696516] CPU: 11 PID: 321 Comm: kworker/11:1 Not tainted 
4.11.0-rc2+ #99
[  333.696517] Hardware name: Supermicro X9DRFR/X9DRFR, BIOS 1.0a 09/11/2012
[  333.696522] Workqueue: srp_remove srp_remove_work [ib_srp]
[  333.696523] Call Trace:
[  333.696530]  dump_stack+0x63/0x90
[  333.696534]  __warn+0xcb/0xf0
[  333.696536]  warn_slowpath_null+0x1d/0x20
[  333.696538]  __scsi_remove_device+0x106/0x110
[  333.696540]  scsi_forget_host+0x60/0x70
[  333.696545]  scsi_remove_host+0x77/0x110
[  333.696547]  srp_remove_work+0x90/0x230 [ib_srp]
[  333.696551]  process_one_work+0x177/0x430
[  333.696553]  worker_thread+0x4e/0x4b0
[  333.696555]  kthread+0x101/0x140
[  333.696556]  ? process_one_work+0x430/0x430
[  333.696558]  ? kthread_create_on_node+0x60/0x60
[  333.696563]  ret_from_fork+0x2c/0x40
[  333.696565] ---[ end trace b1edd584bf21aaba ]---

Israel.


On 3/14/2017 4:35 AM, Bart Van Assche wrote:
> On Mon, 2017-03-13 at 14:55 -0700, James Bottomley wrote:
>> This is true, but I don't see how it can cause the host to be freed
>> before the sdev.  The memory for struct Scsi_Host is freed in the
>> shost_gendev release routine, which should be pinned by the parent
>> traversal from sdev.  So it should not be possible for
>>   scsi_host_dev_release() to be called before
>>   scsi_device_dev_release_usercontext() becase the latter has the final
>> put of the parent device.
> Hello James,
>
> I will run a bisect to see whether that provides more information about
> what caused the change in the reference counting behavior.
>
> Israel, since you did not hit the reference counting issue in your tests,
> can you repeat your test with patches 3, 4 and 5 applied?
>
> Thanks,
>
> Bart.

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Israel Rukshin]

Hello Bart,

Patch number 5 doesn't handle the case when device_for_each_child() is 
called.
device_for_each_child() calls to target_unblock() that uses also 
starget_for_each_device().
After applying also the following change the hang disappeared but it 
didn't fix the warning.
Those fixes are not enough because if fast_io_fail_tmo is set to 
infinity then the hang will remain,
because only __rport_fail_io_fast() calls to scsi_target_unblock() and 
terminate_rport_io()
that free the sync cache command.

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e5d4b50..09f9566 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3068,9 +3068,15 @@ void scsi_device_resume(struct scsi_device *sdev)
  static int
  target_unblock(struct device *dev, void *data)
  {
-       if (scsi_is_target_device(dev))
-               starget_for_each_device(to_scsi_target(dev), data,
-                                       device_unblock);
+       if (scsi_is_target_device(dev)) {
+               struct scsi_target *starget = to_scsi_target(dev);
+               struct Scsi_Host *shost = dev_to_shost(dev->parent);
+               unsigned long flags;
+
+               spin_lock_irqsave(shost->host_lock, flags);
+               __starget_for_each_device(starget, data, device_unblock);
+               spin_unlock_irqrestore(shost->host_lock, flags);
+       }
         return 0;
  }

--
1.8.4.3


Israel


On 3/14/2017 11:44 AM, Israel Rukshin wrote:
> Hello Bart,
>
> I applied and tested patches 3, 4 and 5.
> I am sorry to tell you that it didn't work.
> I saw the warning you added and the hang at device_del().
>
> [  333.696462] ------------[ cut here ]------------
> [  333.696471] WARNING: CPU: 11 PID: 321 at 
> drivers/scsi/scsi_sysfs.c:1280 __scsi_remove_device+0x106/0x110
> [  333.696472] Modules linked in: nfsv3 ib_srp(-) dm_service_time 
> scsi_transport_srp ib_uverbs ib_umad ib_ipoib ib_cm mlx4_ib ib_core 
> rpcsec_gss_krb5 nfsv4 dns_resolver nfs netconsole fscache dm_mirror 
> dm_region_hash dm_log sb_edac edac_core x86_pkg_temp_thermal 
> intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul 
> crc32_pclmul ghash_clmulni_intel iTCO_wdt pcbc iTCO_vendor_support 
> aesni_intel mei_me crypto_simd joydev input_leds ipmi_si ioatdma mei 
> glue_helper lpc_ich pcspkr cryptd ipmi_devintf shpchp ipmi_msghandler 
> sg i2c_i801 mfd_core dm_multipath dm_mod nfsd binfmt_misc auth_rpcgss 
> nfs_acl lockd grace sunrpc ip_tables ext4 jbd2 mbcache sd_mod mgag200 
> drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops isci igb 
> ttm ahci libsas ptp libahci scsi_transport_sas pps_core dca crc32c_intel
> [  333.696511]  drm i2c_algo_bit libata mlx4_core fjes [last unloaded: 
> ib_srp]
> [  333.696516] CPU: 11 PID: 321 Comm: kworker/11:1 Not tainted 
> 4.11.0-rc2+ #99
> [  333.696517] Hardware name: Supermicro X9DRFR/X9DRFR, BIOS 1.0a 
> 09/11/2012
> [  333.696522] Workqueue: srp_remove srp_remove_work [ib_srp]
> [  333.696523] Call Trace:
> [  333.696530]  dump_stack+0x63/0x90
> [  333.696534]  __warn+0xcb/0xf0
> [  333.696536]  warn_slowpath_null+0x1d/0x20
> [  333.696538]  __scsi_remove_device+0x106/0x110
> [  333.696540]  scsi_forget_host+0x60/0x70
> [  333.696545]  scsi_remove_host+0x77/0x110
> [  333.696547]  srp_remove_work+0x90/0x230 [ib_srp]
> [  333.696551]  process_one_work+0x177/0x430
> [  333.696553]  worker_thread+0x4e/0x4b0
> [  333.696555]  kthread+0x101/0x140
> [  333.696556]  ? process_one_work+0x430/0x430
> [  333.696558]  ? kthread_create_on_node+0x60/0x60
> [  333.696563]  ret_from_fork+0x2c/0x40
> [  333.696565] ---[ end trace b1edd584bf21aaba ]---
>
> Israel.
>
>
> On 3/14/2017 4:35 AM, Bart Van Assche wrote:
>> On Mon, 2017-03-13 at 14:55 -0700, James Bottomley wrote:
>>> This is true, but I don't see how it can cause the host to be freed
>>> before the sdev.  The memory for struct Scsi_Host is freed in the
>>> shost_gendev release routine, which should be pinned by the parent
>>> traversal from sdev.  So it should not be possible for
>>>   scsi_host_dev_release() to be called before
>>>   scsi_device_dev_release_usercontext() becase the latter has the final
>>> put of the parent device.
>> Hello James,
>>
>> I will run a bisect to see whether that provides more information about
>> what caused the change in the reference counting behavior.
>>
>> Israel, since you did not hit the reference counting issue in your 
>> tests,
>> can you repeat your test with patches 3, 4 and 5 applied?
>>
>> Thanks,
>>
>> Bart.
>

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

On Tue, 2017-03-14 at 16:23 +0200, Israel Rukshin wrote:
> Patch number 5 doesn't handle the case when device_for_each_child() is 
> called. device_for_each_child() calls to target_unblock() that uses also 
> starget_for_each_device(). After applying also the following change the
> hang disappeared but it didn't fix the warning. Those fixes are not enough
> because if fast_io_fail_tmo is set to infinity then the hang will remain,
> because only __rport_fail_io_fast() calls to scsi_target_unblock() and 
> terminate_rport_io() that free the sync cache command.
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index e5d4b50..09f9566 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3068,9 +3068,15 @@ void scsi_device_resume(struct scsi_device *sdev)
>   static int
>   target_unblock(struct device *dev, void *data)
>   {
> -       if (scsi_is_target_device(dev))
> -               starget_for_each_device(to_scsi_target(dev), data,
> -                                       device_unblock);
> +       if (scsi_is_target_device(dev)) {
> +               struct scsi_target *starget = to_scsi_target(dev);
> +               struct Scsi_Host *shost = dev_to_shost(dev->parent);
> +               unsigned long flags;
> +
> +               spin_lock_irqsave(shost->host_lock, flags);
> +               __starget_for_each_device(starget, data, device_unblock);
> +               spin_unlock_irqrestore(shost->host_lock, flags);
> +       }
>          return 0;
>   }

Hello Israel,

Regarding setting fast_io_fail_tmo to infinity: that does not prevent
kernel module unloading because srp_timed_out() stops resetting the
timer as soon as the SCSI device is unblocked. The above patch should
realize that but suffers from the same issue as a patch attached to
my previous e-mail, namely lock inversion. For at least the following
call chain the block layer queue lock is the outer lock and the SCSI
host lock is the inner lock:
ata_qc_schedule_eh()
-> blk_abort_request()
  -> blk_mq_rq_timed_out()
    -> scsi_timeout()
      -> scsi_times_out()
        -> scsi_eh_scmd_add()

So I think we should avoid introducing code with the SCSI host lock
as outer lock and the block layer queue lock as inner lock. How about
the attached four patches?

Thanks,

Bart.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Warn-if-__scsi_remove_device-is-called-for-a-stopped.patch --]
[-- Type: text/x-patch; name="0001-Warn-if-__scsi_remove_device-is-called-for-a-stopped.patch", Size: 2247 bytes --]

From 458959938476788710738039a7c195e9c48ff338 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Mon, 13 Mar 2017 10:06:13 -0700
Subject: [PATCH 1/4] Warn if __scsi_remove_device() is called for a stopped
 queue

Calling __scsi_remove_device() for a stopped queue is a bug because
the device_del() call can trigger I/O and will trigger e.g. the
following hang:

Call Trace:
[<ffffffff815dd985>] schedule+0x35/0x80
[<ffffffff815e02c7>] schedule_timeout+0x237/0x2d0
[<ffffffff815dcf46>] io_schedule_timeout+0xa6/0x110
[<ffffffff815de2f3>] wait_for_completion_io+0xa3/0x110
[<ffffffff812e66ff>] blk_execute_rq+0xdf/0x120
[<ffffffffa00135de>] scsi_execute+0xce/0x150 [scsi_mod]
[<ffffffffa001548f>] scsi_execute_req_flags+0x8f/0xf0 [scsi_mod]
[<ffffffffa0154849>] sd_sync_cache+0xa9/0x190 [sd_mod]
[<ffffffffa0154c3a>] sd_shutdown+0x6a/0x100 [sd_mod]
[<ffffffffa0154d34>] sd_remove+0x64/0xc0 [sd_mod]
[<ffffffff8144d3fd>] __device_release_driver+0x8d/0x120
[<ffffffff8144d4ae>] device_release_driver+0x1e/0x30
[<ffffffff8144c239>] bus_remove_device+0xf9/0x170
[<ffffffff81448bc7>] device_del+0x127/0x240
[<ffffffffa001c0f1>] __scsi_remove_device+0xc1/0xd0 [scsi_mod]
[<ffffffffa001a5d7>] scsi_forget_host+0x57/0x60 [scsi_mod]
[<ffffffffa000e3d2>] scsi_remove_host+0x72/0x110 [scsi_mod]
[<ffffffffa06f95ab>] srp_remove_work+0x8b/0x200 [ib_srp]

Reported-by: Israel Rukshin <israelr@mellanox.com>
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/scsi_sysfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 82dfe07b1d47..bbe7efd144b2 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1274,6 +1274,12 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	struct device *dev = &sdev->sdev_gendev;
 
 	/*
+	 * Calling __scsi_remove_device() for a stopped queue is a bug because
+	 * the device_del() call can trigger I/O. See also sd_remove().
+	 */
+	WARN_ON_ONCE(blk_queue_stopped(sdev->request_queue));
+
+	/*
 	 * This cleanup path is not reentrant and while it is impossible
 	 * to get a new reference with scsi_device_get() someone can still
 	 * hold a previously acquired one.
-- 
2.12.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-__scsi_iterate_devices-Make-the-get-and-put-function.patch --]
[-- Type: text/x-patch; name="0002-__scsi_iterate_devices-Make-the-get-and-put-function.patch", Size: 3154 bytes --]

From b86b5087698c73f5fdd8cc9fa18ed3f1e9e174bb Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 15 Mar 2017 15:12:43 -0700
Subject: [PATCH 2/4] __scsi_iterate_devices(): Make the get and put functions
 arguments

This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/scsi.c        |  8 +++++---
 include/scsi/scsi_device.h | 16 +++++++++++-----
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b3bb49d06943..45c266009f20 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -609,7 +609,9 @@ EXPORT_SYMBOL(scsi_device_put);
 
 /* helper for shost_for_each_device, see that for documentation */
 struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
-					   struct scsi_device *prev)
+					   struct scsi_device *prev,
+					   int (*get)(struct scsi_device *),
+					   void (*put)(struct scsi_device *))
 {
 	struct list_head *list = (prev ? &prev->siblings : &shost->__devices);
 	struct scsi_device *next = NULL;
@@ -619,7 +621,7 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
 	while (list->next != &shost->__devices) {
 		next = list_entry(list->next, struct scsi_device, siblings);
 		/* skip devices that we can't get a reference to */
-		if (!scsi_device_get(next))
+		if (!get(next))
 			break;
 		next = NULL;
 		list = list->next;
@@ -627,7 +629,7 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	if (prev)
-		scsi_device_put(prev);
+		put(prev);
 	return next;
 }
 EXPORT_SYMBOL(__scsi_iterate_devices);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index cda620ed5922..812dd1cdfb6f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -335,7 +335,9 @@ extern void __starget_for_each_device(struct scsi_target *, void *,
 
 /* only exposed to implement shost_for_each_device */
 extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
-						  struct scsi_device *);
+					struct scsi_device *,
+					int (*get)(struct scsi_device *),
+					void (*put)(struct scsi_device *));
 
 /**
  * shost_for_each_device - iterate over all devices of a host
@@ -346,10 +348,14 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
  * takes a reference on each device and releases it at the end.  If
  * you break out of the loop, you must call scsi_device_put(sdev).
  */
-#define shost_for_each_device(sdev, shost) \
-	for ((sdev) = __scsi_iterate_devices((shost), NULL); \
-	     (sdev); \
-	     (sdev) = __scsi_iterate_devices((shost), (sdev)))
+#define shost_for_each_device(sdev, shost)			\
+	for ((sdev) = __scsi_iterate_devices((shost), NULL,	\
+					     scsi_device_get,	\
+					     scsi_device_put);	\
+	     (sdev);						\
+	     (sdev) = __scsi_iterate_devices((shost), (sdev),	\
+					     scsi_device_get,	\
+					     scsi_device_put))
 
 /**
  * __shost_for_each_device - iterate over all devices of a host (UNLOCKED)
-- 
2.12.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Introduce-starget_for_all_devices-and-shost_for_all_.patch --]
[-- Type: text/x-patch; name="0003-Introduce-starget_for_all_devices-and-shost_for_all_.patch", Size: 5501 bytes --]

From 91921ac966f52aef8236fdf657cbbcb31aa2381c Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 15 Mar 2017 15:13:20 -0700
Subject: [PATCH 3/4] Introduce starget_for_all_devices() and
 shost_for_all_devices()

This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/scsi.c        | 52 +++++++++++++++++++++++++++++++++++++++++++---
 include/scsi/scsi_device.h | 28 ++++++++++++++++++-------
 2 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 45c266009f20..2aeaebdd50bc 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -640,9 +640,11 @@ EXPORT_SYMBOL(__scsi_iterate_devices);
  * @data:	Opaque passed to each function call.
  * @fn:		Function to call on each device
  *
- * This traverses over each device of @starget.  The devices have
- * a reference that must be released by scsi_host_put when breaking
- * out of the loop.
+ * This traverses over each device of @starget except the devices that are in
+ * state SDEV_DEL or SDEV_CANCEL. The devices have a reference that must be
+ * released by scsi_device_put() when breaking out of the loop. If the LLD
+ * associated with the devices is being unloaded, @fn is not called for any
+ * device.
  */
 void starget_for_each_device(struct scsi_target *starget, void *data,
 		     void (*fn)(struct scsi_device *, void *))
@@ -659,6 +661,50 @@ void starget_for_each_device(struct scsi_target *starget, void *data,
 EXPORT_SYMBOL(starget_for_each_device);
 
 /**
+ * scsi_device_get_any() - get a reference to @sdev even if it is being deleted
+ *
+ * See also scsi_device_get().
+ */
+static int scsi_device_get_any(struct scsi_device *sdev)
+{
+	return get_device(&sdev->sdev_gendev) ? 0 : -ENXIO;
+}
+
+/**
+ * scsi_device_put_any() - drop a reference obtained by scsi_device_get_any()
+ *
+ * See also scsi_device_put().
+ */
+static void scsi_device_put_any(struct scsi_device *sdev)
+{
+	put_device(&sdev->sdev_gendev);
+}
+
+/**
+ * starget_for_all_devices - helper to walk all devices of a target
+ * @starget:	target whose devices we want to iterate over.
+ * @data:	Pointer passed to each function call.
+ * @fn:		Function to call on each device
+ *
+ * This traverses over each device of @starget, including the devices in state
+ * SDEV_DEL or SDEV_ANY. The devices have a reference that must be released by
+ * scsi_device_put_any() when breaking out of the loop.
+ */
+void starget_for_all_devices(struct scsi_target *starget, void *data,
+			     void (*fn)(struct scsi_device *, void *))
+{
+	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+	struct scsi_device *sdev;
+
+	shost_for_all_devices(sdev, shost, scsi_device_get_any,
+			      scsi_device_put_any)
+		if (sdev->channel == starget->channel &&
+		    sdev->id == starget->id)
+			fn(sdev, data);
+}
+EXPORT_SYMBOL(starget_for_all_devices);
+
+/**
  * __starget_for_each_device - helper to walk all devices of a target (UNLOCKED)
  * @starget:	target whose devices we want to iterate over.
  * @data:	parameter for callback @fn()
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 812dd1cdfb6f..057d7376dafc 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -329,6 +329,8 @@ extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
 							  u64);
 extern void starget_for_each_device(struct scsi_target *, void *,
 		     void (*fn)(struct scsi_device *, void *));
+extern void starget_for_all_devices(struct scsi_target *, void *,
+				    void (*fn)(struct scsi_device *, void *));
 extern void __starget_for_each_device(struct scsi_target *, void *,
 				      void (*fn)(struct scsi_device *,
 						 void *));
@@ -340,6 +342,23 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
 					void (*put)(struct scsi_device *));
 
 /**
+ * shost_for_all_devices - iterate over all devices of a host
+ * @sdev: the &struct scsi_device to use as a cursor
+ * @shost: the &struct scsi_host to iterate over
+ * @get: function that obtains a reference to a device and returns 0 upon
+ *       success
+ * @put: function that drops a device reference.
+ *
+ * Iterator that returns each device attached to @shost.  This loop
+ * takes a reference on each device and releases it at the end.  If
+ * you break out of the loop, you must call @put(sdev).
+ */
+#define shost_for_all_devices(sdev, shost, get, put)			\
+	for ((sdev) = NULL;						\
+	     ((sdev) = __scsi_iterate_devices((shost), (sdev),		\
+					      (get), (put))) != NULL; )
+
+/**
  * shost_for_each_device - iterate over all devices of a host
  * @sdev: the &struct scsi_device to use as a cursor
  * @shost: the &struct scsi_host to iterate over
@@ -349,13 +368,8 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
  * you break out of the loop, you must call scsi_device_put(sdev).
  */
 #define shost_for_each_device(sdev, shost)			\
-	for ((sdev) = __scsi_iterate_devices((shost), NULL,	\
-					     scsi_device_get,	\
-					     scsi_device_put);	\
-	     (sdev);						\
-	     (sdev) = __scsi_iterate_devices((shost), (sdev),	\
-					     scsi_device_get,	\
-					     scsi_device_put))
+	shost_for_all_devices((sdev), (shost), scsi_device_get,	\
+					     scsi_device_put)
 
 /**
  * __shost_for_each_device - iterate over all devices of a host (UNLOCKED)
-- 
2.12.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Ensure-that-scsi_target_unblock-examines-all-devices.patch --]
[-- Type: text/x-patch; name="0004-Ensure-that-scsi_target_unblock-examines-all-devices.patch", Size: 3196 bytes --]

From 44959a030c1538a38be3a7f2ef0f7488368b2568 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Mon, 13 Mar 2017 10:46:08 -0700
Subject: [PATCH 4/4] Ensure that scsi_target_unblock() examines all devices

Since scsi_target_unblock() uses starget_for_each_device(), since
starget_for_each_device() uses scsi_device_get(), since
scsi_device_get() fails after unloading of the LLD kernel module
has been started scsi_target_unblock() may skip devices that were
affected by scsi_target_block(). Ensure that scsi_target_block()
examines all SCSI devices. This patch avoids that unloading the
ib_srp kernel module can trigger the following hang:

Call Trace:
 schedule+0x35/0x80
 schedule_timeout+0x237/0x2d0
 io_schedule_timeout+0xa6/0x110
 wait_for_completion_io+0xa3/0x110
 blk_execute_rq+0xdf/0x120
 scsi_execute+0xce/0x150 [scsi_mod]
 scsi_execute_req_flags+0x8f/0xf0 [scsi_mod]
 sd_sync_cache+0xa9/0x190 [sd_mod]
 sd_shutdown+0x6a/0x100 [sd_mod]
 sd_remove+0x64/0xc0 [sd_mod]
 __device_release_driver+0x8d/0x120
 device_release_driver+0x1e/0x30
 bus_remove_device+0xf9/0x170
 device_del+0x127/0x240
 __scsi_remove_device+0xc1/0xd0 [scsi_mod]
 scsi_forget_host+0x57/0x60 [scsi_mod]
 scsi_remove_host+0x72/0x110 [scsi_mod]
 srp_remove_work+0x8b/0x200 [ib_srp]

Reported-by: Israel Rukshin <israelr@mellanox.com>
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/scsi_lib.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba2286652ff6..991763fda0af 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3065,21 +3065,37 @@ device_unblock(struct scsi_device *sdev, void *data)
 	scsi_internal_device_unblock(sdev, *(enum scsi_device_state *)data);
 }
 
+/**
+ * target_unblock() - unblock all devices associated with a SCSI target
+ *
+ * Notes:
+ * - Do not use scsi_device_get() nor any of the macros that use this
+ *   function from inside scsi_target_block() because otherwise this function
+ *   won't have any effect when called while the SCSI LLD is being unloaded.
+ * - Do not hold the host lock around the device_unblock() calls because at
+ *   least for blk-sq the block layer queue lock is the outer lock and the
+ *   SCSI host lock is the inner lock.
+ */
 static int
 target_unblock(struct device *dev, void *data)
 {
 	if (scsi_is_target_device(dev))
-		starget_for_each_device(to_scsi_target(dev), data,
+		starget_for_all_devices(to_scsi_target(dev), data,
 					device_unblock);
 	return 0;
 }
 
+/**
+ * scsi_target_unblock() - unblock all devices associated with a SCSI target
+ * @dev: Either a pointer to the dev member of struct scsi_target or a pointer
+ *	to the shost_gendev member of struct Scsi_Host.
+ * @new_state: New SCSI device state.
+ */
 void
 scsi_target_unblock(struct device *dev, enum scsi_device_state new_state)
 {
 	if (scsi_is_target_device(dev))
-		starget_for_each_device(to_scsi_target(dev), &new_state,
-					device_unblock);
+		target_unblock(dev, &new_state);
 	else
 		device_for_each_child(dev, &new_state, target_unblock);
 }
-- 
2.12.0

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Israel Rukshin]

Hi Bart,

I tested your patches and the hang disappeared when fast_io_fail_tmo 
expired.
The warning from patch 1 still exist, so we need an additional fix for that.

Regards,
Israel

On 3/16/2017 1:27 AM, Bart Van Assche wrote:
> On Tue, 2017-03-14 at 16:23 +0200, Israel Rukshin wrote:
>> Patch number 5 doesn't handle the case when device_for_each_child() is
>> called. device_for_each_child() calls to target_unblock() that uses also
>> starget_for_each_device(). After applying also the following change the
>> hang disappeared but it didn't fix the warning. Those fixes are not enough
>> because if fast_io_fail_tmo is set to infinity then the hang will remain,
>> because only __rport_fail_io_fast() calls to scsi_target_unblock() and
>> terminate_rport_io() that free the sync cache command.
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index e5d4b50..09f9566 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -3068,9 +3068,15 @@ void scsi_device_resume(struct scsi_device *sdev)
>>    static int
>>    target_unblock(struct device *dev, void *data)
>>    {
>> -       if (scsi_is_target_device(dev))
>> -               starget_for_each_device(to_scsi_target(dev), data,
>> -                                       device_unblock);
>> +       if (scsi_is_target_device(dev)) {
>> +               struct scsi_target *starget = to_scsi_target(dev);
>> +               struct Scsi_Host *shost = dev_to_shost(dev->parent);
>> +               unsigned long flags;
>> +
>> +               spin_lock_irqsave(shost->host_lock, flags);
>> +               __starget_for_each_device(starget, data, device_unblock);
>> +               spin_unlock_irqrestore(shost->host_lock, flags);
>> +       }
>>           return 0;
>>    }
> Hello Israel,
>
> Regarding setting fast_io_fail_tmo to infinity: that does not prevent
> kernel module unloading because srp_timed_out() stops resetting the
> timer as soon as the SCSI device is unblocked. The above patch should
> realize that but suffers from the same issue as a patch attached to
> my previous e-mail, namely lock inversion. For at least the following
> call chain the block layer queue lock is the outer lock and the SCSI
> host lock is the inner lock:
> ata_qc_schedule_eh()
> -> blk_abort_request()
>    -> blk_mq_rq_timed_out()
>      -> scsi_timeout()
>        -> scsi_times_out()
>          -> scsi_eh_scmd_add()
>
> So I think we should avoid introducing code with the SCSI host lock
> as outer lock and the block layer queue lock as inner lock. How about
> the attached four patches?
>
> Thanks,
>
> Bart.

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

On Thu, 2017-03-16 at 11:02 +0200, Israel Rukshin wrote:
> I tested your patches and the hang disappeared when fast_io_fail_tmo 
> expired. The warning from patch 1 still exist, so we need an additional
> fix for that.

Hello Israel,

Thanks for the testing! I will leave out patch 1 for now since it is not
needed to fix the hang.

I assume that your e-mail counts as a Tested-by?

PS: please do not top-post when replying. This is considered annoying in the
Linux kernel community.

Bart.

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Israel Rukshin]

On 3/16/2017 5:42 PM, Bart Van Assche wrote:
> On Thu, 2017-03-16 at 11:02 +0200, Israel Rukshin wrote:
>> I tested your patches and the hang disappeared when fast_io_fail_tmo
>> expired. The warning from patch 1 still exist, so we need an additional
>> fix for that.
> Hello Israel,
>
> Thanks for the testing! I will leave out patch 1 for now since it is not
> needed to fix the hang.
>
> I assume that your e-mail counts as a Tested-by?
>
> PS: please do not top-post when replying. This is considered annoying in the
> Linux kernel community.
>
> Bart.

Hello Bart,

Yes, you can add me as Tested-by.

Israel.

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

On Mon, 2017-03-13 at 14:55 -0700, James Bottomley wrote:
> On Mon, 2017-03-13 at 20:33 +0000, Bart Van Assche wrote:
> > On Mon, 2017-03-13 at 12:23 -0700, James Bottomley wrote:
> > > On Mon, 2017-03-13 at 18:49 +0000, Bart Van Assche wrote:
> > > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > > > index 7bfbcfa7af40..b3bb49d06943 100644
> > > > --- a/drivers/scsi/scsi.c
> > > > +++ b/drivers/scsi/scsi.c
> > > > @@ -602,7 +602,7 @@ EXPORT_SYMBOL(scsi_device_get);
> > > >   */
> > > >  void scsi_device_put(struct scsi_device *sdev)
> > > >  {
> > > > -       module_put(sdev->host->hostt->module);
> > > > +       module_put(sdev->hostt->module);
> > > >         put_device(&sdev->sdev_gendev);
> > > >  }
> > > >  EXPORT_SYMBOL(scsi_device_put);
> > > > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > > > index 6f7128f49c30..7134487abbb1 100644
> > > > --- a/drivers/scsi/scsi_scan.c
> > > > +++ b/drivers/scsi/scsi_scan.c
> > > > @@ -227,6 +227,7 @@ static struct scsi_device
> > > > *scsi_alloc_sdev(struct
> > > > scsi_target *starget,
> > > >         sdev->model = scsi_null_device_strs;
> > > >         sdev->rev = scsi_null_device_strs;
> > > >         sdev->host = shost;
> > > > +       sdev->hostt = shost->hostt;
> > > >         sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
> > > >         sdev->id = starget->id;
> > > >         sdev->lun = lun;
> > > > diff --git a/include/scsi/scsi_device.h
> > > > b/include/scsi/scsi_device.h
> > > > index 6f22b39f1b0c..cda620ed5922 100644
> > > > --- a/include/scsi/scsi_device.h
> > > > +++ b/include/scsi/scsi_device.h
> > > > @@ -82,6 +82,7 @@ struct scsi_event {
> > > >  
> > > >  struct scsi_device {
> > > >         struct Scsi_Host *host;
> > > > +       struct scsi_host_template *hostt;
> > > >         struct request_queue *request_queue;
> > > >  
> > > 
> > > The apparent assumption behind this patch is that sdev->host can be
> > > freed but the sdev will still exist?  That shouldn't be correct:
> > > the
> > > rule for struct devices is that the child always holds the parent
> > > and
> > > the host is parented (albeit not necessarily directly) to the sdev,
> > > so
> > > it looks like something has gone wrong if the host had been freed
> > > before the sdev.
> > 
> > Hello James,
> > 
> > scsi_remove_host() decreases the sdev reference count but does not 
> > wait until the sdev release work has finished. This is why the SCSI
> > host can already have disappeared before the last scsi_device_put()
> > call occurs.
> 
> This is true, but I don't see how it can cause the host to be freed
> before the sdev.  The memory for struct Scsi_Host is freed in the
> shost_gendev release routine, which should be pinned by the parent
> traversal from sdev.  So it should not be possible for
>  scsi_host_dev_release() to be called before
>  scsi_device_dev_release_usercontext() becase the latter has the final
> put of the parent device.

Hello Hannes,

The following crash only occurs with async aborts enabled:

general protection fault: 0000 [#1] SMP
RIP: 0010:scsi_device_put+0xb/0x30
Call Trace:
 scsi_disk_put+0x2d/0x40
 sd_release+0x3d/0xb0
 __blkdev_put+0x29e/0x360
 blkdev_put+0x49/0x170
 dm_put_table_device+0x58/0xc0 [dm_mod]
 dm_put_device+0x70/0xc0 [dm_mod]
 free_priority_group+0x92/0xc0 [dm_multipath]
 free_multipath+0x70/0xc0 [dm_multipath]
 multipath_dtr+0x19/0x20 [dm_multipath]
 dm_table_destroy+0x67/0x120 [dm_mod]
 dev_suspend+0xde/0x240 [dm_mod]
 ctl_ioctl+0x1f5/0x520 [dm_mod]
 dm_ctl_ioctl+0xe/0x20 [dm_mod]
 do_vfs_ioctl+0x8f/0x700
 SyS_ioctl+0x3c/0x70
 entry_SYSCALL_64_fastpath+0x18/0xad

I hadn't seen this crash before kernel v4.11-rc1 but with kernel v4.11-rc1
and later I see it if I let the srp-test scripts run for a few minutes. The
patch I used to disable async aborts on my test setup is as follows:

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index f2cafae150bc..9075e126d6c8 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -171,6 +171,7 @@ scmd_eh_abort_handler(struct work_struct *work)
 	}
 }
 
+#if 0
 /**
  * scsi_abort_command - schedule a command abort
  * @scmd:	scmd to abort.
@@ -219,6 +220,12 @@ scsi_abort_command(struct scsi_cmnd *scmd)
 	queue_delayed_work(shost->tmf_work_q, &scmd->abort_work, HZ / 100);
 	return SUCCESS;
 }
+#else
+static int scsi_abort_command(struct scsi_cmnd *scmd)
+{
+	return FAILED;
+}
+#endif
 
 /**
  * scsi_eh_scmd_add - add scsi cmd to error handling.

Bart.

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

On Thu, 2017-03-16 at 23:00 +0000, Bart Van Assche wrote:
> The following crash only occurs with async aborts enabled:
> 
> general protection fault: 0000 [#1] SMP
> RIP: 0010:scsi_device_put+0xb/0x30
> Call Trace:
>  scsi_disk_put+0x2d/0x40
>  sd_release+0x3d/0xb0
>  __blkdev_put+0x29e/0x360
>  blkdev_put+0x49/0x170
>  dm_put_table_device+0x58/0xc0 [dm_mod]
>  dm_put_device+0x70/0xc0 [dm_mod]
>  free_priority_group+0x92/0xc0 [dm_multipath]
>  free_multipath+0x70/0xc0 [dm_multipath]
>  multipath_dtr+0x19/0x20 [dm_multipath]
>  dm_table_destroy+0x67/0x120 [dm_mod]
>  dev_suspend+0xde/0x240 [dm_mod]
>  ctl_ioctl+0x1f5/0x520 [dm_mod]
>  dm_ctl_ioctl+0xe/0x20 [dm_mod]
>  do_vfs_ioctl+0x8f/0x700
>  SyS_ioctl+0x3c/0x70
>  entry_SYSCALL_64_fastpath+0x18/0xad

(replying to my own e-mail)

With my three scsi_dh_alua patches applied I don't see this crash anymore so
this crash was probably unrelated to async aborts.

Bart.

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Hannes Reinecke]

On 03/17/2017 12:00 AM, Bart Van Assche wrote:
> On Mon, 2017-03-13 at 14:55 -0700, James Bottomley wrote:
>> On Mon, 2017-03-13 at 20:33 +0000, Bart Van Assche wrote:
>>> On Mon, 2017-03-13 at 12:23 -0700, James Bottomley wrote:
>>>> On Mon, 2017-03-13 at 18:49 +0000, Bart Van Assche wrote:
>>>>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>>>>> index 7bfbcfa7af40..b3bb49d06943 100644
>>>>> --- a/drivers/scsi/scsi.c
>>>>> +++ b/drivers/scsi/scsi.c
>>>>> @@ -602,7 +602,7 @@ EXPORT_SYMBOL(scsi_device_get);
>>>>>   */
>>>>>  void scsi_device_put(struct scsi_device *sdev)
>>>>>  {
>>>>> -       module_put(sdev->host->hostt->module);
>>>>> +       module_put(sdev->hostt->module);
>>>>>         put_device(&sdev->sdev_gendev);
>>>>>  }
>>>>>  EXPORT_SYMBOL(scsi_device_put);
>>>>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>>>>> index 6f7128f49c30..7134487abbb1 100644
>>>>> --- a/drivers/scsi/scsi_scan.c
>>>>> +++ b/drivers/scsi/scsi_scan.c
>>>>> @@ -227,6 +227,7 @@ static struct scsi_device
>>>>> *scsi_alloc_sdev(struct
>>>>> scsi_target *starget,
>>>>>         sdev->model = scsi_null_device_strs;
>>>>>         sdev->rev = scsi_null_device_strs;
>>>>>         sdev->host = shost;
>>>>> +       sdev->hostt = shost->hostt;
>>>>>         sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
>>>>>         sdev->id = starget->id;
>>>>>         sdev->lun = lun;
>>>>> diff --git a/include/scsi/scsi_device.h
>>>>> b/include/scsi/scsi_device.h
>>>>> index 6f22b39f1b0c..cda620ed5922 100644
>>>>> --- a/include/scsi/scsi_device.h
>>>>> +++ b/include/scsi/scsi_device.h
>>>>> @@ -82,6 +82,7 @@ struct scsi_event {
>>>>>  
>>>>>  struct scsi_device {
>>>>>         struct Scsi_Host *host;
>>>>> +       struct scsi_host_template *hostt;
>>>>>         struct request_queue *request_queue;
>>>>>  
>>>>
>>>> The apparent assumption behind this patch is that sdev->host can be
>>>> freed but the sdev will still exist?  That shouldn't be correct:
>>>> the
>>>> rule for struct devices is that the child always holds the parent
>>>> and
>>>> the host is parented (albeit not necessarily directly) to the sdev,
>>>> so
>>>> it looks like something has gone wrong if the host had been freed
>>>> before the sdev.
>>>
>>> Hello James,
>>>
>>> scsi_remove_host() decreases the sdev reference count but does not 
>>> wait until the sdev release work has finished. This is why the SCSI
>>> host can already have disappeared before the last scsi_device_put()
>>> call occurs.
>>
>> This is true, but I don't see how it can cause the host to be freed
>> before the sdev.  The memory for struct Scsi_Host is freed in the
>> shost_gendev release routine, which should be pinned by the parent
>> traversal from sdev.  So it should not be possible for
>>  scsi_host_dev_release() to be called before
>>  scsi_device_dev_release_usercontext() becase the latter has the final
>> put of the parent device.
> 
> Hello Hannes,
> 
> The following crash only occurs with async aborts enabled:
> 
> general protection fault: 0000 [#1] SMP
> RIP: 0010:scsi_device_put+0xb/0x30
> Call Trace:
>  scsi_disk_put+0x2d/0x40
>  sd_release+0x3d/0xb0
>  __blkdev_put+0x29e/0x360
>  blkdev_put+0x49/0x170
>  dm_put_table_device+0x58/0xc0 [dm_mod]
>  dm_put_device+0x70/0xc0 [dm_mod]
>  free_priority_group+0x92/0xc0 [dm_multipath]
>  free_multipath+0x70/0xc0 [dm_multipath]
>  multipath_dtr+0x19/0x20 [dm_multipath]
>  dm_table_destroy+0x67/0x120 [dm_mod]
>  dev_suspend+0xde/0x240 [dm_mod]
>  ctl_ioctl+0x1f5/0x520 [dm_mod]
>  dm_ctl_ioctl+0xe/0x20 [dm_mod]
>  do_vfs_ioctl+0x8f/0x700
>  SyS_ioctl+0x3c/0x70
>  entry_SYSCALL_64_fastpath+0x18/0xad
> 
> I hadn't seen this crash before kernel v4.11-rc1 but with kernel v4.11-rc1
> and later I see it if I let the srp-test scripts run for a few minutes. The
> patch I used to disable async aborts on my test setup is as follows:
> 
How utterly curious.

Thing is, I didn't change anything in the async abort case; all my
patches haven't been merged yet.
So I would rather think this being the side effect of something else

And I just noticed that you found the real issue with your alua fixes,
so I guess this can be ignored, right?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

On Sat, 2017-03-18 at 12:17 +0100, Hannes Reinecke wrote:
> On 03/17/2017 12:00 AM, Bart Van Assche wrote:
> > I hadn't seen this crash before kernel v4.11-rc1 but with kernel v4.11-rc1
> > and later I see it if I let the srp-test scripts run for a few minutes. The
> > patch I used to disable async aborts on my test setup is as follows:
> 
> Thing is, I didn't change anything in the async abort case; all my
> patches haven't been merged yet.
> So I would rather think this being the side effect of something else
> 
> And I just noticed that you found the real issue with your alua fixes,
> so I guess this can be ignored, right?

Yes - sorry for the noise. This crash did not occur with the ALUA fixes
applied and async aborts enabled.

Bart.