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

Fix bp_embedded_type enum definition #8951

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

loli10K
Copy link
Contributor

@loli10K loli10K commented Jun 23, 2019

Motivation and Context

With the addition of BP_EMBEDDED_TYPE_REDACTED in 30af21b a couple of codepaths make wrong assumptions and could potentially result in errors.

(gdb) ptype bp_embedded_type_t
type = enum bp_embedded_type {BP_EMBEDDED_TYPE_DATA, BP_EMBEDDED_TYPE_RESERVED, BP_EMBEDDED_TYPE_REDACTED, NUM_BP_EMBEDDED_TYPES = 1}
(gdb) p/d BP_EMBEDDED_TYPE_REDACTED
$1 = 2
(gdb) 

With a bp (etype = BP_EMBEDDED_TYPE_REDACTED) we could panic in zfs_blkptr_verify():
https://github.com/zfsonlinux/zfs/blob/30af21b02569ac192f52ce6e6511015f8a8d5729/module/zfs/zio.c#L910-L915

Also we would not be able to receive records with drr_etype = BP_EMBEDDED_TYPE_REDACTED:
https://github.com/zfsonlinux/zfs/blob/30af21b02569ac192f52ce6e6511015f8a8d5729/module/zfs/dmu_recv.c#L1849-L1850

And finally we will fail an assetion in dmu_write_embedded():
https://github.com/zfsonlinux/zfs/blob/30af21b02569ac192f52ce6e6511015f8a8d5729/module/zfs/dmu.c#L1270-L1276

I'm not sure if BP_EMBEDDED_TYPE_RESERVED was supposed to be BP_EMBEDDED_TYPE_REDACTED all along. I could not find any usage of BP_SET_REDACTED(), dmu_buf_redact() and dmu_redact() in the code.

Description

Simply update NUM_BP_EMBEDDED_TYPES in bp_embedded_type_t

How Has This Been Tested?

Not tested, pushed as draft.

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:

@loli10K loli10K requested a review from pcd1193182 June 23, 2019 07:03
@codecov
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

Merging #8951 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8951      +/-   ##
==========================================
+ Coverage   78.61%    78.7%   +0.09%     
==========================================
  Files         388      388              
  Lines      120071   120071              
==========================================
+ Hits        94388    94503     +115     
+ Misses      25683    25568     -115
Flag Coverage Δ
#kernel 79.46% <100%> (+0.01%) ⬆️
#user 66.34% <100%> (+0.31%) ⬆️

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 8f12a4f...66caab6. Read the comment docs.

Copy link
Contributor

@chrisrd chrisrd left a comment

Choose a reason for hiding this comment

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

How about fixing struct zdb_cb in cmd/zdb/zdb.c as well? E.g.:

typedef struct zdb_cb {
        ...
        uint64_t        zcb_embedded_blocks[NUM_BP_EMBEDDED_TYPES - 1];
        uint64_t        zcb_embedded_histogram[NUM_BP_EMBEDDED_TYPES - 1]
            [BPE_PAYLOAD_SIZE + 1];
        ...
} zdb_cb_t;

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 24, 2019
Copy link
Contributor

@pcd1193182 pcd1193182 left a comment

Choose a reason for hiding this comment

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

Currently, redaction BPs aren't actually used by redacted receive for performance reasons (need an efficient implementation of redacting large ranges); re-enabling them is on my roadmap.

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.

@loli10K thanks for catching this. @pcd1193182 helpfully referred me to the DxOS source where BP_EMBEDDED_TYPE_RESERVED is defined and used for an entirely different feature. While you're here it would be a nice to update that comment. Maybe:

 	BP_EMBEDDED_TYPE_RESERVED, /* Reserved for Delphix byteswap feature. */

With the addition of BP_EMBEDDED_TYPE_REDACTED in 30af21b a couple of
codepaths make wrong assumptions and could potentially result in errors.

Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
@loli10K loli10K marked this pull request as ready for review June 24, 2019 17:42
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 25, 2019
@behlendorf behlendorf merged commit 746d4a4 into openzfs:master Jun 25, 2019
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 24, 2019
With the addition of BP_EMBEDDED_TYPE_REDACTED in 30af21b a couple of
codepaths make wrong assumptions and could potentially result in errors.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chris Dunlop <chris@onthe.net.au>
Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#8951
Conflicts:
	include/sys/spa.h
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
With the addition of BP_EMBEDDED_TYPE_REDACTED in 30af21b a couple of
codepaths make wrong assumptions and could potentially result in errors.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chris Dunlop <chris@onthe.net.au>
Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#8951
Conflicts:
	include/sys/spa.h
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
With the addition of BP_EMBEDDED_TYPE_REDACTED in 30af21b a couple of
codepaths make wrong assumptions and could potentially result in errors.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chris Dunlop <chris@onthe.net.au>
Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes #8951
Conflicts:
	include/sys/spa.h
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.

4 participants