Re: [patch] mm: fix lockless pagecache reordering bug (was Re: BUG: soft lockup - is this XFS problem?)

[Nick Piggin <npiggin@suse.de>]

On Mon, Jan 05, 2009 at 09:30:55AM -0800, Linus Torvalds wrote:
> 
> 
> On Mon, 5 Jan 2009, Nick Piggin wrote:
> > 
> > This patch should be applied to 2.6.29 and 27/28 stable kernels, please.
> 
> No. I think this patch is utter crap. But please feel free to educate me 
> on why that is not the case.
> 
> Here's my explanation:
> 
> Not only is it ugly (which is already sufficient ground to suspect it is 
> wrong or could at least be done better), but reading the comment, it makes 
> no sense at all. You only put the barrier in the "goto repeat" case, but 
> the thing is, if you worry about radix tree slot not being reloaded in the 
> repeat case, then you damn well should worry about it not being reloaded 
> in the non-repeat case too!

In which case atomic_inc_unless is defined to provide a barrier.

 
> The code is immediately followed by a test to see that the page is still 
> the same in the slot, ie this:
> 
>                 /*
>                  * Has the page moved?
>                  * This is part of the lockless pagecache protocol. See
>                  * include/linux/pagemap.h for details.
>                  */
>                 if (unlikely(page != *pagep)) {
> 
> and if you need a barrier for the repeat case, you need one for this case 
> too.
> 
> In other words, it looks like you fixed the symptom, but not the real 
> cause! That's now how we work in the kernel.
> 
> The real cause, btw, appears to be that radix_tree_deref_slot() is a piece 
> of slimy sh*t, and has not been correctly updated to RCU. The proper fix 
> doesn't require any barriers that I can see - I think the proper fix is 
> this simple one-liner.
> 
> If you use RCU to protect a data structure, then any data loaded from that 
> data structure that can change due to RCU should be loaded with 
> "rcu_dereference()". 

It doesn't need that because the last level pointers in the radix
tree are not necessarily under RCU, but whatever synchronisation
the caller uses (in this case, speculative page references, which
should not require smp_read_barrier_depends, AFAIKS). Putting an
rcu_dereference there might work, but I think it misses a subtlety
of this code.


> Now, I can't test this, because it makes absolutely no difference for me 
> (the diff isn't empty, but the asm changes seem to be all due to just gcc 
> variable numbering changing). I can't seem to see the buggy code. Maybe it 
> needs a specific compiler version, or some specific config option to 
> trigger?
> 
> So because I can't see the issue, I also obviously can't verify that it's 
> the only possible case. Maybe there is some other memory access that 
> should also be done with the proper rcu accessors?
> 
> Of course, it's also possible that we should just put a barrier in 
> page_cache_get_speculative(). That doesn't seem to make a whole lot of 
> conceptual sense, though (the same way that your barrier() didn't make any 
> sense - I don't see that the barrier has absolutely _anything_ to do with 
> whether the speculative getting of the page fails or not!)

When that fails, the caller can (almost) assume the pointer has changed.
So it has to load the new pointer to continue. The object pointed to is
not protected with RCU, nor is there a requirement to see a specific
load execution ordering. 

>
> In general, I'd like fewer "band-aid" patches, and more "deep thinking" 
> patches. I'm not saying mine is very deep either, but I think it's at 
> least scrathing the surface of the real problem rather than just trying to 
> cover it up.