On Fri, 31 Jul 2015, Linus Torvalds wrote:
>
> 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
Yes, the one which grabbed my attention is:
@@ -311,7 +346,7 @@ static void dentry_iput(struct dentry * dentry)
{
struct inode *inode = dentry->d_inode;
if (inode) {
- dentry->d_inode = NULL;
+ __d_clear_type_and_inode(dentry);
hlist_del_init(&dentry->d_u.d_alias);
spin_unlock(&dentry->d_lock);
spin_unlock(&inode->i_lock);
which I think clears the DCACHE_ENTRY_TYPE i.e. makes it DCACHE_MISS_TYPE,
when it was left as is before. While there might be an RCU lookup in
progress, suddenly finding this to be a negative dentry. Perhaps -
this is not an area I've visited for years, and I've not followed up
the sequence count protection.
Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
