From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from out03.mta.xmission.com ([166.70.13.233]:57357 "EHLO
        out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751320AbdATHZ6 (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Fri, 20 Jan 2017 02:25:58 -0500
From: ebiederm@xmission.com (Eric W. Biederman)
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org, Andrei Vagin <avagin@virtuozzo.com>,
        Ram Pai <linuxram@us.ibm.com>
References: <20170103040052.GB1555@ZenIV.linux.org.uk>
        <87y3yr32ig.fsf@xmission.com> <87shoz32g8.fsf_-_@xmission.com>
        <87a8b6r0z5.fsf_-_@xmission.com>
        <20170107050644.GA12074@ZenIV.linux.org.uk>
        <87fukqh2we.fsf@xmission.com>
        <20170111041140.GQ1555@ZenIV.linux.org.uk>
        <87inplinxd.fsf@xmission.com> <87inplh8or.fsf_-_@xmission.com>
        <87d1fth8mh.fsf_-_@xmission.com>
        <20170112054548.GT1555@ZenIV.linux.org.uk>
Date: Fri, 20 Jan 2017 20:20:53 +1300
In-Reply-To: <20170112054548.GT1555@ZenIV.linux.org.uk> (Al Viro's message of
        "Thu, 12 Jan 2017 05:45:49 +0000")
Message-ID: <8737ge5h9m.fsf@xmission.com>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [REVIEW][PATCH 2/2] mnt: Tuck mounts under others instead of creating shadow/side mounts.
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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