lkml.org 
[lkml]   [2010]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/6] union-mount: Introduce union_mount structure and basic operations
    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


    \
     
     \ /
      Last update: 2010-03-09 20:53    [W:0.034 / U:29.476 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site