* [PATCH v2] scsi: ses: don't get power status of SES device slot on probe
@ 2017-04-05 15:18 Mauricio Faria de Oliveira
2017-04-05 15:32 ` Dan Williams
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Mauricio Faria de Oliveira @ 2017-04-05 15:18 UTC (permalink / raw)
To: linux-scsi, martin.petersen, songliubraving, dan.j.williams
Cc: jejb, axboe, hare, hch, gpiccoli
The commit 08024885a2a3 ("ses: Add power_status to SES device slot")
introduced the 'power_status' attribute to enclosure components and
the associated callbacks.
There are 2 callbacks available to get the power status of a device:
1) ses_get_power_status() for 'struct enclosure_component_callbacks'
2) get_component_power_status() for the sysfs device attribute
(these are available for kernel-space and user-space, respectively.)
However, despite both methods being available to get power status
on demand, that commit also introduced a call to get power status
in ses_enclosure_data_process().
This dramatically increased the total probe time for SCSI devices
on larger configurations, because ses_enclosure_data_process() is
called several times during the SCSI devices probe and loops over
the component devices (but that is another problem, another patch).
That results in a tremendous continuous hammering of SCSI Receive
Diagnostics commands to the enclosure-services device, which does
delay the total probe time for the SCSI devices __significantly__:
Originally, ~34 minutes on a system attached to ~170 disks:
[ 9214.490703] mpt3sas version 13.100.00.00 loaded
...
[11256.580231] scsi 17:0:177:0: qdepth(16), tagged(1), simple(0),
ordered(0), scsi_level(6), cmd_que(1)
With this patch, it decreased to ~2.5 minutes -- a 13.6x faster
[ 1002.992533] mpt3sas version 13.100.00.00 loaded
...
[ 1151.978831] scsi 11:0:177:0: qdepth(16), tagged(1), simple(0),
ordered(0), scsi_level(6), cmd_que(1)
Back to the commit discussion.. on the ses_get_power_status() call
introduced in ses_enclosure_data_process(): impact of removing it.
That may possibly be in place to initialize the power status value
on device probe. However, those 2 functions available to retrieve
that value _do_ automatically refresh/update it. So the potential
benefit would be a direct access of the 'power_status' field which
does not use the callbacks...
But the only reader of 'struct enclosure_component::power_status'
is the get_component_power_status() callback for sysfs attribute,
and it _does_ check for and call the .get_power_status callback,
(which indeed is defined and implemented by that commit), so the
power status value is, again, automatically updated.
So, the remaining potential for a direct/non-callback access to
the power_status attribute would be out-of-tree modules -- well,
for those, if they are for whatever reason interested in values
that are set during device probe and not up-to-date by the time
they need it.. well, that would be curious.
Well, to handle that more properly, set the initial power state
value to '-1' (i.e., uninitialized) instead of '1' (power 'on'),
and check for it in that callback which may do an direct access
to the field value _if_ a callback function is not defined.
Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
Fixes: 08024885a2a3 ("ses: Add power_status to SES device slot")
---
v2:
- remove module parameter.
- better return values for -1/uninitalized power_status.
(thanks: Dan Williams <dan.j.williams@intel.com>)
drivers/misc/enclosure.c | 7 ++++++-
drivers/scsi/ses.c | 1 -
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed7146e9b..d3fe3ea902d4 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -148,7 +148,7 @@ struct enclosure_device *
for (i = 0; i < components; i++) {
edev->component[i].number = -1;
edev->component[i].slot = -1;
- edev->component[i].power_status = 1;
+ edev->component[i].power_status = -1;
}
mutex_lock(&container_list_lock);
@@ -594,6 +594,11 @@ static ssize_t get_component_power_status(struct device *cdev,
if (edev->cb->get_power_status)
edev->cb->get_power_status(edev, ecomp);
+
+ /* If still uninitialized, the callback failed or does not exist. */
+ if (ecomp->power_status == -1)
+ return (edev->cb->get_power_status) ? -EIO : -ENOTTY;
+
return snprintf(buf, 40, "%s\n", ecomp->power_status ? "on" : "off");
}
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 50adabbb5808..f1cdf32d7514 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -548,7 +548,6 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
ecomp = &edev->component[components++];
if (!IS_ERR(ecomp)) {
- ses_get_power_status(edev, ecomp);
if (addl_desc_ptr)
ses_process_descriptor(
ecomp,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] scsi: ses: don't get power status of SES device slot on probe
2017-04-05 15:18 [PATCH v2] scsi: ses: don't get power status of SES device slot on probe Mauricio Faria de Oliveira
@ 2017-04-05 15:32 ` Dan Williams
2017-04-05 16:23 ` Song Liu
2017-04-06 16:49 ` Martin K. Petersen
2 siblings, 0 replies; 5+ messages in thread
From: Dan Williams @ 2017-04-05 15:32 UTC (permalink / raw)
To: Mauricio Faria de Oliveira
Cc: linux-scsi, Martin K. Petersen, Song Liu, James E.J. Bottomley,
Jens Axboe, Hannes Reinecke, Christoph Hellwig, gpiccoli
On Wed, Apr 5, 2017 at 8:18 AM, Mauricio Faria de Oliveira
<mauricfo@linux.vnet.ibm.com> wrote:
> The commit 08024885a2a3 ("ses: Add power_status to SES device slot")
> introduced the 'power_status' attribute to enclosure components and
> the associated callbacks.
>
> There are 2 callbacks available to get the power status of a device:
> 1) ses_get_power_status() for 'struct enclosure_component_callbacks'
> 2) get_component_power_status() for the sysfs device attribute
> (these are available for kernel-space and user-space, respectively.)
>
> However, despite both methods being available to get power status
> on demand, that commit also introduced a call to get power status
> in ses_enclosure_data_process().
>
> This dramatically increased the total probe time for SCSI devices
> on larger configurations, because ses_enclosure_data_process() is
> called several times during the SCSI devices probe and loops over
> the component devices (but that is another problem, another patch).
>
> That results in a tremendous continuous hammering of SCSI Receive
> Diagnostics commands to the enclosure-services device, which does
> delay the total probe time for the SCSI devices __significantly__:
>
> Originally, ~34 minutes on a system attached to ~170 disks:
>
> [ 9214.490703] mpt3sas version 13.100.00.00 loaded
> ...
> [11256.580231] scsi 17:0:177:0: qdepth(16), tagged(1), simple(0),
> ordered(0), scsi_level(6), cmd_que(1)
>
> With this patch, it decreased to ~2.5 minutes -- a 13.6x faster
>
> [ 1002.992533] mpt3sas version 13.100.00.00 loaded
> ...
> [ 1151.978831] scsi 11:0:177:0: qdepth(16), tagged(1), simple(0),
> ordered(0), scsi_level(6), cmd_que(1)
>
> Back to the commit discussion.. on the ses_get_power_status() call
> introduced in ses_enclosure_data_process(): impact of removing it.
>
> That may possibly be in place to initialize the power status value
> on device probe. However, those 2 functions available to retrieve
> that value _do_ automatically refresh/update it. So the potential
> benefit would be a direct access of the 'power_status' field which
> does not use the callbacks...
>
> But the only reader of 'struct enclosure_component::power_status'
> is the get_component_power_status() callback for sysfs attribute,
> and it _does_ check for and call the .get_power_status callback,
> (which indeed is defined and implemented by that commit), so the
> power status value is, again, automatically updated.
>
> So, the remaining potential for a direct/non-callback access to
> the power_status attribute would be out-of-tree modules -- well,
> for those, if they are for whatever reason interested in values
> that are set during device probe and not up-to-date by the time
> they need it.. well, that would be curious.
>
> Well, to handle that more properly, set the initial power state
> value to '-1' (i.e., uninitialized) instead of '1' (power 'on'),
> and check for it in that callback which may do an direct access
> to the field value _if_ a callback function is not defined.
>
> Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
> Fixes: 08024885a2a3 ("ses: Add power_status to SES device slot")
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] scsi: ses: don't get power status of SES device slot on probe
2017-04-05 15:18 [PATCH v2] scsi: ses: don't get power status of SES device slot on probe Mauricio Faria de Oliveira
2017-04-05 15:32 ` Dan Williams
@ 2017-04-05 16:23 ` Song Liu
2017-04-05 17:22 ` Mauricio Faria de Oliveira
2017-04-06 16:49 ` Martin K. Petersen
2 siblings, 1 reply; 5+ messages in thread
From: Song Liu @ 2017-04-05 16:23 UTC (permalink / raw)
To: Mauricio Faria de Oliveira
Cc: linux-scsi, martin.petersen, dan.j.williams, jejb, Jens Axboe,
hare, hch, gpiccoli
> On Apr 5, 2017, at 8:18 AM, Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com> wrote:
>
> The commit 08024885a2a3 ("ses: Add power_status to SES device slot")
> introduced the 'power_status' attribute to enclosure components and
> the associated callbacks.
>
> There are 2 callbacks available to get the power status of a device:
> 1) ses_get_power_status() for 'struct enclosure_component_callbacks'
> 2) get_component_power_status() for the sysfs device attribute
> (these are available for kernel-space and user-space, respectively.)
>
> However, despite both methods being available to get power status
> on demand, that commit also introduced a call to get power status
> in ses_enclosure_data_process().
>
> This dramatically increased the total probe time for SCSI devices
> on larger configurations, because ses_enclosure_data_process() is
> called several times during the SCSI devices probe and loops over
> the component devices (but that is another problem, another patch).
>
> That results in a tremendous continuous hammering of SCSI Receive
> Diagnostics commands to the enclosure-services device, which does
> delay the total probe time for the SCSI devices __significantly__:
>
> Originally, ~34 minutes on a system attached to ~170 disks:
>
> [ 9214.490703] mpt3sas version 13.100.00.00 loaded
> ...
> [11256.580231] scsi 17:0:177:0: qdepth(16), tagged(1), simple(0),
> ordered(0), scsi_level(6), cmd_que(1)
>
> With this patch, it decreased to ~2.5 minutes -- a 13.6x faster
>
> [ 1002.992533] mpt3sas version 13.100.00.00 loaded
> ...
> [ 1151.978831] scsi 11:0:177:0: qdepth(16), tagged(1), simple(0),
> ordered(0), scsi_level(6), cmd_que(1)
>
> Back to the commit discussion.. on the ses_get_power_status() call
> introduced in ses_enclosure_data_process(): impact of removing it.
>
> That may possibly be in place to initialize the power status value
> on device probe. However, those 2 functions available to retrieve
> that value _do_ automatically refresh/update it. So the potential
> benefit would be a direct access of the 'power_status' field which
> does not use the callbacks...
>
> But the only reader of 'struct enclosure_component::power_status'
> is the get_component_power_status() callback for sysfs attribute,
> and it _does_ check for and call the .get_power_status callback,
> (which indeed is defined and implemented by that commit), so the
> power status value is, again, automatically updated.
>
> So, the remaining potential for a direct/non-callback access to
> the power_status attribute would be out-of-tree modules -- well,
> for those, if they are for whatever reason interested in values
> that are set during device probe and not up-to-date by the time
> they need it.. well, that would be curious.
>
> Well, to handle that more properly, set the initial power state
> value to '-1' (i.e., uninitialized) instead of '1' (power 'on'),
> and check for it in that callback which may do an direct access
> to the field value _if_ a callback function is not defined.
>
> Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
> Fixes: 08024885a2a3 ("ses: Add power_status to SES device slot")
> ---
>
> v2:
> - remove module parameter.
> - better return values for -1/uninitalized power_status.
> (thanks: Dan Williams <dan.j.williams@intel.com>)
>
> drivers/misc/enclosure.c | 7 ++++++-
> drivers/scsi/ses.c | 1 -
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index 65fed7146e9b..d3fe3ea902d4 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -148,7 +148,7 @@ struct enclosure_device *
> for (i = 0; i < components; i++) {
> edev->component[i].number = -1;
> edev->component[i].slot = -1;
> - edev->component[i].power_status = 1;
> + edev->component[i].power_status = -1;
> }
>
> mutex_lock(&container_list_lock);
> @@ -594,6 +594,11 @@ static ssize_t get_component_power_status(struct device *cdev,
>
> if (edev->cb->get_power_status)
> edev->cb->get_power_status(edev, ecomp);
> +
> + /* If still uninitialized, the callback failed or does not exist. */
> + if (ecomp->power_status == -1)
> + return (edev->cb->get_power_status) ? -EIO : -ENOTTY;
> +
> return snprintf(buf, 40, "%s\n", ecomp->power_status ? "on" : "off");
> }
>
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index 50adabbb5808..f1cdf32d7514 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -548,7 +548,6 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
> ecomp = &edev->component[components++];
>
> if (!IS_ERR(ecomp)) {
> - ses_get_power_status(edev, ecomp);
> if (addl_desc_ptr)
> ses_process_descriptor(
> ecomp,
> --
> 1.8.3.1
>
Reviewed-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] scsi: ses: don't get power status of SES device slot on probe
2017-04-05 16:23 ` Song Liu
@ 2017-04-05 17:22 ` Mauricio Faria de Oliveira
0 siblings, 0 replies; 5+ messages in thread
From: Mauricio Faria de Oliveira @ 2017-04-05 17:22 UTC (permalink / raw)
To: Song Liu
Cc: linux-scsi, martin.petersen, dan.j.williams, jejb, Jens Axboe,
hare, hch, gpiccoli
On 04/05/2017 01:23 PM, Song Liu wrote:
> Reviewed-by: Song Liu <songliubraving@fb.com>
Thanks for reviewing, Song Liu.
It's good to know this patch doesn't break anything for you.
cheers,
--
Mauricio Faria de Oliveira
IBM Linux Technology Center
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] scsi: ses: don't get power status of SES device slot on probe
2017-04-05 15:18 [PATCH v2] scsi: ses: don't get power status of SES device slot on probe Mauricio Faria de Oliveira
2017-04-05 15:32 ` Dan Williams
2017-04-05 16:23 ` Song Liu
@ 2017-04-06 16:49 ` Martin K. Petersen
2 siblings, 0 replies; 5+ messages in thread
From: Martin K. Petersen @ 2017-04-06 16:49 UTC (permalink / raw)
To: Mauricio Faria de Oliveira
Cc: linux-scsi, martin.petersen, songliubraving, dan.j.williams,
jejb, axboe, hare, hch, gpiccoli
Mauricio,
> The commit 08024885a2a3 ("ses: Add power_status to SES device slot")
> introduced the 'power_status' attribute to enclosure components and
> the associated callbacks.
Applied to 4.12/scsi-queue, thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-06 16:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-05 15:18 [PATCH v2] scsi: ses: don't get power status of SES device slot on probe Mauricio Faria de Oliveira
2017-04-05 15:32 ` Dan Williams
2017-04-05 16:23 ` Song Liu
2017-04-05 17:22 ` Mauricio Faria de Oliveira
2017-04-06 16:49 ` Martin K. Petersen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.