On 2014/6/25 23:00, Tejun Heo wrote:

Hey,

On Wed, Jun 25, 2014 at 09:56:31AM +0800, Li Zefan wrote:

quoted

quoted

Hmmm?  Why does that matter?  The only region in cgroup_mount() which
needs to be put inside such mutex would be root lookup, no?

unfortunately that won't help. I think what you suggest is:

cgroup_mount()
{
	mutex_lock();
	lookup_cgroup_root();
	mutex_unlock();
	kernfs_mount();
}

cgroup_kill_sb()
{
	mutex_lock();
	percpu_ref_kill();
	mutex_Unlock();
	kernfs_kill_sb();
}

See, we may still be destroying the superblock after we've succeeded
in getting the refcnt of cgroup root.

Sure, but now the decision to kill is synchronized so the other side
can interlock with it.  e.g.

cgroup_mount()
{
	mutex_lock();
	lookup_cgroup_root();
	if (root isn't killed yet)
		root->this_better_stay_alive++;
	mutex_unlock();
	kernfs_mount();
}

cgroup_kill_sb()
{
	mutex_lock();
	if (check whether root can be killed)
		percpu_ref_kill();
	mutex_unlock();
	if (the above condition was true)
		kernfs_kill_sb();
}

This looks nasty, and I don't think it's correct. If we skip the call
to kernfs_kill_sb(), kernfs_super_info won't be freed but super_block
will, so we will end up either leaking memory or accessing invalid
memory. There are other problems like returning with sb->s_umount still
held.