| Message ID | 20190111000858.76261-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sd patches for kernel v5.1 | expand |
On 1/11/19 1:08 AM, Bart Van Assche wrote: > As explained during the LSF/MM session about increasing SCSI disk > probing concurrency, the problems with the current probing approach > are as follows: > - The driver core is unaware of asynchronous SCSI LUN probing. > wait_for_device_probe() waits for all asynchronous probes except > asynchronous SCSI disk probes. > - There is unnecessary serialization between sd_probe() and sd_remove(). > This can lead to a deadlock. > > Hence this patch that modifies the sd driver such that it uses the > driver core framework for asynchronous probing. The async domains > and get_device()/put_device() pairs that became superfluous due to > this change are removed. > > This patch reduces the time needed for loading the scsi_debug kernel > module with parameters delay=0 and max_luns=256 from 0.7s to 0.1s. In > other words, this specific test runs about seven times faster. > > See also commit 3c31b52f96f7 ("scsi: async sd resume"). > > Cc: Lee Duncan <lduncan@suse.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Luis Chamberlain <mcgrof@kernel.org> > Cc: Johannes Thumshirn <jthumshirn@suse.de> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/scsi.c | 14 -------------- > drivers/scsi/scsi_pm.c | 22 ++-------------------- > drivers/scsi/scsi_priv.h | 3 --- > drivers/scsi/sd.c | 13 +++---------- > 4 files changed, 5 insertions(+), 47 deletions(-) > Very nice. Could you please check if this depends on any changes to the async probing mechanism? IOW, can it be backported? If so, which version would be the earliest one? Other than that: Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
On Fri, 2019-01-11 at 08:31 +0100, Hannes Reinecke wrote: > Could you please check if this depends on any changes to the async > probing mechanism? Hi Hannes, Patches one and two depend on driver core changes. I was assuming that these were already upstream but apparently that is not yet the case :-( See also "Re: [driver-core PATCH v9 1/9] driver core: Establish order of operations for device_add and device_del via bitflag" https://lore.kernel.org/lkml/0a72b8db91f9151ecc7f215b465ec8e69adc239c.camel@linux.intel.com/ Bart.
On 1/11/19 5:10 PM, Bart Van Assche wrote: > On Fri, 2019-01-11 at 08:31 +0100, Hannes Reinecke wrote: >> Could you please check if this depends on any changes to the async >> probing mechanism? > > Hi Hannes, > > Patches one and two depend on driver core changes. I was assuming that > these were already upstream but apparently that is not yet the case :-( > See also "Re: [driver-core PATCH v9 1/9] driver core: Establish order of > operations for device_add and device_del via bitflag" > https://lore.kernel.org/lkml/0a72b8db91f9151ecc7f215b465ec8e69adc239c.camel@linux.intel.com/ > Please state that in the patch description to avoid backports to versions which do not have this patchset included. Cheers, Hannes
On 1/12/19 12:29 AM, Hannes Reinecke wrote: > On 1/11/19 5:10 PM, Bart Van Assche wrote: >> On Fri, 2019-01-11 at 08:31 +0100, Hannes Reinecke wrote: >>> Could you please check if this depends on any changes to the async >>> probing mechanism? >> >> Patches one and two depend on driver core changes. I was assuming that >> these were already upstream but apparently that is not yet the case :-( >> See also "Re: [driver-core PATCH v9 1/9] driver core: Establish order of >> operations for device_add and device_del via bitflag" >> https://lore.kernel.org/lkml/0a72b8db91f9151ecc7f215b465ec8e69adc239c.camel@linux.intel.com/ >> > Please state that in the patch description to avoid backports to > versions which do not have this patchset included. I will do that. Without the driver core changes patches 1/8 and 2/8 make LUN scanning slower instead of faster. Maybe I should leave out the first two patches and repost these after Greg has accepted the driver core changes these patches rely on. Bart.
On 1/14/19 5:56 AM, Bart Van Assche wrote: > On 1/12/19 12:29 AM, Hannes Reinecke wrote: >> On 1/11/19 5:10 PM, Bart Van Assche wrote: >>> On Fri, 2019-01-11 at 08:31 +0100, Hannes Reinecke wrote: >>>> Could you please check if this depends on any changes to the async >>>> probing mechanism? >>> >>> Patches one and two depend on driver core changes. I was assuming that >>> these were already upstream but apparently that is not yet the case :-( >>> See also "Re: [driver-core PATCH v9 1/9] driver core: Establish order of >>> operations for device_add and device_del via bitflag" >>> https://lore.kernel.org/lkml/0a72b8db91f9151ecc7f215b465ec8e69adc239c.camel@linux.intel.com/ >>> >> Please state that in the patch description to avoid backports to >> versions which do not have this patchset included. > > I will do that. > > Without the driver core changes patches 1/8 and 2/8 make LUN scanning > slower instead of faster. Maybe I should leave out the first two patches > and repost these after Greg has accepted the driver core changes these > patches rely on. > Yes, please do. And fter the driver core patches have been accepted please resubmit your first two patches referencing the commit ID(s) of the driver core patches. That will make tracking easier for us poor distro people. Cheers, Hannes
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index 7675ff0ca2ea..fd040e6ce9c3 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -85,19 +85,6 @@ unsigned int scsi_logging_level; EXPORT_SYMBOL(scsi_logging_level); #endif -/* sd, scsi core and power management need to coordinate flushing async actions */ -ASYNC_DOMAIN(scsi_sd_probe_domain); -EXPORT_SYMBOL(scsi_sd_probe_domain); - -/* - * Separate domain (from scsi_sd_probe_domain) to maximize the benefit of - * asynchronous system resume operations. It is marked 'exclusive' to avoid - * being included in the async_synchronize_full() that is invoked by - * dpm_resume() - */ -ASYNC_DOMAIN_EXCLUSIVE(scsi_sd_pm_domain); -EXPORT_SYMBOL(scsi_sd_pm_domain); - /** * scsi_put_command - Free a scsi command block * @cmd: command block to free @@ -836,7 +823,6 @@ static void __exit exit_scsi(void) scsi_exit_devinfo(); scsi_exit_procfs(); scsi_exit_queue(); - async_unregister_domain(&scsi_sd_probe_domain); } subsys_initcall(init_scsi); diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c index a2b4179bfdf7..46a2746618cc 100644 --- a/drivers/scsi/scsi_pm.c +++ b/drivers/scsi/scsi_pm.c @@ -55,9 +55,6 @@ static int scsi_dev_type_suspend(struct device *dev, const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; int err; - /* flush pending in-flight resume operations, suspend is synchronous */ - async_synchronize_full_domain(&scsi_sd_pm_domain); - err = scsi_device_quiesce(to_scsi_device(dev)); if (err == 0) { err = cb(dev, pm); @@ -150,18 +147,7 @@ static int scsi_bus_resume_common(struct device *dev, if (scsi_is_sdev_device(dev) && pm_runtime_suspended(dev)) blk_set_runtime_active(to_scsi_device(dev)->request_queue); - if (fn) { - async_schedule_domain(fn, dev, &scsi_sd_pm_domain); - - /* - * If a user has disabled async probing a likely reason - * is due to a storage enclosure that does not inject - * staggered spin-ups. For safety, make resume - * synchronous as well in that case. - */ - if (strncmp(scsi_scan_type, "async", 5) != 0) - async_synchronize_full_domain(&scsi_sd_pm_domain); - } else { + if (!fn) { pm_runtime_disable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); @@ -171,11 +157,7 @@ static int scsi_bus_resume_common(struct device *dev, static int scsi_bus_prepare(struct device *dev) { - if (scsi_is_sdev_device(dev)) { - /* sd probing uses async_schedule. Wait until it finishes. */ - async_synchronize_full_domain(&scsi_sd_probe_domain); - - } else if (scsi_is_host_device(dev)) { + if (scsi_is_host_device(dev)) { /* Wait until async scanning is finished */ scsi_complete_async_scans(); } diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h index 5f21547b2ad2..b1edf15704c0 100644 --- a/drivers/scsi/scsi_priv.h +++ b/drivers/scsi/scsi_priv.h @@ -174,9 +174,6 @@ static inline int scsi_autopm_get_host(struct Scsi_Host *h) { return 0; } static inline void scsi_autopm_put_host(struct Scsi_Host *h) {} #endif /* CONFIG_PM */ -extern struct async_domain scsi_sd_pm_domain; -extern struct async_domain scsi_sd_probe_domain; - /* scsi_dh.c */ #ifdef CONFIG_SCSI_DH void scsi_dh_add_device(struct scsi_device *sdev); diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index c6ca3d915925..c2ddb697fa90 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -561,6 +561,7 @@ static struct scsi_driver sd_template = { .name = "sd", .owner = THIS_MODULE, .probe = sd_probe, + .probe_type = PROBE_PREFER_ASYNCHRONOUS, .remove = sd_remove, .shutdown = sd_shutdown, .pm = &sd_pm_ops, @@ -3278,12 +3279,8 @@ static int sd_format_disk_name(char *prefix, int index, char *buf, int buflen) return 0; } -/* - * The asynchronous part of sd_probe - */ -static void sd_probe_async(void *data, async_cookie_t cookie) +static void sd_probe_part2(struct scsi_disk *sdkp) { - struct scsi_disk *sdkp = data; struct scsi_device *sdp; struct gendisk *gd; u32 index; @@ -3337,7 +3334,6 @@ static void sd_probe_async(void *data, async_cookie_t cookie) sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n", sdp->removable ? "removable " : ""); scsi_autopm_put_device(sdp); - put_device(&sdkp->dev); } /** @@ -3429,8 +3425,7 @@ static int sd_probe(struct device *dev) get_device(dev); dev_set_drvdata(dev, sdkp); - get_device(&sdkp->dev); /* prevent release before async_schedule */ - async_schedule_domain(sd_probe_async, sdkp, &scsi_sd_probe_domain); + sd_probe_part2(sdkp); return 0; @@ -3465,8 +3460,6 @@ static int sd_remove(struct device *dev) devt = disk_devt(sdkp->disk); scsi_autopm_get_device(sdkp->device); - async_synchronize_full_domain(&scsi_sd_pm_domain); - async_synchronize_full_domain(&scsi_sd_probe_domain); device_del(&sdkp->dev); del_gendisk(sdkp->disk); sd_shutdown(dev);
As explained during the LSF/MM session about increasing SCSI disk probing concurrency, the problems with the current probing approach are as follows: - The driver core is unaware of asynchronous SCSI LUN probing. wait_for_device_probe() waits for all asynchronous probes except asynchronous SCSI disk probes. - There is unnecessary serialization between sd_probe() and sd_remove(). This can lead to a deadlock. Hence this patch that modifies the sd driver such that it uses the driver core framework for asynchronous probing. The async domains and get_device()/put_device() pairs that became superfluous due to this change are removed. This patch reduces the time needed for loading the scsi_debug kernel module with parameters delay=0 and max_luns=256 from 0.7s to 0.1s. In other words, this specific test runs about seven times faster. See also commit 3c31b52f96f7 ("scsi: async sd resume"). Cc: Lee Duncan <lduncan@suse.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Luis Chamberlain <mcgrof@kernel.org> Cc: Johannes Thumshirn <jthumshirn@suse.de> Cc: Christoph Hellwig <hch@lst.de> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/scsi.c | 14 -------------- drivers/scsi/scsi_pm.c | 22 ++-------------------- drivers/scsi/scsi_priv.h | 3 --- drivers/scsi/sd.c | 13 +++---------- 4 files changed, 5 insertions(+), 47 deletions(-)