Re: vfs-scale, general questions (Re: NFS root lockups with -next 20110113)

[Nick Piggin <npiggin@gmail.com>]

On Wed, Jan 19, 2011 at 5:43 PM, J. R. Okajima <hooanon05@yahoo.co.jp> wrote:
>
> Hi,
>
> Nick Piggin:
>> Thanks for your help, can you see how I've fixed it in my vfs-scale
>> tree? What do you think?
>
> Your fix is great. I have no objection at all.
> Other than the fix, here are more generic questions about vfs-scale work.
> I am happy if you reply when you have time.

Thanks for reviewing.


> - getcwd(2) needs d_lock?
>  It acquires rename_lock and then tests whether the pwd is removed by
>  d_unhashed(). If a race condition between vfs_rename_dir() which may
>  unhash/rehash the dentry happens, then getcwd() may return the wrong
>  result due to unprotected d_unhashed() call, I am afraid. rename_lock
>  doesn't help this case.

We have the lock in write mode there, so it should exclude that
particular race. But I need to take another look at this code I
think, I'm not sure it's completely right, so I would appreciate reviews.

A while back I had some extra checks in there and would restart
the entire reverse walk in case of races... but need to think about
it.


> - what is the right order of dget() and mntget()?
>  If I remember correctly, someone said "mntget() first and then
>  dget(). when putting, do in reverse" in the discussion when
>  path_{get,put}() were born. So it is called "the right order" in the
>  commit log.
>  It was many years ago. Is it still true? And should rcu-walk follow it
>  too? The current implementation doesn't seem to care about this order.

Well dget and mntget is not a problem, because we can only do
mntget while already guaranteeing a reference on the mount, and
only dget when already guaranteeing a ref on the dentry (and mount).

But dput must happen before mntput so you don't have dentry ref
without mnt ref. Can you point out where rcu-walk does this wrongly?


> - d_move() and rename_lock
>  This may be out of rcu-walk work, but rename_lock in d_move() looks
>  outstanding since it surely kills concurrency. It is a pity that two
>  unrelated but concurrent d_move-s are serialized when we run rename(2)
>  on two different filesystems. Even if all of dentries, parents and
>  hash buckets are different from each other, d_move() never run
>  concurrently.

Yes I have a patch for that. I made a small hash table of rename locks.
This makes independent same-dir renames scalable. However that was
not the main motivation of the patch. On a really big POWER7 system,
the lookup path goes into a strange bimodal behaviour in the presence
of a relatively small amount of rename activity and sometimes starves
and throughput crashes. Breaking up rename_lock solves that too.

I'll wait until things settle down a bit more and perhaps have a chance
to get more numbers before submitting it (although I can show you when
I get back).

Thanks,
Nick