On Mon, Jan 13, 2014 at 12:05 PM, S.Çağlar Onur <cag...@10ur.org> wrote: > Hi Serge, > > On Mon, Jan 13, 2014 at 11:24 AM, Serge Hallyn <serge.hal...@ubuntu.com> > wrote: >> Quoting S.Çağlar Onur (cag...@10ur.org): >>> Hey there, >>> >>> On Sun, Jan 12, 2014 at 7:24 PM, Stéphane Graber <stgra...@ubuntu.com> >>> wrote: >>> > On Sat, Jan 11, 2014 at 12:14:26AM -0600, Serge Hallyn wrote: >>> >> Currently when a container is shut down, lxc walks the set of all >>> >> cgroup paths it created, in reverse order, and tries to remove them. >>> >> This doesn't suffice if the container has also created new cgroups. >>> >> >>> >> It'd be impolite to recursively remove all the cgroup paths we created, >>> >> since this can include '/lxc' and thereunder all other containers >>> >> started since. >>> >> >>> >> This patch changes container shutdown to only delete the container's own >>> >> path, but do so recursively. Note that if we fail during startup, >>> >> the container won't have created any cgroup paths so it the old >>> >> way works fine. >>> >> >>> >> Signed-off-by: Serge Hallyn <serge.hal...@ubuntu.com> >>> > >>> > Acked-by: Stéphane Graber <stgra...@ubuntu.com> >>> >>> This patch started to cause my add_remove_device_node tests to fail >>> (rmdir started to return EBUSY), following seems to solve it but I'm >>> not sure whether it's a fix or just a workaround. Any ideas what might >>> be triggering that? >> >> Are you sure it was that patch? You can drop my patch and your >> test succeeds again? > > Hmm you are right, after a git bisect it pointed following
[hit send too early] and yes reverting that commit fixes the issue [caglar@qp:~] for i in seq 100; do sudo lxc-test-device-add-remove; done lxc_container: rmdir failed Removing /dev/network_latency from the container (device_add_remove_test) failed... lxc_container: Container /var/lib/lxc:device_add_remove_test already exists Creating the container (device_add_remove_test) failed... [caglar@qp:~] sudo lxc-stop -n device_add_remove_test [caglar@qp:~] sudo lxc-destroy -n device_add_remove_test [revert/install etc.] [caglar@qp:~] for i in seq 100; do sudo lxc-test-device-add-remove; done [caglar@qp:~] > df2d4205073d3f57543951ca7ffabf891b230634 is the first bad commit > commit df2d4205073d3f57543951ca7ffabf891b230634 > Author: Dwight Engen <dwight.en...@oracle.com> > Date: Thu Jan 9 15:36:13 2014 -0500 > > ensure all config items are duplicated on clone/write_config > > Since previously I had found a config item that wasn't being propagated > by lxc-clone, I went through all the config items and made sure that: > a) Each item is documented in lxc.conf > b) Each item is written out by write_config > > The only one that isn't is lxc.include, which by its nature only pulls > in other config item types. > > Signed-off-by: Dwight Engen <dwight.en...@oracle.com> > Acked-by: Serge E. Hallyn <serge.hal...@ubuntu.com> > >>> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c >>> index 48fef74..80e68fb 100644 >>> --- a/src/lxc/lxccontainer.c >>> +++ b/src/lxc/lxccontainer.c >>> @@ -2934,7 +2934,7 @@ static bool add_remove_device_node(struct >>> lxc_container *c, const char *src_path >>> ERROR("unlink failed"); >>> goto out; >>> } >>> - if (rmdir(directory_path) < 0 && errno != ENOTEMPTY) { >>> + if (rmdir(directory_path) < 0 && errno != ENOTEMPTY && >>> errno != EBUSY) { >>> ERROR("rmdir failed"); >>> goto out; >>> } >>> >>> >>> >> --- >>> >> src/lxc/cgroup.c | 101 >>> >> ++++++++++++++++++++++++++++++++++++++++++++++++------- >>> >> 1 file changed, 88 insertions(+), 13 deletions(-) >>> >> >>> >> diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c >>> >> index b1196b4..be43c73 100644 >>> >> --- a/src/lxc/cgroup.c >>> >> +++ b/src/lxc/cgroup.c >>> >> @@ -68,9 +68,9 @@ static char **subsystems_from_mount_options(const char >>> >> *mount_options, char **ke >>> >> static void lxc_cgroup_mount_point_free(struct cgroup_mount_point *mp); >>> >> static void lxc_cgroup_hierarchy_free(struct cgroup_hierarchy *h); >>> >> static bool is_valid_cgroup(const char *name); >>> >> -static int create_or_remove_cgroup(bool remove, struct >>> >> cgroup_mount_point *mp, const char *path); >>> >> +static int create_or_remove_cgroup(bool remove, struct >>> >> cgroup_mount_point *mp, const char *path, int recurse); >>> >> static int create_cgroup(struct cgroup_mount_point *mp, const char >>> >> *path); >>> >> -static int remove_cgroup(struct cgroup_mount_point *mp, const char >>> >> *path); >>> >> +static int remove_cgroup(struct cgroup_mount_point *mp, const char >>> >> *path, bool recurse); >>> >> static char *cgroup_to_absolute_path(struct cgroup_mount_point *mp, >>> >> const char *path, const char *suffix); >>> >> static struct cgroup_process_info *find_info_for_subsystem(struct >>> >> cgroup_process_info *info, const char *subsystem); >>> >> static int do_cgroup_get(const char *cgroup_path, const char >>> >> *sub_filename, char *value, size_t len); >>> >> @@ -81,6 +81,75 @@ static int cgroup_recursive_task_count(const char >>> >> *cgroup_path); >>> >> static int count_lines(const char *fn); >>> >> static int handle_cgroup_settings(struct cgroup_mount_point *mp, char >>> >> *cgroup_path); >>> >> >>> >> +static int cgroup_rmdir(char *dirname) >>> >> +{ >>> >> + struct dirent dirent, *direntp; >>> >> + int saved_errno = 0; >>> >> + DIR *dir; >>> >> + int ret, failed=0; >>> >> + char pathname[MAXPATHLEN]; >>> >> + >>> >> + dir = opendir(dirname); >>> >> + if (!dir) { >>> >> + ERROR("%s: failed to open %s", __func__, dirname); >>> >> + return -1; >>> >> + } >>> >> + >>> >> + while (!readdir_r(dir, &dirent, &direntp)) { >>> >> + struct stat mystat; >>> >> + int rc; >>> >> + >>> >> + if (!direntp) >>> >> + break; >>> >> + >>> >> + if (!strcmp(direntp->d_name, ".") || >>> >> + !strcmp(direntp->d_name, "..")) >>> >> + continue; >>> >> + >>> >> + rc = snprintf(pathname, MAXPATHLEN, "%s/%s", dirname, >>> >> direntp->d_name); >>> >> + if (rc < 0 || rc >= MAXPATHLEN) { >>> >> + ERROR("pathname too long"); >>> >> + failed=1; >>> >> + if (!saved_errno) >>> >> + saved_errno = -ENOMEM; >>> >> + continue; >>> >> + } >>> >> + ret = lstat(pathname, &mystat); >>> >> + if (ret) { >>> >> + SYSERROR("%s: failed to stat %s", __func__, >>> >> pathname); >>> >> + failed=1; >>> >> + if (!saved_errno) >>> >> + saved_errno = errno; >>> >> + continue; >>> >> + } >>> >> + if (S_ISDIR(mystat.st_mode)) { >>> >> + if (cgroup_rmdir(pathname) < 0) { >>> >> + if (!saved_errno) >>> >> + saved_errno = errno; >>> >> + failed=1; >>> >> + } >>> >> + } >>> >> + } >>> >> + >>> >> + if (rmdir(dirname) < 0) { >>> >> + SYSERROR("%s: failed to delete %s", __func__, dirname); >>> >> + if (!saved_errno) >>> >> + saved_errno = errno; >>> >> + failed=1; >>> >> + } >>> >> + >>> >> + ret = closedir(dir); >>> >> + if (ret) { >>> >> + SYSERROR("%s: failed to close directory %s", __func__, >>> >> dirname); >>> >> + if (!saved_errno) >>> >> + saved_errno = errno; >>> >> + failed=1; >>> >> + } >>> >> + >>> >> + errno = saved_errno; >>> >> + return failed ? -1 : 0; >>> >> +} >>> >> + >>> >> struct cgroup_meta_data *lxc_cgroup_load_meta() >>> >> { >>> >> const char *cgroup_use = NULL; >>> >> @@ -745,7 +814,7 @@ extern struct cgroup_process_info >>> >> *lxc_cgroup_create(const char *name, const cha >>> >> * In that case, remove the cgroup from all previous >>> >> hierarchies >>> >> */ >>> >> for (j = 0, info_ptr = base_info; j < i && info_ptr; >>> >> info_ptr = info_ptr->next, j++) { >>> >> - r = >>> >> remove_cgroup(info_ptr->designated_mount_point, >>> >> info_ptr->created_paths[info_ptr->created_paths_count - 1]); >>> >> + r = >>> >> remove_cgroup(info_ptr->designated_mount_point, >>> >> info_ptr->created_paths[info_ptr->created_paths_count - 1], false); >>> >> if (r < 0) >>> >> WARN("could not clean up cgroup we created >>> >> when trying to create container"); >>> >> >>> >> free(info_ptr->created_paths[info_ptr->created_paths_count - 1]); >>> >> @@ -1039,8 +1108,7 @@ void >>> >> lxc_cgroup_process_info_free_and_remove(struct cgroup_process_info *info) >>> >> if (!info) >>> >> return; >>> >> next = info->next; >>> >> - for (pp = info->created_paths; pp && *pp; pp++); >>> >> - for ((void)(pp && --pp); info->created_paths && pp >= >>> >> info->created_paths; --pp) { >>> >> + { >>> >> struct cgroup_mount_point *mp = >>> >> info->designated_mount_point; >>> >> if (!mp) >>> >> mp = lxc_cgroup_find_mount_point(info->hierarchy, >>> >> info->cgroup_path, true); >>> >> @@ -1049,7 +1117,10 @@ void >>> >> lxc_cgroup_process_info_free_and_remove(struct cgroup_process_info *info) >>> >> * '/lxc' cgroup in this container but another >>> >> container >>> >> * is still running (for example) >>> >> */ >>> >> - (void)remove_cgroup(mp, *pp); >>> >> + (void)remove_cgroup(mp, info->cgroup_path, true); >>> >> + } >>> >> + for (pp = info->created_paths; pp && *pp; pp++); >>> >> + for ((void)(pp && --pp); info->created_paths && pp >= >>> >> info->created_paths; --pp) { >>> >> free(*pp); >>> >> } >>> >> free(info->created_paths); >>> >> @@ -1609,7 +1680,7 @@ bool is_valid_cgroup(const char *name) >>> >> return strcmp(name, ".") != 0 && strcmp(name, "..") != 0; >>> >> } >>> >> >>> >> -int create_or_remove_cgroup(bool do_remove, struct cgroup_mount_point >>> >> *mp, const char *path) >>> >> +int create_or_remove_cgroup(bool do_remove, struct cgroup_mount_point >>> >> *mp, const char *path, int recurse) >>> >> { >>> >> int r, saved_errno = 0; >>> >> char *buf = cgroup_to_absolute_path(mp, path, NULL); >>> >> @@ -1617,9 +1688,13 @@ int create_or_remove_cgroup(bool do_remove, >>> >> struct cgroup_mount_point *mp, const >>> >> return -1; >>> >> >>> >> /* create or remove directory */ >>> >> - r = do_remove ? >>> >> - rmdir(buf) : >>> >> - mkdir(buf, 0777); >>> >> + if (do_remove) { >>> >> + if (recurse) >>> >> + r = cgroup_rmdir(buf); >>> >> + else >>> >> + r = rmdir(buf); >>> >> + } else >>> >> + r = mkdir(buf, 0777); >>> >> saved_errno = errno; >>> >> free(buf); >>> >> errno = saved_errno; >>> >> @@ -1628,12 +1703,12 @@ int create_or_remove_cgroup(bool do_remove, >>> >> struct cgroup_mount_point *mp, const >>> >> >>> >> int create_cgroup(struct cgroup_mount_point *mp, const char *path) >>> >> { >>> >> - return create_or_remove_cgroup(false, mp, path); >>> >> + return create_or_remove_cgroup(false, mp, path, false); >>> >> } >>> >> >>> >> -int remove_cgroup(struct cgroup_mount_point *mp, const char *path) >>> >> +int remove_cgroup(struct cgroup_mount_point *mp, const char *path, bool >>> >> recurse) >>> >> { >>> >> - return create_or_remove_cgroup(true, mp, path); >>> >> + return create_or_remove_cgroup(true, mp, path, recurse); >>> >> } >>> >> >>> >> char *cgroup_to_absolute_path(struct cgroup_mount_point *mp, const char >>> >> *path, const char *suffix) >>> >> -- >>> >> 1.8.5.2 >>> >> >>> >> _______________________________________________ >>> >> lxc-devel mailing list >>> >> lxc-devel@lists.linuxcontainers.org >>> >> http://lists.linuxcontainers.org/listinfo/lxc-devel >>> > >>> > -- >>> > Stéphane Graber >>> > Ubuntu developer >>> > http://www.ubuntu.com >>> > >>> > _______________________________________________ >>> > lxc-devel mailing list >>> > lxc-devel@lists.linuxcontainers.org >>> > http://lists.linuxcontainers.org/listinfo/lxc-devel >>> > >>> >>> >>> >>> -- >>> S.Çağlar Onur <cag...@10ur.org> >>> _______________________________________________ >>> lxc-devel mailing list >>> lxc-devel@lists.linuxcontainers.org >>> http://lists.linuxcontainers.org/listinfo/lxc-devel >> _______________________________________________ >> lxc-devel mailing list >> lxc-devel@lists.linuxcontainers.org >> http://lists.linuxcontainers.org/listinfo/lxc-devel > > > > -- > S.Çağlar Onur <cag...@10ur.org> -- S.Çağlar Onur <cag...@10ur.org> _______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel