[Linux-cluster] Re: GFS, what's remaining

[Arjan van de Ven]

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