cgroups: handle failure of cgroup_populate_dir() at mount/remount

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

kernel/cgroup.c +16 -2

index a7267bf..eab83f7 100644
@@ -896,6 +896,7 @@ static int parse_cgroupfs_options(char *data,
 static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 {
 	int ret = 0;
+	unsigned long subsys_bits;
 	struct cgroupfs_root *root = sb->s_fs_info;
 	struct cgroup *cgrp = &root->top_cgroup;
 	struct cgroup_sb_opts opts;

@@ -914,12 +915,17 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 		goto out_unlock;
 	}
 
+	subsys_bits = root->subsys_bits;
 	ret = rebind_subsystems(root, opts.subsys_bits);
 	if (ret)
 		goto out_unlock;
 
 	/* (re)populate subsystem files */
-	cgroup_populate_dir(cgrp);
+	ret = cgroup_populate_dir(cgrp);
+	if (ret) {
+		rebind_subsystems(root, subsys_bits);
+		goto out_unlock;
+	}
 
 	if (opts.release_agent)
 		strcpy(root->release_agent_path, opts.release_agent);

@@ -1122,9 +1128,13 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
 		BUG_ON(!list_empty(&root_cgrp->children));
 		BUG_ON(root->number_of_cgroups != 1);
 
-		cgroup_populate_dir(root_cgrp);
+		ret = cgroup_populate_dir(root_cgrp);
+
 		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&cgroup_mutex);
+
+		if (ret)
+			goto drop_new_super;
 	}
 
 	simple_set_mnt(mnt, sb);

@@ -1803,6 +1813,10 @@ int cgroup_add_file(struct cgroup *cgrp,
 		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
+
+	if (error)
+		printk(KERN_WARNING "cgroup: Failed to create file '%s': %d\n",
+		       name, error);
 	return error;
 }
 

Signature · 7 lines

1.5.4.rc3

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

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

kernel/cgroup.c +1 -0

index a7267bf..eab83f7 100644
@@ -896,6 +896,7 @@ static int parse_cgroupfs_options(char *data,
 static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 {
 	int ret = 0;
+	unsigned long subsys_bits;
 	struct cgroupfs_root *root = sb->s_fs_info;
 	struct cgroup *cgrp = &root->top_cgroup;
 	struct cgroup_sb_opts opts;

@@ -914,12 +915,17 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 		goto out_unlock;
 	}

kernel/cgroup.c +8 -1

+		rebind_subsystems(root, subsys_bits);
+		goto out_unlock;
+	}
 
 	if (opts.release_agent)
 		strcpy(root->release_agent_path, opts.release_agent);

@@ -1122,9 +1128,13 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
 		BUG_ON(!list_empty(&root_cgrp->children));
 		BUG_ON(root->number_of_cgroups != 1);
 
-		cgroup_populate_dir(root_cgrp);
+		ret = cgroup_populate_dir(root_cgrp);
+
 		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&cgroup_mutex);
+
+		if (ret)
+			goto drop_new_super;
 	}
 
 	simple_set_mnt(mnt, sb);

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

kernel/cgroup.c +16 -2

index a7267bf..eab83f7 100644
@@ -896,6 +896,7 @@ static int parse_cgroupfs_options(char *data,
 static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 {
 	int ret = 0;
+	unsigned long subsys_bits;
 	struct cgroupfs_root *root = sb->s_fs_info;
 	struct cgroup *cgrp = &root->top_cgroup;
 	struct cgroup_sb_opts opts;

@@ -914,12 +915,17 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 		goto out_unlock;
 	}
 
+	subsys_bits = root->subsys_bits;
 	ret = rebind_subsystems(root, opts.subsys_bits);
 	if (ret)
 		goto out_unlock;
 
 	/* (re)populate subsystem files */
-	cgroup_populate_dir(cgrp);
+	ret = cgroup_populate_dir(cgrp);
+	if (ret) {
+		rebind_subsystems(root, subsys_bits);
+		goto out_unlock;
+	}
 
 	if (opts.release_agent)
 		strcpy(root->release_agent_path, opts.release_agent);

@@ -1122,9 +1128,13 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
 		BUG_ON(!list_empty(&root_cgrp->children));
 		BUG_ON(root->number_of_cgroups != 1);
 
-		cgroup_populate_dir(root_cgrp);
+		ret = cgroup_populate_dir(root_cgrp);
+
 		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&cgroup_mutex);
+
+		if (ret)
+			goto drop_new_super;
 	}
 
 	simple_set_mnt(mnt, sb);

@@ -1803,6 +1813,10 @@ int cgroup_add_file(struct cgroup *cgrp,
 		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
+
+	if (error)
+		printk(KERN_WARNING "cgroup: Failed to create file '%s': %d\n",
+		       name, error);
 	return error;
 }
 

Signature · 7 lines

1.5.4.rc3

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

Quoted 4 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan <[email protected]> wrote:

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

Quoted 2 lines

With this patch, we fail the mount/remount in this case.

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Quoted 10 lines · KAMEZAWA Hiroyuki

Quoted 6 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan…

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Quoted 4 lines

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, this mount option was introduced for cpuset. But it has been here for a long time and people may use it when mounting other cgroup subsystems, then is it OK to change to restrict its use within cpuset only?

Quoted 10 lines · KAMEZAWA Hiroyuki

Quoted 6 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan <[email protected]> wrote:

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Quoted 4 lines

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, this mount option was introduced for cpuset. But it has been here for a long time and people may use it when mounting other cgroup subsystems, then is it OK to change to restrict its use within cpuset only?

Quoted 16 lines · On Fri, 22 May 2009 16:35:14 +0800 Li Zefan…

Quoted 14 lines · KAMEZAWA Hiroyuki

Quoted 6 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan…

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Quoted 3 lines

Yes, this mount option was introduced for cpuset. But it has been here for a long time and people may use it when mounting other cgroup subsystems, then is it OK to change to restrict its use within cpuset only?

Asking libcgroup people may be appropriate...added CC.

Quoted 16 lines · On Fri, 22 May 2009 16:35:14 +0800 Li Zefan <[email protected]> wrote:

Quoted 14 lines · KAMEZAWA Hiroyuki

Quoted 6 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan <[email protected]> wrote:

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Quoted 3 lines

Yes, this mount option was introduced for cpuset. But it has been here for a long time and people may use it when mounting other cgroup subsystems, then is it OK to change to restrict its use within cpuset only?

Asking libcgroup people may be appropriate...added CC.

Quoted 17 lines · KAMEZAWA Hiroyuki

Quoted 14 lines · On Fri, 22 May 2009 16:35:14 +0800 Li Zefan <[email protected]> wrote:

Quoted 13 lines · KAMEZAWA Hiroyuki

Quoted 6 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan <[email protected]> wrote:

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Quoted 4 lines

Quoted 3 lines

Yes, this mount option was introduced for cpuset. But it has been here for a long time and people may use it when mounting other cgroup subsystems, then is it OK to change to restrict its use within cpuset only?

Asking libcgroup people may be appropriate...added CC.

We just realized that we were not handling the noprefix usage in

libcgroup. From the pov of the library, the option should either not exist for any subsystem or for all of them. Anything else would mean having to add special cases.

Quoted 21 lines · Dhaval Giani

Quoted 20 lines · KAMEZAWA Hiroyuki

Quoted 17 lines · On Fri, 22 May 2009 16:35:14 +0800 Li Zefan…

Quoted 7 lines · KAMEZAWA Hiroyuki

Quoted 4 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan…

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

Quoted 6 lines

Quoted 2 lines

With this patch, we fail the mount/remount in this case.

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, this mount option was introduced for cpuset. But it has been here for a long time and people may use it when mounting other cgroup subsystems, then is it OK to change to restrict its use within cpuset only?

Quoted 3 lines

Quoted 1 line

Asking libcgroup people may be appropriate...added CC.

We just realized that we were not handling the noprefix usage in

I guess handling noprefix will increase complexity of libcgroup? since we can't figure out which subsystem the file belongs to by looking at the prefix of the file.

Quoted 4 lines

libcgroup. From the pov of the library, the option should either not exist for any subsystem or for all of them. Anything else would mean having to add special cases.

I don't see how to totally remove 'noprefix' while keep the backward compatibility of cpuset, so I think we have to reserve it, and fix name collision caused by this option.

Quoted 21 lines · Dhaval Giani

Quoted 20 lines · KAMEZAWA Hiroyuki

Quoted 17 lines · On Fri, 22 May 2009 16:35:14 +0800 Li Zefan <[email protected]> wrote:

Quoted 7 lines · KAMEZAWA Hiroyuki

Quoted 4 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan <[email protected]> wrote:

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

Quoted 6 lines

Quoted 2 lines

With this patch, we fail the mount/remount in this case.

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, this mount option was introduced for cpuset. But it has been here for a long time and people may use it when mounting other cgroup subsystems, then is it OK to change to restrict its use within cpuset only?

Quoted 3 lines

Quoted 1 line

Asking libcgroup people may be appropriate...added CC.

We just realized that we were not handling the noprefix usage in

I guess handling noprefix will increase complexity of libcgroup? since we can't figure out which subsystem the file belongs to by looking at the prefix of the file.

Quoted 4 lines

libcgroup. From the pov of the library, the option should either not exist for any subsystem or for all of them. Anything else would mean having to add special cases.

I don't see how to totally remove 'noprefix' while keep the backward compatibility of cpuset, so I think we have to reserve it, and fix name collision caused by this option.

Quoted 17 lines · KAMEZAWA Hiroyuki

Quoted 14 lines · On Fri, 22 May 2009 16:35:14 +0800 Li Zefan…

Quoted 13 lines · KAMEZAWA Hiroyuki

Quoted 6 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan…

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Quoted 4 lines

Quoted 3 lines

Yes, this mount option was introduced for cpuset. But it has been here for a long time and people may use it when mounting other cgroup subsystems, then is it OK to change to restrict its use within cpuset only?

Asking libcgroup people may be appropriate...added CC.

We just realized that we were not handling the noprefix usage in

libcgroup. From the pov of the library, the option should either not exist for any subsystem or for all of them. Anything else would mean having to add special cases.

Quoted 4 lines · On Fri, 22 May 2009 11:00:12 +0800 Li Zefan…

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

Quoted 2 lines

With this patch, we fail the mount/remount in this case.

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Quoted 3 lines · KAMEZAWA Hiroyuki

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, exposing the "noprefix" option externally was one of the mistakes I made when developing cgroups.

It seems to me really unlikely that anyone is using "noprefix" for

anything other than implicitly when mounting the "cpuset" filesystem. So I'd be inclined to just forbid it if we're mounting more than just the cpuset subsystem. A bit of a nasty abstraction violation, but it makes more sense overall. The only problem is that someone *might* be using it - do we have any way to determine how, and how big do they have to be before we care?

Quoted 10 lines · Paul Menage

Quoted 3 lines · KAMEZAWA Hiroyuki

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, exposing the "noprefix" option externally was one of the mistakes I made when developing cgroups.

It seems to me really unlikely that anyone is using "noprefix" for

And "noprefix" is not documented in cgroups.txt, so I guess not many people know this option. Even libcgroup doesn't handle it.

Quoted 7 lines

anything other than implicitly when mounting the "cpuset" filesystem. So I'd be inclined to just forbid it if we're mounting more than just the cpuset subsystem. A bit of a nasty abstraction violation, but it makes more sense overall. The only problem is that someone *might* be using it - do we have any way to determine how, and how big do they have to be before we care?

I think we can never know..

Quoted 10 lines · Paul Menage

Quoted 3 lines · KAMEZAWA Hiroyuki

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, exposing the "noprefix" option externally was one of the mistakes I made when developing cgroups.

It seems to me really unlikely that anyone is using "noprefix" for

And "noprefix" is not documented in cgroups.txt, so I guess not many people know this option. Even libcgroup doesn't handle it.

Quoted 7 lines

anything other than implicitly when mounting the "cpuset" filesystem. So I'd be inclined to just forbid it if we're mounting more than just the cpuset subsystem. A bit of a nasty abstraction violation, but it makes more sense overall. The only problem is that someone *might* be using it - do we have any way to determine how, and how big do they have to be before we care?

I think we can never know..

Quoted 20 lines · On Wed, 27 May 2009 09:07:31 +0800 Li Zefan <[email protected]> wrote:

Quoted 9 lines · Paul Menage

Quoted 3 lines · KAMEZAWA Hiroyuki

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, exposing the "noprefix" option externally was one of the mistakes I made when developing cgroups.

It seems to me really unlikely that anyone is using "noprefix" for

And "noprefix" is not documented in cgroups.txt, so I guess not many people know this option. Even libcgroup doesn't handle it.

Quoted 7 lines

anything other than implicitly when mounting the "cpuset" filesystem. So I'd be inclined to just forbid it if we're mounting more than just the cpuset subsystem. A bit of a nasty abstraction violation, but it makes more sense overall. The only problem is that someone *might* be using it - do we have any way to determine how, and how big do they have to be before we care?

Quoted 1 line

I think we can never know..

How about this method ?

- add "noprefix" to "to-be-removed" list.
- add "WARNING: noprefix option will be removed in 2.6.32 (or 2.6.31)" now
- remove "noprefix" in 2.6.31-rc or later

Quoted 23 lines · KAMEZAWA Hiroyuki

Quoted 20 lines · On Wed, 27 May 2009 09:07:31 +0800 Li Zefan…

Quoted 9 lines · Paul Menage

Quoted 3 lines · KAMEZAWA Hiroyuki

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, exposing the "noprefix" option externally was one of the mistakes I made when developing cgroups.

It seems to me really unlikely that anyone is using "noprefix" for

And "noprefix" is not documented in cgroups.txt, so I guess not many people know this option. Even libcgroup doesn't handle it.

Quoted 7 lines

anything other than implicitly when mounting the "cpuset" filesystem. So I'd be inclined to just forbid it if we're mounting more than just the cpuset subsystem. A bit of a nasty abstraction violation, but it makes more sense overall. The only problem is that someone *might* be using it - do we have any way to determine how, and how big do they have to be before we care?

Quoted 8 lines

Quoted 1 line

I think we can never know..

How about this method ?

- add "noprefix" to "to-be-removed" list.
- add "WARNING: noprefix option will be removed in 2.6.32 (or 2.6.31)" now
- remove "noprefix" in 2.6.31-rc or later

I don't see how we can remove noprefix while reserve the compatibility of old cpuset..

As Paul Menage said, we can allow noprefix to be used only if we mount just cpuset subsystem:

(pseudo code)

kernel/cgroup.c +5 -0

@@ -886,6 +886,11 @@ static int parse_cgroupfs_options(char *data,
                }
        }
 
+
+       if (test_bit(ROOT_NOPREFIX, &opts->flags) &&
+           (opts->subsys_bits & ~cpuset_subsys_id) != 0)
+               return -EINVAL;
+
        /* We can't have an empty hierarchy */
        if (!opts->subsys_bits)
                return -EINVAL;

Quoted 23 lines · KAMEZAWA Hiroyuki

Quoted 20 lines · On Wed, 27 May 2009 09:07:31 +0800 Li Zefan <[email protected]> wrote:

Quoted 9 lines · Paul Menage

Quoted 3 lines · KAMEZAWA Hiroyuki

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, exposing the "noprefix" option externally was one of the mistakes I made when developing cgroups.

It seems to me really unlikely that anyone is using "noprefix" for

And "noprefix" is not documented in cgroups.txt, so I guess not many people know this option. Even libcgroup doesn't handle it.

Quoted 7 lines

anything other than implicitly when mounting the "cpuset" filesystem. So I'd be inclined to just forbid it if we're mounting more than just the cpuset subsystem. A bit of a nasty abstraction violation, but it makes more sense overall. The only problem is that someone *might* be using it - do we have any way to determine how, and how big do they have to be before we care?

Quoted 8 lines

Quoted 1 line

I think we can never know..

How about this method ?

- add "noprefix" to "to-be-removed" list.
- add "WARNING: noprefix option will be removed in 2.6.32 (or 2.6.31)" now
- remove "noprefix" in 2.6.31-rc or later

I don't see how we can remove noprefix while reserve the compatibility of old cpuset..

As Paul Menage said, we can allow noprefix to be used only if we mount just cpuset subsystem:

(pseudo code)

kernel/cgroup.c +5 -0

@@ -886,6 +886,11 @@ static int parse_cgroupfs_options(char *data,
                }
        }
 
+
+       if (test_bit(ROOT_NOPREFIX, &opts->flags) &&
+           (opts->subsys_bits & ~cpuset_subsys_id) != 0)
+               return -EINVAL;
+
        /* We can't have an empty hierarchy */
        if (!opts->subsys_bits)
                return -EINVAL;

Quoted 20 lines · On Wed, 27 May 2009 09:07:31 +0800 Li Zefan…

Quoted 9 lines · Paul Menage

Quoted 3 lines · KAMEZAWA Hiroyuki

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, exposing the "noprefix" option externally was one of the mistakes I made when developing cgroups.

It seems to me really unlikely that anyone is using "noprefix" for

And "noprefix" is not documented in cgroups.txt, so I guess not many people know this option. Even libcgroup doesn't handle it.

Quoted 7 lines

anything other than implicitly when mounting the "cpuset" filesystem. So I'd be inclined to just forbid it if we're mounting more than just the cpuset subsystem. A bit of a nasty abstraction violation, but it makes more sense overall. The only problem is that someone *might* be using it - do we have any way to determine how, and how big do they have to be before we care?

Quoted 1 line

I think we can never know..

How about this method ?

- add "noprefix" to "to-be-removed" list.
- add "WARNING: noprefix option will be removed in 2.6.32 (or 2.6.31)" now
- remove "noprefix" in 2.6.31-rc or later

Quoted 3 lines · KAMEZAWA Hiroyuki

Hm, shouldn't we allow "noprefix" to be effective only agaisnt cpuset ? I think it's just for backward-compatibility of cpuset.
(I don't like the option at all.)

Yes, exposing the "noprefix" option externally was one of the mistakes I made when developing cgroups.

It seems to me really unlikely that anyone is using "noprefix" for

anything other than implicitly when mounting the "cpuset" filesystem. So I'd be inclined to just forbid it if we're mounting more than just the cpuset subsystem. A bit of a nasty abstraction violation, but it makes more sense overall. The only problem is that someone *might* be using it - do we have any way to determine how, and how big do they have to be before we care?

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

kernel/cgroup.c +1 -0

index a7267bf..eab83f7 100644
@@ -896,6 +896,7 @@ static int parse_cgroupfs_options(char *data,
 static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 {
 	int ret = 0;
+	unsigned long subsys_bits;
 	struct cgroupfs_root *root = sb->s_fs_info;
 	struct cgroup *cgrp = &root->top_cgroup;
 	struct cgroup_sb_opts opts;

@@ -914,12 +915,17 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 		goto out_unlock;
 	}

kernel/cgroup.c +8 -1

+		rebind_subsystems(root, subsys_bits);
+		goto out_unlock;
+	}
 
 	if (opts.release_agent)
 		strcpy(root->release_agent_path, opts.release_agent);

@@ -1122,9 +1128,13 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
 		BUG_ON(!list_empty(&root_cgrp->children));
 		BUG_ON(root->number_of_cgroups != 1);
 
-		cgroup_populate_dir(root_cgrp);
+		ret = cgroup_populate_dir(root_cgrp);
+
 		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&cgroup_mutex);
+
+		if (ret)
+			goto drop_new_super;
 	}
 
 	simple_set_mnt(mnt, sb);

Quoted 32 lines · Li Zefan

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

kernel/cgroup.c +2 -0

index a7267bf..eab83f7 100644
@@ -896,6 +896,7 @@ static int parse_cgroupfs_options(char *data,
 static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 {
 	int ret = 0;
+	unsigned long subsys_bits;
 	struct cgroupfs_root *root = sb->s_fs_info;
 	struct cgroup *cgrp = &root->top_cgroup;
 	struct cgroup_sb_opts opts;

@@ -914,12 +915,17 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 		goto out_unlock;
 	}
 
+	subsys_bits = root->subsys_bits;
 	ret = rebind_subsystems(root, opts.subsys_bits);
 	if (ret)
 		goto out_unlock;
 

kernel/cgroup.c static int cgroup_remount(struct super_block *sb, int *flags, char *data) · line 925

Quoted 4 lines

kernel/cgroup.c +2 -1

 	/* (re)populate subsystem files */
-	cgroup_populate_dir(cgrp);
+	ret = cgroup_populate_dir(cgrp);
+	if (ret) {

if (ret) means a failure right? So we rebind_subsystems to the older subsys_bits? Could you please add a comment, the code can be misread.

Quoted 20 lines

kernel/cgroup.c +8 -1

+		rebind_subsystems(root, subsys_bits);
+		goto out_unlock;
+	}
 
 	if (opts.release_agent)
 		strcpy(root->release_agent_path, opts.release_agent);

@@ -1122,9 +1128,13 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
 		BUG_ON(!list_empty(&root_cgrp->children));
 		BUG_ON(root->number_of_cgroups != 1);
 
-		cgroup_populate_dir(root_cgrp);
+		ret = cgroup_populate_dir(root_cgrp);
+
 		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&cgroup_mutex);
+
+		if (ret)
+			goto drop_new_super;
 	}
 

kernel/cgroup.c int cgroup_add_file(struct cgroup *cgrp, · line 1822

Quoted 12 lines

kernel/cgroup.c +4 -0

 	simple_set_mnt(mnt, sb);

@@ -1803,6 +1813,10 @@ int cgroup_add_file(struct cgroup *cgrp,
 		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
+
+	if (error)
+		printk(KERN_WARNING "cgroup: Failed to create file '%s': %d\n",
+		       name, error);
 	return error;
 }
 

Looks good, should we mark cgroup_populate_dir with __must_check?

Quoted 32 lines · Li Zefan

Now we have 'stat' file in both memory and cpuacct subsystems. If we mount these 2 subsystems with option 'noprefix', the creation of 'stat' file for cpuacct will fail, but without any notificatin to the user.

With this patch, we fail the mount/remount in this case.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

kernel/cgroup.c +2 -0

index a7267bf..eab83f7 100644
@@ -896,6 +896,7 @@ static int parse_cgroupfs_options(char *data,
 static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 {
 	int ret = 0;
+	unsigned long subsys_bits;
 	struct cgroupfs_root *root = sb->s_fs_info;
 	struct cgroup *cgrp = &root->top_cgroup;
 	struct cgroup_sb_opts opts;

@@ -914,12 +915,17 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 		goto out_unlock;
 	}
 
+	subsys_bits = root->subsys_bits;
 	ret = rebind_subsystems(root, opts.subsys_bits);
 	if (ret)
 		goto out_unlock;
 

kernel/cgroup.c static int cgroup_remount(struct super_block *sb, int *flags, char *data) · line 925

Quoted 4 lines

kernel/cgroup.c +2 -1

 	/* (re)populate subsystem files */
-	cgroup_populate_dir(cgrp);
+	ret = cgroup_populate_dir(cgrp);
+	if (ret) {

if (ret) means a failure right? So we rebind_subsystems to the older subsys_bits? Could you please add a comment, the code can be misread.

Quoted 20 lines

kernel/cgroup.c +8 -1

+		rebind_subsystems(root, subsys_bits);
+		goto out_unlock;
+	}
 
 	if (opts.release_agent)
 		strcpy(root->release_agent_path, opts.release_agent);

@@ -1122,9 +1128,13 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
 		BUG_ON(!list_empty(&root_cgrp->children));
 		BUG_ON(root->number_of_cgroups != 1);
 
-		cgroup_populate_dir(root_cgrp);
+		ret = cgroup_populate_dir(root_cgrp);
+
 		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&cgroup_mutex);
+
+		if (ret)
+			goto drop_new_super;
 	}
 

kernel/cgroup.c int cgroup_add_file(struct cgroup *cgrp, · line 1822

Quoted 12 lines

kernel/cgroup.c +4 -0

 	simple_set_mnt(mnt, sb);

@@ -1803,6 +1813,10 @@ int cgroup_add_file(struct cgroup *cgrp,
 		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
+
+	if (error)
+		printk(KERN_WARNING "cgroup: Failed to create file '%s': %d\n",
+		       name, error);
 	return error;
 }
 

Looks good, should we mark cgroup_populate_dir with __must_check?