Messages in this thread | | | Subject | Re: GFS, what's remaining | From | Arjan van de Ven <> | Date | Thu, 01 Sep 2005 13:35:23 +0200 |
| |
On Thu, 2005-09-01 at 18:46 +0800, David Teigland wrote: > Hi, this is the latest set of gfs patches, it includes some minor munging > since the previous set. Andrew, could this be added to -mm? there's not > much in the way of pending changes. > > http://redhat.com/~teigland/gfs2/20050901/gfs2-full.patch > http://redhat.com/~teigland/gfs2/20050901/broken-out/
+static inline void glock_put(struct gfs2_glock *gl) +{ + if (atomic_read(&gl->gl_count) == 1) + gfs2_glock_schedule_for_reclaim(gl); + gfs2_assert(gl->gl_sbd, atomic_read(&gl->gl_count) > 0,); + atomic_dec(&gl->gl_count); +}
this code has a race
what is gfs2_assert() about anyway? please just use BUG_ON directly everywhere
+static inline int queue_empty(struct gfs2_glock *gl, struct list_head *head) +{ + int empty; + spin_lock(&gl->gl_spin); + empty = list_empty(head); + spin_unlock(&gl->gl_spin); + return empty; +}
that looks like a racey interface to me... if so.. why bother locking at all? +void gfs2_glock_hold(struct gfs2_glock *gl) +{ + glock_hold(gl); +}
eh why?
+struct gfs2_holder *gfs2_holder_get(struct gfs2_glock *gl, unsigned int state, + int flags, int gfp_flags) +{ + struct gfs2_holder *gh; + + gh = kmalloc(sizeof(struct gfs2_holder), GFP_KERNEL | gfp_flags);
this looks odd. Either you take flags or you don't.. this looks really half arsed and thus is really surprising to all callers
static int gi_skeleton(struct gfs2_inode *ip, struct gfs2_ioctl *gi, + gi_filler_t filler) +{ + unsigned int size = gfs2_tune_get(ip->i_sbd, gt_lockdump_size); + char *buf; + unsigned int count = 0; + int error; + + if (size > gi->gi_size) + size = gi->gi_size; + + buf = kmalloc(size, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + error = filler(ip, gi, buf, size, &count); + if (error) + goto out; + + if (copy_to_user(gi->gi_data, buf, count + 1)) + error = -EFAULT;
where does count get a sensible value?
+static unsigned int handle_roll(atomic_t *a) +{ + int x = atomic_read(a); + if (x < 0) { + atomic_set(a, 0); + return 0; + } + return (unsigned int)x; +}
this is just plain scary.
you'll have to post the rest of your patches if you want anyone to look at them...
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |