Re: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Coming back to this now, just trying to review the
filehandle-lookup/dcache interactions:

On Fri, Mar 11, 2011 at 03:07:49PM +1100, NeilBrown wrote:
> 1/ Originally DCACHE_DISCONNECTED didn't really mean much - it's presence
>    was only a hint, its absence was a strong statement.
>    If the flag is set, the dentry might not be linked to the root.
>    If it is clear, it definitely is link through to the root.
>    However I think it was used with stronger intent than that.
> 
>    Now it seems to mean a little bit more:  If it is set and the dentry
>    is hashed, then it must be on the sb->s_anon list.

The code that makes that assumption is __d_shrink (which does the work
of d_drop)--it uses DCACHE_DISCONECTED to decide which hash chain to
lock.

I can't find any basis for that assumption.  The only code that clears
DCACHE_DISCONNECTED is in expfs.c, and it isn't done at the same time as
hashing.  Am I missing something?

>    This is a significant
>    which I never noticed (I haven't been watching).  Originally a
>    disconnected dentry would be attached (and hashed) to its parent.  Then
>    that parent would get its own parent and so on until it was attached all
>    the way to the root.  Only then would be start clearing
>    DCACHE_DISCONNECTED.  It seems we must clear it sooner now... I wonder if
>    that is correct.

It looks wrong to me:

If we clear DCACHE_DISCONNECTED too early, then we risk a filehandle
lookup thinking the dentry is OK to use.  That could mean for example
trying to rename across directories that don't have any ancestor
relationship to each other in the dcache yet.

So we need to wait to clear DCACHE_DISCONNECTED until we *know* the
dentry's parents go all the way back to the root.  As you say, that's
what the current code does.

But that means DCACHE_DISCONNECTED dentries can be hashed to their
parents, and __d_shrink can be handed such dentries and then get the
locking wrong.

It looks like this bug might originate with Nick Piggin's ceb5bdc2d246
"fs: dcache per-bucket dcache hash locking"?  There's no discussion in
the changelog, so probably it was just based on an unexamined assumption
about DCACHE_DISCONNECTED.

I wonder if an IS_ROOT() test could replace the DCACHE_DISCONNECTED test
in __d_shrink(), or if we need another flag, or ?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux