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.

Sign up for GitHub

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

Jump to bottom

Fix data corruption when cloning embedded blocks. #14739

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Fix data corruption when cloning embedded blocks. #14739

merged 1 commit into from
Apr 12, 2023

Conversation

pjd
Copy link
Contributor

@pjd pjd commented Apr 11, 2023

Don't overwrite blk_phys_birth, as for embedded blocks it is part of the payload.

Motivation and Context

Description

How Has This Been Tested?

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:

@pjd
Fix data corruption when cloning embedded blocks.
d3a6e5c 
Don't overwrite blk_phys_birth, as for embedded blocks it is part of
the payload.

Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
@pjd pjd mentioned this pull request Apr 11, 2023
Merged
13 tasks
behlendorf
behlendorf approved these changes Apr 12, 2023
View reviewed changes
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 running this down so quickly. The fix itself looks right to me.

Maybe integrating pjdfstest would be beneficial?

Adding this to the CI to provide more low level coverage of the system calls would be great.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Apr 12, 2023
@ryao
Copy link
Contributor

ryao commented Apr 12, 2023

I'm also working on adding tests, but I'd like to be able to have a more complete coverage and currently there is no userland tool apart from cp(1) that uses copy_file_range(2).

We can add our own tools to the ZTS, although getting tests that use cp(1) would be great in itself.

Maybe integrating pjdfstest would be beneficial? I could extend fstest tool to allow testing copy_file_range(2) with different offsets and sizes.

We could use a github runner to integrate it into PRs, although at the moment, github runners do not support FreeBSD and we would like test coverage of this on FreeBSD since it is the only platform in the main repository where this feature is enabled.

@rincebrain
Copy link
Contributor

The problem with using cp(1) on Linux, at least, would be that you'd need to get a very new cp(1) to make the relevant calls once that's plumbed in, unless you were trying to test the behavior of older versions specifically...

@behlendorf behlendorf merged commit c71fe71 into openzfs:master Apr 12, 2023
@ryao
Copy link
Contributor

ryao commented Apr 13, 2023

The problem with using cp(1) on Linux, at least, would be that you'd need to get a very new cp(1) to make the relevant calls once that's plumbed in, unless you were trying to test the behavior of older versions specifically...

It is possible to have tests disabled if the userland tools are not new enough.

That said, I envision the initial version of the test being FreeBSD specific with Linux support added later when Linux support is merged into the repository.

@pjd
Copy link
Contributor Author

pjd commented Apr 13, 2023

I've cp(1) tests ready that work on FreeBSD. I'll create a pull request soon.

freebsd-git pushed a commit to freebsd/freebsd-src that referenced this pull request Apr 17, 2023
zfs: Merge openzfs/zfs#14739
1959e12 
The zfs_log_clone_range() function is never called from the
zfs_clone_range_replay() function, so I assumed it is safe to assert
that zil_replaying() is never TRUE here. It turns out zil_replaying()
also returns TRUE when the sync property is set to disabled.

Fix the problem by just returning if zil_replaying() returns TRUE.

Reported by: Florian Smeets
Signed-off-by: Pawel Jakub Dawidek pawel@dawidek.net

Approved by: oshogbo, mm
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Apr 30, 2023
@pjd @andrewc12
Fix data corruption when cloning embedded blocks
3d61173 
Don't overwrite blk_phys_birth, as for embedded blocks it is part of
the payload.

Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
Issue openzfs#13392 
Closes openzfs#14739
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Apr 30, 2023
@pjd @andrewc12
Fix data corruption when cloning embedded blocks
56405ae 
Don't overwrite blk_phys_birth, as for embedded blocks it is part of
the payload.

Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
Issue openzfs#13392 
Closes openzfs#14739
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Jun 21, 2023
@bsdjhb
zfs: Merge openzfs/zfs#14739
64c3a7c 
The zfs_log_clone_range() function is never called from the
zfs_clone_range_replay() function, so I assumed it is safe to assert
that zil_replaying() is never TRUE here. It turns out zil_replaying()
also returns TRUE when the sync property is set to disabled.

Fix the problem by just returning if zil_replaying() returns TRUE.

Reported by: Florian Smeets
Signed-off-by: Pawel Jakub Dawidek pawel@dawidek.net

Approved by: oshogbo, mm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@ryao ryao ryao approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pjd @ryao @rincebrain @behlendorf