Messages in this thread | | | Date | Sun, 31 Jul 2022 09:41:02 -0400 (EDT) | From | Mikulas Patocka <> | Subject | Re: [PATCH] Add a read memory barrier to wait_on_buffer |
| |
On Sun, 31 Jul 2022, Ard Biesheuvel wrote:
> This has little to do with speculation, so better to drop this S bomb > from your commit message. This is about concurrency and weak memory > ordering.
Yes.
> This doesn't seem like a very robust fix to me, tbh - I suppose this > makes the symptom you encountered go away, but the underlying issue > remains afaict. > > Given that the lock and uptodate fields etc are just bits in a > bitfield, wouldn't it be better to use cmpxchg() with acquire/release > semantics (as appropriate) to manage these bits?
The kernel already uses clear_bit_unlock, test_and_set_bit_lock and wait_on_bit_lock_io to manage the BH_Lock bit - and they have acquire/release semantics.
The only problem is that test_bit doesn't provide any memory barriers. Should we add the barrier to buffer_locked() instead of wait_on_buffer()? Perhaps it would fix more bugs - in reiserfs, there's this piece of code:
if (buffer_locked(bh)) { spin_unlock(lock); wait_on_buffer(bh); spin_lock(lock); } if (!buffer_uptodate(bh)) { ret = -EIO; }
or this: if (buffer_locked(bh)) { int depth; PROC_INFO_INC(sb, scan_bitmap.wait); depth = reiserfs_write_unlock_nested(sb); __wait_on_buffer(bh); reiserfs_write_lock_nested(sb, depth); } BUG_ON(!buffer_uptodate(bh)); BUG_ON(atomic_read(&bh->b_count) == 0);
That assumes that buffer_locked provides a barrier.
Mikulas
| |