[PATCH review 3/8] dcache: Clearly separate the two directory rename cases in d_splice_alias

Eric W. Biederman ebiederm at xmission.com
Sat Aug 15 18:25:55 UTC 2015


Al Viro <viro at ZenIV.linux.org.uk> writes:

> On Thu, Aug 13, 2015 at 11:31:47PM -0500, Eric W. Biederman wrote:
>
>> The problem is that as the lookup locking stands today grabbing the
>> s_vfs_rename_mutex must use mutex_trylock which can fail, so for reliability
>> reasons we need to avoid using the rename_mutex as much as possible.
>
> I really don't like it.  For one thing, you *still* are taking it - have to,
> really.  So this argument is moot anyway.  

Not moot.  We don't take s_vfs_rename_mutex when we are connecting a
disconnected dentry alias.  If d_splice_alias could sleep and take
s_vfs_rename_mutex then we could have a single path through the code.

Unfortunately the code can't sleep when taking s_vfs_rename_mutex so
attempting to take s_vfs_rename_mutex for both paths will introduce
unnecessary -ESTALE failures into d_splice_alias.

> ESTALE can happen here.  For
> another, I'm not convinced that this "we don't need no stinkin'
> extra locks for attaching a detached subtree" is correct.  E.g. what's
> to protect that IS_ROOT(new) from changing right under you?

You are quite correct that I missed that nothing protects the result of
IS_ROOT(new).   So my change does introduce a case where we don't
hold the appropriate inode mutexes when renaming a dentry and that
introduces races elsewhere in the code so it is not acceptable.

But it is true that we only take rename_lock and don't take any
additional mutex when connecting a disconnected dentry.  Aka "we don't
need no stinkin' extra locks".  We clearly can not take the
new->d_parent->d_inode->i_mutex when IS_ROOT(new) as that is
meaningless.  Further I do not see a point in taking s_vfs_rename_mutex
in that case.

Not for this round but if you can see any reason why our not taking
s_vfs_rename_mutex when connecting disconnected dentries is wrong
and we need to take it and risk -ESTALE.  I would love to know because I
would love to clean up that mess.

> I'm not saying that it wouldn't benefit from cleaner locking - it sure
> as hell would.  But it's in a really incestous relationship with a lot
> of other pieces, both in fs/dcache.c and elsewhere.  Let's not mix that
> into anything else - driveby cleanups in that place are very likely to
> cause serious trouble.

Fair enough. 

I do have to touch d_splice_alias and change the locking, so
unfortunately I have to do some of the nasty locking analysis anyway.

I am keeping the i_lock cleanup because it is trivial and removes the
need to figure out if there is any existing ordering between i_lock and
mount_lock, and if taking mount_lock could induce a deadlock.  That
audit would not be trivial.

Eric


More information about the Containers mailing list