Re: [REVIEW][PATCH 2/2] mnt: Tuck mounts under others instead of creating shadow/side mounts.

[ebiederm@xmission.com (Eric W. Biederman)]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Thu, Jan 12, 2017 at 05:19:34AM +1300, Eric W. Biederman wrote:
>
>> +		if (child->mnt.mnt_root == smp->m_dentry) {
>> +			struct mount *q;
>> +			q = __lookup_mnt(&child->mnt_parent->mnt,
>> +					 child->mnt_mountpoint);
>> +			if (q)
>> +				mnt_change_mountpoint(child, smp, q);
>> +		}
>
> This is wrong; condition will be true for *all* mounts seen by that loop.  
> Feel free to add else WARN_ON(1); to the line above and try to trigger
> it.  You are misinterpreting what propagate_mnt() and commit_tree() are
> doing - the loop in commit_tree() goes through the submounts and sets ->mnt_ns
> on those.  The of the fields is already set up by that point.  For roots
> of those copies we need to set ->mnt_hash/->mnt_child as well, but for
> all submounts it's already been done by copy_tree().  Again, commit_tree()
> is called once per secondary copy of source tree, not once per created
> mount.

*doh*

>> +static struct mount *find_topper(struct mount *mnt)
>> +{
>> +	/* 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_parent != mnt ||
>
> The first part can't happen.  Turn that into WARN_ON(child->mnt_parent != mnt)
> if you wish, but that never occurs unless the data structures are
> corrupted.

Agreed.

>> +	    child->mnt_mountpoint != mnt->mnt.mnt_root)
>> +		return NULL;
>> @@ -431,6 +459,15 @@ 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
>> +		 * replace child with that mount.
>> +		 */
>> +		topper = find_topper(child);
>> +		if (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;
>
> Umm...  With fallthrough from "is completely overmounted" case?  And
> I'm not sure I understand what that list_empty() is doing there after
> your previous semantics change - how _can_ we reach that point with
> non-empty ->mnt_mounts now?

With the semantic change to propagate_mnt_busy, to reach the list_empty
with a non-empty mnt_mounts requires the a umount(MNT_DETACH) as that
skips the propagate_mnt_busy call.

Without the semantic change it is even easier to get there.

A respin of this patch without the semantic change in a moment.

Eric