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

zio_decompress_data always ASSERTs successful decompression #9630

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

PaulZ-98
Copy link
Contributor

@PaulZ-98 PaulZ-98 commented Nov 26, 2019

zio_decompress_data affects zdb by always ASSERTing successful decompression

This interferes with zdb_read_block trying all the decompression algoirithms when the 'd' flag is specified, as some are expected to fail. Also control the output when guessing algorithms, try the more common compression types first, allow specifying lsize/psize, and fix an uninitialized variable.

Signed-off-by: Paul Zuchowski pzuchowski@datto.com
Fixes #9612

Motivation and Context

Improve the behavior of zdb -R with the 'd' flag for decompression. As noted above, not all the use cases are working.

Description

Remove the ASSERT. Control the output when guessing algorithms by adding a 'v' verbose flag.
Try the more common compression types first.
Allow specifying lsize/psize to limit the upper range of the decompressed size when guessing. (You can copy and paste lsize/psize from the block pointer output.). Examples:

zdb -eR tank 0:2e0004992000:4000L/1000P:d
zdb -eR tank 0:2e0004992000:4000/1000:d
zdb -eR tank 0:2e0004992000:1000:d

Fix an uninitialized variable dmu_objset_stats_t dds in dump_objset() which results in random false positive for inconsistent dataset in zdb output.
Eliminate the 'p' flag for -R since it is never used.
Man page changes.

How Has This Been Tested?

Manually, and created a zfs test for the decompression features.

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:

@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

Merging #9630 into master will increase coverage by <1%.
The diff coverage is 91%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #9630    +/-   ##
========================================
+ Coverage      79%      79%   +<1%     
========================================
  Files         418      418            
  Lines      123577   123589    +12     
========================================
+ Hits        97951    97990    +39     
+ Misses      25626    25599    -27
Flag Coverage Δ
#kernel 80% <ø> (ø) ⬆️
#user 67% <91%> (ø) ⬇️

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 3c502d3...98f7544. Read the comment docs.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 27, 2019
cmd/zdb/zdb.c Outdated Show resolved Hide resolved
cmd/zdb/zdb.c Outdated Show resolved Hide resolved
cmd/zdb/zdb.c Show resolved Hide resolved
module/zfs/zio_compress.c Outdated Show resolved Hide resolved
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.

Looks to me, but it does need a rebase.

@behlendorf behlendorf requested a review from dweeezil December 6, 2019 01:26
@ikozhukhov
Copy link
Contributor

ikozhukhov commented Dec 6, 2019

missed previous update with zdb_checksum.ksh ?

@PaulZ-98
Copy link
Contributor Author

PaulZ-98 commented Dec 6, 2019

I will rebase making sure to pick up the most recent zdb related changes, and update the description above to reflect what we decided to do.

@behlendorf behlendorf requested a review from ikozhukhov December 9, 2019 21:53
@ikozhukhov
Copy link
Contributor

give me please 1-2 days on testing. if it is critical, you can merge it and i'll report tests/review results later.

@PaulZ-98
Copy link
Contributor Author

Later today I will be addressing the test procedure issue that @freqlabs pointed out.

This interferes with zdb_read_block trying all the decompression
algorithms when the 'd' flag is specified, as some are
expected to fail.  Also control the output when guessing
algorithms, try the more common compression types first, allow
specifying lsize/psize, and fix an uninitialized variable.

Signed-off-by: Paul Zuchowski <pzuchowski@datto.com>
Fixes openzfs#9612
@behlendorf behlendorf requested a review from a user December 10, 2019 22:03
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks, that makes more sense 👍

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 10, 2019
@behlendorf behlendorf merged commit f0bf435 into openzfs:master Dec 10, 2019
@ikozhukhov
Copy link
Contributor

Test: /opt/zfs-tests/tests/functional/cli_root/zdb/zdb_001_neg.ksh (run as root) [03:54] [PASS]
Test: /opt/zfs-tests/tests/functional/cli_root/zdb/zdb_002_pos.ksh (run as root) [00:28] [PASS]
Test: /opt/zfs-tests/tests/functional/cli_root/zdb/zdb_003_pos.ksh (run as root) [00:15] [PASS]
Test: /opt/zfs-tests/tests/functional/cli_root/zdb/zdb_004_pos.ksh (run as root) [00:15] [PASS]
Test: /opt/zfs-tests/tests/functional/cli_root/zdb/zdb_005_pos.ksh (run as root) [00:22] [PASS]
Test: /opt/zfs-tests/tests/functional/cli_root/zdb/zdb_006_pos.ksh (run as root) [01:15] [PASS]
Test: /opt/zfs-tests/tests/functional/cli_root/zdb/zdb_checksum.ksh (run as root) [00:18] [PASS]
Test: /opt/zfs-tests/tests/functional/cli_root/zdb/zdb_decompress.ksh (run as root) [00:59] [PASS]

Results Summary
PASS       8

Running Time:   00:07:50
Percent passed: 100.0%
Log directory:  /var/tmp/test_results/20191211T222800

@PaulZ-98
Copy link
Contributor Author

Thanks for checking @ikozhukhov

tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 26, 2019
This interferes with zdb_read_block trying all the decompression
algorithms when the 'd' flag is specified, as some are
expected to fail.  Also control the output when guessing
algorithms, try the more common compression types first, allow
specifying lsize/psize, and fix an uninitialized variable.

Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Paul Zuchowski <pzuchowski@datto.com>
Closes openzfs#9612
Closes openzfs#9630
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
This interferes with zdb_read_block trying all the decompression
algorithms when the 'd' flag is specified, as some are
expected to fail.  Also control the output when guessing
algorithms, try the more common compression types first, allow
specifying lsize/psize, and fix an uninitialized variable.

Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Paul Zuchowski <pzuchowski@datto.com>
Closes openzfs#9612
Closes openzfs#9630
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
This interferes with zdb_read_block trying all the decompression
algorithms when the 'd' flag is specified, as some are
expected to fail.  Also control the output when guessing
algorithms, try the more common compression types first, allow
specifying lsize/psize, and fix an uninitialized variable.

Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Paul Zuchowski <pzuchowski@datto.com>
Closes #9612
Closes #9630
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.

zio_decompress_data always ASSERTs successful decompression
3 participants