reiserfsck on PPC: checkmem fails

reiserfsck on PPC: checkmem fails

[Patrick Smith]

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

I'm using ReiserFS on a PowerPC.  The filesystem works fine in normal 
use, but I'm having a bit of trouble with reiserfsck.

Versions:  reiserfsprogs 3.6.4, Linux kernel 2.4.20.

In order to get reiserfsprogs to compile, I applied the attached patch.

When I run reiserfsprogs, it reports one or two problems, and then fails 
in checkmem.  For now, I'm ignoring the problems, as I don't know 
whether they're caused by the same thing as checkmem or are real problems.

I've tracked down the cause of the checkmem failure to this bit of code 
in reiserfs_fetch_ondisk_bitmap in bitmap.c:

     last_byte_unused_bits = bm->bm_byte_size * 8 - bm->bm_bit_size;
     for (i = 0; i < last_byte_unused_bits; i ++)
         clear_bit (bm->bm_bit_size + i, bm->bm_map);

Thing is, the set_bit/clear_bit functions on PowerPC handle bits in 
groups of 32, with the low-order bits getting the lower numbers.  And, 
when this code runs, bm->bm_byte_size is 506733, congruent to 1 modulo 
4.  So the last byte of data bits is aligned on a word (4 byte) 
boundary, followed by the "mem_end" sentinel inserted by getmem:

	Bmem _end

(where B is the last byte of data).

When the code above calls clear_bit to clear the last few bits, this 
clears low-order bits in the Bmem word.  But those are in the 'm' byte, 
not the data byte.  Oops!

Does anyone have any ideas as to whether there's an easy fix?  And 
whether there could be similar problems elsewhere?  Any help would be 
much appreciated.

Also, would there be any serious drawbacks to using 3.6.3 or 3.6.2 
instead?  And would they have the same problem?

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1139 bytes --]

--- ./include/misc.h.orig	2002-09-13 06:09:28.000000000 -0400
+++ ./include/misc.h	2002-12-24 22:31:08.000000000 -0500
@@ -49,7 +49,10 @@
 int uuid_is_correct (unsigned char * uuid);
 int set_uuid (const unsigned char * text, unsigned char * UUID);
 
+#include <asm/byteorder.h>
+#define __KERNEL__
 #include <asm/bitops.h>
+#undef __KERNEL__
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 int le_set_bit (int nr, void * addr);
 int le_clear_bit (int nr, void * addr);
--- ./include/io.h.orig	2002-09-01 11:32:07.000000000 -0400
+++ ./include/io.h	2002-12-24 22:31:08.000000000 -0500
@@ -2,6 +2,7 @@
  * Copyright 1996-2002 Hans Reiser
  */
 
+#include "misc.h"
 
 struct buffer_head {
     unsigned long b_blocknr;
--- ./include/swab.h.orig	2002-08-07 05:12:07.000000000 -0400
+++ ./include/swab.h	2002-12-24 22:32:09.000000000 -0500
@@ -4,5 +4,15 @@
 #ifndef _REISERFS_SWAB_H_
 #define _REISERFS_SWAB_H_
 
+#include <asm/byteorder.h>
+#define __KERNEL__
+#include <asm/bitops.h>
+#undef __KERNEL__
+
 #include <linux/byteorder/swab.h>
+
+#define swab16 __swab16
+#define swab32 __swab32
+#define swab64 __swab64
+
 #endif /* _REISERFS_SWAB_H_ */

Re: reiserfsck on PPC: checkmem fails

[Patrick Smith]

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

> I'm using ReiserFS on a PowerPC.  The filesystem works fine in normal 
> use, but I'm having a bit of trouble with reiserfsck.
> 
> Versions:  reiserfsprogs 3.6.4, Linux kernel 2.4.20.

After looking into this a bit more, I've become quite confused and worried.

Various places in the code, arrays of bits are maintained.  Two 
different approaches seem to be taken:

ext2_test_bit, ext2_set_bit, etc. group bits into 8-bit bytes.  The 
kernel's bitmap.c uses this approach (but not everywhere).

test_bit, set_bit, etc. group bits into 32-bit words (at least in the 
PPC version).  The kernel's journal.c uses this approach.  As far as I 
can tell, only this approach is used in reiserfsprogs.

On a little-endian machine, the two approaches should be equivalent, so 
the distinction wouldn't matter much.

On a big-endian machine such as PPC, they are quite different, so each 
piece of data should be accessed with only one of the two approaches. 
But as far as I can tell, the kernel's bitmap.c accesses the block 
bitmaps with the first approach, and reiserfsprogs' bitmap.c uses the 
second approach.  :-(

On the other hand, when I tried modifying the reiserfsprogs bitmap.c to 
group bits by bytes instead of words, reiserfsck --check spewed a 
zillion error messages.  So maybe I've got something wrong above.

In any case, might I suggest standardizing on one of the two approaches 
throughout?  Preferably the byte-wise approach, for the sake of portability.

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1140 bytes --]

--- ./include/misc.h.orig	2002-09-13 06:09:28.000000000 -0400
+++ ./include/misc.h	2002-12-24 22:31:08.000000000 -0500
@@ -49,7 +49,10 @@
 int uuid_is_correct (unsigned char * uuid);
 int set_uuid (const unsigned char * text, unsigned char * UUID);
 
+#include <asm/byteorder.h>
+#define __KERNEL__
 #include <asm/bitops.h>
+#undef __KERNEL__
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 int le_set_bit (int nr, void * addr);
 int le_clear_bit (int nr, void * addr);
--- ./include/io.h.orig	2002-09-01 11:32:07.000000000 -0400
+++ ./include/io.h	2002-12-24 22:31:08.000000000 -0500
@@ -2,6 +2,7 @@
  * Copyright 1996-2002 Hans Reiser
  */
 
+#include "misc.h"
 
 struct buffer_head {
     unsigned long b_blocknr;
--- ./include/swab.h.orig	2002-08-07 05:12:07.000000000 -0400
+++ ./include/swab.h	2002-12-24 22:32:09.000000000 -0500
@@ -4,5 +4,15 @@
 #ifndef _REISERFS_SWAB_H_
 #define _REISERFS_SWAB_H_
 
+#include <asm/byteorder.h>
+#define __KERNEL__
+#include <asm/bitops.h>
+#undef __KERNEL__
+
 #include <linux/byteorder/swab.h>
+
+#define swab16 __swab16
+#define swab32 __swab32
+#define swab64 __swab64
+
 #endif /* _REISERFS_SWAB_H_ */

Re: reiserfsck on PPC: checkmem fails

[Patrick Smith]

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

Yesterday, I reported having tried a experimental fix for this problem, 
that resulted in a spew of error messages.  Turns out those were due to 
a bug in my fix.  With that repaired, checkmem error message does indeed 
disappear.

I've attached a patch showing what I did.  With this applied, reiserfsck 
--check produces the same error messages it did previously, minus the 
checkmem one.  It also produces one new message:

	check_and_free_buffer_mem: dirty buffer (3 16) found

I've no idea whether this is due to a problem with my fix, a problem on 
the disk, or what.  The fact that this message wasn't produced before 
application of my fix doesn't mean much, as in that case reiserfsck was 
dying in checkmem before getting to the point where this message is 
produced.

For what it's worth, the _2_ other error messages I'm getting (a) refer 
to a block used more than once, and (b) say the ondisk bitmap is not 
correct.  Though there's also a message about _3_ corruptions repairable 
with --fix-fixable.  It's quite possible these represent real problems 
with the filesystem, as the disk has been hit by at least one power 
failure.  Or they could be lingering side effects of the bitmap problem.

At this point, I have no intention of trying to repair the disk without 
some reassurance that this problem is indeed fixed.  My "fix" may be 
introducing other problems, and there may be other cases of 
big-endian/little-endian confusion.  And I'd rather not spend the time 
to read all the code to ferret these out.  Someone who knows the code 
can presumably do it much more quickly.  :-)

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 5782 bytes --]

diff -ur original/include/io.h reiserfsprogs-3.6.4/include/io.h
--- original/include/io.h	2003-02-05 21:42:44.000000000 -0500
+++ reiserfsprogs-3.6.4/include/io.h	2003-02-05 21:52:44.000000000 -0500
@@ -2,6 +2,7 @@
  * Copyright 1996-2002 Hans Reiser
  */
 
+#include "misc.h"
 
 struct buffer_head {
     unsigned long b_blocknr;
diff -ur original/include/misc.h reiserfsprogs-3.6.4/include/misc.h
--- original/include/misc.h	2003-02-05 21:42:44.000000000 -0500
+++ reiserfsprogs-3.6.4/include/misc.h	2003-02-05 22:06:42.000000000 -0500
@@ -49,7 +49,6 @@
 int uuid_is_correct (unsigned char * uuid);
 int set_uuid (const unsigned char * text, unsigned char * UUID);
 
-#include <asm/bitops.h>
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 int le_set_bit (int nr, void * addr);
 int le_clear_bit (int nr, void * addr);
diff -ur original/include/swab.h reiserfsprogs-3.6.4/include/swab.h
--- original/include/swab.h	2003-02-05 21:42:44.000000000 -0500
+++ reiserfsprogs-3.6.4/include/swab.h	2003-02-05 22:11:23.000000000 -0500
@@ -4,5 +4,15 @@
 #ifndef _REISERFS_SWAB_H_
 #define _REISERFS_SWAB_H_
 
+#include <asm/byteorder.h>
+#define __KERNEL__
+#include <asm/bitops.h>
+#undef __KERNEL__
+
 #include <linux/byteorder/swab.h>
+
+#define swab16 __swab16
+#define swab32 __swab32
+#define swab64 __swab64
+
 #endif /* _REISERFS_SWAB_H_ */
diff -ur original/reiserfscore/bitmap.c reiserfsprogs-3.6.4/reiserfscore/bitmap.c
--- original/reiserfscore/bitmap.c	2003-02-05 21:42:44.000000000 -0500
+++ reiserfsprogs-3.6.4/reiserfscore/bitmap.c	2003-02-05 21:49:59.000000000 -0500
@@ -10,6 +10,51 @@
 #include <assert.h>
 
 
+/* The on-disk bitmaps use little-endian byte order.  But the PPC versions
+   of the bit-manipulation functions use big-endian byte order.  Hence we
+   use these functions instead of the normal set_bit, clear_bit, ...
+
+   Note that the versions here are intended to be simple and work.  Some
+   optimization would be a good thing to add.  */
+
+static __inline__ int bm_set_bit(int nr, void* a)
+{
+   unsigned char* p = a + (nr >> 3);
+   unsigned char mask = 1 << (nr & 7);
+   int retval;
+   retval = ((*p & mask) != 0);
+   *p |= mask;
+   return retval;
+}
+
+static __inline__ int bm_clear_bit(int nr, void* a)
+{
+   unsigned char* p = a + (nr >> 3);
+   unsigned char mask = 1 << (nr & 7);
+   int retval;
+   retval = ((*p & mask) != 0);
+   *p &= ~mask;
+   return retval;
+}
+
+static __inline__ int bm_test_bit(int nr, void* a)
+{
+   unsigned char* p = a + (nr >> 3);
+   unsigned char mask = 1 << (nr & 7);
+   return (*p & mask) != 0;
+}
+
+static __inline__ unsigned long bm_find_next_zero_bit(void * addr,
+               unsigned long size, unsigned long offset)
+{
+   unsigned long nr;
+   for (nr = offset; nr < size; nr++)
+      if (!bm_test_bit(nr, addr))
+         return nr;
+   return size;
+}
+
+
 /* create clean bitmap */
 reiserfs_bitmap_t * reiserfs_create_bitmap (unsigned int bit_count)
 {
@@ -127,8 +172,8 @@
 	    to->bm_bit_size == from->bm_bit_size);
 
     for (i = 0; i < to->bm_bit_size; i++) {
-	if (test_bit(i, from->bm_map) && !test_bit(i, to->bm_map)) {
-	    set_bit(i, to->bm_map);
+	if (bm_test_bit(i, from->bm_map) && !bm_test_bit(i, to->bm_map)) {
+	    bm_set_bit(i, to->bm_map);
 	    to->bm_set_bits ++;
 	    to->bm_dirty = 1;	
 	}
@@ -148,8 +193,8 @@
 	    base->bm_bit_size == exclude->bm_bit_size);
 
     for (i = 0; i < base->bm_bit_size; i++) {
-	if (test_bit(i, exclude->bm_map) && test_bit(i, base->bm_map)) {
-	    clear_bit(i, base->bm_map);
+	if (bm_test_bit(i, exclude->bm_map) && bm_test_bit(i, base->bm_map)) {
+	    bm_clear_bit(i, base->bm_map);
 	    base->bm_set_bits --;
 	    base->bm_dirty = 1;
 	}
@@ -159,9 +204,9 @@
 void reiserfs_bitmap_set_bit (reiserfs_bitmap_t * bm, unsigned int bit_number)
 {
     assert(bit_number < bm->bm_bit_size);
-    if (test_bit (bit_number, bm->bm_map))
+    if (bm_test_bit (bit_number, bm->bm_map))
 	return;
-    set_bit(bit_number, bm->bm_map);
+    bm_set_bit(bit_number, bm->bm_map);
     bm->bm_set_bits ++;
     bm->bm_dirty = 1;
 }
@@ -170,9 +215,9 @@
 void reiserfs_bitmap_clear_bit (reiserfs_bitmap_t * bm, unsigned int bit_number)
 {
     assert(bit_number < bm->bm_bit_size);
-    if (!test_bit (bit_number, bm->bm_map))
+    if (!bm_test_bit (bit_number, bm->bm_map))
 	return;
-    clear_bit (bit_number, bm->bm_map);
+    bm_clear_bit (bit_number, bm->bm_map);
     bm->bm_set_bits --;
     bm->bm_dirty = 1;
 }
@@ -183,7 +228,7 @@
     if (bit_number >= bm->bm_bit_size)
 	printf ("bit %u, bitsize %lu\n", bit_number, bm->bm_bit_size);
     assert(bit_number < bm->bm_bit_size);
-    return test_bit(bit_number, bm->bm_map);
+    return bm_test_bit(bit_number, bm->bm_map);
 }
 
 
@@ -204,7 +249,7 @@
     unsigned int  bit_nr = *start;
     assert(*start < bm->bm_bit_size);
 
-    bit_nr = find_next_zero_bit(bm->bm_map, bm->bm_bit_size, *start);
+    bit_nr = bm_find_next_zero_bit(bm->bm_map, bm->bm_bit_size, *start);
 
     if (bit_nr >= bm->bm_bit_size) { /* search failed */	
 	return 1;
@@ -266,7 +311,7 @@
        reiserfs_bitmap_t has those bits set to 0 */
     last_byte_unused_bits = bm->bm_byte_size * 8 - bm->bm_bit_size;
     for (i = 0; i < last_byte_unused_bits; i ++)
-	clear_bit (bm->bm_bit_size + i, bm->bm_map);
+	bm_clear_bit (bm->bm_bit_size + i, bm->bm_map);
 
     bm->bm_set_bits = 0;
     /* FIXME: optimize that */
@@ -332,7 +377,7 @@
 	    /* set unused bits of last byte of a bitmap to 1 */
 	    last_byte_unused_bits = bm->bm_byte_size * 8 - bm->bm_bit_size;
 	    for (i = 0; i < last_byte_unused_bits; i ++)
-		set_bit ((bm->bm_bit_size % (fs->fs_blocksize * 8)) + i, bh->b_data);
+		bm_set_bit ((bm->bm_bit_size % (fs->fs_blocksize * 8)) + i, bh->b_data);
 	}
 	mark_buffer_dirty (bh);
 	brelse (bh);

Re: reiserfsck on PPC: checkmem fails

[Vitaly Fertman]

Hi, Patrich!

Sorry for delay, I did not work these few days. Thank you for your patch, 
I have a look at it, it seems to work right but I would like to rewrite 
it a bit, to check the whole code for similar problems and to test the result.
Could you provide me an access to your computer - I do not have a possiblity 
to test big endian code locally for now. Do you have a spare partition for 
experiments there? 

On Thursday 06 February 2003 06:58, Patrick Smith wrote:
> Yesterday, I reported having tried a experimental fix for this problem,
> that resulted in a spew of error messages.  Turns out those were due to
> a bug in my fix.  With that repaired, checkmem error message does indeed
> disappear.
>
> I've attached a patch showing what I did.  With this applied, reiserfsck
> --check produces the same error messages it did previously, minus the
> checkmem one.  It also produces one new message:
>
> 	check_and_free_buffer_mem: dirty buffer (3 16) found

this is a small inessential bug which I fixed recently - not endian related.

-- 

Thanks,
Vitaly Fertman