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 dmu_recv_stream test for resumable #12034

Merged
merged 1 commit into from
May 14, 2021
Merged

Conversation

PaulZ-98
Copy link
Contributor

Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Signed-off-by: Paul Zuchowski [email protected]

There is code at the top of dmu_recv_stream() to check if the stream is resumable. The code as written seems to imply that if dsl_dataset_is_zapified(), then a DS_FIELD_RESUME_BYTES zap entry will be found. This is inconsistent with other places in ZFS where the more complete dsl_dataset_has_resume_receive_state() is used for this.

Because of this test, the zap_lookup for DS_FIELD_RESUME_BYTES does fail from time to time with ENOENT, and an uninitialized value can end up in drc_bytes_read, sometimes causing a panic at places where bytes_read is VERIFY'd.

Motivation and Context

This caused a panic in #9372 automated testing, under FreeBSD 13.

Description

This fix uses dsl_dataset_has_resume_receive_state() as the test, and initializes the bytes variable to 0 in case the zap lookup fails.

How Has This Been Tested?

Ran rsend and redacted_send portions of zfs tests.

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:

Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Signed-off-by: Paul Zuchowski <[email protected]>
@ahrens ahrens requested a review from pcd1193182 May 12, 2021 16:23
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.

I don't think changing the check helps in practice, but it illustrates the intent at least. Nice find.

@PaulZ-98
Copy link
Contributor Author

PaulZ-98 commented May 12, 2021

I just investigated this, and noted that the change to dsl_dataset_has_resume_receive_state eliminates the occurrences of zap_lookup / DS_FIELD_RESUME_BYTES returning ENOENT while running the redacted_send zfs tests. If the check is dsl_dataset_is_zapified, you get many instances of ENOENT while running the redacted_send zfs tests.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label May 12, 2021
@behlendorf behlendorf merged commit fce29d6 into openzfs:master May 14, 2021
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request May 17, 2021
Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Paul Zuchowski <[email protected]>
Closes openzfs#12034
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request May 17, 2021
Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Paul Zuchowski <[email protected]>
Closes openzfs#12034
ghost pushed a commit to truenas/zfs that referenced this pull request May 27, 2021
Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Paul Zuchowski <[email protected]>
Closes openzfs#12034
ghost pushed a commit to truenas/zfs that referenced this pull request May 27, 2021
Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Paul Zuchowski <[email protected]>
Closes openzfs#12034
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 28, 2021
Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Paul Zuchowski <[email protected]>
Closes openzfs#12034
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Paul Zuchowski <[email protected]>
Closes openzfs#12034
tonyhutter pushed a commit that referenced this pull request Jun 2, 2021
Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Paul Zuchowski <[email protected]>
Closes #12034
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jun 2, 2021
Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Paul Zuchowski <[email protected]>
Closes openzfs#12034
tonyhutter pushed a commit that referenced this pull request Jun 23, 2021
Use dsl_dataset_has_resume_receive_state()
not dsl_dataset_is_zapified() to check if
stream is resumable.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Paul Zuchowski <[email protected]>
Closes #12034
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