Re: v4.2-rc dcache regression, probably 75a6f82a0d10

Linus Torvalds
2015-07-31T22:52:41Z

On Fri, Jul 31, 2015 at 1:50 PM, Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> It's probably an old race that was very hard to hit because of cache
> coherency.
> Basically, before the wmb/rmb, the dentry was always updated closely to
> its flags, so the other CPU would "usually" get both updates at the same
> time; the barriers make it so the updates are split and it's possible to
> get it, and would explain why I could pick 4bf46a2726 as "the one"

I doubt that's it. wmb/rmb is actually a no-op on x86, and only
affects instruction scheduling. So yes, timing could be affected, but
it's such a _tiny_ effect that I don't really believe it's an issue.

I'd be more suspicious about other effects. For example, iot's not at
all obvious that the commit in question just changes the order of the
flags/inode field accesses, there are potentialy bigger changes there.
For example, this part (in __d_obtain_alias):

- tmp->d_inode = inode;
- tmp->d_flags |= add_flags;
+ __d_set_inode_and_type(tmp, inode, add_flags);

looks a bit off, because it *used* to just add those flags, but now,
through __d_set_inode_and_type, it does

+ dentry->d_inode = inode;
+ smp_wmb();
+ flags = READ_ONCE(dentry->d_flags);
+ flags &= ~(DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU);
+ flags |= type_flags;
+ WRITE_ONCE(dentry->d_flags, flags);

so it clears DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU.

Is that correct? Maybe, I haven't checked. And maybe it's a big bad
bug. Regardless, it sure as hell isn't just changing the order of the
access to those fields. That "DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU"
clearing came from __d_instantiate(), but now it hits __d_obtain_alias
too.

There may be other changes like that for all I know. I didn't look
more closely. But I'd blame changes like that rather than any timing
by rmb/wmb.

(Side note: the instruction scheduling changes can be big - especially
together with the changes to use READ_ONCE/WRITE_ONCE, it basically
means that gcc won't be using r-m-w instructions etc to set flags, so
I'm not saying rmb/wmb is necessarily a no-op in general, but it's
definitely generally not a hugely noticeable thing).

Added DavidH to the cc list, since it's his commit. But Dominique, it
would probably be a good idea for you to try to double-check that that
commit really is what matters for you and causes problems.

Linus