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

[Valerie Aurora <vaurora@redhat.com>]

On Thu, Mar 04, 2010 at 05:24:57PM +0100, Miklos Szeredi wrote:
> 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.

I agree, I'll make that change.

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

Could you elaborate more?  I don't see what's so tricky about an
atomic counter.

Thanks,

-VAL