Date:	Thu, 04 Mar 2010 17:24:57 +0100
From:	Miklos Szeredi <miklos@...redi.hu>
To:	Valerie Aurora <vaurora@...hat.com>
CC:	miklos@...redi.hu, viro@...iv.linux.org.uk, hch@...radead.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	jblunck@...e.de
Subject: Re: [PATCH 1/6] union-mount: Introduce union_mount structure and basic operations

On Wed, 3 Mar 2010, Valerie Aurora wrote:
> 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?

If it's just two temporary vars, then IMO it's a win.  It's much
easier to read the functions if it has half the arguments.

> 
> > > +     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). :)

i_count does some tricky things.  If you just want plain an simple
refcounting then you should be using krefs.

> > > > +/*
> > > + * 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.

Okay.

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists