Reduce bloat in ereport.fs.zfs.checksum events #15052

asomers · 2023-07-11T21:43:10Z

Fixes #14717

Motivation and Context

On FreeBSD, a kernel event must fit within a 1016 byte buffer. ZFS checksum events don't, so they never reach userland.

Description

Remove four not-very-useful fields from the events to reduce their size:

Remove the histogram fields. They were originally intended to be used with parallel ATA and SCSI busses, which are obsolete. They probably aren't of any use whatsoever on modern hardware. This is the first commit.
Remove the cksum_actual and cksum_expected fields. With modern checksum functions, they don't really convey any information other than "they don't match". This is the second commit.

How Has This Been Tested?

Tested with FreeBSD's zfsd test suite.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

asomers · 2023-07-14T22:26:27Z

cc @rincebrain

rincebrain · 2023-07-14T22:57:35Z

I'm a little sad about the cksum_actual and cksum_expected fields, but needs must when the devil drives.

How does this work for backward compat? Wouldn't this cause horrible mangling just removing the fields outright and having mixed kernel/userland?

asomers · 2023-07-14T23:03:25Z

I don't think there's any backwards-compatibility problem, because AFAIK nothing in userland ever reads those fields. And they aren't crammed into fixed-size structs, so there's no serialization risk. At least, on FreeBSD they're serialized into ASCII strings.

rincebrain · 2023-07-14T23:20:09Z

I absolutely know nothing about how the fmreport events get passed around, it was just my immediate question on changing the structure.

Even if it's not passing a fixed struct around, though, how does old userland handle getting events with those missing? I suppose if it didn't always have them it already had to...

Seems fine to me.

behlendorf

Slimming these down seems reasonable to me. Can you just rebase this to resolve the conflict.

The checksum histograms were intended to be used with ATA and parallel SCSI, which are obsolete. With modern storage hardware, they will almost always look like white noise; all bits will be wrong. They only serve to bloat the event. That's a particular problem on FreeBSD, where events must fit into a 1016 byte buffer. This fixes issue openzfs#14717 for RAIDZ pools, but not for mirror pools. Sponsored-by: Axcient Signed-off-by: Alan Somers <asomers@gmail.com>

With anything but fletcher-4, even a tiny change in the input will cause the checksum value to change completely. So knowing the actual and expected checksums doesn't provide much more information than "they don't match". The harm in sending them is simply that they bloat the event. In particular, on FreeBSD the event must fit into a 1016 byte buffer. Fixes openzfs#14717 for mirrored pools. Sponsored-by: Axcient Signed-off-by: Alan Somers <asomers@gmail.com>

asomers · 2023-07-20T17:45:35Z

rebased.

With anything but fletcher-4, even a tiny change in the input will cause the checksum value to change completely. So knowing the actual and expected checksums doesn't provide much more information than "they don't match". The harm in sending them is simply that they bloat the event. In particular, on FreeBSD the event must fit into a 1016 byte buffer. Fixes #14717 for mirrored pools. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Rich Ercolani <rincebrain@gmail.com> Signed-off-by: Alan Somers <asomers@gmail.com> Sponsored-by: Axcient Closes #14717 Closes #15052

The checksum histograms were intended to be used with ATA and parallel SCSI, which are obsolete. With modern storage hardware, they will almost always look like white noise; all bits will be wrong. They only serve to bloat the event. That's a particular problem on FreeBSD, where events must fit into a 1016 byte buffer. This fixes issue openzfs#14717 for RAIDZ pools, but not for mirror pools. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Rich Ercolani <rincebrain@gmail.com> Signed-off-by: Alan Somers <asomers@gmail.com> Sponsored-by: Axcient Closes openzfs#15052

With anything but fletcher-4, even a tiny change in the input will cause the checksum value to change completely. So knowing the actual and expected checksums doesn't provide much more information than "they don't match". The harm in sending them is simply that they bloat the event. In particular, on FreeBSD the event must fit into a 1016 byte buffer. Fixes openzfs#14717 for mirrored pools. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Rich Ercolani <rincebrain@gmail.com> Signed-off-by: Alan Somers <asomers@gmail.com> Sponsored-by: Axcient Closes openzfs#14717 Closes openzfs#15052

The checksum histograms were intended to be used with ATA and parallel SCSI, which are obsolete. With modern storage hardware, they will almost always look like white noise; all bits will be wrong. They only serve to bloat the event. That's a particular problem on FreeBSD, where events must fit into a 1016 byte buffer. This fixes issue #14717 for RAIDZ pools, but not for mirror pools. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Rich Ercolani <rincebrain@gmail.com> Signed-off-by: Alan Somers <asomers@gmail.com> Sponsored-by: Axcient Closes #15052

With anything but fletcher-4, even a tiny change in the input will cause the checksum value to change completely. So knowing the actual and expected checksums doesn't provide much more information than "they don't match". The harm in sending them is simply that they bloat the event. In particular, on FreeBSD the event must fit into a 1016 byte buffer. Fixes #14717 for mirrored pools. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Rich Ercolani <rincebrain@gmail.com> Signed-off-by: Alan Somers <asomers@gmail.com> Sponsored-by: Axcient Closes #14717 Closes #15052

The checksum histograms were intended to be used with ATA and parallel SCSI, which are obsolete. With modern storage hardware, they will almost always look like white noise; all bits will be wrong. They only serve to bloat the event. That's a particular problem on FreeBSD, where events must fit into a 1016 byte buffer. This fixes issue openzfs#14717 for RAIDZ pools, but not for mirror pools. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Rich Ercolani <rincebrain@gmail.com> Signed-off-by: Alan Somers <asomers@gmail.com> Sponsored-by: Axcient Closes openzfs#15052

With anything but fletcher-4, even a tiny change in the input will cause the checksum value to change completely. So knowing the actual and expected checksums doesn't provide much more information than "they don't match". The harm in sending them is simply that they bloat the event. In particular, on FreeBSD the event must fit into a 1016 byte buffer. Fixes openzfs#14717 for mirrored pools. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Rich Ercolani <rincebrain@gmail.com> Signed-off-by: Alan Somers <asomers@gmail.com> Sponsored-by: Axcient Closes openzfs#14717 Closes openzfs#15052

behlendorf added the Status: Code Review Needed Ready for review and testing label Jul 13, 2023

rincebrain approved these changes Jul 14, 2023

View reviewed changes

rincebrain mentioned this pull request Jul 14, 2023

Could we keep versioned instances of the man pages at least around? openzfs/openzfs-docs#447

Closed

behlendorf approved these changes Jul 20, 2023

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 20, 2023

asomers added 2 commits July 20, 2023 11:45

asomers force-pushed the no-checksums branch from 9347220 to 51c97d5 Compare July 20, 2023 17:45

behlendorf closed this in cf2a225 Jul 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce bloat in ereport.fs.zfs.checksum events #15052

Reduce bloat in ereport.fs.zfs.checksum events #15052

asomers commented Jul 11, 2023

asomers commented Jul 14, 2023

rincebrain commented Jul 14, 2023

asomers commented Jul 14, 2023

rincebrain commented Jul 14, 2023

behlendorf left a comment

asomers commented Jul 20, 2023

Reduce bloat in ereport.fs.zfs.checksum events #15052

Reduce bloat in ereport.fs.zfs.checksum events #15052

Conversation

asomers commented Jul 11, 2023

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

asomers commented Jul 14, 2023

rincebrain commented Jul 14, 2023

asomers commented Jul 14, 2023

rincebrain commented Jul 14, 2023

behlendorf left a comment

Choose a reason for hiding this comment

asomers commented Jul 20, 2023