Re: [PATCH 1/6] union-mount: Introduce union_mount structure and basic operations

[Valerie Aurora <vaurora@redhat.com>]

On Wed, Mar 03, 2010 at 06:33:20PM +0100, Miklos Szeredi wrote:
> On Tue,  2 Mar 2010, Valerie Aurora wrote:
> > +struct union_mount *union_alloc(struct dentry *this, struct vfsmount *this_mnt,
> > +				struct dentry *next, struct vfsmount *next_mnt)
> 
> 
> Why doesn't union_alloc, append_to_union, union_lookup,
> union_down_one, etc use "struct path *" arg instead of separate
> vfsmount and dentry pointers?

I'd prefer that too, but it isn't a clear win.  For append_to_union(),
the reason is that we call it when a file system is mounted, using mnt
and mnt->mnt_root as the first args:

int attach_mnt_union(struct vfsmount *mnt, struct vfsmount *dest_mnt,
		     struct dentry *dest_dentry)
{
	if (!IS_MNT_UNION(mnt))
		return 0;

	return append_to_union(mnt, mnt->mnt_root, dest_mnt, dest_dentry);
}

Same thing happens in detach_mnt_union() with union_lookup().  That
trickles down into the rest.  I suppose I could create a temporary
path variable for those two functions and then we'd be paths
everywhere else.  What do you think?

> > +     um = kmem_cache_alloc(union_cache, GFP_ATOMIC);
> > +     if (!um)
> > +             return NULL;
> > +
> > +     atomic_set(&um->u_count, 1);
> 
> Why is u_count not a "struct kref"?

We stole this from the inode cache code, so for the same reason inodes
have i_count as atomic_t instead of a kref (whatever that is). :)

> > > +/*
> > + * WARNING! Confusing terminology alert.
> > + *
> > + * Note that the directions "up" and "down" in union mounts are the
> > + * opposite of "up" and "down" in normal VFS operation terminology.
> > + * "up" in the rest of the VFS means "towards the root of the mount
> > + * tree."  If you mount B on top of A, following B "up" will get you
> > + * A.  In union mounts, "up" means "towards the most recently mounted
> > + * layer of the union stack."  If you union mount B on top of A,
> > + * following A "up" will get you to B.  Another way to put it is that
> > + * "up" in the VFS means going from this mount towards the direction
> > + * of its mnt->mnt_parent pointer, but "up" in union mounts means
> > + * going in the opposite direction (until you run out of union
> > + * layers).
> > + */
> 
> So if this is confusing, why not use a different terminology for union
> layers?  Like "next" and "prev" like it is already used in the
> structures.

Unfortunately, "upper" and "lower" are fairly well established
concepts in layering file systems and seem to be the most natural way
for programmers to think about unioned file systems.  It's only the
VFS (which most people never touch) that uses "up" and "down" in the
opposite sense.  I think the better path is to replace "next" and
"prev" in the structure.

-VAL