[CRIU] [PATCH] restore: copy special cpuset props recursively

[Pavel Emelyanov]

On 10/01/2014 10:48 PM, Tycho Andersen wrote:
> Hi Pavel,
> 
> On Wed, Oct 01, 2014 at 04:33:29PM +0400, Pavel Emelyanov wrote:
>> On 09/30/2014 11:50 PM, Tycho Andersen wrote:
>>> The symptom of this bug was that users restoring tasks to a nested cgroup where
>>> the top level group was created by criu (and not previously configured) e.g.
>>> cpuset:/lxc/u1 would get an ENOSPC. criu would try to copy the special
>>> properties into /lxc/u1 directly and (silently) fail, and then tried to copy
>>> the task into the cg and fail with ENOSPC:
>>>
>>> ENOSPC Attempted  to  write(2)  an empty cpuset.cpus or cpuset.mems setting to
>>>        a cpuset that has tasks attached.
>>>
>>> Fixing the silent failure to a loud failure, it gave EACCES:
>>>
>>> EACCES Attempted to add, using write(2), a CPU or memory node to a cpuset, when
>>>        that CPU or memory node was not already in its parent.
>>>
>>> So, we need to copy the the special props down the entire tree. Additionally,
>>> we shouldn't copy props directly from the top, since some intermediate point in
>>> the tree could add restrictions. We first walk back up the tree to find the
>>> first point where the props are empty, and then copy that parent's props all
>>> the way down.
>>
>> Maybe the better solution would be to move the call to copy_specual_cg_props
>> into the prepare_cgroup_dirs and populate the e.g. cpumask right after the
>> directory creation?
> 
> I played around with this just now, but it turns out we still need
> something like the code above, since we do a mkdir -p of the initial
> path in the images. It could exist and have multiple levels, so we
> still need to walk backwards to find the first one with a value
> defined, and then walk forwards to define it. I think it is probably
> best to merge this as-is, the other way seems more complicated to me.

Ah, you mean that could be directories with empty masks and even
if we do mkdirp() and just don't create them we're still in trouble?

> Tycho
> 
>>> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
>>> ---
>>>  cgroup.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++--------------
>>>  1 file changed, 88 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/cgroup.c b/cgroup.c
>>> index fc220c9..26b0a17 100644
>>> --- a/cgroup.c
>>> +++ b/cgroup.c
>>> @@ -868,42 +868,106 @@ static const char *special_cpuset_props[] = {
>>>  	NULL,
>>>  };
>>>  
>>> -static int copy_special_cg_props(const char *controller, const char *path)
>>> +static int find_value(const char *controller, const char *path, const char *prop,
>>> +			char *value, char *prefix, char *missing_path)
>>>  {
>>> -	int cg = get_service_fd(CGROUP_YARD);
>>> +	char fpath[PATH_MAX], my_path[PATH_MAX];
>>> +	char *pos;
>>> +	int ret, cg;
>>>  
>>> +	cg = get_service_fd(CGROUP_YARD);
>>> +	strncpy(my_path, path, PATH_MAX);
>>> +
>>> +	while (1) {
>>> +		FILE *f;
>>> +
>>> +		pos = strrchr(my_path, '/');
>>> +		if (pos) {
>>> +			*pos = 0;
>>> +			snprintf(prefix, PATH_MAX, "%s/%s", controller, my_path);
>>> +		} else {
>>> +			snprintf(fpath, PATH_MAX, "%s", controller);
>>> +		}
>>> +		snprintf(fpath, PATH_MAX, "%s/%s", prefix, prop);
>>> +
>>> +		f = fopenat(cg, fpath, "r");
>>> +		if (!f)
>>> +			return -1;
>>> +
>>> +		ret = fscanf(f, "%1024s", value);
>>> +		fclose(f);
>>> +
>>> +		if (ret > 0) {
>>> +			strcpy(missing_path, path + (pos - my_path));
>>> +			return 0;
>>> +		} else if (!pos) {
>>> +			pr_err("didn't find a value to propogate in the root!\n");
>>> +			return -1;
>>> +		}
>>> +	}
>>> +
>>> +	return -1;
>>> +}
>>> +
>>> +static int copy_recursive(const char *prefix, char *path, const char *prop, const char *value)
>>> +{
>>> +	char fpath[PATH_MAX];
>>> +	char *pos;
>>> +	int ret, out, cg;
>>> +
>>> +	cg = get_service_fd(CGROUP_YARD);
>>> +
>>> +	/* skip the first / */
>>> +	pos = path + 1;
>>> +	while (1) {
>>> +		pos = strchr(pos, '/');
>>> +
>>> +		if (pos)
>>> +			*pos = 0;
>>> +		snprintf(fpath, PATH_MAX, "%s/%s/%s", prefix, path, prop);
>>> +		if (pos) {
>>> +			*pos = '/';
>>> +			pos++;
>>> +		}
>>> +
>>> +		out = openat(cg, fpath, O_RDWR);
>>> +		if (out < 0) {
>>> +			pr_perror("couldn't open cg prop at %s\n", fpath);
>>> +			return -1;
>>> +		}
>>> +
>>> +		ret = write(out, value, strlen(value));
>>> +		close(out);
>>> +
>>> +		if (ret < 0) {
>>> +			pr_perror("copying property %s to %s failed\n", prop, fpath);
>>> +			return -1;
>>> +		}
>>> +
>>> +		if (!pos)
>>> +			break;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int copy_special_cg_props(const char *controller, char *path)
>>> +{
>>>  	pr_info("copying special cg props for %s\n", controller);
>>>  
>>>  	if (strstr(controller, "cpuset")) {
>>>  		int i;
>>>  
>>>  		for (i = 0; special_cpuset_props[i]; i++) {
>>> -			char fpath[PATH_MAX], buf[1024];
>>> +			char buf[1024], prefix[PATH_MAX], missing_path[PATH_MAX];
>>>  			const char *prop = special_cpuset_props[i];
>>> -			int ret;
>>> -			FILE *f;
>>> -
>>> -			snprintf(fpath, PATH_MAX, "%s/%s", controller, prop);
>>> -			f = fopenat(cg, fpath, "r");
>>> -			if (!f)
>>> +			if (find_value(controller, path, prop, buf, prefix, missing_path) < 0)
>>>  				return -1;
>>>  
>>> -			ret = fscanf(f, "%1024s", buf);
>>> -			fclose(f);
>>> -
>>> -			if (ret <= 0) {
>>> -				continue;
>>> -			}
>>> -
>>> -			pr_info("copying %s for %s/%s\n", buf, controller, prop);
>>> -
>>> -			snprintf(fpath, PATH_MAX, "%s/%s/%s", controller, path, prop);
>>> -			f = fopenat(cg, fpath, "w");
>>> -			if (!f)
>>> +			if (copy_recursive(prefix, missing_path, prop, buf) < 0) {
>>> +				pr_err("copying prop %s failed\n", prop);
>>>  				return -1;
>>> -
>>> -			fprintf(f, "%s", buf);
>>> -			fclose(f);
>>> +			}
>>>  		}
>>>  	}
>>>  
>>>
>>
> .
>