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

zstreamdump: include embedded writes in total_write_size #8431

Closed

Conversation

allanjude
Copy link
Contributor

Signed-off-by: Allan Jude allanjude@freebsd.org

Motivation and Context

zstreamdump is a tool used to analyze the contents of ZFS replication streams.
zstreamdump outputs a summary at the end that includes the total byte
count of all DRR_WRITE records. This excludes the bytes from any
DRR_WRITE_EMBEDDED records, resulting in a total that is not accurate.

DRR_WRITE_EMBEDDED records are only included in a replication stream if it is
generated with zfs send -e.

Description

This change includes the payload size of any embedded write records in the
total_write_size statistic displayed at the end of the zstreamdump run.

How Has This Been Tested?

zfs send -e dataset@snapshot | zstreamdump

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:

zstreamdump outputs a summary at the end that includes the total byte
count of all DRR_WRITE records. This change makes it also include
the bytes from DRR_WRITE_EMBEDDED records, where the data was small
enough to be embedded directly into the block pointer.

Signed-off-by: Allan Jude <allanjude@freebsd.org>
@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #8431 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8431      +/-   ##
==========================================
+ Coverage   78.43%   78.61%   +0.18%     
==========================================
  Files         380      380              
  Lines      115901   115902       +1     
==========================================
+ Hits        90909    91122     +213     
+ Misses      24992    24780     -212
Flag Coverage Δ
#kernel 79.04% <ø> (+0.01%) ⬆️
#user 67.53% <100%> (+0.64%) ⬆️

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 f545b6a...c55fc22. Read the comment docs.

@bunder2015 bunder2015 added the Status: Code Review Needed Ready for review and testing label Feb 20, 2019
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.

If we're expanding what is encompassed by total_write_size we may want to include the payload for DRR_SPILL records too. Or conversely remove total_write_size and instead depend on your new per-record sizes from #8432.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 3, 2019
@allanjude
Copy link
Contributor Author

If we're expanding what is encompassed by total_write_size we may want to include the payload for DRR_SPILL records too. Or conversely remove total_write_size and instead depend on your new per-record sizes from #8432.

Maybe the #8432 version makes more sense. What do people think?

@ahrens
Copy link
Member

ahrens commented Mar 3, 2019

I'd like to see #8432 completed!

@behlendorf
Copy link
Contributor

@allanjude shall we close this out in favor of an updated #8432?

@behlendorf
Copy link
Contributor

Closing for now, let's tackle this in the context of a completed #8432.

@behlendorf behlendorf closed this Mar 15, 2019
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