Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

abd: lift ABD zero scan from zio_compress_data() to abd_cmp_zero() #16326

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

robn
Copy link
Member

@robn robn commented Jul 8, 2024

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

In #16323 I made all compression users call zio_compress_data() rather than doing their own bounce through zio_compress_table. It was (rightly) pointed out that this meant things that were not getting hole detection/flattening before now would.

For now I've reverted that change there. This PR is how I think I want to tackle that particular interface quirk though,

Description

This lifts the zero-detection code from inside zio_compress_data() into abd_cmp_zero() and abd_cmp_zero_off(), matching the interface of the other abd_cmp_* functions.

It's now the caller's responsibility do special handling for holes if that's something it wants.

All call sites have been checked and updated where necessary. The ones that haven't been touched are believed to either be impossible to have fully-zero data for input, or already did not properly handle a zero return from abd_compress_data().

How Has This Been Tested?

compression and rsend test tags pass successfully.

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:

Copy link
Contributor

@mcmilk mcmilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about few places where we re-compress the data and expect achieving identical results, like L2ARC writing, encryption signature checks, etc. But I hope we should never reach any of them after the buffer is declared empty here.

Also I am not happy that comparison function requires buffer to be multiple of 8 bytes. Could we assert it while there? At some point experimenting with ZIL using odd gang ABDs to avoid memory copying I had a problem to figure out what are alignments requirements of ZIO pipeline. Assertion would make sure we'll notice immediately if anything go wrong.

module/zfs/abd.c Outdated Show resolved Hide resolved
@robn
Copy link
Member Author

robn commented Jul 15, 2024

I was thinking about few places where we re-compress the data and expect achieving identical results, like L2ARC writing, encryption signature checks, etc. But I hope we should never reach any of them after the buffer is declared empty here.

I checked them all and I think we're safe. So long as it fails loudly I won't be too sad if we missed one.

Also I am not happy that comparison function requires buffer to be multiple of 8 bytes. Could we assert it while there? At some point experimenting with ZIL using odd gang ABDs to avoid memory copying I had a problem to figure out what are alignments requirements of ZIO pipeline. Assertion would make sure we'll notice immediately if anything go wrong.

Good call; added the assert. It's fairly straightforward to fix up, but I'll leave that until something needs it.

It's now the caller's responsibility do special handling for holes if
that's something it wants.

This also makes zio_compress_data() and zio_decompress_data() properly
the inverse of each other.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <rob.norris@klarasystems.com>
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Aug 8, 2024
@behlendorf behlendorf merged commit b0bf14c into openzfs:master Aug 9, 2024
24 checks passed
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
It's now the caller's responsibility do special handling for holes if
that's something it wants.

This also makes zio_compress_data() and zio_decompress_data() properly
the inverse of each other.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de>
Reviewed-by: Jason Lee <jasonlee@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rob Norris <rob.norris@klarasystems.com>
Closes openzfs#16326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants