md bitmap broken on big endian machines

Neil,

We just discovered this problem on a 64-bit IBM POWER (ppc64) system. The symptom was this BUG():

Sep 20 20:55:51 caspian kernel: kernel BUG in sync_request at drivers/md/raid1.c
:1743!
Sep 20 20:55:51 caspian kernel: Oops: Exception in kernel mode, sig: 5 [#1]
Sep 20 20:55:51 caspian kernel: SMP NR_CPUS=128 NUMA PSERIES LPAR
Sep 20 20:55:51 caspian kernel: Modules linked in: nbd raid1 ipv6 nfs lockd nfs_

acl sunrpc apparmor aamatch_pcre loop dm_mod e1000 ide_cd cdrom lpfc scsi_transp ort_fc pdc202xx_new sg st ipr firmware_class sd_mod scsi_mod

I traced the problem back to a bad value in bitmap->chunkshift (the value was 8, instead of 16, with a chunksize of 64K). I think this means that bitmaps are broken on all big endian systems. It looks like we should be using ffs instead of find_first_bit. Patch has been compile tested only.

Thanks,
Paul

@@ -1578,8 +1578,7 @@ int bitmap_create(mddev_t *mddev)
 	if (err)
 		goto error;
 
-	bitmap->chunkshift = find_first_bit(&bitmap->chunksize,
-					sizeof(bitmap->chunksize));
+	bitmap->chunkshift = ffs(bitmap->chunksize) - 1;
 
 	/* now that chunksize and chunkshift are set, we can use these macros */
  	chunks = (blocks + CHUNK_BLOCK_RATIO(bitmap) - 1) /

The SET_BITMAP_FILE ioctl currently doesn't have a compat entry, so on 64-bit systems a 32-bit mdadm will fail:

Sep 20 16:34:52 caspian kernel: ioctl32(mdadm:8056): Unknown cmd fd(3) cmd(8004092b){00} arg(00000004) on /dev/md0

The fix is to build a 64-bit mdadm or apply the following patch (compile tested only).

--
Paul

@@ -121,6 +121,7 @@ ULONG_IOCTL(START_ARRAY)
 COMPATIBLE_IOCTL(STOP_ARRAY)
 COMPATIBLE_IOCTL(STOP_ARRAY_RO)
 COMPATIBLE_IOCTL(RESTART_ARRAY_RW)
+ULONG_IOCTL(SET_BITMAP_FILE)
 /* DM */
 COMPATIBLE_IOCTL(DM_VERSION_32)
 COMPATIBLE_IOCTL(DM_REMOVE_ALL_32)

groan....

Does this mean we need to up the superblock version number? When reading a version-4 (or earlier) bitmap you cannot be sure if the correct chunk_shift was used so you cannot trust it. But I don't like the idea of forcing everyone to upgrade their bitmap...
Maybe it doesn't matter too much. If there does turn out to be a problem - which there won't be for the little-endian majority - a simple 'repair' process will resolve everything.

 ./drivers/md/bitmap.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff .prev/drivers/md/bitmap.c ./drivers/md/bitmap.c
@@ -1444,8 +1444,7 @@ int bitmap_create(mddev_t *mddev)
 	if (err)
 		goto error;
 
-	bitmap->chunkshift = find_first_bit(&bitmap->chunksize,
-					sizeof(bitmap->chunksize));
+	bitmap->chunkshift = ffz(~bitmap->chunksize);
 
 	/* now that chunksize and chunkshift are set, we can use these macros */
  	chunks = (blocks + CHUNK_BLOCK_RATIO(bitmap) - 1) /

Thanks. I'll make sure it gets to akpm.

NeilBrown

Neil Brown wrote:
[]

So we don't use ffs(int) for an unsigned value because of int vs unsigned int, but we use ffz() with negated UNSIGNED. Looks even more broken to me, even if it happens to work correctly... ;)

/mjt

No, it doesn't matter about the signedness, these are just bit operations. The problem is the size (int vs. long), even though in practice it's very unlikely you'd ever have a bitmap chunk size that exceeded 32 bits. But it's better to be correct and not have to worry about it.

--
Paul

I understand the point, in the first place (I didn't mentioned long vs int above, however). The thing is: when reading the code, it looks just plain wrong. Esp. since function prototypes aren't here, but for those ffs(), ffz() etc they're hidden somewhere in include/asm/* (as they're architecture-dependent), and it's not at all obvious which is signed and which is unsigned, which is long or int etc.

At the very least, return -ENOCOMMENT :)

/mjt