On Mon, 1 Aug 2022 at 00:14, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > On Sun, Jul 31, 2022 at 04:43:08PM -0400, Mikulas Patocka wrote: > > Let's have a look at this piece of code in __bread_slow: > > get_bh(bh); > > bh->b_end_io = end_buffer_read_sync; > > submit_bh(REQ_OP_READ, 0, bh); > > wait_on_buffer(bh); > > if (buffer_uptodate(bh)) > > return bh; > > Neither wait_on_buffer nor buffer_uptodate contain a memory barrier. > > Consequently, if someone calls sb_bread and then reads the buffer data, > > the read of buffer data may be executed before wait_on_buffer(bh) on > > architectures with weak memory ordering and it may return invalid data. > > I think we should be consistent between PageUptodate() and > buffer_uptodate(). Here's how it's done for pages currently: > > static inline bool folio_test_uptodate(struct folio *folio) > bool ret = test_bit(PG_uptodate, folio_flags(folio, 0)); > /* > * Must ensure that the data we read out of the folio is loaded > * _after_ we've loaded folio->flags to check the uptodate bit. > * We can skip the barrier if the folio is not uptodate, because > * we wouldn't be reading anything from it. > * > * See folio_mark_uptodate() for the other side of the story. > */ > if (ret) > smp_rmb(); > > return ret; > > ... > > static __always_inline void folio_mark_uptodate(struct folio *folio) > /* > * Memory barrier must be issued before setting the PG_uptodate bit, > * so that all previous stores issued in order to bring the folio > * uptodate are actually visible before folio_test_uptodate becomes true. > */ > smp_wmb(); > set_bit(PG_uptodate, folio_flags(folio, 0)); > > I'm happy for these to also be changed to use acquire/release; no > attachment to the current code. But bufferheads & pages should have the > same semantics, or we'll be awfully confused. I suspect that adding acquire/release annotations at the bitops level is not going to get us anywhere, given that for the uptodate flag, it is the set operation that has release semantics, whereas for a lock flag, it will be the clear operation. Reverting to the legacy barrier instructions to try and avoid this ambiguity will likely only make things worse. I was cc'ed only on patch #1 of your v3, so I'm not sure where this is headed, but I strongly +1 Matthew's point above that this should be done at the level that defines how the bit fields should be interpreted wrt to the contents of the data structure that they describe/guard.
