| Message ID | 1331760786.17334.18.camel@ultramagnus.opencreations.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
> > 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
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);