Skip to content

Commit 2f79cdf

Browse files
committed
fs: only do a memory barrier for the first set_buffer_uptodate()
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]>
1 parent f280b98 commit 2f79cdf

File tree

1 file changed

+11
-0
lines changed

1 file changed

+11
-0
lines changed

include/linux/buffer_head.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,17 @@ BUFFER_FNS(Defer_Completion, defer_completion)
137137

138138
static __always_inline void set_buffer_uptodate(struct buffer_head *bh)
139139
{
140+
/*
141+
* If somebody else already set this uptodate, they will
142+
* have done the memory barrier, and a reader will thus
143+
* see *some* valid buffer state.
144+
*
145+
* Any other serialization (with IO errors or whatever that
146+
* might clear the bit) has to come from other state (eg BH_Lock).
147+
*/
148+
if (test_bit(BH_Uptodate, &bh->b_state))
149+
return;
150+
140151
/*
141152
* make it consistent with folio_mark_uptodate
142153
* pairs with smp_load_acquire in buffer_uptodate

0 commit comments

Comments
 (0)