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

[Dave Chinner <david@fromorbit.com>]

On Fri, Nov 09, 2012 at 04:09:30PM -0600, Mark Tinguely wrote:
> 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?

Because it gives a different CRC value. If we zero the CRC field,
and then do an entire block CRC ignoring the location of the CRC, we
get the same value as using the above algorithm. While we aren't
going to do this type of verification/calculation in the kernel
code, there are use cases for it, say in repair, where we don't have
to worry about multiple verifications of the object occurring.

Hence by making sure we zero the CRC field during the calculation,
we retain the flexibility of doing faster, single pass calculation
and verification where it makes sense to use it.  If we optimise
away the zero block for the CRC, then we that flexibility when it
comes to implementing other tools that check and recalculate CRC
values.

> >+	/* 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?

Good catch, I hadn't noticed that fix - it's been quite a while
since this particular patch was originally written. So, yeah, it
probably does need that fix.

FWIW, I don't have a big endian machine immediately handy to test
this. Do you?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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