| Message ID | 20170208225353.82334-1-songliubraving@fb.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
> On Feb 8, 2017, at 2:53 PM, Song Liu <songliubraving@fb.com> wrote: > > When a device is deleted through sysfs handle "delete", the code > locks shost->scan_mutex. If multiple devices are deleted at the > same time, these deletes will be handled in series. > > On the other hand, sd_shutdown() sometimes issues long latency > commands: sync cache and start_stop. It is not necessary for these > commands to run in series. > > To reduce latency of parallel "delete" requests, this patch unlock > shost->scan_mutex before long latency commands and relock the mutex > after the command. > > Fixed bug from previous version. > > Reported-by: kernel test robot <fengguang.wu@intel.com> > Signed-off-by: Song Liu <songliubraving@fb.com> > --- > drivers/scsi/sd.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 9e0783b..14c5815 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -3304,6 +3304,9 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start) > static void sd_shutdown(struct device *dev) > { > struct scsi_disk *sdkp = dev_get_drvdata(dev); > + struct scsi_device *sdev; > + struct Scsi_Host *shost; > + int try_lock_scan_mutex; > > if (!sdkp) > return; /* this can happen */ > @@ -3311,15 +3314,26 @@ static void sd_shutdown(struct device *dev) > if (pm_runtime_suspended(dev)) > return; > > + sdev = sdkp->device; > + shost = sdev->host; > + try_lock_scan_mutex = mutex_trylock(&shost->scan_mutex); > + > if (sdkp->WCE && sdkp->media_present) { > + mutex_unlock(&shost->scan_mutex); > sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n"); > sd_sync_cache(sdkp); > + mutex_lock(&shost->scan_mutex); > } > > if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) { > + mutex_unlock(&shost->scan_mutex); > sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n"); > sd_start_stop_device(sdkp, 0); > + mutex_lock(&shost->scan_mutex); > } > + > + if (try_lock_scan_mutex) > + mutex_unlock(&shost->scan_mutex); > } > > static int sd_suspend_common(struct device *dev, bool ignore_stop_errors) > -- > 2.9.3 > Forgot to CC Christoph
On Wed, 2017-02-08 at 14:53 -0800, Song Liu wrote:
> + try_lock_scan_mutex = mutex_trylock(&shost->scan_mutex);
This is at least as bad as the approach of your previous patch because
whether or not this mutex_trylock() call succeeds not only depends on
whether or not the caller holds the scan_mutex but also on whether any
other thread accidentally holds that mutex. Please stop hacking and do
what Christoph proposed, namely address the caller of this method.
Bart.
> On Feb 8, 2017, at 2:58 PM, Bart Van Assche <Bart.VanAssche@sandisk.com> wrote: > > On Wed, 2017-02-08 at 14:53 -0800, Song Liu wrote: >> + try_lock_scan_mutex = mutex_trylock(&shost->scan_mutex); > > This is at least as bad as the approach of your previous patch because > whether or not this mutex_trylock() call succeeds not only depends on > whether or not the caller holds the scan_mutex but also on whether any > other thread accidentally holds that mutex. Please stop hacking and do > what Christoph proposed, namely address the caller of this method. > > Bart. > Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer: > > This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system. > Thanks for the feedback. I will dig more in the caller side. Song
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 9e0783b..14c5815 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3304,6 +3304,9 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start) static void sd_shutdown(struct device *dev) { struct scsi_disk *sdkp = dev_get_drvdata(dev); + struct scsi_device *sdev; + struct Scsi_Host *shost; + int try_lock_scan_mutex; if (!sdkp) return; /* this can happen */ @@ -3311,15 +3314,26 @@ static void sd_shutdown(struct device *dev) if (pm_runtime_suspended(dev)) return; + sdev = sdkp->device; + shost = sdev->host; + try_lock_scan_mutex = mutex_trylock(&shost->scan_mutex); + if (sdkp->WCE && sdkp->media_present) { + mutex_unlock(&shost->scan_mutex); sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n"); sd_sync_cache(sdkp); + mutex_lock(&shost->scan_mutex); } if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) { + mutex_unlock(&shost->scan_mutex); sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n"); sd_start_stop_device(sdkp, 0); + mutex_lock(&shost->scan_mutex); } + + if (try_lock_scan_mutex) + mutex_unlock(&shost->scan_mutex); } static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
When a device is deleted through sysfs handle "delete", the code locks shost->scan_mutex. If multiple devices are deleted at the same time, these deletes will be handled in series. On the other hand, sd_shutdown() sometimes issues long latency commands: sync cache and start_stop. It is not necessary for these commands to run in series. To reduce latency of parallel "delete" requests, this patch unlock shost->scan_mutex before long latency commands and relock the mutex after the command. Fixed bug from previous version. Reported-by: kernel test robot <fengguang.wu@intel.com> Signed-off-by: Song Liu <songliubraving@fb.com> --- drivers/scsi/sd.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)