Skip to content

Add new L2ARC tests with Compressed ARC enabled/disabled #10692

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

Closed
wants to merge 1 commit into from

Conversation

allanjude
Copy link
Contributor

@allanjude allanjude commented Aug 9, 2020

Signed-off-by: Allan Jude allanjude@freebsd.org
Sponsored-by: The FreeBSD Foundation

Motivation and Context

Blocks are handled differently depending on the state of the
zfs_compressed_arc_enabled tunable.

If a block is compressed on-disk, and compressed_arc is enabled:

  • the block is read from disk
  • It is NOT decompressed
  • It is added to the ARC in its compressed form
  • l2arc_write_buffers() may write it to the L2ARC (as is)
  • l2arc_read_done() compares the checksum to the BP (compressed)

However, if compressed_arc is disabled:

  • the block is read from disk
  • It is decompressed
  • It is added to the ARC (uncompressed)
  • l2arc_write_buffers() will use l2arc_apply_transforms() to
    recompress the block, before writing it to the L2ARC
  • l2arc_read_done() compares the checksum to the BP (compressed)
  • l2arc_read_done() will use l2arc_untransform() to uncompress it

Description

This test writes out a test file to a pool consisting of one disk
and one cache device, then randomly reads from it. Since the arc_max
in the tests is low, this will feed the L2ARC, and result in reads
from the L2ARC.

We compare the value of the kstat l2_cksum_bad before and after
to determine if any blocks failed to survive the trip through the
L2ARC.

Note: It is expected for the second test, with compressed ARC disabled to fail. This test found a bug, I'll post the fix as a separate PR. I wanted to be able to show the test failing before, and passing after.

How Has This Been Tested?

The tests have been run on Linux (CentOS 7) and FreeBSD (12.1)

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux 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.

@allanjude allanjude force-pushed the l2arc_compressed_arc branch from ce4f5d2 to 1773a4d Compare August 9, 2020 16:08
@bghira
Copy link

bghira commented Aug 9, 2020

#7896

@codecov
Copy link

codecov bot commented Aug 9, 2020

Codecov Report

Merging #10692 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10692      +/-   ##
==========================================
+ Coverage   79.77%   79.80%   +0.02%     
==========================================
  Files         394      394              
  Lines      124647   124647              
==========================================
+ Hits        99438    99474      +36     
+ Misses      25209    25173      -36     
Flag Coverage Δ
#kernel 80.35% <ø> (+0.01%) ⬆️
#user 66.33% <ø> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
module/os/linux/spl/spl-zlib.c 55.35% <0.00%> (-28.58%) ⬇️
module/zfs/zio_compress.c 89.74% <0.00%> (-2.57%) ⬇️
module/zfs/dsl_synctask.c 92.40% <0.00%> (-2.54%) ⬇️
lib/libzfs/libzfs_changelist.c 84.26% <0.00%> (-1.88%) ⬇️
cmd/zed/agents/fmd_api.c 88.61% <0.00%> (-1.78%) ⬇️
cmd/ztest/ztest.c 78.88% <0.00%> (-1.60%) ⬇️
cmd/zed/agents/zfs_diagnosis.c 77.55% <0.00%> (-1.17%) ⬇️
module/zfs/dsl_userhold.c 89.17% <0.00%> (-1.12%) ⬇️
cmd/zdb/zdb.c 81.70% <0.00%> (-0.86%) ⬇️
module/zfs/txg.c 93.96% <0.00%> (-0.73%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f95ddc...42ecea1. Read the comment docs.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Aug 11, 2020
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

You'll need to add both of these new tests to the tests/runfiles/common.run file so they're run by default.

Blocks are handled differently depending on the state of the
zfs_compressed_arc_enabled tunable.

If a block is compressed on-disk, and compressed_arc is enabled:
- the block is read from disk
- It is NOT decompressed
- It is added to the ARC in its compressed form
- l2arc_write_buffers() may write it to the L2ARC (as is)
- l2arc_read_done() compares the checksum to the BP (compressed)

However, if compressed_arc is disabled:
- the block is read from disk
- It is decompressed
- It is added to the ARC (uncompressed)
- l2arc_write_buffers() will use l2arc_apply_transforms() to
  recompress the block, before writing it to the L2ARC
- l2arc_read_done() compares the checksum to the BP (compressed)
- l2arc_read_done() will use l2arc_untransform() to uncompress it

This test writes out a test file to a pool consisting of one disk
and one cache device, then randomly reads from it. Since the arc_max
in the tests is low, this will feed the L2ARC, and result in reads
from the L2ARC.

We compare the value of the kstat l2_cksum_bad before and after
to determine if any blocks failed to survive the trip through the
L2ARC.

Signed-off-by: Allan Jude <allanjude@freebsd.org>
Sponsored-by: The FreeBSD Foundation
@allanjude allanjude force-pushed the l2arc_compressed_arc branch from 1773a4d to 42ecea1 Compare August 11, 2020 20:47
@allanjude
Copy link
Contributor Author

You'll need to add both of these new tests to the tests/runfiles/common.run file so they're run by default.

Ahh, that explains a lot.

Ok, that is done, lets see what CI things. It should fail the l2arc_compressed_arc_disabled test since that requires the fix from #10693

@behlendorf
Copy link
Contributor

It looks like the test failed as expected. Can you add this change to the fix in #10693, then we can verify it passes and merge both the fix and test in a single commit.

@allanjude
Copy link
Contributor Author

Pushed

@allanjude allanjude closed this Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants