Commit 540eb1eef 'scsi: libfc: fix seconds_since_last_reset calculation'
removed the use of 'struct timespec' from fc_get_host_stats(). This broke the
output of 'fcoeadm -s' after kernel 4.8-rc1 as lport->boot_time - jiffies
could become negative as in this example:
$ cat /sys/class/fc_host/host8/statistics/seconds_since_last_reset
0x10624dd2f1977b4
Take this into account so
/sys/class/fc_host/hostX/statistics/seconds_since_last_reset is sane again.
Fixes: 540eb1eef ('scsi: libfc: fix seconds_since_last_reset calculation')
Signed-off-by: Johannes Thumshirn <[email protected]>
Tested-by: Holger Schranz <[email protected]>
---
drivers/scsi/libfc/fc_lport.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index 04ce7cf..475c0a9 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -304,11 +304,15 @@ struct fc_host_statistics *fc_get_host_stats(struct Scsi_Host *shost)
unsigned int cpu;
u64 fcp_in_bytes = 0;
u64 fcp_out_bytes = 0;
+ unsigned long boot_time = lport->boot_time;
fc_stats = &lport->host_stats;
memset(fc_stats, 0, sizeof(struct fc_host_statistics));
- fc_stats->seconds_since_last_reset = (lport->boot_time - jiffies) / HZ;
+ if (boot_time > jiffies)
+ fc_stats->seconds_since_last_reset = (boot_time - jiffies) / HZ;
+ else
+ fc_stats->seconds_since_last_reset = (jiffies - boot_time) / HZ;
for_each_possible_cpu(cpu) {
struct fc_stats *stats;
--
1.8.5.6
On 11/08/16 00:45, Johannes Thumshirn wrote:
> Commit 540eb1eef 'scsi: libfc: fix seconds_since_last_reset calculation'
> removed the use of 'struct timespec' from fc_get_host_stats(). This broke the
> output of 'fcoeadm -s' after kernel 4.8-rc1 as lport->boot_time - jiffies
> could become negative as in this example:
>
> $ cat /sys/class/fc_host/host8/statistics/seconds_since_last_reset
> 0x10624dd2f1977b4
>
> Take this into account so
> /sys/class/fc_host/hostX/statistics/seconds_since_last_reset is sane again.
>
> Fixes: 540eb1eef ('scsi: libfc: fix seconds_since_last_reset calculation')
> Signed-off-by: Johannes Thumshirn <[email protected]>
> Tested-by: Holger Schranz <[email protected]>
> ---
> drivers/scsi/libfc/fc_lport.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
> index 04ce7cf..475c0a9 100644
> --- a/drivers/scsi/libfc/fc_lport.c
> +++ b/drivers/scsi/libfc/fc_lport.c
> @@ -304,11 +304,15 @@ struct fc_host_statistics *fc_get_host_stats(struct Scsi_Host *shost)
> unsigned int cpu;
> u64 fcp_in_bytes = 0;
> u64 fcp_out_bytes = 0;
> + unsigned long boot_time = lport->boot_time;
>
> fc_stats = &lport->host_stats;
> memset(fc_stats, 0, sizeof(struct fc_host_statistics));
>
> - fc_stats->seconds_since_last_reset = (lport->boot_time - jiffies) / HZ;
> + if (boot_time > jiffies)
> + fc_stats->seconds_since_last_reset = (boot_time - jiffies) / HZ;
> + else
> + fc_stats->seconds_since_last_reset = (jiffies - boot_time) / HZ;
>
> for_each_possible_cpu(cpu) {
> struct fc_stats *stats;
Hello Johannes,
I think the above code will miscalculate seconds_since_last_reset if
'jiffies' wraps around after an lport has been created and before
seconds_since_last_reset is computed. Shouldn't seconds_since_last_reset
be computed as follows?
fc_stats->seconds_since_last_reset = (jiffies - boot_time) / HZ;
Bart.
On Tue, Nov 08, 2016 at 03:04:43PM +0000, Bart Van Assche wrote:
[...]
> Hello Johannes,
>
> I think the above code will miscalculate seconds_since_last_reset if
> 'jiffies' wraps around after an lport has been created and before
> seconds_since_last_reset is computed. Shouldn't seconds_since_last_reset
> be computed as follows?
>
> fc_stats->seconds_since_last_reset = (jiffies - boot_time) / HZ;
But what happens when jiffies - boot_time becomes negative? Then we
reintroduce the bug again and have 'fcoeadm -s' show weird values.
Byte,
Johannes
--
Johannes Thumshirn Storage
[email protected] +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
On Tue, 2016-11-15 at 10:18 +0100, Johannes Thumshirn wrote:
> On Tue, Nov 08, 2016 at 03:04:43PM +0000, Bart Van Assche wrote:
> > I think the above code will miscalculate seconds_since_last_reset
> > if
> > 'jiffies' wraps around after an lport has been created and before
> > seconds_since_last_reset is computed. Shouldn't
> > seconds_since_last_reset
> > be computed as follows?
> >
> > fc_stats->seconds_since_last_reset = (jiffies - boot_time) /
> > HZ;
>
> But what happens when jiffies - boot_time becomes negative? Then we
> reintroduce the bug again and have 'fcoeadm -s' show weird values.
Hello Johannes,
If your concern is about 'jiffies' wrapping around on 32-bit systems
then you should use get_jiffies_64(). get_jiffies_64() - boot_time
can't become negative. It namely takes several million years before a
64-bit HZ counter wraps around.
Bart.
On Tue, Nov 15, 2016 at 02:50:17PM +0000, Bart Van Assche wrote:
> On Tue, 2016-11-15 at 10:18 +0100, Johannes Thumshirn wrote:
> > On Tue, Nov 08, 2016 at 03:04:43PM +0000, Bart Van Assche wrote:
> > > I think the above code will miscalculate seconds_since_last_reset
> > > if?
> > > 'jiffies' wraps around after an lport has been created and before?
> > > seconds_since_last_reset is computed. Shouldn't
> > > seconds_since_last_reset?
> > > be computed as follows?
> > >
> > > fc_stats->seconds_since_last_reset = (jiffies - boot_time) /
> > > HZ;
> >
> > But what happens when jiffies - boot_time becomes negative? Then we
> > reintroduce the bug again and have 'fcoeadm -s' show weird values.
>
> Hello Johannes,
>
> If your concern is about 'jiffies' wrapping around on 32-bit systems
> then you should use get_jiffies_64(). get_jiffies_64() - boot_time
> can't become negative. It namely takes several million years before a
> 64-bit HZ counter wraps around.
You're right. I'll respin using get_jiffies_64() and resent once it is tested.
Byte,
Johannes
--
Johannes Thumshirn Storage
[email protected] +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
On Tuesday, November 15, 2016 4:05:31 PM CET Johannes Thumshirn wrote:
> On Tue, Nov 15, 2016 at 02:50:17PM +0000, Bart Van Assche wrote:
> > On Tue, 2016-11-15 at 10:18 +0100, Johannes Thumshirn wrote:
> > > On Tue, Nov 08, 2016 at 03:04:43PM +0000, Bart Van Assche wrote:
> > > > I think the above code will miscalculate seconds_since_last_reset
> > > > if
> > > > 'jiffies' wraps around after an lport has been created and before
> > > > seconds_since_last_reset is computed. Shouldn't
> > > > seconds_since_last_reset
> > > > be computed as follows?
> > > >
> > > > fc_stats->seconds_since_last_reset = (jiffies - boot_time) /
> > > > HZ;
> > >
> > > But what happens when jiffies - boot_time becomes negative? Then we
> > > reintroduce the bug again and have 'fcoeadm -s' show weird values.
> >
> > Hello Johannes,
> >
> > If your concern is about 'jiffies' wrapping around on 32-bit systems
> > then you should use get_jiffies_64(). get_jiffies_64() - boot_time
> > can't become negative. It namely takes several million years before a
> > 64-bit HZ counter wraps around.
>
> You're right. I'll respin using get_jiffies_64() and resent once it is tested.
Sorry for the bug I introduced and for not noticing this thread earlier.
Looking at this again now, I think it's clear that the bug was simply
mixing up the left and right side of the subtraction, the simple fix
would be
diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index 2d3133f62463..fe643f2195f0 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -311,7 +311,7 @@ struct fc_host_statistics *fc_get_host_stats(struct Scsi_Host *shost)
fc_stats = &lport->host_stats;
memset(fc_stats, 0, sizeof(struct fc_host_statistics));
- fc_stats->seconds_since_last_reset = (lport->boot_time - jiffies) / HZ;
+ fc_stats->seconds_since_last_reset = (jiffies - lport->boot_time) / HZ;
for_each_possible_cpu(cpu) {
struct fc_stats *stats;
This works correctly across jiffies overflows, as long as there is at least one
reset for every jiffies overflow (49 days or more). If we can have longer times
between resets, then we could either use get_jiffies_64() or ktime_get_seconds().
The latter would only need a 32-bit variable (overflow is after 136 years).
Arnd
On Wed, Nov 16, 2016 at 05:58:14PM +0100, Arnd Bergmann wrote:
> On Tuesday, November 15, 2016 4:05:31 PM CET Johannes Thumshirn wrote:
> > On Tue, Nov 15, 2016 at 02:50:17PM +0000, Bart Van Assche wrote:
> > > On Tue, 2016-11-15 at 10:18 +0100, Johannes Thumshirn wrote:
> > > > On Tue, Nov 08, 2016 at 03:04:43PM +0000, Bart Van Assche wrote:
> > > > > I think the above code will miscalculate seconds_since_last_reset
> > > > > if
> > > > > 'jiffies' wraps around after an lport has been created and before
> > > > > seconds_since_last_reset is computed. Shouldn't
> > > > > seconds_since_last_reset
> > > > > be computed as follows?
> > > > >
> > > > > fc_stats->seconds_since_last_reset = (jiffies - boot_time) /
> > > > > HZ;
> > > >
> > > > But what happens when jiffies - boot_time becomes negative? Then we
> > > > reintroduce the bug again and have 'fcoeadm -s' show weird values.
> > >
> > > Hello Johannes,
> > >
> > > If your concern is about 'jiffies' wrapping around on 32-bit systems
> > > then you should use get_jiffies_64(). get_jiffies_64() - boot_time
> > > can't become negative. It namely takes several million years before a
> > > 64-bit HZ counter wraps around.
> >
> > You're right. I'll respin using get_jiffies_64() and resent once it is tested.
>
> Sorry for the bug I introduced and for not noticing this thread earlier.
> Looking at this again now, I think it's clear that the bug was simply
> mixing up the left and right side of the subtraction, the simple fix
> would be
>
> diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
> index 2d3133f62463..fe643f2195f0 100644
> --- a/drivers/scsi/libfc/fc_lport.c
> +++ b/drivers/scsi/libfc/fc_lport.c
> @@ -311,7 +311,7 @@ struct fc_host_statistics *fc_get_host_stats(struct Scsi_Host *shost)
> fc_stats = &lport->host_stats;
> memset(fc_stats, 0, sizeof(struct fc_host_statistics));
>
> - fc_stats->seconds_since_last_reset = (lport->boot_time - jiffies) / HZ;
> + fc_stats->seconds_since_last_reset = (jiffies - lport->boot_time) / HZ;
>
> for_each_possible_cpu(cpu) {
> struct fc_stats *stats;
>
> This works correctly across jiffies overflows, as long as there is at least one
> reset for every jiffies overflow (49 days or more). If we can have longer times
> between resets, then we could either use get_jiffies_64() or ktime_get_seconds().
Yes I was going to resend this today, but I'm trapped in the s390 pit...
Johannes
--
Johannes Thumshirn Storage
[email protected] +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850