Re: [PATCH 1/2] xfs: add CRC infrastructure

Mark Tinguely
2012-11-09T22:07:41Z

On 11/07/12 07:37, Dave Chinner wrote:
> From: Christoph Hellwig<hch@lst.de>
>
> - add a mount feature bit for CRC enabled filesystems
> - add some helpers for generating and verifying the CRCs
> - add a copy_uuid helper
>
> The checksumming helpers are losely based on similar ones in sctp,
> all other bits come from Dave Chinner.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---
> +/*
> + * Calculate the intermediate checksum for a buffer that has the CRC field
> + * inside it. The offset of the 32bit crc fields is passed as the
> + * cksum_offset parameter.
> + */
> +static inline __uint32_t
> +xfs_start_cksum(char *buffer, size_t length, unsigned long cksum_offset)
> +{
> + __uint32_t zero = 0;
> + __uint32_t crc;
> +
> + /* Calculate CRC up to the checksum. */
> + crc = crc32c(XFS_CRC_SEED, buffer, cksum_offset);
> +
> + /* Skip checksum field */
> + crc = crc32c(crc,&zero, sizeof(__u32));

I know this came from sctp and I know I can't convince you to copy/null
the *cksum_offset to make one block for those with hardware crc32c devices.

Since the *cksum_offset value is never used in creating and verifying
the checksum, the 4 bytes of zeros does not add any new information,
why not just drop it from the cksum calculation?

> +
> + /* Calculate the rest of the CRC. */
> + return crc32c(crc,&buffer[cksum_offset + sizeof(__be32)],
> + length - (cksum_offset + sizeof(__be32)));
> +}
> +
> +/*
> + * Convert the intermediate checksum to the final ondisk format.
> + *
> + * Note that crc32c is already endianess agnostic, so no additional
> + * byte swap is needed.
> + */
> +static inline __be32
> +xfs_end_cksum(__uint32_t crc)
> +{
> + return (__force __be32)~crc;
> +}
> +

Wouldn't you have to cpu_to_le32() for big endian machines?

commit 9c5ff5f75d0d0a1c7928ecfae3f38418b51a88e3
Author: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu Jan 22 14:52:23 2009 -0800

sctp: Fix crc32c calculations on big-endian arhes.

crc32c algorithm provides a byteswaped result. On little-endian
arches, the result ends up in big-endian/network byte order.
On big-endinan arches, the result ends up in little-endian
order and needs to be byte swapped again. Thus calling cpu_to_le32
gives the right output.

Tested-by: Jukka Taimisto <jukka.taimisto@mail.suomi.net>
Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
index b799fb2..2fec3c3 100644
--- a/include/net/sctp/checksum.h
+++ b/include/net/sctp/checksum.h
@@ -79,5 +79,5 @@ static inline __be32 sctp_update_cksum(__u8 *buffer,
__u16 length, __be32 crc32)

static inline __be32 sctp_end_cksum(__be32 crc32)
{
- return ~crc32;
+ return (__force __be32)~cpu_to_le32((__force u32)crc32);
}