Re: [PATCH 09/18] autofs4: Add d_manage() dentry operation [ver #4]

[Nick Piggin <npiggin@gmail.com>]

On Sat, Jan 15, 2011 at 2:35 AM, David Howells <dhowells@redhat.com> wrote:
> Nick Piggin <npiggin@gmail.com> wrote:
>
>> > On Thu, 2011-01-13 at 21:54 +0000, David Howells wrote:
>> >> From: Ian Kent <raven@themaw.net>
>> >> +     //spin_lock(&dcache_lock);  /////////////// JUST DELETE THIS LOCK?
>> >> +     if (!d_mountpoint(dentry) && list_empty(&dentry->d_subdirs)) {
>> >> +             spin_lock(&dentry->d_lock);
>> >> +             if (!(dentry->d_flags & DCACHE_MANAGE_TRANSIT) &&
>> >> +                  (dentry->d_flags & DCACHE_NEED_AUTOMOUNT))
>> >> +                     __managed_dentry_set_transit(path->dentry);
>> >> +             spin_unlock(&dentry->d_lock);
>> >> +     }
>> >> +     //spin_unlock(&dcache_lock);
>> >
>> > In this case I think the dcache_lock needs to be deleted and the d_lock
>> > moved out of the if to protect the d_subdirs access.
>>
>> Right. If you follow the vfs-scale-working git branch series of
>> patches leading up to dcache_lock removal, it gives a pretty
>> good template of how to convert old dcache_lock using code
>> to new locking.
>>
>> Although you can also just look at locking in fs/dcache.c and
>> convert code from that.
>>
>> Any time you are dealing with just a *single* dentry, then
>> ->d_lock would be enough to replace dcache_lock (it
>> actually protects more than dcache_lock alone did).
>
> Does it make sense to leave the lock where it is and repeat the outer test
> after we've taken the lock?

I'll make the usual suggestion to make the locking simple, and
even avoiding all unlocked-load-then-recheck type of access,
unless there is a good reason to need it.

Many cases of ordering bugs I've found are due to these
seemingly simple and obvious optimisations.

It's up to you of course, is it worth having to think a little bit
harder about? Ian and yourself could probably answer that
better than me.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html