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

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

[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 <lizf@cn.fujitsu.com>
---
 kernel/cgroup.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a7267bf..eab83f7 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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;
 }
 
-- 
1.5.4.rc3

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[KAMEZAWA Hiroyuki]

On Fri, 22 May 2009 11:00:12 +0800
Li Zefan <lizf@cn.fujitsu.com> 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.)

Thanks,
-Kame

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index a7267bf..eab83f7 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -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;
>  }
>  
> -- 
> 1.5.4.rc3
> 
> --
> 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/
> 

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[Li Zefan]

KAMEZAWA Hiroyuki wrote:
> On Fri, 22 May 2009 11:00:12 +0800
> Li Zefan <lizf@cn.fujitsu.com> 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.)
> 

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?

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[KAMEZAWA Hiroyuki]

On Fri, 22 May 2009 16:35:14 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> KAMEZAWA Hiroyuki wrote:
> > On Fri, 22 May 2009 11:00:12 +0800
> > Li Zefan <lizf@cn.fujitsu.com> 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.)
> > 
> 
> 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.

Thanks,
-Kame

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[Dhaval Giani]

On Fri, May 22, 2009 at 05:34:21PM +0900, KAMEZAWA Hiroyuki wrote:
> On Fri, 22 May 2009 16:35:14 +0800
> Li Zefan <lizf@cn.fujitsu.com> wrote:
> 
> > KAMEZAWA Hiroyuki wrote:
> > > On Fri, 22 May 2009 11:00:12 +0800
> > > Li Zefan <lizf@cn.fujitsu.com> 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.)
> > > 
> > 
> > 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.

thanks,
-- 
regards,
Dhaval

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[Li Zefan]

Dhaval Giani wrote:
> On Fri, May 22, 2009 at 05:34:21PM +0900, KAMEZAWA Hiroyuki wrote:
>> On Fri, 22 May 2009 16:35:14 +0800
>> Li Zefan <lizf@cn.fujitsu.com> wrote:
>>
>>> KAMEZAWA Hiroyuki wrote:
>>>> On Fri, 22 May 2009 11:00:12 +0800
>>>> Li Zefan <lizf@cn.fujitsu.com> 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.)
>>>>
>>> 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

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.

> 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.

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[Dhaval Giani]

On Mon, May 25, 2009 at 05:14:58PM +0800, Li Zefan wrote:
> Dhaval Giani wrote:
> > On Fri, May 22, 2009 at 05:34:21PM +0900, KAMEZAWA Hiroyuki wrote:
> >> On Fri, 22 May 2009 16:35:14 +0800
> >> Li Zefan <lizf@cn.fujitsu.com> wrote:
> >>
> >>> KAMEZAWA Hiroyuki wrote:
> >>>> On Fri, 22 May 2009 11:00:12 +0800
> >>>> Li Zefan <lizf@cn.fujitsu.com> 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.)
> >>>>
> >>> 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
> 
> 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.
> 

Yes, but that should be handled with the next set of features planned
for the library.

> > 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.

I agree with you.

-- 
regards,
Dhaval

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[Paul Menage]

On Fri, May 22, 2009 at 1:25 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
> 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?

Paul

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[Li Zefan]

Paul Menage wrote:
> On Fri, May 22, 2009 at 1:25 AM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>> 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.

> 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..

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[KAMEZAWA Hiroyuki]

On Wed, 27 May 2009 09:07:31 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> Paul Menage wrote:
> > On Fri, May 22, 2009 at 1:25 AM, KAMEZAWA Hiroyuki
> > <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >> 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.
> 
> > 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..

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

Thanks,
-Kame

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[Li Zefan]

KAMEZAWA Hiroyuki wrote:
> On Wed, 27 May 2009 09:07:31 +0800
> Li Zefan <lizf@cn.fujitsu.com> wrote:
> 
>> Paul Menage wrote:
>>> On Fri, May 22, 2009 at 1:25 AM, KAMEZAWA Hiroyuki
>>> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>>>> 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.
>>
>>> 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..
> 
> 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)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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;

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[KAMEZAWA Hiroyuki]

On Wed, 27 May 2009 11:24:22 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> KAMEZAWA Hiroyuki wrote:
> > On Wed, 27 May 2009 09:07:31 +0800
> > Li Zefan <lizf@cn.fujitsu.com> wrote:
> > 
> >> Paul Menage wrote:
> >>> On Fri, May 22, 2009 at 1:25 AM, KAMEZAWA Hiroyuki
> >>> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >>>> 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.
> >>
> >>> 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..
> > 
> > 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:
> 
I have no objection.

-Kame


> (pseudo code)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -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;
> 
> 

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[Balbir Singh]

* Li Zefan <lizf@cn.fujitsu.com> [2009-05-22 11:00:12]:

> 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 <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index a7267bf..eab83f7 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -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) {

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.

> +		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;
>  }
>

Looks good, should we mark cgroup_populate_dir with __must_check? 

-- 
	Balbir

Re: [PATCH] cgroups: handle failure of cgroup_populate_dir() at mount/remount

[Li Zefan]

Balbir Singh wrote:
> * Li Zefan <lizf@cn.fujitsu.com> [2009-05-22 11:00:12]:
> 
>> 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 <lizf@cn.fujitsu.com>
>> ---
>>  kernel/cgroup.c |   18 ++++++++++++++++--
>>  1 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index a7267bf..eab83f7 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -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) {
> 
> 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.
> 

Sure.

>> +		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;
>>  }
>>
> 
> Looks good, should we mark cgroup_populate_dir with __must_check? 
> 

Actually cgroup_populate_dir() is called in 3 cases:
  - mount
  - remount
  - mkdir

This patch only handles the former 2 cases. The return value of
cgroup_populate_dir() is also ignored when mkdir, and that piece of code
is inhereted from cpuset. I guess it's because it's hard to handle this
failure and there was no possibility of name collisions in old cpuset.

Normally name collisions can be caught when mount/remount, thus there
won't be subsequent mkdir()s.