[Cluster-devel] [PATCH] gfs2: Make statistics unsigned, suitable for use with do_div()

[Cluster-devel] [PATCH] gfs2: Make statistics unsigned, suitable for use with do_div()

[Ben Hutchings]

None of these statistics can meaningfully be negative, and the
numerator for do_div() must have the type u64.  The generic
implementation of do_div() used on some 32-bit architectures asserts
that, resulting in a compiler error in gfs2_rgrp_congested().

Fixes: 0166b197c2ed ("GFS2: Average in only non-zero round-trip times ...")
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 fs/gfs2/glock.c      | 22 +++++++++++-----------
 fs/gfs2/incore.h     |  2 +-
 fs/gfs2/rgrp.c       |  8 ++++----
 fs/gfs2/trace_gfs2.h | 16 ++++++++--------
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index a38e38f..1b6aebe 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1733,17 +1733,17 @@ static int gfs2_glstats_seq_show(struct seq_file *seq, void *iter_ptr)
 {
 	struct gfs2_glock *gl = iter_ptr;
 
-	seq_printf(seq, "G: n:%u/%llx rtt:%lld/%lld rttb:%lld/%lld irt:%lld/%lld dcnt: %lld qcnt: %lld\n",
+	seq_printf(seq, "G: n:%u/%llx rtt:%llu/%llu rttb:%llu/%llu irt:%llu/%llu dcnt: %llu qcnt: %llu\n",
 		   gl->gl_name.ln_type,
 		   (unsigned long long)gl->gl_name.ln_number,
-		   (long long)gl->gl_stats.stats[GFS2_LKS_SRTT],
-		   (long long)gl->gl_stats.stats[GFS2_LKS_SRTTVAR],
-		   (long long)gl->gl_stats.stats[GFS2_LKS_SRTTB],
-		   (long long)gl->gl_stats.stats[GFS2_LKS_SRTTVARB],
-		   (long long)gl->gl_stats.stats[GFS2_LKS_SIRT],
-		   (long long)gl->gl_stats.stats[GFS2_LKS_SIRTVAR],
-		   (long long)gl->gl_stats.stats[GFS2_LKS_DCOUNT],
-		   (long long)gl->gl_stats.stats[GFS2_LKS_QCOUNT]);
+		   (unsigned long long)gl->gl_stats.stats[GFS2_LKS_SRTT],
+		   (unsigned long long)gl->gl_stats.stats[GFS2_LKS_SRTTVAR],
+		   (unsigned long long)gl->gl_stats.stats[GFS2_LKS_SRTTB],
+		   (unsigned long long)gl->gl_stats.stats[GFS2_LKS_SRTTVARB],
+		   (unsigned long long)gl->gl_stats.stats[GFS2_LKS_SIRT],
+		   (unsigned long long)gl->gl_stats.stats[GFS2_LKS_SIRTVAR],
+		   (unsigned long long)gl->gl_stats.stats[GFS2_LKS_DCOUNT],
+		   (unsigned long long)gl->gl_stats.stats[GFS2_LKS_QCOUNT]);
 	return 0;
 }
 
@@ -1780,7 +1780,7 @@ static int gfs2_sbstats_seq_show(struct seq_file *seq, void *iter_ptr)
 	struct gfs2_sbd *sdp = gi->sdp;
 	unsigned index = gi->hash >> 3;
 	unsigned subindex = gi->hash & 0x07;
-	s64 value;
+	u64 value;
 	int i;
 
 	if (index == 0 && subindex != 0)
@@ -1796,7 +1796,7 @@ static int gfs2_sbstats_seq_show(struct seq_file *seq, void *iter_ptr)
 		} else {
 			value = lkstats->lkstats[index - 1].stats[subindex];
 		}
-		seq_printf(seq, " %15lld", (long long)value);
+		seq_printf(seq, " %15llu", (long long)value);
 	}
 	seq_putc(seq, '\n');
 	return 0;
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index a1ec7c2..7647e31 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -241,7 +241,7 @@ enum {
 };
 
 struct gfs2_lkstats {
-	s64 stats[GFS2_NR_LKSTATS];
+	u64 stats[GFS2_NR_LKSTATS];
 };
 
 enum {
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index c6c6232..2884f6f 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1862,11 +1862,11 @@ static bool gfs2_rgrp_congested(const struct gfs2_rgrpd *rgd, int loops)
 	const struct gfs2_glock *gl = rgd->rd_gl;
 	const struct gfs2_sbd *sdp = gl->gl_sbd;
 	struct gfs2_lkstats *st;
-	s64 r_dcount, l_dcount;
-	s64 l_srttb, a_srttb = 0;
+	u64 r_dcount, l_dcount;
+	u64 l_srttb, a_srttb = 0;
 	s64 srttb_diff;
-	s64 sqr_diff;
-	s64 var;
+	u64 sqr_diff;
+	u64 var;
 	int cpu, nonzero = 0;
 
 	preempt_disable();
diff --git a/fs/gfs2/trace_gfs2.h b/fs/gfs2/trace_gfs2.h
index 20c007d..03a7049 100644
--- a/fs/gfs2/trace_gfs2.h
+++ b/fs/gfs2/trace_gfs2.h
@@ -267,14 +267,14 @@ TRACE_EVENT(gfs2_glock_lock_time,
 		__field(	int,	status		)
 		__field(	char,	flags		)
 		__field(	s64,	tdiff		)
-		__field(	s64,	srtt		)
-		__field(	s64,	srttvar		)
-		__field(	s64,	srttb		)
-		__field(	s64,	srttvarb	)
-		__field(	s64,	sirt		)
-		__field(	s64,	sirtvar		)
-		__field(	s64,	dcount		)
-		__field(	s64,	qcount		)
+		__field(	u64,	srtt		)
+		__field(	u64,	srttvar		)
+		__field(	u64,	srttb		)
+		__field(	u64,	srttvarb	)
+		__field(	u64,	sirt		)
+		__field(	u64,	sirtvar		)
+		__field(	u64,	dcount		)
+		__field(	u64,	qcount		)
 	),
 
 	TP_fast_assign(
-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20150826/df429bc6/attachment.sig>

[Cluster-devel] [PATCH] gfs2: Make statistics unsigned, suitable for use with do_div()

[Andreas Gruenbacher]

Ben,

2015-08-26 2:21 GMT+02:00 Ben Hutchings <andreas.gruenbacher@gmail.com>:
> None of these statistics can meaningfully be negative, and the
> numerator for do_div() must have the type u64.  The generic
> implementation of do_div() used on some 32-bit architectures asserts
> that, resulting in a compiler error in gfs2_rgrp_congested().

ACK. The only problem I see is when the clock runs backwards and we
end up with a garbage round-trip time; this could theoretically lead
to a negative average. (The variance cannot possible go negative
though.)

While reviewing this, I found an unrelated likely bug; posting separately.

Thanks,
Andreas

[Cluster-devel] [PATCH] gfs2: Make statistics unsigned, suitable for use with do_div()

[Andreas Gruenbacher]

Ben,

[sorry about the previous botched reply, I didn't reveive your
original message via cluster-devel for some reason and bouncing it
through attachment / mutt into gmail somehow messed up your address.]

2015-08-26 2:21 GMT+02:00 Ben Hutchings <andreas.gruenbacher@gmail.com>:
> None of these statistics can meaningfully be negative, and the
> numerator for do_div() must have the type u64.  The generic
> implementation of do_div() used on some 32-bit architectures asserts
> that, resulting in a compiler error in gfs2_rgrp_congested().

ACK. The only problem I see is when the clock runs backwards and we
end up with a garbage round-trip time; this could theoretically lead
to a negative average. (The variance cannot possible go negative
though.)

While reviewing this, I found an unrelated likely bug; posting separately.

Thanks,
Andreas

[Cluster-devel] [PATCH] gfs2: A minor "sbstats" cleanup

[Andreas Gruenbacher]

It seems cleaner to avoid the temporary value here.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/glock.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 9b6e1a0..f6f51c6 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1780,7 +1780,6 @@ static int gfs2_sbstats_seq_show(struct seq_file *seq, void *iter_ptr)
 	struct gfs2_sbd *sdp = gi->sdp;
 	unsigned index = gi->hash >> 3;
 	unsigned subindex = gi->hash & 0x07;
-	u64 value;
 	int i;
 
 	if (index == 0 && subindex != 0)
@@ -1791,12 +1790,12 @@ static int gfs2_sbstats_seq_show(struct seq_file *seq, void *iter_ptr)
 
 	for_each_possible_cpu(i) {
                 const struct gfs2_pcpu_lkstats *lkstats = per_cpu_ptr(sdp->sd_lkstats, i);
-		if (index == 0) {
-			value = i;
-		} else {
-			value = lkstats->lkstats[index - 1].stats[subindex];
-		}
-		seq_printf(seq, " %15llu", (long long)value);
+
+		if (index == 0)
+			seq_printf(seq, " %15u", i);
+		else
+			seq_printf(seq, " %15llu", (unsigned long long)lkstats->
+						lkstats[index - 1].stats[subindex]);
 	}
 	seq_putc(seq, '\n');
 	return 0;
-- 
2.4.3