[CRIU] [PATCH] cg: don't try to copy special props to cpuset:/

[Pavel Emelyanov]

On 10/03/2014 05:30 PM, Tycho Andersen wrote:
> On Fri, Oct 03, 2014 at 12:47:39PM +0400, Pavel Emelyanov wrote:
>> On 10/03/2014 03:40 AM, Tycho Andersen wrote:
>>> We can't write to cpuset's root cg (indeed, we didn't create it and it has
>>> stuff in it), so ignore this special case.
>>
>> Wait a second. I though we should avoid writing the "special props"
>> to all directories that we did *not* created on restore. Is this
>> assumption correct?
> 
> Yes, that's what we should do (and you're right that this patch
> doesn't guarantee that). However, that does mean that users could set
> their cgroups up in a way that the restore can't succeed.

But isn't it the user's problem? Let's make CRIU's behavior simple and
predictable -- if the directory exists, CRIU assumes that it's already
set up properly and just doesn't touch one. Does this make sense you?

> Another issue is that if the user migrates to a node with fewer CPUs,
> the restore will fail. e.g. if they have a cpuset.cpus == 0-3, and
> they migrate to a machine with only two cores (cpuset.cpus == 0-1), we
> will try to write 0-3 in and fail.

I agree.

BTW, won't we hit the similar issue with e.g. memory? If we migrate from
large-RAM box with big memory limit to small-RAM box and set this big mem
limit so that it effectively covers all the new box's RAM? I suppose these
two are the problems of different level and we shouldn't teach CRIU to
solve them in the fully automatic way.

> I will think about a better way to do this patch. In the meantime, the
> first hunk of this diff needs to be applied since it just fixes a
> typo. I will resend that.

Thanks! :)

> Tycho
> 
>>> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
>>> ---
>>>  cgroup.c | 13 ++++++++++++-
>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cgroup.c b/cgroup.c
>>> index 29e373d..36e3487 100644
>>> --- a/cgroup.c
>>> +++ b/cgroup.c
>>> @@ -886,7 +886,7 @@ static int find_value(const char *controller, const char *path, const char *prop
>>>  			*pos = 0;
>>>  			snprintf(prefix, PATH_MAX, "%s/%s", controller, my_path);
>>>  		} else {
>>> -			snprintf(fpath, PATH_MAX, "%s", controller);
>>> +			snprintf(prefix, PATH_MAX, "%s", controller);
>>>  		}
>>>  		snprintf(fpath, PATH_MAX, "%s/%s", prefix, prop);
>>>  
>>> @@ -964,6 +964,17 @@ static int copy_special_cg_props(const char *controller, char *path)
>>>  			if (find_value(controller, path, prop, buf, prefix, missing_path) < 0)
>>>  				return -1;
>>>  
>>> +			/*
>>> +			 * If the path that was missing is "/", we should skip
>>> +			 * copying this property, since cpuset won't allow us
>>> +			 * to write to it because there are already processes
>>> +			 * there (e.g. init). The root cg had no restrictions
>>> +			 * on cpuset resources here, so ignoring this and
>>> +			 * writing nothing is safe.
>>> +			 */
>>> +			if (strcmp(missing_path, "/") == 0)
>>> +				continue;
>>> +
>>>  			if (copy_recursive(prefix, missing_path, prop, buf) < 0) {
>>>  				pr_err("copying prop %s failed\n", prop);
>>>  				return -1;
>>>
>>
> .
>