| Message ID | 20170515182704.GA15539@outlook.office365.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Andrei Vagin <avagin@virtuozzo.com> writes: > On Sun, May 14, 2017 at 04:26:18AM -0500, Eric W. Biederman wrote: >> ebiederm@xmission.com (Eric W. Biederman) writes: >> >> > Andrei Vagin <avagin@virtuozzo.com> writes: >> > >> >> Hi Eric, >> >> >> >> I found one issue about this patch. Take a look at the next script. The >> >> idea is simple, we call mount twice and than we call umount twice, but >> >> the second umount fails. >> > >> > Definitely an odd. I will take a look. >> >> After a little more looking I have to say the only thing I can see wrong >> in the behavior is that the first umount doesn't unmount everything. >> >> Changing the mount propagation tree while it is being traversed >> apparently prevents a complete traversal of the propgation tree. > > Is it be enough to find topper which will not be umounted? I attached a > patch which does this. I can't find a reason why this will not work. > > The idea of the patch is that we try to find a topper, check whether it is > marked, and if it is marked, then we try to find the next one. All > marked toppers are umounted and the first unmarked topper is attached to > the parent mount. That isn't completely wrong. But I don't believe the it is the proper logic. Fundamentally the issue is that we are reparenting while remounting. This results in mounts that should be unmounted moving before they are unmounted. Which means that if we leave all of the mnt_change_mountpoint work for a final pass over the mounts everything works correctly. Which is fabulous news. That means multiple passes through a mount propagation tree for a given mountpoint can no longer change what winds up unmounted. Which means we can skip subtrees that have already seen mount propagation. Which means the optimizations I was playing with earlier fundamentally will work without changing the semantics of MNT_DETACH. > And I think we need to add tests in tools/testing/selftests/... Feel free. Patch to sort this immediate issue out in a moment... Eric
diff --git a/fs/pnode.c b/fs/pnode.c index 5bc7896..7aefd06 100644 --- a/fs/pnode.c +++ b/fs/pnode.c @@ -435,6 +435,17 @@ static void mark_umount_candidates(struct mount *mnt) } } +static void __marked_umount(struct mount *mnt, struct mount *child) +{ + CLEAR_MNT_MARK(child); + + if (list_empty(&child->mnt_mounts)) { + list_del_init(&child->mnt_child); + child->mnt.mnt_flags |= MNT_UMOUNT; + list_move_tail(&child->mnt_list, &mnt->mnt_list); + } +} + /* * NOTE: unmounting 'mnt' naturally propagates to all other mounts its * parent propagates to. @@ -448,8 +459,13 @@ static void __propagate_umount(struct mount *mnt) for (m = propagation_next(parent, parent); m; m = propagation_next(m, parent)) { - struct mount *topper; - struct mount *child = __lookup_mnt(&m->mnt, + struct mount *topper, *topper_to_umount; + struct mount *child; + + if (m->mnt.mnt_flags & MNT_UMOUNT) + continue; + + child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint); /* * umount the child only if the child has no children @@ -457,21 +473,28 @@ static void __propagate_umount(struct mount *mnt) */ if (!child || !IS_MNT_MARKED(child)) continue; - CLEAR_MNT_MARK(child); - /* If there is exactly one mount covering all of child + /* If there is exactly one mount covering all of childV * replace child with that mount. */ - topper = find_topper(child); + topper = topper_to_umount = child; + while (1) { + topper = find_topper(topper); + if (topper == NULL) + break; + if (!IS_MNT_MARKED(topper)) + break; + topper_to_umount = topper; + } if (topper) - mnt_change_mountpoint(child->mnt_parent, child->mnt_mp, - topper); + mnt_change_mountpoint(child->mnt_parent, child->mnt_mp, topper); - if (list_empty(&child->mnt_mounts)) { - list_del_init(&child->mnt_child); - child->mnt.mnt_flags |= MNT_UMOUNT; - list_move_tail(&child->mnt_list, &mnt->mnt_list); + while (topper_to_umount != child) { + __marked_umount(mnt, topper_to_umount); + topper_to_umount = topper_to_umount->mnt_parent; } + + __marked_umount(mnt, child); } }