From mboxrd@z Thu Jan  1 00:00:00 1970
From: ebiederm@xmission.com (Eric W. Biederman)
Subject: Re: [PATCH review  16/19] mnt: Track which mounts use a dentry as root.
Date: Tue, 07 Apr 2015 15:22:52 -0500
Message-ID: <87egnvelib.fsf@x220.int.ebiederm.org>
References: <87a8yqou41.fsf_-_@x220.int.ebiederm.org>
	<1428026183-14879-16-git-send-email-ebiederm@xmission.com>
	<20150403055449.GE889@ZenIV.linux.org.uk>
Mime-Version: 1.0
Content-Type: text/plain
Cc: Linux Containers <containers@lists.linux-foundation.org>,
	linux-fsdevel@vger.kernel.org,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Andy Lutomirski <luto@amacapital.net>,
	Richard Weinberger <richard@nod.at>,
	Andrey Vagin <avagin@openvz.org>, Jann Horn <jann@thejh.net>,
	Willy Tarreau <w@1wt.eu>, Omar Sandoval <osandov@osandov.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from out02.mta.xmission.com ([166.70.13.232]:48608 "EHLO
	out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753264AbbDGU07 (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Tue, 7 Apr 2015 16:26:59 -0400
In-Reply-To: <20150403055449.GE889@ZenIV.linux.org.uk> (Al Viro's message of
	"Fri, 3 Apr 2015 06:54:50 +0100")
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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

> On Thu, Apr 02, 2015 at 08:56:20PM -0500, Eric W. Biederman wrote:
>
> One general note - I'd probably put a pointer to that sucker into struct
> mount.  For one thing, root-preserving clone_mnt() is a fairly common
> case.  For another, searching for that thing in mnt_put_root() looks
> wrong.  Matter of taste, but...

So I just played with the possibilities and adding a field in struct
mount makes the code more complicated not less.  So I have developed a
distaste for the idea of having a struct mountroot pointer in struct mount.

It especially complicates clone_mnt where I always have to have the code
look at the dentry to find the associated a dentry.  Resuing a current
struct mountroot and/or preallocating one is just a complicated mess.
The current implementation has a much more localized (and thus
understandable and maintainable) implementation.


> Another thing is that IMO it's better to preallocate that thing in
> vfs_kern_mount() and free if it turns out to be unused.  Simpler cleanup
> path that way...

It is a touch cleaner in vfs_kern_mount (not as many things need to be
freed) and much uglier and in clone_mnt.

>> -	mnt->mnt.mnt_root = root;
>> +	err = mnt_set_root(mnt, root);
>> +	if (err) {
>> +		dput(mnt->mnt.mnt_root);
>
> 	Unless I'm misreading your code, mnt_set_root() does *not* set it
> on failure, so what's going on here?

Typo.  That should simply have been dput(root);

Eric