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

[Mark Tinguely <tinguely@sgi.com>]

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);
  }

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs