GFS2: Block alloc fixes

GFS2: Block alloc fixes

[Steven Whitehouse]

These three patches fix a couple of bugs in the block allocation
code that was updated at the merge window. Thanks are due to
Benny Halevy, Christoph Lameter and Willy Tarreau for looking
at the __ffs64 patch. I've updated the comment in that patch
in the light of Willy Tarreau's suggestion,

Steve.

[PATCH 1/3] bitops: Add __ffs64 bitop

[Steven Whitehouse]

Finds the first set bit in a 64 bit word. This is required in order
to fix a bug in GFS2, but I think it should be a generic function
in case of future users.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
Reviewed-by: Christoph Lameter <cl@linux.com>
Reviewed-by: Willy Tarreau <w@1wt.eu>

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 6182913..c05a29c 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -112,6 +112,25 @@ static inline unsigned fls_long(unsigned long l)
 	return fls64(l);
 }
 
+/**
+ * __ffs64 - find first set bit in a 64 bit word
+ * @word: The 64 bit word
+ *
+ * On 64 bit arches this is a synomyn for __ffs
+ * The result is not defined if no bits are set, so check that @word
+ * is non-zero before calling this.
+ */
+static inline unsigned long __ffs64(u64 word)
+{
+#if BITS_PER_LONG == 32
+	if (((u32)word) == 0UL)
+		return __ffs((u32)(word >> 32)) + 32;
+#elif BITS_PER_LONG != 64
+#error BITS_PER_LONG not 32 or 64
+#endif
+	return __ffs((unsigned long)word);
+}
+
 #ifdef __KERNEL__
 #ifdef CONFIG_GENERIC_FIND_FIRST_BIT
 
-- 
1.6.0.6

[PATCH 2/3] GFS2: Fix bug in block allocation

[Steven Whitehouse]

The new bitfit algorithm was counting from the wrong end of
64 bit words in the bitfield. This fixes it by using __ffs64
instead of fls64

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index f03d024..c9786a4 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -212,8 +212,7 @@ static u32 gfs2_bitfit(const u8 *buf, const unsigned int len,
 	if (tmp == 0)
 		return BFITNOENT;
 	ptr--;
-	bit = fls64(tmp);
-	bit--;		/* fls64 always adds one to the bit count */
+	bit = __ffs64(tmp);
 	bit /= 2;	/* two bits per entry in the bitmap */
 	return (((const unsigned char *)ptr - buf) * GFS2_NBBY) + bit;
 }
-- 
1.6.0.6

[PATCH 3/3] GFS2: Ensure that the inode goal block settings are updated

[Steven Whitehouse]

GFS2 has a goal block associated with each inode indicating the
search start position for future block allocations (in fact there
are two, but thats for backward compatibility with GFS1 as they
are set to identical locations in GFS2).

In some circumstances, depending on the ordering of updates to
the inode it was possible for the goal block settings to not
be updated on disk. This patch ensures that the goal block will
always get updated, thus reducing the potential for searching
the same (already allocated) blocks again when looking for free
space during block allocation.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index c9786a4..5650382 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1444,10 +1444,12 @@ static struct gfs2_rgrpd *rgblk_free(struct gfs2_sbd *sdp, u64 bstart,
 u64 gfs2_alloc_block(struct gfs2_inode *ip, unsigned int *n)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
+	struct buffer_head *dibh;
 	struct gfs2_alloc *al = ip->i_alloc;
 	struct gfs2_rgrpd *rgd = al->al_rgd;
 	u32 goal, blk;
 	u64 block;
+	int error;
 
 	if (rgrp_contains_block(rgd, ip->i_goal))
 		goal = ip->i_goal - rgd->rd_data0;
@@ -1460,7 +1462,13 @@ u64 gfs2_alloc_block(struct gfs2_inode *ip, unsigned int *n)
 	rgd->rd_last_alloc = blk;
 	block = rgd->rd_data0 + blk;
 	ip->i_goal = block;
-
+	error = gfs2_meta_inode_buffer(ip, &dibh);
+	if (error == 0) {
+		struct gfs2_dinode *di = (struct gfs2_dinode *)dibh->b_data;
+		gfs2_trans_add_bh(ip->i_gl, dibh, 1);
+		di->di_goal_meta = di->di_goal_data = cpu_to_be64(ip->i_goal);
+		brelse(dibh);
+	}
 	gfs2_assert_withdraw(sdp, rgd->rd_free >= *n);
 	rgd->rd_free -= *n;
 
-- 
1.6.0.6

Re: [PATCH 1/3] bitops: Add __ffs64 bitop

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On Thu, 23 Apr 2009 10:16:54 BST, Steven Whitehouse said:
> Finds the first set bit in a 64 bit word. This is required in order
> to fix a bug in GFS2, but I think it should be a generic function
> in case of future users.

Seems like a sane idea..

> +static inline unsigned long __ffs64(u64 word)
> +{
> +#if BITS_PER_LONG == 32
> +	if (((u32)word) == 0UL)
> +		return __ffs((u32)(word >> 32)) + 32;
> +#elif BITS_PER_LONG != 64
> +#error BITS_PER_LONG not 32 or 64
> +#endif
> +	return __ffs((unsigned long)word);
> +}
> +

Does this have endian-ness issues (is that (u32)word the "high" or "low"
part)?  Or is this intended only for looking at bitmaps and the like, and we
don't really care?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 1/3] bitops: Add __ffs64 bitop

[Steven Whitehouse]

Hi,

On Mon, 2009-04-27 at 09:40 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Thu, 23 Apr 2009 10:16:54 BST, Steven Whitehouse said:
> > Finds the first set bit in a 64 bit word. This is required in order
> > to fix a bug in GFS2, but I think it should be a generic function
> > in case of future users.
> 
> Seems like a sane idea..
> 
> > +static inline unsigned long __ffs64(u64 word)
> > +{
> > +#if BITS_PER_LONG == 32
> > +	if (((u32)word) == 0UL)
> > +		return __ffs((u32)(word >> 32)) + 32;
> > +#elif BITS_PER_LONG != 64
> > +#error BITS_PER_LONG not 32 or 64
> > +#endif
> > +	return __ffs((unsigned long)word);
> > +}
> > +
> 
> Does this have endian-ness issues (is that (u32)word the "high" or "low"
> part)?  Or is this intended only for looking at bitmaps and the like, and we
> don't really care?

The intent was that it would operate on native endian u64 words so that
it shouldn't be affected by the endianess. In the GFS2 code where it is
used, the byte ordering is converted to native order before this
function is applied,

Steve.

Re: [PATCH 1/3] bitops: Add __ffs64 bitop

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Mon, 27 Apr 2009 16:41:06 BST, Steven Whitehouse said:
> The intent was that it would operate on native endian u64 words so that
> it shouldn't be affected by the endianess.

Hmm.. it's passing a 64-bit by value, not by a pointer ref - a subtle
distinction - if that code had done   *(u32)word instead it would break
on some archs.  That will teach me to read code when not caffeinated enough.

(I also admit that often I post comments on the basis of "If I can misread
this, so can some other poor decaffienated Joe Programmer on a Monday morning" ;)

>                                            In the GFS2 code where it is
> used, the byte ordering is converted to native order before this
> function is applied,

Is that a reasonable expectation when other parts of the kernel start
using it?  We've seen bugs before when 32-bit code tries to deal with
a 64-bit or bigger bitfield as a pair or series of 32-bit fields...

- * @word: The 64 bit word
+ * @word: The 64 bit word - must be in native byte order

probably is enough for those who bother finding the function def/doc. And
the ones who don't bother, we can't help anyhow. ;)


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]