* [PATCH 0/2 v2] percpu_counter: xfs requires custom compare batch size @ 2015-05-12 23:52 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-12 23:52 UTC (permalink / raw) To: xfs; +Cc: linux-kernel, peterz, tj Hi folks, This is v2 of the regression fix for the new generic per-cpu superblock counter code in XFS. The problems fixed arise from using custom batch sizes for addition and decrement exceeding the "accurate compare" bounds in percpu_counter_compare() and hence resulting in incorrect comparisons being made. This regression was introduced in 4.1-rc1 and it requires a small tweak to the percpu counter infrastructure to fix, hence the two patches. Comments welcome! -Dave. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 0/2 v2] percpu_counter: xfs requires custom compare batch size @ 2015-05-12 23:52 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-12 23:52 UTC (permalink / raw) To: xfs; +Cc: peterz, tj, linux-kernel Hi folks, This is v2 of the regression fix for the new generic per-cpu superblock counter code in XFS. The problems fixed arise from using custom batch sizes for addition and decrement exceeding the "accurate compare" bounds in percpu_counter_compare() and hence resulting in incorrect comparisons being made. This regression was introduced in 4.1-rc1 and it requires a small tweak to the percpu counter infrastructure to fix, hence the two patches. Comments welcome! -Dave. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-12 23:52 ` Dave Chinner @ 2015-05-12 23:52 ` Dave Chinner -1 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-12 23:52 UTC (permalink / raw) To: xfs; +Cc: linux-kernel, peterz, tj From: Dave Chinner <dchinner@redhat.com> XFS uses non-stanard batch sizes for avoiding frequent global counter updates on it's allocated inode counters, as they increment or decrement in batches of 64 inodes. Hence the standard percpu counter batch of 32 means that the counter is effectively a global counter. Currently Xfs uses a batch size of 128 so that it doesn't take the global lock on every single modification. However, Xfs also needs to compare accurately against zero, which means we need to use percpu_counter_compare(), and that has a hard-coded batch size of 32, and hence will spuriously fail to detect when it is supposed to use precise comparisons and hence the accounting goes wrong. Add __percpu_counter_compare() to take a custom batch size so we can use it sanely in XFS and factor percpu_counter_compare() to use it. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- include/linux/percpu_counter.h | 13 ++++++++++++- lib/percpu_counter.c | 6 +++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h index 50e5009..4c82e60 100644 --- a/include/linux/percpu_counter.h +++ b/include/linux/percpu_counter.h @@ -41,7 +41,12 @@ void percpu_counter_destroy(struct percpu_counter *fbc); void percpu_counter_set(struct percpu_counter *fbc, s64 amount); void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch); s64 __percpu_counter_sum(struct percpu_counter *fbc); -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs); +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch); + +static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +{ + return __percpu_counter_compare(fbc, rhs, percpu_counter_batch); +} static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) return 0; } +static inline int +percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) +{ + return percpu_counter_compare(fbc, rhs); +} + static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c index 48144cd..f051d69 100644 --- a/lib/percpu_counter.c +++ b/lib/percpu_counter.c @@ -197,13 +197,13 @@ static int percpu_counter_hotcpu_callback(struct notifier_block *nb, * Compare counter against given value. * Return 1 if greater, 0 if equal and -1 if less */ -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) { s64 count; count = percpu_counter_read(fbc); /* Check to see if rough count will be sufficient for comparison */ - if (abs(count - rhs) > (percpu_counter_batch*num_online_cpus())) { + if (abs(count - rhs) > (batch * num_online_cpus())) { if (count > rhs) return 1; else @@ -218,7 +218,7 @@ int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) else return 0; } -EXPORT_SYMBOL(percpu_counter_compare); +EXPORT_SYMBOL(__percpu_counter_compare); static int __init percpu_counter_startup(void) { -- 2.0.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-12 23:52 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-12 23:52 UTC (permalink / raw) To: xfs; +Cc: peterz, tj, linux-kernel From: Dave Chinner <dchinner@redhat.com> XFS uses non-stanard batch sizes for avoiding frequent global counter updates on it's allocated inode counters, as they increment or decrement in batches of 64 inodes. Hence the standard percpu counter batch of 32 means that the counter is effectively a global counter. Currently Xfs uses a batch size of 128 so that it doesn't take the global lock on every single modification. However, Xfs also needs to compare accurately against zero, which means we need to use percpu_counter_compare(), and that has a hard-coded batch size of 32, and hence will spuriously fail to detect when it is supposed to use precise comparisons and hence the accounting goes wrong. Add __percpu_counter_compare() to take a custom batch size so we can use it sanely in XFS and factor percpu_counter_compare() to use it. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- include/linux/percpu_counter.h | 13 ++++++++++++- lib/percpu_counter.c | 6 +++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h index 50e5009..4c82e60 100644 --- a/include/linux/percpu_counter.h +++ b/include/linux/percpu_counter.h @@ -41,7 +41,12 @@ void percpu_counter_destroy(struct percpu_counter *fbc); void percpu_counter_set(struct percpu_counter *fbc, s64 amount); void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch); s64 __percpu_counter_sum(struct percpu_counter *fbc); -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs); +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch); + +static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +{ + return __percpu_counter_compare(fbc, rhs, percpu_counter_batch); +} static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) return 0; } +static inline int +percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) +{ + return percpu_counter_compare(fbc, rhs); +} + static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c index 48144cd..f051d69 100644 --- a/lib/percpu_counter.c +++ b/lib/percpu_counter.c @@ -197,13 +197,13 @@ static int percpu_counter_hotcpu_callback(struct notifier_block *nb, * Compare counter against given value. * Return 1 if greater, 0 if equal and -1 if less */ -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) { s64 count; count = percpu_counter_read(fbc); /* Check to see if rough count will be sufficient for comparison */ - if (abs(count - rhs) > (percpu_counter_batch*num_online_cpus())) { + if (abs(count - rhs) > (batch * num_online_cpus())) { if (count > rhs) return 1; else @@ -218,7 +218,7 @@ int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) else return 0; } -EXPORT_SYMBOL(percpu_counter_compare); +EXPORT_SYMBOL(__percpu_counter_compare); static int __init percpu_counter_startup(void) { -- 2.0.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-12 23:52 ` Dave Chinner @ 2015-05-13 13:59 ` Tejun Heo -1 siblings, 0 replies; 26+ messages in thread From: Tejun Heo @ 2015-05-13 13:59 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs, linux-kernel, peterz Hello, Dave. On Wed, May 13, 2015 at 09:52:33AM +1000, Dave Chinner wrote: > @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) > return 0; > } > > +static inline int > +percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) > +{ > + return percpu_counter_compare(fbc, rhs); > +} I don't think this is right. Looks fine to me otherwise. Thanks. -- tejun ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-13 13:59 ` Tejun Heo 0 siblings, 0 replies; 26+ messages in thread From: Tejun Heo @ 2015-05-13 13:59 UTC (permalink / raw) To: Dave Chinner; +Cc: peterz, linux-kernel, xfs Hello, Dave. On Wed, May 13, 2015 at 09:52:33AM +1000, Dave Chinner wrote: > @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) > return 0; > } > > +static inline int > +percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) > +{ > + return percpu_counter_compare(fbc, rhs); > +} I don't think this is right. Looks fine to me otherwise. Thanks. -- tejun _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-13 13:59 ` Tejun Heo @ 2015-05-14 0:55 ` Dave Chinner -1 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-14 0:55 UTC (permalink / raw) To: Tejun Heo; +Cc: xfs, linux-kernel, peterz On Wed, May 13, 2015 at 09:59:19AM -0400, Tejun Heo wrote: > Hello, Dave. > > On Wed, May 13, 2015 at 09:52:33AM +1000, Dave Chinner wrote: > > @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) > > return 0; > > } > > > > +static inline int > > +percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) > > +{ > > + return percpu_counter_compare(fbc, rhs); > > +} > > I don't think this is right. Looks fine to me otherwise. Ah, no, it's not. My bad, stale patch. Corrected version below. Cheers, Dave. -- Dave Chinner david@fromorbit.com percpu_counter: batch size aware __percpu_counter_compare() From: Dave Chinner <dchinner@redhat.com> XFS uses non-stanard batch sizes for avoiding frequent global counter updates on it's allocated inode counters, as they increment or decrement in batches of 64 inodes. Hence the standard percpu counter batch of 32 means that the counter is effectively a global counter. Currently Xfs uses a batch size of 128 so that it doesn't take the global lock on every single modification. However, Xfs also needs to compare accurately against zero, which means we need to use percpu_counter_compare(), and that has a hard-coded batch size of 32, and hence will spuriously fail to detect when it is supposed to use precise comparisons and hence the accounting goes wrong. Add __percpu_counter_compare() to take a custom batch size so we can use it sanely in XFS and factor percpu_counter_compare() to use it. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- include/linux/percpu_counter.h | 13 ++++++++++++- lib/percpu_counter.c | 6 +++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h index 50e5009..84a1094 100644 --- a/include/linux/percpu_counter.h +++ b/include/linux/percpu_counter.h @@ -41,7 +41,12 @@ void percpu_counter_destroy(struct percpu_counter *fbc); void percpu_counter_set(struct percpu_counter *fbc, s64 amount); void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch); s64 __percpu_counter_sum(struct percpu_counter *fbc); -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs); +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch); + +static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +{ + return __percpu_counter_compare(fbc, rhs, percpu_counter_batch); +} static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) return 0; } +static inline int +__percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) +{ + return percpu_counter_compare(fbc, rhs); +} + static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c index 48144cd..f051d69 100644 --- a/lib/percpu_counter.c +++ b/lib/percpu_counter.c @@ -197,13 +197,13 @@ static int percpu_counter_hotcpu_callback(struct notifier_block *nb, * Compare counter against given value. * Return 1 if greater, 0 if equal and -1 if less */ -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) { s64 count; count = percpu_counter_read(fbc); /* Check to see if rough count will be sufficient for comparison */ - if (abs(count - rhs) > (percpu_counter_batch*num_online_cpus())) { + if (abs(count - rhs) > (batch * num_online_cpus())) { if (count > rhs) return 1; else @@ -218,7 +218,7 @@ int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) else return 0; } -EXPORT_SYMBOL(percpu_counter_compare); +EXPORT_SYMBOL(__percpu_counter_compare); static int __init percpu_counter_startup(void) { ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-14 0:55 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-14 0:55 UTC (permalink / raw) To: Tejun Heo; +Cc: peterz, linux-kernel, xfs On Wed, May 13, 2015 at 09:59:19AM -0400, Tejun Heo wrote: > Hello, Dave. > > On Wed, May 13, 2015 at 09:52:33AM +1000, Dave Chinner wrote: > > @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) > > return 0; > > } > > > > +static inline int > > +percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) > > +{ > > + return percpu_counter_compare(fbc, rhs); > > +} > > I don't think this is right. Looks fine to me otherwise. Ah, no, it's not. My bad, stale patch. Corrected version below. Cheers, Dave. -- Dave Chinner david@fromorbit.com percpu_counter: batch size aware __percpu_counter_compare() From: Dave Chinner <dchinner@redhat.com> XFS uses non-stanard batch sizes for avoiding frequent global counter updates on it's allocated inode counters, as they increment or decrement in batches of 64 inodes. Hence the standard percpu counter batch of 32 means that the counter is effectively a global counter. Currently Xfs uses a batch size of 128 so that it doesn't take the global lock on every single modification. However, Xfs also needs to compare accurately against zero, which means we need to use percpu_counter_compare(), and that has a hard-coded batch size of 32, and hence will spuriously fail to detect when it is supposed to use precise comparisons and hence the accounting goes wrong. Add __percpu_counter_compare() to take a custom batch size so we can use it sanely in XFS and factor percpu_counter_compare() to use it. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- include/linux/percpu_counter.h | 13 ++++++++++++- lib/percpu_counter.c | 6 +++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h index 50e5009..84a1094 100644 --- a/include/linux/percpu_counter.h +++ b/include/linux/percpu_counter.h @@ -41,7 +41,12 @@ void percpu_counter_destroy(struct percpu_counter *fbc); void percpu_counter_set(struct percpu_counter *fbc, s64 amount); void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch); s64 __percpu_counter_sum(struct percpu_counter *fbc); -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs); +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch); + +static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +{ + return __percpu_counter_compare(fbc, rhs, percpu_counter_batch); +} static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) return 0; } +static inline int +__percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) +{ + return percpu_counter_compare(fbc, rhs); +} + static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c index 48144cd..f051d69 100644 --- a/lib/percpu_counter.c +++ b/lib/percpu_counter.c @@ -197,13 +197,13 @@ static int percpu_counter_hotcpu_callback(struct notifier_block *nb, * Compare counter against given value. * Return 1 if greater, 0 if equal and -1 if less */ -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) { s64 count; count = percpu_counter_read(fbc); /* Check to see if rough count will be sufficient for comparison */ - if (abs(count - rhs) > (percpu_counter_batch*num_online_cpus())) { + if (abs(count - rhs) > (batch * num_online_cpus())) { if (count > rhs) return 1; else @@ -218,7 +218,7 @@ int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) else return 0; } -EXPORT_SYMBOL(percpu_counter_compare); +EXPORT_SYMBOL(__percpu_counter_compare); static int __init percpu_counter_startup(void) { _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-14 0:55 ` Dave Chinner @ 2015-05-14 15:02 ` Tejun Heo -1 siblings, 0 replies; 26+ messages in thread From: Tejun Heo @ 2015-05-14 15:02 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs, linux-kernel, peterz On Thu, May 14, 2015 at 10:55:53AM +1000, Dave Chinner wrote: > percpu_counter: batch size aware __percpu_counter_compare() > > From: Dave Chinner <dchinner@redhat.com> > > XFS uses non-stanard batch sizes for avoiding frequent global > counter updates on it's allocated inode counters, as they increment > or decrement in batches of 64 inodes. Hence the standard percpu > counter batch of 32 means that the counter is effectively a global > counter. Currently Xfs uses a batch size of 128 so that it doesn't > take the global lock on every single modification. > > However, Xfs also needs to compare accurately against zero, which > means we need to use percpu_counter_compare(), and that has a > hard-coded batch size of 32, and hence will spuriously fail to > detect when it is supposed to use precise comparisons and hence > the accounting goes wrong. > > Add __percpu_counter_compare() to take a custom batch size so we can > use it sanely in XFS and factor percpu_counter_compare() to use it. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Please feel free to route the patch however you see fit. Thanks. -- tejun ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-14 15:02 ` Tejun Heo 0 siblings, 0 replies; 26+ messages in thread From: Tejun Heo @ 2015-05-14 15:02 UTC (permalink / raw) To: Dave Chinner; +Cc: peterz, linux-kernel, xfs On Thu, May 14, 2015 at 10:55:53AM +1000, Dave Chinner wrote: > percpu_counter: batch size aware __percpu_counter_compare() > > From: Dave Chinner <dchinner@redhat.com> > > XFS uses non-stanard batch sizes for avoiding frequent global > counter updates on it's allocated inode counters, as they increment > or decrement in batches of 64 inodes. Hence the standard percpu > counter batch of 32 means that the counter is effectively a global > counter. Currently Xfs uses a batch size of 128 so that it doesn't > take the global lock on every single modification. > > However, Xfs also needs to compare accurately against zero, which > means we need to use percpu_counter_compare(), and that has a > hard-coded batch size of 32, and hence will spuriously fail to > detect when it is supposed to use precise comparisons and hence > the accounting goes wrong. > > Add __percpu_counter_compare() to take a custom batch size so we can > use it sanely in XFS and factor percpu_counter_compare() to use it. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Please feel free to route the patch however you see fit. Thanks. -- tejun _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/2] xfs: inode and free block counters need to use __percpu_counter_compare 2015-05-12 23:52 ` Dave Chinner @ 2015-05-12 23:52 ` Dave Chinner -1 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-12 23:52 UTC (permalink / raw) To: xfs; +Cc: linux-kernel, peterz, tj From: Dave Chinner <dchinner@redhat.com> Because the counters use a custom batch size, the comparison functions need to be aware of that batch size otherwise the comparison does not work correctly. This leads to ASSERT failures on generic/027 like this: XFS: Assertion failed: 0, file: fs/xfs/xfs_mount.c, line: 1099 ------------[ cut here ]------------ .... Call Trace: [<ffffffff81522a39>] xfs_mod_icount+0x99/0xc0 [<ffffffff815285cb>] xfs_trans_unreserve_and_mod_sb+0x28b/0x5b0 [<ffffffff8152f941>] xfs_log_commit_cil+0x321/0x580 [<ffffffff81528e17>] xfs_trans_commit+0xb7/0x260 [<ffffffff81503d4d>] xfs_bmap_finish+0xcd/0x1b0 [<ffffffff8151da41>] xfs_inactive_ifree+0x1e1/0x250 [<ffffffff8151dbe0>] xfs_inactive+0x130/0x200 [<ffffffff81523a21>] xfs_fs_evict_inode+0x91/0xf0 [<ffffffff811f3958>] evict+0xb8/0x190 [<ffffffff811f433b>] iput+0x18b/0x1f0 [<ffffffff811e8853>] do_unlinkat+0x1f3/0x320 [<ffffffff811d548a>] ? filp_close+0x5a/0x80 [<ffffffff811e999b>] SyS_unlinkat+0x1b/0x40 [<ffffffff81e0892e>] system_call_fastpath+0x12/0x71 This is a regression introduced by commit 501ab32 ("xfs: use generic percpu counters for inode counter"). This patch fixes the same problem for both the inode counter and the free block counter in the superblocks. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_mount.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 02f827f..461e791 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -1100,14 +1100,18 @@ xfs_log_sbcount(xfs_mount_t *mp) return xfs_sync_sb(mp, true); } +/* + * Deltas for the inode count are +/-64, hence we use a large batch size + * of 128 so we don't need to take the counter lock on every update. + */ +#define XFS_ICOUNT_BATCH 128 int xfs_mod_icount( struct xfs_mount *mp, int64_t delta) { - /* deltas are +/-64, hence the large batch size of 128. */ - __percpu_counter_add(&mp->m_icount, delta, 128); - if (percpu_counter_compare(&mp->m_icount, 0) < 0) { + __percpu_counter_add(&mp->m_icount, delta, XFS_ICOUNT_BATCH); + if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) { ASSERT(0); percpu_counter_add(&mp->m_icount, -delta); return -EINVAL; @@ -1129,6 +1133,14 @@ xfs_mod_ifree( return 0; } +/* + * Deltas for the block count can vary from 1 to very large, but lock contention + * only occurs on frequent small block count updates such as in the delayed + * allocation path for buffered writes (page a time updates). Hence we set + * a large batch count (1024) to minimise global counter updates except when + * we get near to ENOSPC and we have to be very accurate with our updates. + */ +#define XFS_FDBLOCKS_BATCH 1024 int xfs_mod_fdblocks( struct xfs_mount *mp, @@ -1167,25 +1179,19 @@ xfs_mod_fdblocks( * Taking blocks away, need to be more accurate the closer we * are to zero. * - * batch size is set to a maximum of 1024 blocks - if we are - * allocating of freeing extents larger than this then we aren't - * going to be hammering the counter lock so a lock per update - * is not a problem. - * * If the counter has a value of less than 2 * max batch size, * then make everything serialise as we are real close to * ENOSPC. */ -#define __BATCH 1024 - if (percpu_counter_compare(&mp->m_fdblocks, 2 * __BATCH) < 0) + if (__percpu_counter_compare(&mp->m_fdblocks, 2 * XFS_FDBLOCKS_BATCH, + XFS_FDBLOCKS_BATCH) < 0) batch = 1; else - batch = __BATCH; -#undef __BATCH + batch = XFS_FDBLOCKS_BATCH; __percpu_counter_add(&mp->m_fdblocks, delta, batch); - if (percpu_counter_compare(&mp->m_fdblocks, - XFS_ALLOC_SET_ASIDE(mp)) >= 0) { + if (__percpu_counter_compare(&mp->m_fdblocks, XFS_ALLOC_SET_ASIDE(mp), + XFS_FDBLOCKS_BATCH) >= 0) { /* we had space! */ return 0; } -- 2.0.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/2] xfs: inode and free block counters need to use __percpu_counter_compare @ 2015-05-12 23:52 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-12 23:52 UTC (permalink / raw) To: xfs; +Cc: peterz, tj, linux-kernel From: Dave Chinner <dchinner@redhat.com> Because the counters use a custom batch size, the comparison functions need to be aware of that batch size otherwise the comparison does not work correctly. This leads to ASSERT failures on generic/027 like this: XFS: Assertion failed: 0, file: fs/xfs/xfs_mount.c, line: 1099 ------------[ cut here ]------------ .... Call Trace: [<ffffffff81522a39>] xfs_mod_icount+0x99/0xc0 [<ffffffff815285cb>] xfs_trans_unreserve_and_mod_sb+0x28b/0x5b0 [<ffffffff8152f941>] xfs_log_commit_cil+0x321/0x580 [<ffffffff81528e17>] xfs_trans_commit+0xb7/0x260 [<ffffffff81503d4d>] xfs_bmap_finish+0xcd/0x1b0 [<ffffffff8151da41>] xfs_inactive_ifree+0x1e1/0x250 [<ffffffff8151dbe0>] xfs_inactive+0x130/0x200 [<ffffffff81523a21>] xfs_fs_evict_inode+0x91/0xf0 [<ffffffff811f3958>] evict+0xb8/0x190 [<ffffffff811f433b>] iput+0x18b/0x1f0 [<ffffffff811e8853>] do_unlinkat+0x1f3/0x320 [<ffffffff811d548a>] ? filp_close+0x5a/0x80 [<ffffffff811e999b>] SyS_unlinkat+0x1b/0x40 [<ffffffff81e0892e>] system_call_fastpath+0x12/0x71 This is a regression introduced by commit 501ab32 ("xfs: use generic percpu counters for inode counter"). This patch fixes the same problem for both the inode counter and the free block counter in the superblocks. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_mount.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 02f827f..461e791 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -1100,14 +1100,18 @@ xfs_log_sbcount(xfs_mount_t *mp) return xfs_sync_sb(mp, true); } +/* + * Deltas for the inode count are +/-64, hence we use a large batch size + * of 128 so we don't need to take the counter lock on every update. + */ +#define XFS_ICOUNT_BATCH 128 int xfs_mod_icount( struct xfs_mount *mp, int64_t delta) { - /* deltas are +/-64, hence the large batch size of 128. */ - __percpu_counter_add(&mp->m_icount, delta, 128); - if (percpu_counter_compare(&mp->m_icount, 0) < 0) { + __percpu_counter_add(&mp->m_icount, delta, XFS_ICOUNT_BATCH); + if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) { ASSERT(0); percpu_counter_add(&mp->m_icount, -delta); return -EINVAL; @@ -1129,6 +1133,14 @@ xfs_mod_ifree( return 0; } +/* + * Deltas for the block count can vary from 1 to very large, but lock contention + * only occurs on frequent small block count updates such as in the delayed + * allocation path for buffered writes (page a time updates). Hence we set + * a large batch count (1024) to minimise global counter updates except when + * we get near to ENOSPC and we have to be very accurate with our updates. + */ +#define XFS_FDBLOCKS_BATCH 1024 int xfs_mod_fdblocks( struct xfs_mount *mp, @@ -1167,25 +1179,19 @@ xfs_mod_fdblocks( * Taking blocks away, need to be more accurate the closer we * are to zero. * - * batch size is set to a maximum of 1024 blocks - if we are - * allocating of freeing extents larger than this then we aren't - * going to be hammering the counter lock so a lock per update - * is not a problem. - * * If the counter has a value of less than 2 * max batch size, * then make everything serialise as we are real close to * ENOSPC. */ -#define __BATCH 1024 - if (percpu_counter_compare(&mp->m_fdblocks, 2 * __BATCH) < 0) + if (__percpu_counter_compare(&mp->m_fdblocks, 2 * XFS_FDBLOCKS_BATCH, + XFS_FDBLOCKS_BATCH) < 0) batch = 1; else - batch = __BATCH; -#undef __BATCH + batch = XFS_FDBLOCKS_BATCH; __percpu_counter_add(&mp->m_fdblocks, delta, batch); - if (percpu_counter_compare(&mp->m_fdblocks, - XFS_ALLOC_SET_ASIDE(mp)) >= 0) { + if (__percpu_counter_compare(&mp->m_fdblocks, XFS_ALLOC_SET_ASIDE(mp), + XFS_FDBLOCKS_BATCH) >= 0) { /* we had space! */ return 0; } -- 2.0.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] xfs: inode and free block counters need to use __percpu_counter_compare 2015-05-12 23:52 ` Dave Chinner @ 2015-05-14 14:21 ` Brian Foster -1 siblings, 0 replies; 26+ messages in thread From: Brian Foster @ 2015-05-14 14:21 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs, peterz, tj, linux-kernel On Wed, May 13, 2015 at 09:52:34AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Because the counters use a custom batch size, the comparison > functions need to be aware of that batch size otherwise the > comparison does not work correctly. This leads to ASSERT failures > on generic/027 like this: > > XFS: Assertion failed: 0, file: fs/xfs/xfs_mount.c, line: 1099 > ------------[ cut here ]------------ > .... > Call Trace: > [<ffffffff81522a39>] xfs_mod_icount+0x99/0xc0 > [<ffffffff815285cb>] xfs_trans_unreserve_and_mod_sb+0x28b/0x5b0 > [<ffffffff8152f941>] xfs_log_commit_cil+0x321/0x580 > [<ffffffff81528e17>] xfs_trans_commit+0xb7/0x260 > [<ffffffff81503d4d>] xfs_bmap_finish+0xcd/0x1b0 > [<ffffffff8151da41>] xfs_inactive_ifree+0x1e1/0x250 > [<ffffffff8151dbe0>] xfs_inactive+0x130/0x200 > [<ffffffff81523a21>] xfs_fs_evict_inode+0x91/0xf0 > [<ffffffff811f3958>] evict+0xb8/0x190 > [<ffffffff811f433b>] iput+0x18b/0x1f0 > [<ffffffff811e8853>] do_unlinkat+0x1f3/0x320 > [<ffffffff811d548a>] ? filp_close+0x5a/0x80 > [<ffffffff811e999b>] SyS_unlinkat+0x1b/0x40 > [<ffffffff81e0892e>] system_call_fastpath+0x12/0x71 > > This is a regression introduced by commit 501ab32 ("xfs: use generic > percpu counters for inode counter"). > > This patch fixes the same problem for both the inode counter and the > free block counter in the superblocks. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_mount.c | 34 ++++++++++++++++++++-------------- > 1 file changed, 20 insertions(+), 14 deletions(-) > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 02f827f..461e791 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -1100,14 +1100,18 @@ xfs_log_sbcount(xfs_mount_t *mp) > return xfs_sync_sb(mp, true); > } > > +/* > + * Deltas for the inode count are +/-64, hence we use a large batch size > + * of 128 so we don't need to take the counter lock on every update. > + */ > +#define XFS_ICOUNT_BATCH 128 > int > xfs_mod_icount( > struct xfs_mount *mp, > int64_t delta) > { > - /* deltas are +/-64, hence the large batch size of 128. */ > - __percpu_counter_add(&mp->m_icount, delta, 128); > - if (percpu_counter_compare(&mp->m_icount, 0) < 0) { > + __percpu_counter_add(&mp->m_icount, delta, XFS_ICOUNT_BATCH); > + if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) { > ASSERT(0); > percpu_counter_add(&mp->m_icount, -delta); > return -EINVAL; > @@ -1129,6 +1133,14 @@ xfs_mod_ifree( > return 0; > } > > +/* > + * Deltas for the block count can vary from 1 to very large, but lock contention > + * only occurs on frequent small block count updates such as in the delayed > + * allocation path for buffered writes (page a time updates). Hence we set > + * a large batch count (1024) to minimise global counter updates except when > + * we get near to ENOSPC and we have to be very accurate with our updates. > + */ > +#define XFS_FDBLOCKS_BATCH 1024 > int > xfs_mod_fdblocks( > struct xfs_mount *mp, > @@ -1167,25 +1179,19 @@ xfs_mod_fdblocks( > * Taking blocks away, need to be more accurate the closer we > * are to zero. > * > - * batch size is set to a maximum of 1024 blocks - if we are > - * allocating of freeing extents larger than this then we aren't > - * going to be hammering the counter lock so a lock per update > - * is not a problem. > - * > * If the counter has a value of less than 2 * max batch size, > * then make everything serialise as we are real close to > * ENOSPC. > */ > -#define __BATCH 1024 > - if (percpu_counter_compare(&mp->m_fdblocks, 2 * __BATCH) < 0) > + if (__percpu_counter_compare(&mp->m_fdblocks, 2 * XFS_FDBLOCKS_BATCH, > + XFS_FDBLOCKS_BATCH) < 0) > batch = 1; > else > - batch = __BATCH; > -#undef __BATCH > + batch = XFS_FDBLOCKS_BATCH; > > __percpu_counter_add(&mp->m_fdblocks, delta, batch); > - if (percpu_counter_compare(&mp->m_fdblocks, > - XFS_ALLOC_SET_ASIDE(mp)) >= 0) { > + if (__percpu_counter_compare(&mp->m_fdblocks, XFS_ALLOC_SET_ASIDE(mp), > + XFS_FDBLOCKS_BATCH) >= 0) { > /* we had space! */ > return 0; > } > -- > 2.0.0 > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] xfs: inode and free block counters need to use __percpu_counter_compare @ 2015-05-14 14:21 ` Brian Foster 0 siblings, 0 replies; 26+ messages in thread From: Brian Foster @ 2015-05-14 14:21 UTC (permalink / raw) To: Dave Chinner; +Cc: peterz, tj, linux-kernel, xfs On Wed, May 13, 2015 at 09:52:34AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Because the counters use a custom batch size, the comparison > functions need to be aware of that batch size otherwise the > comparison does not work correctly. This leads to ASSERT failures > on generic/027 like this: > > XFS: Assertion failed: 0, file: fs/xfs/xfs_mount.c, line: 1099 > ------------[ cut here ]------------ > .... > Call Trace: > [<ffffffff81522a39>] xfs_mod_icount+0x99/0xc0 > [<ffffffff815285cb>] xfs_trans_unreserve_and_mod_sb+0x28b/0x5b0 > [<ffffffff8152f941>] xfs_log_commit_cil+0x321/0x580 > [<ffffffff81528e17>] xfs_trans_commit+0xb7/0x260 > [<ffffffff81503d4d>] xfs_bmap_finish+0xcd/0x1b0 > [<ffffffff8151da41>] xfs_inactive_ifree+0x1e1/0x250 > [<ffffffff8151dbe0>] xfs_inactive+0x130/0x200 > [<ffffffff81523a21>] xfs_fs_evict_inode+0x91/0xf0 > [<ffffffff811f3958>] evict+0xb8/0x190 > [<ffffffff811f433b>] iput+0x18b/0x1f0 > [<ffffffff811e8853>] do_unlinkat+0x1f3/0x320 > [<ffffffff811d548a>] ? filp_close+0x5a/0x80 > [<ffffffff811e999b>] SyS_unlinkat+0x1b/0x40 > [<ffffffff81e0892e>] system_call_fastpath+0x12/0x71 > > This is a regression introduced by commit 501ab32 ("xfs: use generic > percpu counters for inode counter"). > > This patch fixes the same problem for both the inode counter and the > free block counter in the superblocks. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_mount.c | 34 ++++++++++++++++++++-------------- > 1 file changed, 20 insertions(+), 14 deletions(-) > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 02f827f..461e791 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -1100,14 +1100,18 @@ xfs_log_sbcount(xfs_mount_t *mp) > return xfs_sync_sb(mp, true); > } > > +/* > + * Deltas for the inode count are +/-64, hence we use a large batch size > + * of 128 so we don't need to take the counter lock on every update. > + */ > +#define XFS_ICOUNT_BATCH 128 > int > xfs_mod_icount( > struct xfs_mount *mp, > int64_t delta) > { > - /* deltas are +/-64, hence the large batch size of 128. */ > - __percpu_counter_add(&mp->m_icount, delta, 128); > - if (percpu_counter_compare(&mp->m_icount, 0) < 0) { > + __percpu_counter_add(&mp->m_icount, delta, XFS_ICOUNT_BATCH); > + if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) { > ASSERT(0); > percpu_counter_add(&mp->m_icount, -delta); > return -EINVAL; > @@ -1129,6 +1133,14 @@ xfs_mod_ifree( > return 0; > } > > +/* > + * Deltas for the block count can vary from 1 to very large, but lock contention > + * only occurs on frequent small block count updates such as in the delayed > + * allocation path for buffered writes (page a time updates). Hence we set > + * a large batch count (1024) to minimise global counter updates except when > + * we get near to ENOSPC and we have to be very accurate with our updates. > + */ > +#define XFS_FDBLOCKS_BATCH 1024 > int > xfs_mod_fdblocks( > struct xfs_mount *mp, > @@ -1167,25 +1179,19 @@ xfs_mod_fdblocks( > * Taking blocks away, need to be more accurate the closer we > * are to zero. > * > - * batch size is set to a maximum of 1024 blocks - if we are > - * allocating of freeing extents larger than this then we aren't > - * going to be hammering the counter lock so a lock per update > - * is not a problem. > - * > * If the counter has a value of less than 2 * max batch size, > * then make everything serialise as we are real close to > * ENOSPC. > */ > -#define __BATCH 1024 > - if (percpu_counter_compare(&mp->m_fdblocks, 2 * __BATCH) < 0) > + if (__percpu_counter_compare(&mp->m_fdblocks, 2 * XFS_FDBLOCKS_BATCH, > + XFS_FDBLOCKS_BATCH) < 0) > batch = 1; > else > - batch = __BATCH; > -#undef __BATCH > + batch = XFS_FDBLOCKS_BATCH; > > __percpu_counter_add(&mp->m_fdblocks, delta, batch); > - if (percpu_counter_compare(&mp->m_fdblocks, > - XFS_ALLOC_SET_ASIDE(mp)) >= 0) { > + if (__percpu_counter_compare(&mp->m_fdblocks, XFS_ALLOC_SET_ASIDE(mp), > + XFS_FDBLOCKS_BATCH) >= 0) { > /* we had space! */ > return 0; > } > -- > 2.0.0 > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 0/2] xfs: fix inode count underrun @ 2015-05-05 22:01 Dave Chinner 2015-05-05 22:01 ` Dave Chinner 0 siblings, 1 reply; 26+ messages in thread From: Dave Chinner @ 2015-05-05 22:01 UTC (permalink / raw) To: xfs; +Cc: linux-kernel Hi folks, With the conversion of XFS to use the generic per-cpu superblocks, I overlooked the fact that the update batch size is important to the accuracy of the comparison function. Using different batch sizes means percpu_counter_compare() doesn't detect when it should fall back to percpu_counter_sum() for accuracy correctly, resulting in counter comparisons being inaccurate. This leads to problems with zero threshold detection in XFS. To fix, add __percpu_counter_compare() to take a caller supplied batch size. This fixes the XFS regression introduced in 4.1-rc1. -Dave. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-05 22:01 [PATCH 0/2] xfs: fix inode count underrun Dave Chinner @ 2015-05-05 22:01 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-05 22:01 UTC (permalink / raw) To: xfs; +Cc: linux-kernel From: Dave Chinner <dchinner@redhat.com> From: Dave Chinner <dchinner@redhat.com> XFS uses non-stanard batch sizes for avoiding frequent global counter updates on it's allocated inode counters, as they increment or decrement in batches of 64 inodes. Hence the standard percpu counter batch of 32 means that the counter is effectively a global counter. Currently Xfs uses a batch size of 128 so that it doesn't take the global lock on every single modification. However, Xfs also needs to compare accurately against zero, which means we need to use percpu_counter_compare(), and that has a hard-coded batch size of 32, and hence will spuriously fail to detect when it is supposed to use precise comparisons and hence the accounting goes wrong. Add __percpu_counter_compare() to take a custom batch size so we can use it sanely in XFS and factor percpu_counter_compare() to use it. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- include/linux/percpu_counter.h | 13 ++++++++++++- lib/percpu_counter.c | 6 +++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h index 50e5009..4c82e60 100644 --- a/include/linux/percpu_counter.h +++ b/include/linux/percpu_counter.h @@ -41,7 +41,12 @@ void percpu_counter_destroy(struct percpu_counter *fbc); void percpu_counter_set(struct percpu_counter *fbc, s64 amount); void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch); s64 __percpu_counter_sum(struct percpu_counter *fbc); -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs); +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch); + +static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +{ + return __percpu_counter_compare(fbc, rhs, percpu_counter_batch); +} static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) return 0; } +static inline int +percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) +{ + return percpu_counter_compare(fbc, rhs); +} + static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c index 48144cd..f051d69 100644 --- a/lib/percpu_counter.c +++ b/lib/percpu_counter.c @@ -197,13 +197,13 @@ static int percpu_counter_hotcpu_callback(struct notifier_block *nb, * Compare counter against given value. * Return 1 if greater, 0 if equal and -1 if less */ -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) { s64 count; count = percpu_counter_read(fbc); /* Check to see if rough count will be sufficient for comparison */ - if (abs(count - rhs) > (percpu_counter_batch*num_online_cpus())) { + if (abs(count - rhs) > (batch * num_online_cpus())) { if (count > rhs) return 1; else @@ -218,7 +218,7 @@ int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) else return 0; } -EXPORT_SYMBOL(percpu_counter_compare); +EXPORT_SYMBOL(__percpu_counter_compare); static int __init percpu_counter_startup(void) { -- 2.0.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-05 22:01 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-05 22:01 UTC (permalink / raw) To: xfs; +Cc: linux-kernel From: Dave Chinner <dchinner@redhat.com> From: Dave Chinner <dchinner@redhat.com> XFS uses non-stanard batch sizes for avoiding frequent global counter updates on it's allocated inode counters, as they increment or decrement in batches of 64 inodes. Hence the standard percpu counter batch of 32 means that the counter is effectively a global counter. Currently Xfs uses a batch size of 128 so that it doesn't take the global lock on every single modification. However, Xfs also needs to compare accurately against zero, which means we need to use percpu_counter_compare(), and that has a hard-coded batch size of 32, and hence will spuriously fail to detect when it is supposed to use precise comparisons and hence the accounting goes wrong. Add __percpu_counter_compare() to take a custom batch size so we can use it sanely in XFS and factor percpu_counter_compare() to use it. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- include/linux/percpu_counter.h | 13 ++++++++++++- lib/percpu_counter.c | 6 +++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h index 50e5009..4c82e60 100644 --- a/include/linux/percpu_counter.h +++ b/include/linux/percpu_counter.h @@ -41,7 +41,12 @@ void percpu_counter_destroy(struct percpu_counter *fbc); void percpu_counter_set(struct percpu_counter *fbc, s64 amount); void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch); s64 __percpu_counter_sum(struct percpu_counter *fbc); -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs); +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch); + +static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +{ + return __percpu_counter_compare(fbc, rhs, percpu_counter_batch); +} static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { @@ -116,6 +121,12 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) return 0; } +static inline int +percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) +{ + return percpu_counter_compare(fbc, rhs); +} + static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount) { diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c index 48144cd..f051d69 100644 --- a/lib/percpu_counter.c +++ b/lib/percpu_counter.c @@ -197,13 +197,13 @@ static int percpu_counter_hotcpu_callback(struct notifier_block *nb, * Compare counter against given value. * Return 1 if greater, 0 if equal and -1 if less */ -int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) +int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) { s64 count; count = percpu_counter_read(fbc); /* Check to see if rough count will be sufficient for comparison */ - if (abs(count - rhs) > (percpu_counter_batch*num_online_cpus())) { + if (abs(count - rhs) > (batch * num_online_cpus())) { if (count > rhs) return 1; else @@ -218,7 +218,7 @@ int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs) else return 0; } -EXPORT_SYMBOL(percpu_counter_compare); +EXPORT_SYMBOL(__percpu_counter_compare); static int __init percpu_counter_startup(void) { -- 2.0.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-05 22:01 ` Dave Chinner @ 2015-05-06 4:36 ` Christoph Hellwig -1 siblings, 0 replies; 26+ messages in thread From: Christoph Hellwig @ 2015-05-06 4:36 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs, linux-kernel This looks reasonable, but I miss Ccs to the authors of the percpu_counter code and linux-kernel. On Wed, May 06, 2015 at 08:01:38AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > From: Dave Chinner <dchinner@redhat.com> And this looks wrong, too :) ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-06 4:36 ` Christoph Hellwig 0 siblings, 0 replies; 26+ messages in thread From: Christoph Hellwig @ 2015-05-06 4:36 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-kernel, xfs This looks reasonable, but I miss Ccs to the authors of the percpu_counter code and linux-kernel. On Wed, May 06, 2015 at 08:01:38AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > From: Dave Chinner <dchinner@redhat.com> And this looks wrong, too :) _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-06 4:36 ` Christoph Hellwig @ 2015-05-06 4:46 ` Christoph Hellwig -1 siblings, 0 replies; 26+ messages in thread From: Christoph Hellwig @ 2015-05-06 4:46 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs, linux-kernel On Tue, May 05, 2015 at 09:36:36PM -0700, Christoph Hellwig wrote: > This looks reasonable, but I miss Ccs to the authors of the > percpu_counter code and linux-kernel. Ok, saw it on lkml. Guess I need more coffee, sorry.. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-06 4:46 ` Christoph Hellwig 0 siblings, 0 replies; 26+ messages in thread From: Christoph Hellwig @ 2015-05-06 4:46 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-kernel, xfs On Tue, May 05, 2015 at 09:36:36PM -0700, Christoph Hellwig wrote: > This looks reasonable, but I miss Ccs to the authors of the > percpu_counter code and linux-kernel. Ok, saw it on lkml. Guess I need more coffee, sorry.. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-06 4:36 ` Christoph Hellwig @ 2015-05-06 5:43 ` Dave Chinner -1 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-06 5:43 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs, linux-kernel On Tue, May 05, 2015 at 09:36:36PM -0700, Christoph Hellwig wrote: > This looks reasonable, but I miss Ccs to the authors of the > percpu_counter code and linux-kernel. linux-kernel was cc'd. > On Wed, May 06, 2015 at 08:01:38AM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > From: Dave Chinner <dchinner@redhat.com> > > And this looks wrong, too :) That looks like a change of tool behaviour. I just upgraded guilt; the old version stripped "from" lines in patch descriptions from the git commit log as goes into the author field of the commit. The new version does not appear to strip the from lines, and so when git-send-email formats it, it puts a from line in the message, and then the commit message has one too. Thanks for pointing it out. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-06 5:43 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-06 5:43 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-kernel, xfs On Tue, May 05, 2015 at 09:36:36PM -0700, Christoph Hellwig wrote: > This looks reasonable, but I miss Ccs to the authors of the > percpu_counter code and linux-kernel. linux-kernel was cc'd. > On Wed, May 06, 2015 at 08:01:38AM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > From: Dave Chinner <dchinner@redhat.com> > > And this looks wrong, too :) That looks like a change of tool behaviour. I just upgraded guilt; the old version stripped "from" lines in patch descriptions from the git commit log as goes into the author field of the commit. The new version does not appear to strip the from lines, and so when git-send-email formats it, it puts a from line in the message, and then the commit message has one too. Thanks for pointing it out. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-06 5:43 ` Dave Chinner @ 2015-05-06 5:53 ` Dave Chinner -1 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-06 5:53 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-kernel, xfs On Wed, May 06, 2015 at 03:43:10PM +1000, Dave Chinner wrote: > On Tue, May 05, 2015 at 09:36:36PM -0700, Christoph Hellwig wrote: > > This looks reasonable, but I miss Ccs to the authors of the > > percpu_counter code and linux-kernel. > > linux-kernel was cc'd. > > > On Wed, May 06, 2015 at 08:01:38AM +1000, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > From: Dave Chinner <dchinner@redhat.com> > > > > And this looks wrong, too :) > > That looks like a change of tool behaviour. I just upgraded guilt; > the old version stripped "from" lines in patch descriptions from the > git commit log as goes into the author field of the commit. The new > version does not appear to strip the from lines, and so when > git-send-email formats it, it puts a from line in the message, and > then the commit message has one too. Ok, so I remembered it the wrong way around, but it is a tool bug: $ guilt patchbomb -s d480578..cd83c77 cd83c773 xfs: inode counter needs to use __percpu_counter_compare 1e41b5ad percpu_counter: batch size aware __percpu_counter_compare() Are these what you want to send? [Y/n] .... Note the use of "-s". from the man page: -s Don't add additional repository committer sign-offs to the patch. This allows the sign-off chain to be fully expressed in the commit messages and not changed by the act of sending a patchbomb. It would appear that the "-s" option is not working properly. I'll fix that before sending the next round of patches.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-06 5:53 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-06 5:53 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-kernel, xfs On Wed, May 06, 2015 at 03:43:10PM +1000, Dave Chinner wrote: > On Tue, May 05, 2015 at 09:36:36PM -0700, Christoph Hellwig wrote: > > This looks reasonable, but I miss Ccs to the authors of the > > percpu_counter code and linux-kernel. > > linux-kernel was cc'd. > > > On Wed, May 06, 2015 at 08:01:38AM +1000, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > From: Dave Chinner <dchinner@redhat.com> > > > > And this looks wrong, too :) > > That looks like a change of tool behaviour. I just upgraded guilt; > the old version stripped "from" lines in patch descriptions from the > git commit log as goes into the author field of the commit. The new > version does not appear to strip the from lines, and so when > git-send-email formats it, it puts a from line in the message, and > then the commit message has one too. Ok, so I remembered it the wrong way around, but it is a tool bug: $ guilt patchbomb -s d480578..cd83c77 cd83c773 xfs: inode counter needs to use __percpu_counter_compare 1e41b5ad percpu_counter: batch size aware __percpu_counter_compare() Are these what you want to send? [Y/n] .... Note the use of "-s". from the man page: -s Don't add additional repository committer sign-offs to the patch. This allows the sign-off chain to be fully expressed in the commit messages and not changed by the act of sending a patchbomb. It would appear that the "-s" option is not working properly. I'll fix that before sending the next round of patches.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() 2015-05-06 5:53 ` Dave Chinner @ 2015-05-06 6:11 ` Dave Chinner -1 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-06 6:11 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-kernel, xfs On Wed, May 06, 2015 at 03:53:49PM +1000, Dave Chinner wrote: > On Wed, May 06, 2015 at 03:43:10PM +1000, Dave Chinner wrote: > > On Tue, May 05, 2015 at 09:36:36PM -0700, Christoph Hellwig wrote: > > > This looks reasonable, but I miss Ccs to the authors of the > > > percpu_counter code and linux-kernel. > > > > linux-kernel was cc'd. > > > > > On Wed, May 06, 2015 at 08:01:38AM +1000, Dave Chinner wrote: > > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > And this looks wrong, too :) > > > > That looks like a change of tool behaviour. I just upgraded guilt; > > the old version stripped "from" lines in patch descriptions from the > > git commit log as goes into the author field of the commit. The new > > version does not appear to strip the from lines, and so when > > git-send-email formats it, it puts a from line in the message, and > > then the commit message has one too. > > Ok, so I remembered it the wrong way around, but it is a tool bug: > > $ guilt patchbomb -s d480578..cd83c77 > cd83c773 xfs: inode counter needs to use __percpu_counter_compare > 1e41b5ad percpu_counter: batch size aware __percpu_counter_compare() > Are these what you want to send? [Y/n] > .... > > Note the use of "-s". from the man page: > > -s > Don't add additional repository committer sign-offs to > the patch. This allows the sign-off chain to be fully > expressed in the commit messages and not changed by the > act of sending a patchbomb. > > It would appear that the "-s" option is not working properly. I'll > fix that before sending the next round of patches.... <sigh> So, the packaged version of guilt on a brand new debian unstable installation is actually older than the 2 year version of guilt I had previously installed and used on a different machine. IOWs, the debian distro version has this bug in it: http://repo.or.cz/w/guilt.git?a=commit;h=8f88f953580a0cacf111bf64c0e838014ee30c01 Yeah, that was fixed more than 3 years ago. Oh well, back to using a locally installed version... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() @ 2015-05-06 6:11 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2015-05-06 6:11 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-kernel, xfs On Wed, May 06, 2015 at 03:53:49PM +1000, Dave Chinner wrote: > On Wed, May 06, 2015 at 03:43:10PM +1000, Dave Chinner wrote: > > On Tue, May 05, 2015 at 09:36:36PM -0700, Christoph Hellwig wrote: > > > This looks reasonable, but I miss Ccs to the authors of the > > > percpu_counter code and linux-kernel. > > > > linux-kernel was cc'd. > > > > > On Wed, May 06, 2015 at 08:01:38AM +1000, Dave Chinner wrote: > > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > And this looks wrong, too :) > > > > That looks like a change of tool behaviour. I just upgraded guilt; > > the old version stripped "from" lines in patch descriptions from the > > git commit log as goes into the author field of the commit. The new > > version does not appear to strip the from lines, and so when > > git-send-email formats it, it puts a from line in the message, and > > then the commit message has one too. > > Ok, so I remembered it the wrong way around, but it is a tool bug: > > $ guilt patchbomb -s d480578..cd83c77 > cd83c773 xfs: inode counter needs to use __percpu_counter_compare > 1e41b5ad percpu_counter: batch size aware __percpu_counter_compare() > Are these what you want to send? [Y/n] > .... > > Note the use of "-s". from the man page: > > -s > Don't add additional repository committer sign-offs to > the patch. This allows the sign-off chain to be fully > expressed in the commit messages and not changed by the > act of sending a patchbomb. > > It would appear that the "-s" option is not working properly. I'll > fix that before sending the next round of patches.... <sigh> So, the packaged version of guilt on a brand new debian unstable installation is actually older than the 2 year version of guilt I had previously installed and used on a different machine. IOWs, the debian distro version has this bug in it: http://repo.or.cz/w/guilt.git?a=commit;h=8f88f953580a0cacf111bf64c0e838014ee30c01 Yeah, that was fixed more than 3 years ago. Oh well, back to using a locally installed version... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2015-05-14 15:02 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-12 23:52 [PATCH 0/2 v2] percpu_counter: xfs requires custom compare batch size Dave Chinner 2015-05-12 23:52 ` Dave Chinner 2015-05-12 23:52 ` [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() Dave Chinner 2015-05-12 23:52 ` Dave Chinner 2015-05-13 13:59 ` Tejun Heo 2015-05-13 13:59 ` Tejun Heo 2015-05-14 0:55 ` Dave Chinner 2015-05-14 0:55 ` Dave Chinner 2015-05-14 15:02 ` Tejun Heo 2015-05-14 15:02 ` Tejun Heo 2015-05-12 23:52 ` [PATCH 2/2] xfs: inode and free block counters need to use __percpu_counter_compare Dave Chinner 2015-05-12 23:52 ` Dave Chinner 2015-05-14 14:21 ` Brian Foster 2015-05-14 14:21 ` Brian Foster -- strict thread matches above, loose matches on Subject: below -- 2015-05-05 22:01 [PATCH 0/2] xfs: fix inode count underrun Dave Chinner 2015-05-05 22:01 ` [PATCH 1/2] percpu_counter: batch size aware __percpu_counter_compare() Dave Chinner 2015-05-05 22:01 ` Dave Chinner 2015-05-06 4:36 ` Christoph Hellwig 2015-05-06 4:36 ` Christoph Hellwig 2015-05-06 4:46 ` Christoph Hellwig 2015-05-06 4:46 ` Christoph Hellwig 2015-05-06 5:43 ` Dave Chinner 2015-05-06 5:43 ` Dave Chinner 2015-05-06 5:53 ` Dave Chinner 2015-05-06 5:53 ` Dave Chinner 2015-05-06 6:11 ` Dave Chinner 2015-05-06 6:11 ` Dave Chinner
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.