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

Let zfs diff be more permissive. #12072

Merged
merged 1 commit into from
Jun 4, 2021
Merged

Conversation

rincebrain
Copy link
Contributor

Motivation and Context

#6335

Description

In the current world, zfs diff will die on first encounter of certain kinds of errors that come up on ordinary, not-mangled snapshots - like EINVAL, which can apparently come from a file with multiple hardlinks having the one whose name is referenced deleted.

Since they don't appear to indicate the snapshot is mangled in any way, let's relax about ENOENT and EINVAL - still print an error to stderr, but don't immediately abort when we encounter them.

How Has This Been Tested?

Quickly ran it locally on a pair of snapshots with 1TB of diff between them -

  • Vanilla zfs diff dies after 48 files on the first EINVAL
  • With this PR, zfs diff spits out 9326 lines, and the last one does not appear to be it dying on another error code.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

lib/libzfs/libzfs_diff.c Outdated Show resolved Hide resolved
Copy link
Contributor

@tonynguien tonynguien left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rincebrain
Copy link
Contributor Author

This may take a little while for me to attempt the kernel changes version for, I need to do some poking around to figure out where the relevant return codes are coming from, but that's going to require making a copy of the dataset I have that triggers these conditions to play with in a VM, and it's several TB.

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.

Yup, I think this makes sense.

lib/libzfs/libzfs_diff.c Outdated Show resolved Hide resolved
In the current world, `zfs diff` will die on certain kinds of errors
that come up on ordinary, not-mangled filesystems - like EINVAL,
which can come from a file with multiple hardlinks having the one
whose name is referenced deleted.

Since it should always be safe to continue, let's relax about all
error codes - still print something for most, but don't immediately
abort when we encounter them.

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@tonynguien tonynguien added Status: Accepted Ready to integrate (reviewed, tested) Status: Code Review Needed Ready for review and testing and removed Status: Code Review Needed Ready for review and testing labels Jun 4, 2021
@tonynguien
Copy link
Contributor

@rincebrain - I updated the description/commit message to reflect current code. Can you let me know if this is OK or provide a description that I can use? Thanks!

======== Description ========
In the current world, zfs diff will die on first encounter of certain
kinds of errors that come up on ordinary, not-mangled snapshots - like
ENOENT for non-existent object and ENOTSUP for non-ZPL object.

Since they don't appear to indicate the snapshot is mangled in any way,
let's relax about ENOENT and ENOTSUP - still print an error to stderr,
but don't immediately abort when we encounter them.

@rincebrain
Copy link
Contributor Author

This seems inaccurate, as it's (almost entirely; there is one ENOENT change because it can turn up somewhere else now) not the behavior of ENOTSUP/ENOENT we're changing, it's everything else - what about the current commit message do you disagree with?

@tonynguien
Copy link
Contributor

This seems inaccurate, as it's (almost entirely; there is one ENOENT change because it can turn up somewhere else now) not the behavior of ENOTSUP/ENOENT we're changing, it's everything else - what about the current commit message do you disagree with?

You're right, of course. I misread that.

I'll use current commit message. Thanks!

@tonynguien tonynguien removed the Status: Code Review Needed Ready for review and testing label Jun 4, 2021
@tonynguien tonynguien merged commit 50353db into openzfs:master Jun 4, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 8, 2021
In the current world, `zfs diff` will die on certain kinds of errors
that come up on ordinary, not-mangled filesystems - like EINVAL,
which can come from a file with multiple hardlinks having the one
whose name is referenced deleted.

Since it should always be safe to continue, let's relax about all
error codes - still print something for most, but don't immediately
abort when we encounter them.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12072
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 8, 2021
In the current world, `zfs diff` will die on certain kinds of errors
that come up on ordinary, not-mangled filesystems - like EINVAL,
which can come from a file with multiple hardlinks having the one
whose name is referenced deleted.

Since it should always be safe to continue, let's relax about all
error codes - still print something for most, but don't immediately
abort when we encounter them.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12072
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 9, 2021
In the current world, `zfs diff` will die on certain kinds of errors
that come up on ordinary, not-mangled filesystems - like EINVAL,
which can come from a file with multiple hardlinks having the one
whose name is referenced deleted.

Since it should always be safe to continue, let's relax about all
error codes - still print something for most, but don't immediately
abort when we encounter them.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12072
rincebrain added a commit to rincebrain/zfs that referenced this pull request Oct 9, 2021
In the current world, `zfs diff` will die on certain kinds of errors
that come up on ordinary, not-mangled filesystems - like EINVAL,
which can come from a file with multiple hardlinks having the one
whose name is referenced deleted.

Since it should always be safe to continue, let's relax about all
error codes - still print something for most, but don't immediately
abort when we encounter them.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12072
tonyhutter pushed a commit that referenced this pull request Nov 1, 2021
In the current world, `zfs diff` will die on certain kinds of errors
that come up on ordinary, not-mangled filesystems - like EINVAL,
which can come from a file with multiple hardlinks having the one
whose name is referenced deleted.

Since it should always be safe to continue, let's relax about all
error codes - still print something for most, but don't immediately
abort when we encounter them.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes #12072
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 13, 2021
In the current world, `zfs diff` will die on certain kinds of errors
that come up on ordinary, not-mangled filesystems - like EINVAL,
which can come from a file with multiple hardlinks having the one
whose name is referenced deleted.

Since it should always be safe to continue, let's relax about all
error codes - still print something for most, but don't immediately
abort when we encounter them.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12072
@rincebrain rincebrain mentioned this pull request Dec 11, 2021
13 tasks
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.

None yet

3 participants