[Devel] [PATCH rh7 v3 2/2] ve/fs/sync: fix CT's mountpoints traversal

[Vladimir Davydov]

On Fri, Feb 26, 2016 at 02:41:19PM +0300, Andrey Ryabinin wrote:
> 
> 
> On 02/26/2016 02:10 PM, Vladimir Davydov wrote:
> > On Thu, Feb 25, 2016 at 07:19:38PM +0300, Andrey Ryabinin wrote:
> > ...
> >> @@ -2673,7 +2676,10 @@ static struct mnt_namespace *create_mnt_ns(struct vfsmount *m)
> >>  		struct mount *mnt = real_mount(m);
> >>  		mnt->mnt_ns = new_ns;
> >>  		new_ns->root = mnt;
> >> +		down_write(&namespace_sem);
> >>  		list_add(&mnt->mnt_list, &new_ns->list);
> >> +		list_add(&new_ns->mntns_list, &get_exec_env()->mntns_list);
> > 
> > Do we really need to link such mnt_ns?
> > 
> 
> I think, it shouldn't hurt at least.
> Do we have a reason to not link these namespaces? 

I think so. AFAICS mount_subtree is only used to create a hidden mnt to
ease dcache traversal, and it doesn't hide mount in the current
namespace. But check this please.

> 
> >> +		up_write(&namespace_sem);
> >>  	} else {
> >>  		mntput(m);
> >>  	}
> >> @@ -2954,6 +2960,7 @@ void put_mnt_ns(struct mnt_namespace *ns)
> >>  	namespace_lock();
> >>  	br_write_lock(&vfsmount_lock);
> >>  	umount_tree(ns->root, 0);
> >> +	list_del(&ns->mntns_list);
> > 
> > Why under br_write_lock? namespace_lock should be enough.
> 
> Whether this this under br_write_lock or not doesn't matter absolutely.
> It equally could be placed under lock or out of it.

But it looks misleading. By looking at the code one might erroneously
assume that the list is protected by vfsmount_lock.

> 
> >>  	br_write_unlock(&vfsmount_lock);
> >>  	namespace_unlock();
> >>  	free_mnt_ns(ns);