[Cluster-devel] [PATCH 1/3] GFS2: Use rbtree for resource groups and clean up bitmap buffer ref count scheme

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Wed, 2011-09-07 at 15:42 -0400, Bob Peterson wrote:
> See below.
> 
> ----- Original Message -----
> | diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> | index 7f8af1e..00f6e3d 100644
> | --- a/fs/gfs2/rgrp.c
> | +++ b/fs/gfs2/rgrp.c
> | @@ -328,15 +329,25 @@ static inline int rgrp_contains_block(struct
> | gfs2_rgrpd *rgd, u64 block)
> | 
> | struct gfs2_rgrpd *gfs2_blk2rgrpd(struct gfs2_sbd *sdp, u64 blk)
> | {
> | - struct gfs2_rgrpd *rgd;
> | + struct rb_node **newn, *parent = NULL;
> | 
> | spin_lock(&sdp->sd_rindex_spin);
> | 
> | - list_for_each_entry(rgd, &sdp->sd_rindex_mru_list, rd_list_mru) {
> | - if (rgrp_contains_block(rgd, blk)) {
> | - list_move(&rgd->rd_list_mru, &sdp->sd_rindex_mru_list);
> | + newn = &sdp->sd_rindex_tree.rb_node;
> | +
> | + /* Figure out where to put new node */
> | + while (*newn) {
> | + struct gfs2_rgrpd *cur = rb_entry(*newn, struct gfs2_rgrpd,
> | + rd_node);
> | +
> | + parent = *newn;
> | + if (blk < cur->rd_addr)
> | + newn = &((*newn)->rb_left);
> | + else if (blk > cur->rd_data0 + cur->rd_data)
> 
> I found an off-by-one problem with how I coded this section:
> It should be:
> 
> + else if (blk >= cur->rd_data0 + cur->rd_data)
> 
> In fact, cur->rd_data0 + cur->rd_data is the start of the next
> rgrp (the next ri_addr), so without the "=" check it can land on
> the wrong rgrp.
> 
> In all normal cases, this won't be a problem: you're searching
> for a block _within_ the rgrp, which will pass the test properly.
> Where it gets into trouble is if you search the rgrps for the
> block exactly equal to ri_addr.  I don't think anything in the
> kernel does this, but I found a place in gfs2-utils gfs2_edit
> where it does.  So I definitely need to fix it in libgfs2.  I'd
> like to suggest we fix it in the kernel as well for the sake of
> keeping the functions similar.
> 
> | + newn = &((*newn)->rb_right);
> | + else {
> | spin_unlock(&sdp->sd_rindex_spin);
> | - return rgd;
> | + return cur;
> | }
> | }
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems

Here is a patch to try and resolve this issue:

From 9cea87d253461397d79706381a82c45bc6a3fa75 Mon Sep 17 00:00:00 2001
From: Steven Whitehouse <swhiteho@redhat.com>
Date: Thu, 8 Sep 2011 10:21:13 +0100
Subject: [PATCH] GFS2: Fix off-by-one in gfs2_blk2rgrpd

Bob reported:

I found an off-by-one problem with how I coded this section:
It should be:

+ else if (blk >= cur->rd_data0 + cur->rd_data)

In fact, cur->rd_data0 + cur->rd_data is the start of the next
rgrp (the next ri_addr), so without the "=" check it can land on
the wrong rgrp.

In all normal cases, this won't be a problem: you're searching
for a block _within_ the rgrp, which will pass the test properly.
Where it gets into trouble is if you search the rgrps for the
block exactly equal to ri_addr.  I don't think anything in the
kernel does this, but I found a place in gfs2-utils gfs2_edit
where it does.  So I definitely need to fix it in libgfs2.  I'd
like to suggest we fix it in the kernel as well for the sake of
keeping the functions similar.

So this patch fixes the above mentioned off by one error as well
as removing the unused parent pointer.

Reported-by: Bob Peterson <rpeterso@redhat.com>
Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 8ec4174..1daf8a7 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -329,17 +329,16 @@ static inline int rgrp_contains_block(struct gfs2_rgrpd *rgd, u64 block)
 
 struct gfs2_rgrpd *gfs2_blk2rgrpd(struct gfs2_sbd *sdp, u64 blk)
 {
-	struct rb_node **newn, *parent = NULL;
+	struct rb_node **newn;
+	struct gfs2_rgrpd *cur;
 
 	spin_lock(&sdp->sd_rindex_spin);
 	newn = &sdp->sd_rindex_tree.rb_node;
 	while (*newn) {
-		struct gfs2_rgrpd *cur = rb_entry(*newn, struct gfs2_rgrpd,
-						  rd_node);
-		parent = *newn;
+		cur = rb_entry(*newn, struct gfs2_rgrpd, rd_node);
 		if (blk < cur->rd_addr)
 			newn = &((*newn)->rb_left);
-		else if (blk > cur->rd_data0 + cur->rd_data)
+		else if (blk >= cur->rd_data0 + cur->rd_data)
 			newn = &((*newn)->rb_right);
 		else {
 			spin_unlock(&sdp->sd_rindex_spin);
-- 
1.7.4