[PATCH review 16/19] mnt: Track which mounts use a dentry as root.

Eric W. Biederman ebiederm at xmission.com
Tue Apr 7 20:22:52 UTC 2015


Al Viro <viro at 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


More information about the Containers mailing list