From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zeniv.linux.org.uk ([195.92.253.2]:60116 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750698AbdALFpv (ORCPT ); Thu, 12 Jan 2017 00:45:51 -0500 Date: Thu, 12 Jan 2017 05:45:49 +0000 From: Al Viro To: "Eric W. Biederman" Cc: linux-fsdevel@vger.kernel.org, Andrei Vagin , Ram Pai Subject: Re: [REVIEW][PATCH 2/2] mnt: Tuck mounts under others instead of creating shadow/side mounts. Message-ID: <20170112054548.GT1555@ZenIV.linux.org.uk> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87d1fth8mh.fsf_-_@xmission.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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. > +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. > + child->mnt_mountpoint != mnt->mnt.mnt_root) > + return NULL; > @@ -342,7 +358,7 @@ static inline int do_refcount_check(struct mount *mnt, int count) > */ > int propagate_mount_busy(struct mount *mnt, int refcnt) > { > - struct mount *m, *child; > + struct mount *m, *child, *topper; > struct mount *parent = mnt->mnt_parent; > > if (mnt == parent) > @@ -358,11 +374,21 @@ int propagate_mount_busy(struct mount *mnt, int refcnt) > > for (m = propagation_next(parent, parent); m; > m = propagation_next(m, parent)) { > - child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint); > + int count = 1; > + child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint); > if (!child) > continue; > - if (!list_empty(&child->mnt_mounts) || > - do_refcount_check(child, 1)) > + > + /* Is there exactly one mount on the child that covers > + * it completely whose reference should be ignored? > + */ > + topper = find_topper(child); > + if (topper) > + count += 1; > + else if (!list_empty(&child->mnt_mounts)) > + return 1; > + > + if (do_refcount_check(child, count)) > return 1; Again, subject to the comments re semantics change (see the reply to previous patch). > @@ -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?