The patch is in the attached file.
Thanks,
Zoltan
Andrew Morton <[email protected]> wrote:
Zoltan Menyhart <[email protected]> wrote:
I'm afraid "unlock_buffer()" works incorrectly
(at least on the ia64 architecture).
As far As I can see, nothing makes it sure that data modifications
issued inside the critical section be globally visible before the
"BH_Lock" bit gets cleared.
When we acquire a resource, we need the "acq" semantics, when we
release the resource, we need the "rel" semantics (obviously).
Some architectures (at least the ia64) require explicit operations
to ensure these semantics, the ordinary "loads" and "stores" do not
guarantee any ordering.
For the "stand alone" bit operations, these semantics do not matter.
They are implemented by use of atomic operations in SMP mode, which
operations need to follow either the "acq" semantics or the "rel"
semantics (at least the ia64). An arbitrary choice was made to use
the "acq" semantics.
We use bit operations to implement buffer locking.
As a consequence, the requirement of the "rel" semantics, when we
release the resource, is not met (at least on the ia64).
- Either an "smp_mb__before_clear_bit()" is lacking
(if we want to keep the existing definition of "clear_bit()"
with its "acq" semantics).
Note that "smp_mb__before_clear_bit()" is a bidirectional fence
operation on ia64, it is less efficient than the simple
"rel" semantics.
- Or a new bit clearing service needs to be added that includes
the "rel" semantics, say "release_N_clear_bit()"
(since we are actually _releasing_ a lock :-))
Thanks,
Zoltan Menyhart
buffer.c:
void fastcall unlock_buffer(struct buffer_head *bh)
{
clear_buffer_locked(bh);
smp_mb__after_clear_bit();
wake_up_bit(&bh->b_state, BH_Lock);
}
asm-ia64/bitops.h:
/*
* clear_bit() has "acquire" semantics.
*/
#define smp_mb__before_clear_bit() smp_mb()
#define smp_mb__after_clear_bit() do { /* skip */; } while (0)
/**
* clear_bit - Clears a bit in memory
* @nr: Bit to clear
* @addr: Address to start counting from
*
* clear_bit() is atomic and may not be reordered. However, it does
* not contain a memory barrier, so if it is used for locking purposes,
* you should call smp_mb__before_clear_bit() and/or
smp_mb__after_clear_bit()
* in order to ensure changes are visible on other processors.
*/
alpha and powerpc define both of these as smp_mb(). sparc64 is similar,
but smarter.
atomic_ops.txt says:
/* All memory operations before this call will
* be globally visible before the clear_bit().
*/
smp_mb__before_clear_bit();
clear_bit( ... );
/* The clear_bit() will be visible before all
* subsequent memory operations.
*/
smp_mb__after_clear_bit();
So it would appear that to make all the modifications which were made to
the buffer visible after the clear_bit(), yes, we should be using
smp_mb__before_clear_bit();
unlock_page() does both...
--- save/fs/buffer.c 2006-03-27 10:39:53.000000000 +0200
+++ linux-2.6.16/fs/buffer.c 2006-03-27 10:40:46.000000000 +0200
@@ -78,6 +78,7 @@ EXPORT_SYMBOL(__lock_buffer);
void fastcall unlock_buffer(struct buffer_head *bh)
{
+ smp_mb__before_clear_bit();
clear_buffer_locked(bh);
smp_mb__after_clear_bit();
wake_up_bit(&bh->b_state, BH_Lock);
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
