Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280560 > unrolled thread

Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init cgroupns

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns "Serge E. Hallyn" <serge.hallyn@ubuntu.com> - 2015-12-01 05:10 +0100
    Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns Tejun Heo <tj@kernel.org> - 2015-12-01 17:50 +0100
      Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns "Serge E. Hallyn" <serge.hallyn@ubuntu.com> - 2015-12-01 23:00 +0100
        Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns Tejun Heo <tj@kernel.org> - 2015-12-02 18:00 +0100
          Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns Tejun Heo <tj@kernel.org> - 2015-12-02 18:10 +0100
            Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns "Serge E. Hallyn" <serge.hallyn@ubuntu.com> - 2015-12-03 23:50 +0100
              Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns Tejun Heo <tj@kernel.org> - 2015-12-07 16:40 +0100
                Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns Serge Hallyn <serge.hallyn@ubuntu.com> - 2015-12-07 17:00 +0100
          Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns "Serge E. Hallyn" <serge.hallyn@ubuntu.com> - 2015-12-02 18:10 +0100
        Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns "Serge E. Hallyn" <serge.hallyn@ubuntu.com> - 2015-12-02 18:00 +0100
        Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init  cgroupns Tejun Heo <tj@kernel.org> - 2015-12-02 18:00 +0100

#1280560 — Re: [PATCH 7/8] cgroup: mount cgroupns-root when inside non-init cgroupns

On Mon, Nov 30, 2015 at 10:09:38AM -0500, Tejun Heo wrote:
> Hello, Serge.
> 
> On Thu, Nov 26, 2015 at 11:17:45PM -0600, Serge E. Hallyn wrote:
> > > Wouldn't it be simpler to walk dentry from kernfs root than
> > > duplicating dentry instantiation?
> > 
> > Sorry I don't think I'm following.  Are you suggesting walking the
> > kn->parent chain backward and doing d_lookup() at each point starting
> > with sb->s_root?
> 
> Yeah, something like that.  I wonder whether there are already code
> paths doing that.  What we need is a straight path walk.  I could be
> wrong but it shouldn't be that complex and if it works out we can
> avoid introducing another instantiation / lookup path.
> 
> Thanks.

So actually the way the code is now, the first mount cannot
be done from a non-init user namespace; and kernfs_obtain_root()
is only called from non-init user namespace.  So can we assume
that the root dentry will be instantiated?  (or can it get
evicted?)

If we can assume that then most of that fn can go away.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1281040

Hey, Serge.

On Mon, Nov 30, 2015 at 10:07:04PM -0600, Serge E. Hallyn wrote:
> So actually the way the code is now, the first mount cannot
> be done from a non-init user namespace; and kernfs_obtain_root()
> is only called from non-init user namespace.  So can we assume
> that the root dentry will be instantiated?  (or can it get
> evicted?)
> 
> If we can assume that then most of that fn can go away.

The v2 hierarchy is always mounted and non-init ns shouldn't be able
to create new v1 hierarchies, so the root dentry should always be
there.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281239

On Tue, Dec 01, 2015 at 11:46:49AM -0500, Tejun Heo wrote:
> Hey, Serge.
> 
> On Mon, Nov 30, 2015 at 10:07:04PM -0600, Serge E. Hallyn wrote:
> > So actually the way the code is now, the first mount cannot
> > be done from a non-init user namespace; and kernfs_obtain_root()
> > is only called from non-init user namespace.  So can we assume
> > that the root dentry will be instantiated?  (or can it get
> > evicted?)
> > 
> > If we can assume that then most of that fn can go away.
> 
> The v2 hierarchy is always mounted and non-init ns shouldn't be able
> to create new v1 hierarchies, so the root dentry should always be
> there.

I mispoke before though - it's not the hierarchy's root dentry,
but rather a dentry for a descendent cgroup which will become the
root dentry for the new superblock.  We do know that there must be
a css_set with a cgroup.  I'm still trying to track down whether
that cgrou's inode's dentry can ever be flushed.  I would think
not but am not sure.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282019

On Wed, Dec 02, 2015 at 10:56:37AM -0600, Serge E. Hallyn wrote:
> Can it be flushed when we know that the cgroup is being pinned by
> a css_set?  (There's either a task or a cgroup_namespace pinning it
> or we wouldn't get here)

Yeap, it can be flushed.  There's no ref coming out of cgroup to the
vfs objects.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282050

On Wed, Dec 02, 2015 at 11:02:39AM -0600, Serge E. Hallyn wrote:
> On Wed, Dec 02, 2015 at 11:58:39AM -0500, Tejun Heo wrote:
> > On Wed, Dec 02, 2015 at 10:56:37AM -0600, Serge E. Hallyn wrote:
> > > Can it be flushed when we know that the cgroup is being pinned by
> > > a css_set?  (There's either a task or a cgroup_namespace pinning it
> > > or we wouldn't get here)
> > 
> > Yeap, it can be flushed.  There's no ref coming out of cgroup to the
> > vfs objects.
> 
> Ok, thanks.  Still seems to me to be more work to actually walk the
> path ourselves, but I'll go that route and see what it looks like :)

I just dislike having two separate paths instantiating the same
objects and would prefer doing it the same way userland would do if
that isn't too complex but yeah it might turn out to be a lot more
work.

Thanks a lot!

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283435

On Wed, Dec 02, 2015 at 12:05:51PM -0500, Tejun Heo wrote:
> On Wed, Dec 02, 2015 at 11:02:39AM -0600, Serge E. Hallyn wrote:
> > On Wed, Dec 02, 2015 at 11:58:39AM -0500, Tejun Heo wrote:
> > > On Wed, Dec 02, 2015 at 10:56:37AM -0600, Serge E. Hallyn wrote:
> > > > Can it be flushed when we know that the cgroup is being pinned by
> > > > a css_set?  (There's either a task or a cgroup_namespace pinning it
> > > > or we wouldn't get here)
> > > 
> > > Yeap, it can be flushed.  There's no ref coming out of cgroup to the
> > > vfs objects.
> > 
> > Ok, thanks.  Still seems to me to be more work to actually walk the
> > path ourselves, but I'll go that route and see what it looks like :)
> 
> I just dislike having two separate paths instantiating the same
> objects and would prefer doing it the same way userland would do if
> that isn't too complex but yeah it might turn out to be a lot more
> work.
> 
> Thanks a lot!

Here's a patch to make that change.  Seems to be working for me.  If it
looks ok I can fold it into the prevoius patches and resend the new set.

PATCH 1/1] kernfs_obtain_root: switch to walking the path [fold up]

Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
 fs/kernfs/mount.c | 80 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 33 deletions(-)

diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index cc41fe1..027f4ca 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -14,6 +14,7 @@
 #include <linux/magic.h>
 #include <linux/slab.h>
 #include <linux/pagemap.h>
+#include <linux/namei.h>
 
 #include "kernfs-internal.h"
 
@@ -62,6 +63,27 @@ struct kernfs_root *kernfs_root_from_sb(struct super_block *sb)
 	return NULL;
 }
 
+/*
+ * find the next ancestor in the path down to @child, where @parent was the
+ * parent whose child we want to find.
+ *
+ * Say the path is /a/b/c/d.  @child is d, @parent is NULL.  We return the root
+ * node.  If @parent is b, then we return the node for c.
+ * Passing in d as @parent is not ok.
+ */
+static struct kernfs_node *
+find_kn_ancestor_below(struct kernfs_node *child, struct kernfs_node *parent)
+{
+	BUG_ON(child == parent);
+
+	while (child->parent != parent) {
+		BUG_ON(!child->parent);
+		child = child->parent;
+	}
+
+	return child;
+}
+
 /**
  * kernfs_obtain_root - get a dentry for the given kernfs_node
  * @sb: the kernfs super_block
@@ -74,42 +96,34 @@ struct dentry *kernfs_obtain_root(struct super_block *sb,
 				  struct kernfs_node *kn)
 {
 	struct dentry *dentry;
-	struct inode *inode;
+	struct kernfs_node *knparent = NULL;
 
 	BUG_ON(sb->s_op != &kernfs_sops);
 
-	/* inode for the given kernfs_node should already exist. */
-	inode = kernfs_get_inode(sb, kn);
-	if (!inode) {
-		pr_debug("kernfs: could not get inode for '");
-		pr_cont_kernfs_path(kn);
-		pr_cont("'.\n");
-		return ERR_PTR(-EINVAL);
-	}
-
-	/* instantiate and link root dentry */
-	dentry = d_obtain_root(inode);
-	if (!dentry) {
-		pr_debug("kernfs: could not get dentry for '");
-		pr_cont_kernfs_path(kn);
-		pr_cont("'.\n");
-		return ERR_PTR(-ENOMEM);
-	}
-
-	/*
-	 * If this is a new dentry, set it up. We need kernfs_mutex because
-	 * this may be called by callers other than kernfs_fill_super.
-	 */
-	mutex_lock(&kernfs_mutex);
-	if (!dentry->d_fsdata) {
-		kernfs_get(kn);
-		dentry->d_fsdata = kn;
-	} else {
-		WARN_ON(dentry->d_fsdata != kn);
-	}
-	mutex_unlock(&kernfs_mutex);
-
-	return dentry;
+	dentry = dget(sb->s_root);
+	if (!kn->parent) // this is the root
+		return dentry;
+
+	knparent = find_kn_ancestor_below(kn, NULL);
+	BUG_ON(!knparent);
+
+	do {
+		struct dentry *dtmp;
+		struct kernfs_node *kntmp;
+
+		if (kn == knparent)
+			return dentry;
+		kntmp = find_kn_ancestor_below(kn, knparent);
+		BUG_ON(!kntmp);
+		dtmp = lookup_one_len(kntmp->name, dentry, strlen(kntmp->name));
+		dput(dentry);
+		if (IS_ERR(dtmp))
+			return dtmp;
+		knparent = kntmp;
+		dentry = dtmp;
+	} while (1);
+
+	// notreached
 }
 
 static int kernfs_fill_super(struct super_block *sb, unsigned long magic)
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285614

Hello, Serge.

On Thu, Dec 03, 2015 at 04:47:06PM -0600, Serge E. Hallyn wrote:
...
> +	dentry = dget(sb->s_root);
> +	if (!kn->parent) // this is the root
> +		return dentry;
> +
> +	knparent = find_kn_ancestor_below(kn, NULL);
> +	BUG_ON(!knparent);

Doing WARN_ON() and returning failure is better, I think.  Failing ns
mount is an okay failure mode and a lot better than crashing the
system.  Also, how about find_next_ancestor() for the name of the
function?

> +	do {
> +		struct dentry *dtmp;
> +		struct kernfs_node *kntmp;
> +
> +		if (kn == knparent)
> +			return dentry;
> +		kntmp = find_kn_ancestor_below(kn, knparent);
> +		BUG_ON(!kntmp);
> +		dtmp = lookup_one_len(kntmp->name, dentry, strlen(kntmp->name));
> +		dput(dentry);
> +		if (IS_ERR(dtmp))
> +			return dtmp;
> +		knparent = kntmp;
> +		dentry = dtmp;
> +	} while (1);

Other than the nitpicks, looks good to me.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285689

Quoting Tejun Heo (tj@kernel.org):
> Hello, Serge.
> 
> On Thu, Dec 03, 2015 at 04:47:06PM -0600, Serge E. Hallyn wrote:
> ...
> > +	dentry = dget(sb->s_root);
> > +	if (!kn->parent) // this is the root
> > +		return dentry;
> > +
> > +	knparent = find_kn_ancestor_below(kn, NULL);
> > +	BUG_ON(!knparent);
> 
> Doing WARN_ON() and returning failure is better, I think.  Failing ns
> mount is an okay failure mode and a lot better than crashing the
> system.

Ok - this shouldn't be user-triggerable, so if it happens it really
is a bug in our code, but I'll change it,

> Also, how about find_next_ancestor() for the name of the
> function?

Yeah it's static anyway :)

will change, squash, and resend the set.

> > +	do {
> > +		struct dentry *dtmp;
> > +		struct kernfs_node *kntmp;
> > +
> > +		if (kn == knparent)
> > +			return dentry;
> > +		kntmp = find_kn_ancestor_below(kn, knparent);
> > +		BUG_ON(!kntmp);
> > +		dtmp = lookup_one_len(kntmp->name, dentry, strlen(kntmp->name));
> > +		dput(dentry);
> > +		if (IS_ERR(dtmp))
> > +			return dtmp;
> > +		knparent = kntmp;
> > +		dentry = dtmp;
> > +	} while (1);
> 
> Other than the nitpicks, looks good to me.
> 
> Thanks.
> 
> -- 
> tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282055

On Wed, Dec 02, 2015 at 11:58:39AM -0500, Tejun Heo wrote:
> On Wed, Dec 02, 2015 at 10:56:37AM -0600, Serge E. Hallyn wrote:
> > Can it be flushed when we know that the cgroup is being pinned by
> > a css_set?  (There's either a task or a cgroup_namespace pinning it
> > or we wouldn't get here)
> 
> Yeap, it can be flushed.  There's no ref coming out of cgroup to the
> vfs objects.

Ok, thanks.  Still seems to me to be more work to actually walk the
path ourselves, but I'll go that route and see what it looks like :)

thanks
-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282021

On Wed, Dec 02, 2015 at 11:53:12AM -0500, Tejun Heo wrote:
> Hello, Serge.
> 
> On Tue, Dec 01, 2015 at 03:58:53PM -0600, Serge E. Hallyn wrote:
> > I mispoke before though - it's not the hierarchy's root dentry,
> > but rather a dentry for a descendent cgroup which will become the
> > root dentry for the new superblock.  We do know that there must be
> > a css_set with a cgroup.  I'm still trying to track down whether
> > that cgrou's inode's dentry can ever be flushed.  I would think
> > not but am not sure.
> 
> Hmmm... I'm not really following.  The inode can be flushed and that's
> why it needs to be walked down from root.  What am I missing here?

Can it be flushed when we know that the cgroup is being pinned by
a css_set?  (There's either a task or a cgroup_namespace pinning it
or we wouldn't get here)

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282025

Hello, Serge.

On Tue, Dec 01, 2015 at 03:58:53PM -0600, Serge E. Hallyn wrote:
> I mispoke before though - it's not the hierarchy's root dentry,
> but rather a dentry for a descendent cgroup which will become the
> root dentry for the new superblock.  We do know that there must be
> a css_set with a cgroup.  I'm still trying to track down whether
> that cgrou's inode's dentry can ever be flushed.  I would think
> not but am not sure.

Hmmm... I'm not really following.  The inode can be flushed and that's
why it needs to be walked down from root.  What am I missing here?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web