Ram Pai <linuxram@xxxxxxxxxx> writes:
> Shared-subtree has had many cases where unmount operation would leave a lot of
> residual mounts, because it was way too difficult to remove them. Eric
> Biederman came up with the idea of tucked mount which relieved the situation
> tremendously. But it still lacks the reversibility property -- A mount
> followed by a umount should reverse the mount-tree to the exact same state as
> before the mount.
There is a lot here so I am taking my time an digging through it all.
I have found two issues that I am going to point out before I comment
on the big picture.
> diff --git a/fs/pnode.c b/fs/pnode.c
> index 5bc7896..7dfbe9c 100644
> --- a/fs/pnode.c
> +++ b/fs/pnode.c
> @@ -322,19 +322,9 @@ int propagate_mnt(struct mount *dest_mnt, struct mountpoint *dest_mp,
> return ret;
> }
>
> -static struct mount *find_topper(struct mount *mnt)
> +static struct mount *find_covering_child(struct mount *m)
> {
> - /* If there is exactly one mount covering mnt completely return it. */
> - struct mount *child;
> -
> - if (!list_is_singular(&mnt->mnt_mounts))
> - return NULL;
> -
> - child = list_first_entry(&mnt->mnt_mounts, struct mount, mnt_child);
> - if (child->mnt_mountpoint != mnt->mnt.mnt_root)
> - return NULL;
> -
> - return child;
> + return __lookup_mnt(&m->mnt, m->mnt.mnt_root);
> }
Unless there is some reason you can't use mnt_mounts using
__lookup_mnt is a pessimization here as __lookup_mnt can go slowly
when the has chains get long.
> /*
> @@ -357,8 +347,10 @@ static inline int do_refcount_check(struct mount *mnt, int count)
> */
> int propagate_mount_busy(struct mount *mnt, int refcnt)
> {
> - struct mount *m, *child, *topper;
> + struct mount *m, *child;
> struct mount *parent = mnt->mnt_parent;
> + bool tucks_can_ignore = !(IS_MNT_TUCK_END(parent) &&
> + mnt->mnt_mountpoint == parent->mnt.mnt_root);
>
> if (mnt == parent)
> return do_refcount_check(mnt, refcnt);
> @@ -372,19 +364,19 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
> return 1;
>
> for (m = propagation_next(parent, parent); m;
> - m = propagation_next(m, parent)) {
> + m = propagation_next(m, parent)) {
> int count = 1;
> - child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
> - if (!child)
> - continue;
>
> - /* Is there exactly one mount on the child that covers
> - * it completely whose reference should be ignored?
> + /*
> + * read the comment in __propagate_umount()
> */
> - topper = find_topper(child);
> - if (topper)
> - count += 1;
> - else if (!list_empty(&child->mnt_mounts))
> + if (tucks_can_ignore &&
> + IS_MNT_TUCK_END(m) &&
> + (mnt->mnt_mountpoint == m->mnt.mnt_root))
> + continue;
> +
> + child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
> + if (!child || !list_empty(&child->mnt_mounts))
> continue;
>
> if (do_refcount_check(child, count))
Unless I am misreading this code you are skipping a refcount check that
we should be performing.
Eric
