Re: VM in 2.4.7-pre hurts...

Linus Torvalds
2001-07-10T04:21:52Z

On Mon, 9 Jul 2001, Linus Torvalds wrote:
>
> Look:
>
> CPU #1 CPU #2
>
> try_to_free_buffers()
>
> if (atomic_read(&bh->b_count)
>
> end_buffer_io_sync()
>
> atomic_inc(&bh->b_count);
> bit_clear(BH_Locked, &bh->b_flags);
>
> || bh->b_flags & BUSY_BITS)
> free bh

I forgot to note that it doesn't help to re-order the tests here - but we
_could_ do

if (bh->b_flags & BUSY_BITS)
goto buffer_busy;
rmb();
if (atomic_read(&bh->b_count))
goto buffer_busy;

together with having the proper write memory barriers in
"end_buffer_io_sync()" to make sure that the BH_Locked thing shows up in
the right order with bh->b_count updates.

In contrast, the version in pre4 doesn't depend on any memory ordering
between BH_Locked at all - it really only depends on a memory barrier
before the final atomic_dec() that releases the buffer, as it ends up
being sufficient for try_to_free_buffers() to just worry about the buffer
count when it comes to IO completion. The b_flags BUSY bits don't matter
wrt the IO completion at all - they end up being used only for "idle"
buffers (which in turn are totally synchronized by the LRU and hash
spinlocks, so that is the "obviously correct" case)

I personally think it's a hard thing to depend on memory ordering,
especially if there are two independent fields. Which is why I really
don't think that the pre4 fix is "overkill".

Oh, it does really need a

smp_mb_before_atomic_dec();

as part of the "put_bh()". On x86, this obviously is a no-op. And we
actually need that one in general - not just for IO completion - as long
as we consider the "atomic_dec(&bh->b_flags)" to "release" the buffer.

Andrea?

Linus