Re: [PATCH v4 2/2] change buffer_locked, so that it has acquire semantics

[Matthew Wilcox]

On Mon, Aug 01, 2022 at 11:01:40AM -0400, Mikulas Patocka wrote:
> 
> 
> On Mon, 1 Aug 2022, Matthew Wilcox wrote:
> 
> > On Mon, Aug 01, 2022 at 06:43:55AM -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 any 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.
> > > 
> > > Fix this bug by changing the function buffer_locked to have the acquire
> > > semantics - so that code that follows buffer_locked cannot be moved before
> > > it.
> > 
> > I think this is the wrong approach.  Instead, buffer_set_uptodate()
> > should have the smp_wmb() and buffer_uptodate should have the smp_rmb().
> > Just like the page flags.  As I said last night.
> 
> Linus said that he prefers acquire/release to smp_rmb/smp_wmb. So, sort it 
> out with him :)
> 
> In most cases, the buffer is set uptodate while it is locked, so that 
> there is no race on the uptodate flag (the race exists on the locked 
> flag). Are there any cases where the uptodate flag is modified on unlocked 
> buffer, so that it needs special treatment too?

I think you misunderstand the purpose of locked/uptodate.  At least
for pages, the lock flag does not order access to the data in the page.
Indeed, the contents of the page can be changed while you hold the lock.
But the uptodate flag does order access to the data.  At the point where
you can observe the uptodate flag set, you know the contents of the page
have been completely read from storage.  And you don't need to hold the
lock to check the uptodate flag.  So this is wrong:

	buffer_lock()
	*data = 0x12345678;
	buffer_set_uptodate_not_ordered()
	buffer_unlock_ordered()

because a reader can do:

	while (!buffer_test_uptodate()) {
		buffer_lock();
		buffer_unlock();
	}
	x = *data;

and get x != 0x12345678 because the compiler can move the
buffer_set_uptodate_not_ordered() before the store to *data.