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

[Tycho Andersen]

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.

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