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

2017-03-18 Thread 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.

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

2017-03-18 Thread 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 +, Bart Van Assche wrote:
>>> On Mon, 2017-03-13 at 12:23 -0700, James Bottomley wrote:
 On Mon, 2017-03-13 at 18:49 +, 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_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:  [#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
h...@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

2017-03-17 Thread Bart Van Assche 
On Thu, 2017-03-16 at 23:00 +, Bart Van Assche wrote:
> The following crash only occurs with async aborts enabled:
> 
> general protection fault:  [#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

2017-03-16 Thread Bart Van Assche 
On Mon, 2017-03-13 at 14:55 -0700, James Bottomley wrote:
> On Mon, 2017-03-13 at 20:33 +, Bart Van Assche wrote:
> > On Mon, 2017-03-13 at 12:23 -0700, James Bottomley wrote:
> > > On Mon, 2017-03-13 at 18:49 +, 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_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:  [#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, >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

2017-03-16 Thread 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

2017-03-16 Thread 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

2017-03-16 Thread 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

2017-03-15 Thread Bart Van Assche 
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.From 458959938476788710738039a7c195e9c48ff338 Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
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:
[] 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 
Signed-off-by: Bart Van Assche 
---
 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_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

From b86b5087698c73f5fdd8cc9fa18ed3f1e9e174bb Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
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 
Cc: 
---
 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,

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

2017-03-14 Thread 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

2017-03-14 Thread 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

2017-03-13 Thread 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

2017-03-13 Thread James Bottomley 
On Mon, 2017-03-13 at 20:33 +, Bart Van Assche wrote:
> On Mon, 2017-03-13 at 12:23 -0700, James Bottomley wrote:
> > On Mon, 2017-03-13 at 18:49 +, 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_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

2017-03-13 Thread Bart Van Assche 
On Mon, 2017-03-13 at 12:23 -0700, James Bottomley wrote:
> On Mon, 2017-03-13 at 18:49 +, 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_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

2017-03-13 Thread James Bottomley 
On Mon, 2017-03-13 at 18:49 +, 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_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

2017-03-13 Thread Bart Van Assche 
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.From 3c5882c9cd88265ff81f6321edc3caf2a2981f55 Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
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:  [#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 
Cc: 
---
 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_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

From ec349df5c6b0336c746c109895d717b2be8151cb Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
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 
---
 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

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

2017-03-12 Thread 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_intel 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;

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

2017-03-09 Thread 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_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(>device_busy),
+   atomic_read(>device_blocked));
+
    device_del(dev);
    } else
    put_device(>sdev_dev);
-- 
2.12.0