Skip to content

Commit

Permalink
fs: only do a memory barrier for the first set_buffer_uptodate()
Browse files Browse the repository at this point in the history
commit 2f79cdf upstream.

Commit d425207 ("add barriers to buffer_uptodate and
set_buffer_uptodate") added proper memory barriers to the buffer head
BH_Uptodate bit, so that anybody who tests a buffer for being up-to-date
will be guaranteed to actually see initialized state.

However, that commit didn't _just_ add the memory barrier, it also ended
up dropping the "was it already set" logic that the BUFFER_FNS() macro
had.

That's conceptually the right thing for a generic "this is a memory
barrier" operation, but in the case of the buffer contents, we really
only care about the memory barrier for the _first_ time we set the bit,
in that the only memory ordering protection we need is to avoid anybody
seeing uninitialized memory contents.

Any other access ordering wouldn't be about the BH_Uptodate bit anyway,
and would require some other proper lock (typically BH_Lock or the folio
lock).  A reader that races with somebody invalidating the buffer head
isn't an issue wrt the memory ordering, it's a serialization issue.

Now, you'd think that the buffer head operations don't matter in this
day and age (and I certainly thought so), but apparently some loads
still end up being heavy users of buffer heads.  In particular, the
kernel test robot reported that not having this bit access optimization
in place caused a noticeable direct IO performance regression on ext4:

  fxmark.ssd_ext4_no_jnl_DWTL_54_directio.works/sec -26.5% regression

although you presumably need a fast disk and a lot of cores to actually
notice.

Link: https://lore.kernel.org/all/Yw8L7HTZ%2FdE2%2Fo9C@xsang-OptiPlex-9020/
Reported-by: kernel test robot <[email protected]>
Tested-by: Fengwei Yin <[email protected]>
Cc: Mikulas Patocka <[email protected]>
Cc: Matthew Wilcox (Oracle) <[email protected]>
Cc: [email protected]
Signed-off-by: Linus Torvalds <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
  • Loading branch information
torvalds authored and gregkh committed Sep 15, 2022
1 parent 44b402d commit 7c63334
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions include/linux/buffer_head.h
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,17 @@ BUFFER_FNS(Defer_Completion, defer_completion)

static __always_inline void set_buffer_uptodate(struct buffer_head *bh)
{
/*
* If somebody else already set this uptodate, they will
* have done the memory barrier, and a reader will thus
* see *some* valid buffer state.
*
* Any other serialization (with IO errors or whatever that
* might clear the bit) has to come from other state (eg BH_Lock).
*/
if (test_bit(BH_Uptodate, &bh->b_state))
return;

/*
* make it consistent with folio_mark_uptodate
* pairs with smp_load_acquire in buffer_uptodate
Expand Down

0 comments on commit 7c63334

Please sign in to comment.