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

[Li Zefan <lizf@cn.fujitsu.com>]

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.