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

Test for clone, mmap and write for block cloning #15717

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Test for clone, mmap and write for block cloning #15717

merged 1 commit into from
Jan 16, 2024

Conversation

usaleem-ix
Copy link
Contributor

@usaleem-ix usaleem-ix commented Dec 28, 2023 •
edited
Loading

Motivation and Context

For block cloning, if we mmap the cloned file and write from the map into the file, it triggers a panic in dbuf_redirty() on Linux.

The same scenario causes data corruption on FreeBSD. Both these issues are fixed under #15656 and #15665.

Description

It would be good to add a test for this scenario in ZTS. The test program and issue was produced by @robn.

How Has This Been Tested?

Ran the test without the fixes for both issues on Linux and FreeBSD. Without both fixes, the test fails on Debian and FreeBSD.

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:

amotin
amotin reviewed Dec 28, 2023
View reviewed changes
tests/zfs-tests/tests/functional/block_cloning/block_cloning_clone_mmap_write.ksh Outdated
if is_linux; then
log_must [ "$blocks" = "$(seq -s " " 1 4095)" ]
elif is_freebsd; then
log_must [ "$blocks " = "$(seq -s " " 1 4095)" ]
Copy link
Member

@amotin amotin Dec 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@pjd changed this extra space behavior on FreeBSD a week ago to match Linux. It has to be handled somehow else or this test will break after next CI update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, as @pjd has handled under #15686.

pjd
pjd reviewed Dec 31, 2023
View reviewed changes
tests/zfs-tests/cmd/clone_mmap_write.c Outdated
open_file(char *source)
{
int fd;
if ((fd = open(source, O_RDWR | O_APPEND, S_IRUSR | S_IWUSR)) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The third argument to open(2) is redundant if there is no O_CREAT flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated.

tests/zfs-tests/cmd/clone_mmap_write.c Outdated
__attribute__((weak));

static int
open_file(char *source)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

tests/zfs-tests/cmd/clone_mmap_write.c Outdated
}

static int
clone_file(int sfd, long long size, char *dest)
Copy link
Contributor

Choose a reason for hiding this comment

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

dest can be const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

tests/zfs-tests/cmd/clone_mmap_write.c Outdated
#include <errno.h>
#include <sys/stat.h>
#include <sys/mman.h>
#include <sys/stat.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated include.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@behlendorf
Copy link
Contributor

Can be rebased and merged after #15686 is integrated.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jan 9, 2024
@behlendorf
Copy link
Contributor

@usaleem-ix usaleem-ix sorry about the delay, this can be rebased now.

@usaleem-ix
Test for clone, mmap and write for block cloning
b7594ae 
For block cloning, if we mmap the cloned file and write from the
map into the file, it triggers a panic in dbuf_redirty() on Linux.

The same scenario causes data corruption on FreeBSD. Both these
issues are fixed under PR#15656 and PR#15665.

It would be good to add a test for this scenario in ZTS. The test
program and issue was produced by @robn.

Signed-off-by: Umer Saleem <[email protected]>
@usaleem-ix
Copy link
Contributor Author

@behlendorf NP, I have rebased.

@behlendorf behlendorf merged commit 995734e into openzfs:master Jan 16, 2024
23 of 25 checks passed
mmatuska pushed a commit to mmatuska/zfs that referenced this pull request Jan 17, 2024
@usaleem-ix @mmatuska
ZTS: Test for clone, mmap and write for block cloning
8d82549 
For block cloning, if we mmap the cloned file and write from the
map into the file, it triggers a panic in dbuf_redirty() on Linux.

The same scenario causes data corruption on FreeBSD. Both these
issues are fixed under PR#15656 and PR#15665.

It would be good to add a test for this scenario in ZTS. The test
program and issue was produced by @robn.

Reviewed-by: Pawel Jakub Dawidek <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Ameer Hamza <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15717
@mmatuska mmatuska mentioned this pull request Jan 17, 2024
Merged
13 tasks
mmatuska pushed a commit to mmatuska/zfs that referenced this pull request Jan 18, 2024
@usaleem-ix @mmatuska
ZTS: Test for clone, mmap and write for block cloning
0486018 
For block cloning, if we mmap the cloned file and write from the
map into the file, it triggers a panic in dbuf_redirty() on Linux.

The same scenario causes data corruption on FreeBSD. Both these
issues are fixed under PR#15656 and PR#15665.

It would be good to add a test for this scenario in ZTS. The test
program and issue was produced by @robn.

Reviewed-by: Pawel Jakub Dawidek <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Ameer Hamza <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15717
mmatuska pushed a commit to mmatuska/zfs that referenced this pull request Jan 18, 2024
@usaleem-ix @mmatuska
ZTS: Test for clone, mmap and write for block cloning
5e73af0 
For block cloning, if we mmap the cloned file and write from the
map into the file, it triggers a panic in dbuf_redirty() on Linux.

The same scenario causes data corruption on FreeBSD. Both these
issues are fixed under PR#15656 and PR#15665.

It would be good to add a test for this scenario in ZTS. The test
program and issue was produced by @robn.

Reviewed-by: Pawel Jakub Dawidek <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Ameer Hamza <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15717
behlendorf pushed a commit that referenced this pull request Jan 19, 2024
@usaleem-ix @behlendorf
ZTS: Test for clone, mmap and write for block cloning
d2f7b2e 
For block cloning, if we mmap the cloned file and write from the
map into the file, it triggers a panic in dbuf_redirty() on Linux.

The same scenario causes data corruption on FreeBSD. Both these
issues are fixed under PR#15656 and PR#15665.

It would be good to add a test for this scenario in ZTS. The test
program and issue was produced by @robn.

Reviewed-by: Pawel Jakub Dawidek <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Ameer Hamza <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes #15717
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@usaleem-ix @lundman
ZTS: Test for clone, mmap and write for block cloning
88e8e2a 
For block cloning, if we mmap the cloned file and write from the
map into the file, it triggers a panic in dbuf_redirty() on Linux.

The same scenario causes data corruption on FreeBSD. Both these
issues are fixed under PR#15656 and PR#15665.

It would be good to add a test for this scenario in ZTS. The test
program and issue was produced by @robn.

Reviewed-by: Pawel Jakub Dawidek <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Ameer Hamza <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15717
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@usaleem-ix @lundman
ZTS: Test for clone, mmap and write for block cloning
484d8b9 
For block cloning, if we mmap the cloned file and write from the
map into the file, it triggers a panic in dbuf_redirty() on Linux.

The same scenario causes data corruption on FreeBSD. Both these
issues are fixed under PR#15656 and PR#15665.

It would be good to add a test for this scenario in ZTS. The test
program and issue was produced by @robn.

Reviewed-by: Pawel Jakub Dawidek <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Ameer Hamza <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15717
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ixhamza ixhamza ixhamza approved these changes

@amotin amotin amotin approved these changes

@pjd pjd pjd approved these changes

@behlendorf behlendorf behlendorf 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.
5 participants
@usaleem-ix @behlendorf @pjd @amotin @ixhamza