On Tue, 22 Sep 2009 10:38:31 -0700 Sage Weil <sage@xxxxxxxxxxxx> 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? > + > +/* 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 > +/* > + * 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html