Patchwork [isci,v3,3/4] libsas: suspend / resume support

login
register
mail settings
Submitter Dan Williams
Date March 14, 2012, 9:33 p.m.
Message ID <1331760786.17334.18.camel@ultramagnus.opencreations.com>
Download mbox | patch
Permalink /patch/146724/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Dan Williams - March 14, 2012, 9:33 p.m.
On Wed, 2012-03-14 at 17:11 -0400, Alan Stern wrote:
> I see.  It's a nasty situation; I guess the best way to describe it is 
> a conflict between the requirements of the PM and SCSI subsystems:
> 
> 	The PM core runs suspend and resume operations in async 
> 	threads, and these threads need to acquire the device lock;
> 
> 	The sd driver needs to insure that async probing is finished
> 	when starting its remove routine, and it is called with the
> 	device lock held.
> 
> The best solution might be to use a workqueue for sd's async probing 
> instead of the "async" threads.  Then the work routine could be 
> cancelled without doing async_synchronize_full().

Hmm... I was wondering why we needed a kernel global sync.  If this is
just for probe work what about just doing something like the following?
Keep the sync operation local to probe-work and not entangle with the
resume-work?  I'll give this a shot when I get back to my test system.





--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - March 15, 2012, 7:28 p.m.
On Wed, Mar 14, 2012 at 2:33 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Wed, 2012-03-14 at 17:11 -0400, Alan Stern wrote:
>> I see.  It's a nasty situation; I guess the best way to describe it is
>> a conflict between the requirements of the PM and SCSI subsystems:
>>
>>       The PM core runs suspend and resume operations in async
>>       threads, and these threads need to acquire the device lock;
>>
>>       The sd driver needs to insure that async probing is finished
>>       when starting its remove routine, and it is called with the
>>       device lock held.
>>
>> The best solution might be to use a workqueue for sd's async probing
>> instead of the "async" threads.  Then the work routine could be
>> cancelled without doing async_synchronize_full().
>
> Hmm... I was wondering why we needed a kernel global sync.  If this is
> just for probe work what about just doing something like the following?
> Keep the sync operation local to probe-work and not entangle with the
> resume-work?  I'll give this a shot when I get back to my test system.
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index bd17cf8..ec69e35 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2629,6 +2629,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>        put_device(&sdkp->dev);
>  }
>
> +static LIST_HEAD(sd_probe_domain);
> +
>  /**
>  *     sd_probe - called during driver initialization and whenever a
>  *     new scsi device is attached to the system. It is called once
> @@ -2717,7 +2719,7 @@ static int sd_probe(struct device *dev)
>        dev_set_drvdata(dev, sdkp);
>
>        get_device(&sdkp->dev); /* prevent release before async_schedule */
> -       async_schedule(sd_probe_async, sdkp);
> +       async_schedule_domain(sd_probe_async, sdkp, &sd_probe_domain);
>
>        return 0;
>
> @@ -2751,7 +2753,7 @@ static int sd_remove(struct device *dev)
>        sdkp = dev_get_drvdata(dev);
>        scsi_autopm_get_device(sdkp->device);
>
> -       async_synchronize_full();
> +       async_synchronize_full_domain(&sd_probe_domain);
>        blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
>        blk_queue_unprep_rq(sdkp->device->request_queue, NULL);
>        device_del(&sdkp->dev);

This works fine for me for resolving the deadlock, but I found I also
needed the following to fix a spurious:

   scsi 6:0:1:0: Illegal state transition deleted->running

...in the resume path.

@@ -2348,10 +2349,11 @@ EXPORT_SYMBOL(scsi_device_quiesce);
  *
  *     Must be called with user context, may sleep.
  */
-void
-scsi_device_resume(struct scsi_device *sdev)
+void scsi_device_resume(struct scsi_device *sdev)
 {
-       if(scsi_device_set_state(sdev, SDEV_RUNNING))
+       /* check if the device was deleted during suspend */
+       if (sdev->sdev_state == SDEV_DEL ||
+           scsi_device_set_state(sdev, SDEV_RUNNING))
                return;
        scsi_run_queue(sdev->request_queue);
 }

Unless someone can point out something I'm missing I'll go ahead and
roll this into it's own patch and rebase/drop the hack out of the
libsas resume code.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - March 15, 2012, 10:16 p.m.
On Thu, Mar 15, 2012 at 12:54 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 15 Mar 2012, Williams, Dan J wrote:
>
>> > Hmm... I was wondering why we needed a kernel global sync.  If this is
>> > just for probe work what about just doing something like the following?
>> > Keep the sync operation local to probe-work and not entangle with the
>> > resume-work?  I'll give this a shot when I get back to my test system.
>> >
>> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> > index bd17cf8..ec69e35 100644
>> > --- a/drivers/scsi/sd.c
>> > +++ b/drivers/scsi/sd.c
>> > @@ -2629,6 +2629,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>> >        put_device(&sdkp->dev);
>> >  }
>> >
>> > +static LIST_HEAD(sd_probe_domain);
>
> Take a look at fea6d607e154cf96ab22254ccb48addfd43d4cb5; it might be a
> good idea to make this domain available to scsi_bus_prepare().  For
> example, it could made into a SCSI-wide domain, defined in the SCSI
> core and exported for use by sd.

Nice, thanks for the pointer.  Yes, I'll up-level this.

>
>> This works fine for me for resolving the deadlock, but I found I also
>> needed the following to fix a spurious:
>>
>>    scsi 6:0:1:0: Illegal state transition deleted->running
>>
>> ...in the resume path.
>>
>> @@ -2348,10 +2349,11 @@ EXPORT_SYMBOL(scsi_device_quiesce);
>>   *
>>   *     Must be called with user context, may sleep.
>>   */
>> -void
>> -scsi_device_resume(struct scsi_device *sdev)
>> +void scsi_device_resume(struct scsi_device *sdev)
>>  {
>> -       if(scsi_device_set_state(sdev, SDEV_RUNNING))
>> +       /* check if the device was deleted during suspend */
>> +       if (sdev->sdev_state == SDEV_DEL ||
>> +           scsi_device_set_state(sdev, SDEV_RUNNING))
>>                 return;
>>         scsi_run_queue(sdev->request_queue);
>>  }
>>
>> Unless someone can point out something I'm missing I'll go ahead and
>> roll this into it's own patch and rebase/drop the hack out of the
>> libsas resume code.
>
> The device might be in some other state.  Perhaps it would be better to
> do
>
>        if (sdev->sdev_state != SDEV_QUIESCE ||
>                        scsi_device_set_state(sdev, SDEV_RUNNING))
>
> I'm not sure what guarantees this function is supposed to provide, but
> the comment indicates that it's meant just to handle quiesced devices.
>

I'm not sure either, but I can get on board with this change to say
"the world changed when you weren't looking, assume whomever changed
the state is taking care of the device".

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jack Wang - March 16, 2012, 9:18 a.m.
Dan,

I found problem about SATA error handle and this also led to panic when run suspend/resume test. Thanks again for your share debug method and patch of testing suspend/resume problem.

The problem is:

When probe libata probe sata will try to reset device and send IDENTIFY DEVICE.. commands when 3 times fail will give up but never tell libsas and lldd to clean up the resource. And if some time later the device came up report BROADCAST CHANGE and libsas found device try to  tell lldd, but lldd is not forget the original one will reject this. 
Should we export sas_unregister_devs_sas_addr let ata call this when give up.


For suspend/resume here I've seen Hard LOCKUP when this kind of things happen.
Will look into the problem deeper to see the root cause.

************************************************************************

Jack Wang 王金浦 Software Engineer

RD2 (Storage) 

Department of Storage Products R&D, SCS

Universal Scientific Industrial (Shanghai) Co., Ltd

421 Lishizhen Rd, Pudong New Area, Shanghai, 

P.R. China, 201203

上海市浦东新区李时珍路421号    邮编: 201203

Tel :  +86 -21-5896 6996  ext. 81526

Fax : +86 -21-5080 4268

http://www.usi.com.tw  MAIL: jack_wang@usish.com

************************************************************************

 
> -----邮件原件-----
> 发件人: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org] 代表 Williams, Dan J
> 发送时间: 2012年3月16日 6:16
> 收件人: Alan Stern
> 抄送: linux-scsi@vger.kernel.org; linux-ide@vger.kernel.org; Jacek Danecki
> 主题: Re: [isci PATCH v3 3/4] libsas: suspend / resume support
> 
> On Thu, Mar 15, 2012 at 12:54 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 15 Mar 2012, Williams, Dan J wrote:
> >
> >> > Hmm... I was wondering why we needed a kernel global sync.  If this is
> >> > just for probe work what about just doing something like the following?
> >> > Keep the sync operation local to probe-work and not entangle with the
> >> > resume-work?  I'll give this a shot when I get back to my test system.
> >> >
> >> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> >> > index bd17cf8..ec69e35 100644
> >> > --- a/drivers/scsi/sd.c
> >> > +++ b/drivers/scsi/sd.c
> >> > @@ -2629,6 +2629,8 @@ static void sd_probe_async(void *data,
> async_cookie_t cookie)
> >> >        put_device(&sdkp->dev);
> >> >  }
> >> >
> >> > +static LIST_HEAD(sd_probe_domain);
> >
> > Take a look at fea6d607e154cf96ab22254ccb48addfd43d4cb5; it might be a
> > good idea to make this domain available to scsi_bus_prepare().  For
> > example, it could made into a SCSI-wide domain, defined in the SCSI
> > core and exported for use by sd.
> 
> Nice, thanks for the pointer.  Yes, I'll up-level this.
> 
> >
> >> This works fine for me for resolving the deadlock, but I found I also
> >> needed the following to fix a spurious:
> >>
> >>    scsi 6:0:1:0: Illegal state transition deleted->running
> >>
> >> ...in the resume path.
> >>
> >> @@ -2348,10 +2349,11 @@ EXPORT_SYMBOL(scsi_device_quiesce);
> >>   *
> >>   *     Must be called with user context, may sleep.
> >>   */
> >> -void
> >> -scsi_device_resume(struct scsi_device *sdev)
> >> +void scsi_device_resume(struct scsi_device *sdev)
> >>  {
> >> -       if(scsi_device_set_state(sdev, SDEV_RUNNING))
> >> +       /* check if the device was deleted during suspend */
> >> +       if (sdev->sdev_state == SDEV_DEL ||
> >> +           scsi_device_set_state(sdev, SDEV_RUNNING))
> >>                 return;
> >>         scsi_run_queue(sdev->request_queue);
> >>  }
> >>
> >> Unless someone can point out something I'm missing I'll go ahead and
> >> roll this into it's own patch and rebase/drop the hack out of the
> >> libsas resume code.
> >
> > The device might be in some other state.  Perhaps it would be better to
> > do
> >
> >        if (sdev->sdev_state != SDEV_QUIESCE ||
> >                        scsi_device_set_state(sdev, SDEV_RUNNING))
> >
> > I'm not sure what guarantees this function is supposed to provide, but
> > the comment indicates that it's meant just to handle quiesced devices.
> >
> 
> I'm not sure either, but I can get on board with this change to say
> "the world changed when you weren't looking, assume whomever changed
> the state is taking care of the device".
> 
> --
> Dan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - March 22, 2012, 6:27 a.m.
2012/3/16 Jack Wang <jack_wang@usish.com>:
> Dan,
>
> I found problem about SATA error handle and this also led to panic when run suspend/resume test. Thanks again for your share debug method and patch of testing suspend/resume problem.
>
> The problem is:
>
> When probe libata probe sata will try to reset device and send IDENTIFY DEVICE.. commands when 3 times fail will give up but never tell libsas and lldd to clean up the resource. And if some time later the device came up report BROADCAST CHANGE and libsas found device try to  tell lldd, but lldd is not forget the original one will reject this.
> Should we export sas_unregister_devs_sas_addr let ata call this when give up.

Have a look at "libsas: fix ata_eh clobbering ex_phys via
smp_ata_check_ready" [1], this fixed up cases where libsas failed to
handle ata device unplug in our environment.

--
Dan

[1]: http://marc.info/?l=linux-scsi&m=133239709603452&w=2
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jack Wang - March 22, 2012, 7:44 a.m.
> 
> 2012/3/16 Jack Wang <jack_wang@usish.com>:
> > Dan,
> >
> > I found problem about SATA error handle and this also led to panic when run
> suspend/resume test. Thanks again for your share debug method and patch of
> testing suspend/resume problem.
> >
> > The problem is:
> >
> > When probe libata probe sata will try to reset device and send IDENTIFY DEVICE..
> commands when 3 times fail will give up but never tell libsas and lldd to clean
> up the resource. And if some time later the device came up report BROADCAST
> CHANGE and libsas found device try to  tell lldd, but lldd is not forget the
> original one will reject this.
> > Should we export sas_unregister_devs_sas_addr let ata call this when give
> up.
> 
> Have a look at "libsas: fix ata_eh clobbering ex_phys via
> smp_ata_check_ready" [1], this fixed up cases where libsas failed to
> handle ata device unplug in our environment.
> 
> --
> Dan
> 
> [1]: http://marc.info/?l=linux-scsi&m=133239709603452&w=2
[Jack Wang] 
Thanks Dan, we are testing your new patchset in our test environment, will report back result.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index bd17cf8..ec69e35 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2629,6 +2629,8 @@  static void sd_probe_async(void *data, async_cookie_t cookie)
 	put_device(&sdkp->dev);
 }
 
+static LIST_HEAD(sd_probe_domain);
+
 /**
  *	sd_probe - called during driver initialization and whenever a
  *	new scsi device is attached to the system. It is called once
@@ -2717,7 +2719,7 @@  static int sd_probe(struct device *dev)
 	dev_set_drvdata(dev, sdkp);
 
 	get_device(&sdkp->dev);	/* prevent release before async_schedule */
-	async_schedule(sd_probe_async, sdkp);
+	async_schedule_domain(sd_probe_async, sdkp, &sd_probe_domain);
 
 	return 0;
 
@@ -2751,7 +2753,7 @@  static int sd_remove(struct device *dev)
 	sdkp = dev_get_drvdata(dev);
 	scsi_autopm_get_device(sdkp->device);
 
-	async_synchronize_full();
+	async_synchronize_full_domain(&sd_probe_domain);
 	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
 	blk_queue_unprep_rq(sdkp->device->request_queue, NULL);
 	device_del(&sdkp->dev);