On 2018/5/4 10:28 PM, Michael Lyle wrote:
> Hi--
>
Hi Michael,
> On Fri, May 4, 2018 at 5:51 AM, Coly Li <colyli@xxxxxxx> wrote:
>> So far cache_sb.csum is defined as __u64, and it should be le64 or
>> __le64 as on-disk format. For on-disk format, we need something like
>> crc64_le() to calculate 64bits crc.
>
> There's two different things here-- the endianess of the CRC
> computation (this SHOULD NOT CHANGE between architectures or there
> will be inconsistent results, and is almost always little endian), and
> the endian-ness of the result-- this needs swapping.
Yes, these are my points too. And indeed this is what I do in the WIP
big endian support patch set (I posted a RFC version weeks ago).
>
> Only crc32 of the other CRC code in the kernel, or elsewhere, has
> BE/LE variants; all the other CRC routines produce a (8, 16, 32) bit
> quantity in native byte order. Yes, the final value after the
> computation needs to be byte-swapped (native to LE or BE) before
> writing to disk or sending over the network.
Not only the result should be swapped, but also the input value should
be swapped into an explicit specific endian (which is little endian
indeed) before the crc calculation, otherwise the result is still
undefined on different endianness machines. This is what I meant 'API'.
> CRC32 does because a few strange things (e.g. ATM, jbd2) do stuff on
> CRCs defined for bigger words as big-endian.
>
> Why is it not OK to just use a LE CRC like almost everything else (and
> that we already are) and byteswap the result? Seems the only thing
> missing in bcache is the swap of the CRC data itself before compare /
> before writing.
As I said, moving the crc64 code into lib/crc64.c is good idea, I agree
with it, but not for now. A good timing in my brain is,
1, After the big endian fixes are done, then we have a clean idea what
the crc64 API should be.
2, One more user of crc64 routines appears in kernel code. Otherwise
moving the code into lib/ does not contribute any benefit.
Normally people moving single-user-routine out of public code, like
'commit 25d8be77e192 ("block: move bio_alloc_pages() to bcache")'.
If crc64 rounine is only used by bcache and we move it into lib/crc64,c,
I am not sure whether it is good at this moment.
As I said, I like this idea, but do it for now IMHO is too early, it
will be a change for change without real benefit.
Thanks.
Coly Li
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
