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

[Bob Peterson]

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