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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux