Message-Id: <20081207205250.dbb7fe4b.akpm@linux-foundation.org>
Date:	Sun, 7 Dec 2008 20:52:50 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Eric Dumazet <dada1@...mosbay.com>
Cc:	linux kernel <linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Mingming Cao <cmm@...ibm.com>, "Theodore Ts'o" <tytso@....edu>,
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH] percpu_counter: Fix __percpu_counter_sum()

On Sun, 07 Dec 2008 19:00:32 +0100 Eric Dumazet <dada1@...mosbay.com> wrote:

> Andrew Morton a __crit :
> > On Sun, 07 Dec 2008 14:28:00 +0100 Eric Dumazet <dada1@...mosbay.com> wrote:
> > 
> >> Andrew Morton a __crit :
> >>> On Wed, 03 Dec 2008 21:24:36 +0100 Eric Dumazet <dada1@...mosbay.com> wrote:
> >>>
> >>>> Eric Dumazet a __crit :
> >>>>
> >>>> 1) __percpu_counter_sum() is buggy, it should not write
> >>>> on per_cpu_ptr(fbc->counters, cpu), or another cpu
> >>>> could get its changes lost.
> >>>>
> >>>> __percpu_counter_sum should be read only (const struct percpu_counter *fbc),
> >>>> and no locking needed.
> >>> No, we can't do this - it will break ext4.
> >>>
> >>> Take a closer look at 1f7c14c62ce63805f9574664a6c6de3633d4a354 and at
> >>> e8ced39d5e8911c662d4d69a342b9d053eaaac4e.
> >>>
> >>> I suggest that what we do is to revert both those changes.  We can
> >>> worry about the possibly-unneeded spin_lock later, in a separate patch.
> >>>
> >>> It should have been a separate patch anyway.  It's conceptually
> >>> unrelated and is not a bugfix, but it was mixed in with a bugfix.
> >>>
> >>> Mingming, this needs urgent consideration, please.  Note that I had to
> >>> make additional changes to ext4 due to the subsequent introduction of
> >>> the dirty_blocks counter.
> >>>
> >>>
> >>> Please read the below changelogs carefully and check that I have got my
> >>> head around this correctly - I may not have done.
> >>>
> >>
> >> Hum... e8ced39d5e8911c662d4d69a342b9d053eaaac4e is probably following
> >> the wrong path, but I see the intent. Even in the 'nr_files' case, it could
> >> help to reduce excessive calls to percpu_counter_sum()
> >>
> > 
> > We should fix this in 2.6.28 - right now percpu_counter_sum() is subtly
> > corrupting the counter's value.
> > 
> > I sent two revert patches which I hope to merge into 2.6.28.  Could you
> > guys please read/review/maybe-test them?
> 
> Your revert patches have the same effect than my first patch : No writes
> in percpu_counter_sum()
> 
> I am lost here Andrew...
> 

heh.  Here's the problem...

The first patch which was added (pre-2.6.27) was "percpu_counter: new
function percpu_counter_sum_and_set".  This added the broken-by-design
percpu_counter_sum_and_set() function, **and used it in ext4**.

Later, during 2.6.28 development came the "percpu counter: clean up
percpu_counter_sum_and_set()" which propagated the
percpu_counter_sum_and_set() brokenness into percpu_counter_sum() as
well.

If we were to now merge your simple dont-modify-the-percpu-counters fix
then this would break ext4, because of the **and used it in ext4**,
above.

You see, ext4 stopped using the accurate/slow percpu_counter_sum() and
switched to percpu_counter_sum_and_set() because this new function
increases the accuracy of percpu_counter_read() in other parts of ext4.

Also, e8ced39d5e8911c662d4d69a342b9d053eaaac4e ("percpu_counter: new
function percpu_counter_sum_and_set") replaced a call to
percpu_counter_sum_positive() with a call to
percpu_counter_sum_and_set(), but there's nothing which prevents
percpu_counter_sum_and_set() from returning negative values, afacit.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists