On Tue, May 13, 2014 at 01:09:51PM +0200, Peter Zijlstra wrote:
> On Tue, May 13, 2014 at 10:45:48AM +0100, Mel Gorman wrote:
> > Discarding buffers uses a bunch of atomic operations when discarding buffers
> > because ...... I can't think of a reason. Use a cmpxchg loop to clear all the
> > necessary flags. In most (all?) cases this will be a single atomic operations.
> >
> > Signed-off-by: Mel Gorman <mgorman@xxxxxxx>
> > ---
> > fs/buffer.c | 14 +++++++++-----
> > include/linux/buffer_head.h | 5 +++++
> > 2 files changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index 9ddb9fc..e80012d 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -1485,14 +1485,18 @@ EXPORT_SYMBOL(set_bh_page);
> > */
> > static void discard_buffer(struct buffer_head * bh)
> > {
> > + unsigned long b_state, b_state_old;
> > +
> > lock_buffer(bh);
> > clear_buffer_dirty(bh);
> > bh->b_bdev = NULL;
> > - clear_buffer_mapped(bh);
> > - clear_buffer_req(bh);
> > - clear_buffer_new(bh);
> > - clear_buffer_delay(bh);
> > - clear_buffer_unwritten(bh);
> > + b_state = bh->b_state;
> > + for (;;) {
> > + b_state_old = cmpxchg(&bh->b_state, b_state, (b_state & ~BUFFER_FLAGS_DISCARD));
> > + if (b_state_old == b_state)
> > + break;
> > + b_state = b_state_old;
> > + }
> > unlock_buffer(bh);
> > }
>
> So.. I'm soon going to introduce atomic_{or,and}() and
> atomic64_{or,and}() across the board, but of course this isn't an
> atomic_long_t but a regular unsigned long.
>
> Its a bit unfortunate we have this discrepancy with types vs atomic ops,
> there's:
>
> cmpxchg, xchg -- mostly available for all 1,2,3,4 (and 8 where
> appropriate) byte values.
>
> bitops -- operate on unsigned long *
>
> atomic* -- operate on atomic_*t
I hit the same problem when dealing with pageblock bitmap. I would have
preferred it to do an atomic_read() but the actual conversion to use
atomic_t for the map became a mess with little or no upside.
>
> operation which is available on a lot of architectures, we'll be stuck
> with a cmpxchg loop instead :/
>
> *sigh*
>
> Anyway, nothing wrong with this patch, however, you could, if you really
> wanted to push things, also include BH_Lock in that clear :-)
That's a bold strategy Cotton.
Untested patch on top
---8<---
diff --git a/fs/buffer.c b/fs/buffer.c
index e80012d..42fcb6d 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1490,6 +1490,8 @@ static void discard_buffer(struct buffer_head * bh)
lock_buffer(bh);
clear_buffer_dirty(bh);
bh->b_bdev = NULL;
+
+ smp_mb__before_clear_bit();
b_state = bh->b_state;
for (;;) {
b_state_old = cmpxchg(&bh->b_state, b_state, (b_state & ~BUFFER_FLAGS_DISCARD));
@@ -1497,7 +1499,13 @@ static void discard_buffer(struct buffer_head * bh)
break;
b_state = b_state_old;
}
- unlock_buffer(bh);
+
+ /*
+ * BUFFER_FLAGS_DISCARD include BH_lock so it has been cleared so the
+ * wake_up_bit is the last part of a unlock_buffer
+ */
+ smp_mb__after_clear_bit();
+ wake_up_bit(&bh->b_state, BH_Lock);
}
/**
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 95f565a..523db58 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -80,7 +80,7 @@ struct buffer_head {
/* Bits that are cleared during an invalidate */
#define BUFFER_FLAGS_DISCARD \
(1 << BH_Mapped | 1 << BH_New | 1 << BH_Req | \
- 1 << BH_Delay | 1 << BH_Unwritten)
+ 1 << BH_Delay | 1 << BH_Unwritten | 1 << BH_Lock)
/*
* macro tricks to expand the set_buffer_foo(), clear_buffer_foo()
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
