Re: [PATCH v5 3/3] scsi: async sd resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 7, 2014 at 7:42 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 5 Mar 2014, Dan Williams wrote:
>
>> async_schedule() sd resume work to allow disks and other devices to
>> resume in parallel.
>>
>> This moves the entirety of scsi_device resume to an async context to
>> ensure that scsi_device_resume() remains ordered with respect to the
>> completion of the start/stop command.  For the duration of the resume,
>> new command submissions (that do not originate from the scsi-core) will
>> be deferred (BLKPREP_DEFER).
>>
>> It adds a new ASYNC_DOMAIN_EXCLUSIVE(scsi_sd_pm_domain) as a container
>> of these operations.  Like scsi_sd_probe_domain it is flushed at
>> sd_remove() time to ensure async ops do not continue past the
>> end-of-life of the sdev.  The implementation explicitly refrains from
>> reusing scsi_sd_probe_domain directly for this purpose as it is flushed
>> at the end of dpm_resume(), potentially defeating some of the benefit.
>> Given sdevs are quiesced it is permissible for these resume operations
>> to bleed past the async_synchronize_full() calls made by the driver
>> core.
>>
>> We defer the resolution of which pm callback to call until
>> scsi_dev_type_{suspend|resume} time and guarantee that the 'int
>> (*cb)(struct device *)' parameter is never NULL.  With this in place the
>> type of resume operation is encoded in the async function identifier.
>>
>> Inspired by Todd's analysis and initial proposal [2]:
>> https://01.org/suspendresume/blogs/tebrandt/2013/hard-disk-resume-optimization-simpler-approach
>>
>> Cc: Len Brown <len.brown@xxxxxxxxx>
>> Cc: Phillip Susi <psusi@xxxxxxxxxx>
>> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>> Suggested-by: Todd Brandt <todd.e.brandt@xxxxxxxxxxxxxxx>
>> [djbw: kick all resume work to the async queue]
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>> ---
>
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>> @@ -18,17 +18,52 @@
>>
>>  #ifdef CONFIG_PM_SLEEP
>>
>> +#define do_pm_op(dev, op) \
>> +     dev->driver ? dev->driver->pm ? \
>> +     dev->driver->pm->op(dev) : 0 : 0
>
> This will crash if dev->driver->pm->op is NULL.  How about making it
> easier to read, too?
>
> #define RETURN_PM_OP(dev, op)                                           \
>         if (dev->driver && dev->driver->pm && dev->driver->pm->op)      \
>                 return dev->driver->pm->op(dev);                        \
>         else                                                            \
>                 return 0
>
> static int do_scsi_suspend(struct device *dev)
> {
>         RETURM_PM_OP(dev, suspend);
> }
>
> etc.
>
> Alternatively, you could put the "dev->driver && dev->driver->pm" part
> of the test directly into scsi_dev_type_suspend and
> scsi_dev_type_resume, to save a little code space.  Then the original
> macro formulation would become sufficiently simple: one test and one
> function call.

I like that better than hiding too much (especially bugs) in the macro.

>
>> +static int do_scsi_suspend(struct device *dev)
>> +{
>> +     return do_pm_op(dev, suspend);
>> +}
>> +
>> +static int do_scsi_freeze(struct device *dev)
>> +{
>> +     return do_pm_op(dev, freeze);
>> +}
>> +
>> +static int do_scsi_poweroff(struct device *dev)
>> +{
>> +     return do_pm_op(dev, poweroff);
>> +}
>> +
>> +static int do_scsi_resume(struct device *dev)
>> +{
>> +     return do_pm_op(dev, resume);
>> +}
>> +
>> +static int do_scsi_thaw(struct device *dev)
>> +{
>> +     return do_pm_op(dev, thaw);
>> +}
>> +
>> +static int do_scsi_restore(struct device *dev)
>> +{
>> +     return do_pm_op(dev, restore);
>> +}
>> +
>>  static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
>>  {
>>       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) {
>> -             if (cb) {
>> -                     err = cb(dev);
>> -                     if (err)
>> -                             scsi_device_resume(to_scsi_device(dev));
>> -             }
>> +             err = cb(dev);
>> +             if (err)
>> +                     scsi_device_resume(to_scsi_device(dev));
>>       }
>>       dev_dbg(dev, "scsi suspend: %d\n", err);
>>       return err;
>> @@ -38,10 +73,16 @@ static int scsi_dev_type_resume(struct device *dev, int (*cb)(struct device *))
>>  {
>>       int err = 0;
>>
>> -     if (cb)
>> -             err = cb(dev);
>> +     err = cb(dev);
>>       scsi_device_resume(to_scsi_device(dev));
>>       dev_dbg(dev, "scsi resume: %d\n", err);
>> +
>> +     if (err == 0) {
>> +             pm_runtime_disable(dev);
>> +             pm_runtime_set_active(dev);
>> +             pm_runtime_enable(dev);
>> +     }
>> +
>>       return err;
>>  }
>>
>> @@ -66,20 +107,50 @@ scsi_bus_suspend_common(struct device *dev, int (*cb)(struct device *))
>>       return err;
>>  }
>>
>> +static void async_sdev_resume(void *dev, async_cookie_t cookie)
>> +{
>> +     scsi_dev_type_resume(dev, do_scsi_resume);
>> +}
>> +
>> +static void async_sdev_thaw(void *dev, async_cookie_t cookie)
>> +{
>> +     scsi_dev_type_resume(dev, do_scsi_thaw);
>> +}
>> +
>> +static void async_sdev_restore(void *dev, async_cookie_t cookie)
>> +{
>> +     scsi_dev_type_resume(dev, do_scsi_restore);
>> +}
>> +
>> +static async_func_t to_async_sdev_resume_fn(struct device *dev,
>> +             int (*cb)(struct device *))
>> +{
>> +     if (!scsi_is_sdev_device(dev))
>> +             return NULL;
>> +
>> +     if (cb == do_scsi_resume)
>> +             return async_sdev_resume;
>> +     else if (cb == do_scsi_thaw)
>> +             return async_sdev_thaw;
>> +     else if (cb == do_scsi_restore)
>> +             return async_sdev_restore;
>> +     else
>> +             return NULL;
>> +}
>
> Given that this function is used in only one place; I would put it
> inline.

Personal preference, but since you mention it, sure.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux