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

Dont iterate through filesystems unnecessarily #8539

Merged
merged 1 commit into from
Apr 1, 2019

Conversation

tcaputi
Copy link
Contributor

@tcaputi tcaputi commented Mar 27, 2019

Currently, when attempting to list snapshots ZFS may do a lot of
extra work checking child datasets. This is because the code does
not realize that it will not be able to reach any snapshots
contained within snapshots that are at the depth limit since the
snapshots of those datasets are counted as an additional layer
deeper. This patch corrects this issue.

In addition, this patch adds the ability to do perform the commands:

$ zfs list -t snapshot
$ zfs get -t snapshot

as a convenient way to list out properties of all snapshots of a
given dataset without having to use the depth limit.

Signed-off-by: Tom Caputi tcaputi@datto.com

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:

@tcaputi tcaputi force-pushed the fix_snap_iteration branch from ac7ee3c to b3caec4 Compare March 27, 2019 18:21
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Mar 27, 2019
*/
if ((cb->cb_depth < cb->cb_depth_limit ||
(cb->cb_types & ZFS_TYPE_FILESYSTEM)) &&
zfs_get_type(zhp) == ZFS_TYPE_FILESYSTEM)
Copy link
Member

Choose a reason for hiding this comment

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

Should this if have braces? The second part of the diff exists only to add them to the if below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes indeed it should, @tcaputi can you please add these.

@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #8539 into master will increase coverage by 0.1%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #8539     +/-   ##
=========================================
+ Coverage   78.59%    78.7%   +0.1%     
=========================================
  Files         380      380             
  Lines      116440   116450     +10     
=========================================
+ Hits        91521    91652    +131     
+ Misses      24919    24798    -121
Flag Coverage Δ
#kernel 79.14% <ø> (+0.03%) ⬆️
#user 67.44% <75%> (+0.24%) ⬆️

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 85a150c...b3caec4. Read the comment docs.

@tcaputi tcaputi force-pushed the fix_snap_iteration branch from b3caec4 to 2591097 Compare March 29, 2019 18:11
@tcaputi
Copy link
Contributor Author

tcaputi commented Mar 29, 2019

@behlendorf I fixed the issues found in the test cases. Please re-review.

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.

Thanks for resolving the test failures. This still looks good, just the one minor style nit.

*/
if ((cb->cb_depth < cb->cb_depth_limit ||
(cb->cb_types & ZFS_TYPE_FILESYSTEM)) &&
zfs_get_type(zhp) == ZFS_TYPE_FILESYSTEM)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes indeed it should, @tcaputi can you please add these.

Currently, when attempting to list snapshots ZFS may do a lot of
extra work checking child datasets. This is because the code does
not realize that it will not be able to reach any snapshots
conatined within snapshots that are at the depth limit since the
snapshots of those datasets are counted as an additional layer
deeper. This patch corrects this issue.

In addition, this patch adds the ability to do perform the commands:

$ zfs list -t snapshot <dataset>
$ zfs get -t snapshot <prop> <dataset>

as a convenient way to list out properties of all snapshots of a
given dataset without having to use the depth limit.

Signed-off-by: Tom Caputi <tcaputi@datto.com>
@tcaputi tcaputi force-pushed the fix_snap_iteration branch from 2591097 to 55372e5 Compare March 29, 2019 22:03
@tcaputi
Copy link
Contributor Author

tcaputi commented Mar 29, 2019

@behlendorf fixed.

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

codecov bot commented Mar 30, 2019

Codecov Report

Merging #8539 into master will decrease coverage by 0.06%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8539      +/-   ##
==========================================
- Coverage   78.72%   78.66%   -0.07%     
==========================================
  Files         381      380       -1     
  Lines      117499   116451    -1048     
==========================================
- Hits        92497    91601     -896     
+ Misses      25002    24850     -152
Flag Coverage Δ
#kernel 79.04% <ø> (-0.27%) ⬇️
#user 67.38% <80%> (+0.07%) ⬆️

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 1b93956...55372e5. Read the comment docs.

@behlendorf behlendorf merged commit df58307 into openzfs:master Apr 1, 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.

6 participants