xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: add CRC infrastructure
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Sun, 11 Nov 2012 13:54:28 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121111012643.GH6434@dastard>
References: <1352295452-4726-1-git-send-email-david@xxxxxxxxxxxxx> <1352295452-4726-2-git-send-email-david@xxxxxxxxxxxxx> <509D7F1A.3090201@xxxxxxx> <20121111012643.GH6434@dastard>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 11/10/12 19:26, Dave Chinner wrote:
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@xxxxxx>

  - 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@xxxxxx>
Signed-off-by: Dave Chinner<dchinner@xxxxxxxxxx>
---
+/*
+ * 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.

I was mostly thinking about down the road when crc32c offloading is
common. The copy/null/checksum/replace model of the routine can be done
anytime that it make sense to do so (as long as only one checksum can
happen at one time).


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

I am sure there is something in the pool. I will ask on Monday.
Someone was going to do some testing on a big endian machine before the
user space release anyway. I guess I just volunteered. A log recovery
test from a different endian machine sounds interesting.

Cheers,

Dave.


--Mark.

<Prev in Thread] Current Thread [Next in Thread>