lkml.org 
[lkml]   [2009]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 03/21] ceph: client types
    On Tue, 29 Sep 2009, Andrew Morton wrote:

    > On Tue, 22 Sep 2009 10:38:31 -0700
    > Sage Weil <sage@newdream.net> wrote:
    >
    > > We first define constants, types, and prototypes for the kernel client
    > > proper.
    > >
    > > A few subsystems are defined separately later: the MDS, OSD, and
    > > monitor clients, and the messaging layer.
    > >
    > >
    > > ...
    > >
    > > +static inline bool ceph_i_test(struct inode *inode, unsigned mask)
    > > +{
    > > + struct ceph_inode_info *ci = ceph_inode(inode);
    > > + bool r;
    > > +
    > > + spin_lock(&inode->i_lock);
    > > + r = (ci->i_ceph_flags & mask) == mask;
    > > + spin_unlock(&inode->i_lock);
    > > + return r;
    > > +}
    >
    > I'm not sure that the locking in ceph_i_test() makes much sense. The
    > condition we just checked could change one femtosecond after we dropped
    > the lock, so why take the lock?

    Right. Would I need an smb_mb() or something in it's place?



    > > +
    > > +/* find a specific frag @f */
    > > +static inline struct ceph_inode_frag *
    > > +__ceph_find_frag(struct ceph_inode_info *ci, u32 f)
    > > +{
    > > + struct rb_node *n = ci->i_fragtree.rb_node;
    > > +
    > > + while (n) {
    > > + struct ceph_inode_frag *frag =
    > > + rb_entry(n, struct ceph_inode_frag, node);
    > > + int c = frag_compare(f, frag->frag);
    > > + if (c < 0)
    > > + n = n->rb_left;
    > > + else if (c > 0)
    > > + n = n->rb_right;
    > > + else
    > > + return frag;
    > > + }
    > > + return NULL;
    > > +}
    >
    > Please review the entire fs and verify that all inlining decisions were
    > appropriate. Unless this function has a single call site, this
    > decision wasn't appropriate.
    >
    > And if it _does_ have a single callsite, it should be defined in the
    > relevant .c file, not in .h

    Yep!

    > > +/*
    > > + * choose fragment for value @v. copy frag content to pfrag, if leaf
    > > + * exists
    > > + */
    > > +extern u32 ceph_choose_frag(struct ceph_inode_info *ci, u32 v,
    > > + struct ceph_inode_frag *pfrag,
    > > + int *found);
    > > +
    > >
    > > ...
    > >
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    >
    >


    \
     
     \ /
      Last update: 2009-09-30 19:45    [W:0.034 / U:209.704 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site