Created an attachment (id=120) [details]
A patch to fix XFS recovery on 64-bit little-endian architectures (like
sparc64)

As it is currently written, the xfs_contig_bits function in xfs_bit.c, when
BITS_PER_LONG != 32, implicitely assumes that the arch is little-endian, as it
considers that the LSB of *((uint *)map) is the same as the LSB of *((unsigned
long int *)map).

This patch implements a rewrite of xfs_contig_bits, that is supposed to work on
all arches. Moreover, it adds two ASSERTs that would have been useful to track
this bug.

Created an attachment (id=121) [details]
A patch to fix XFS recovery on 64-bit little-endian architectures (like
sparc64)

As it is currently written, the xfs_contig_bits function in xfs_bit.c, when
BITS_PER_LONG != 32, implicitely assumes that the arch is little-endian, as it
considers that the LSB of *((uint *)map) is the same as the LSB of *((unsigned
long int *)map).

This patch implements a rewrite of xfs_contig_bits, that is supposed to work on
all arches. Moreover, it adds two ASSERTs that would have been useful to track
this bug.

(From update of attachment 121 [details])
As it is currently written, the xfs_contig_bits function in xfs_bit.c, when
BITS_PER_LONG != 32, implicitely assumes that the arch is little-endian, as it
considers that the LSB of *((uint *)map) is the same as the LSB of *((unsigned
long int *)map).

This patch implements a rewrite of xfs_contig_bits, that is supposed to work on
all arches. Moreover, it adds two ASSERTs that would have been useful to track
this bug.

Sorry for the extra noise, that's roughly the first time I use bugzilla. I hope
everything is now fine. If it is not, please ask.

Nicolas

Hi Nicolas,

Thanks very much for the patch.

I see the problem that you point out.
For xfs_buf_item's we store a blf_data_map which is an array of 32bit ints
and we use xfs_buf_item_log() to add to the map. When we call xfs_contig_bits on
a 64 bit word system we have to be careful. On little endian systems the LSB of
a 32 bit word and a 64 bit word for the same data is the same but on a big
endian system they differ. So one can't just call the linux find_next_zero_bit()
function when we have 64 bit words on big endian boxes (as you mentioned).
I noticed that for xfs_next_bit() we are doing the right thing and doing our own
32 bit int code.
The xfs_contig_bits() is fairly similar to xfs_next_bit() and is largely just a
xfs_next_zero_bit() except that we want a count of all the 1s so far even if we
never find a zero (which would return -1 in the xfs_next* case).
So I compared your xfs_contig_bits() code with our existing xfs_next_bit() code
and I realised that we have a few simplifications to make which you've already
done in your code. The thing is, unlike in the kernel functions, the size
parameter in xfs code is in 32 bit ints and NOT in bits. We only do ops on size
which keep it a multiple of 32 bits and therefore some size related tests in
xfs_next_bit() can be simplified/removed. I'll make some changes for
xfs_next_bit() in a separate incident :-)

So the only thing I noticed in your code is that
  while (size >= NBWORD) {
could probably be simplified to
  while (size) {
and the assert removed,
since size must be a multiple of NBWORD.

Did you write this code based on xfs_next_bit() or from
somewhere else?

--Tim

Hi Tim,

I do agree with your explanation of the bug.
About the
  while (size >= NBWORD) {
that could be simplified to
  while (size) {
I agree that there should be no difference, but I'd rather keep the assert,
"just in case"... I can't think of a situation where this would be needed, but,
as far as I know, asserts don't hurt when debug is disabled...

And you're right, that code is actually based on xfs_next_bit.


Regards,

Nicolas

Tim's patch has been in for a while an verified on a few 64bit BE plattforms.