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 cloning into mmaped and cached file. #15772

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Fix cloning into mmaped and cached file. #15772

merged 1 commit into from
Jan 17, 2024

Conversation

pjd
Copy link
Contributor

@pjd pjd commented Jan 14, 2024 •
edited
Loading

Motivation and Context

If the destination file is mmaped and the mmaped region was already read, so it is cached, we need to update mmaped pages after sucessful clone using update_pages().

Description

I've added update_pages() call if the destination znode is cached similar to what zfs_write() does.

How Has This Been Tested?

I've added clone_mmap_cached tool which can test four cases:

  1. Source and destination files are mmaped, but not cached.
  2. Source and destination files are mmaped, but only source is cached.
  3. Source and destination files are mmaped, but only destination is cached.
  4. Source and destination files are mmaped and both are cached.

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 pjd force-pushed the bclone-mmap-cached branch 3 times, most recently from 4d2a816 to 430bac2 Compare January 14, 2024 01:37
amotin
amotin approved these changes Jan 16, 2024
View reviewed changes
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

Looks right to me. I just wonder if we could invalidate the affected pages, not read them, since in case of cloning, unlike write, we may not have the new data in cache, and reading from disk may be unneeded while being expensive. Though I suppose this is likely a rare scenario, so not a blocker IMO.

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

@pjd when you get a chance can you rebase this to resolve the conflicts. There was an issue with the FreeBSD builders last time as well so they didn't run. The rebase will trigger a fresh run where they should run this time.

@pjd
Fix cloning into mmaped and cached file.
3732779 
If the destination file is mmaped and the mmaped region was already
read, so it is cached, we need to update mmaped pages after sucessful
clone using update_pages().

Pointed out by: Ka Ho Ng <khng@freebsd.org>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
@pjd pjd force-pushed the bclone-mmap-cached branch from 430bac2 to 3732779 Compare January 17, 2024 04:13
@pjd
Copy link
Contributor Author

pjd commented Jan 17, 2024

@pjd when you get a chance can you rebase this to resolve the conflicts. There was an issue with the FreeBSD builders last time as well so they didn't run. The rebase will trigger a fresh run where they should run this time.

Done.

@mmatuska
Copy link
Contributor

@pjd it would be great to have this change in 2.2.3

@behlendorf behlendorf merged commit f45dd90 into openzfs:master Jan 17, 2024
22 of 26 checks passed
@behlendorf
Copy link
Contributor

behlendorf commented Jan 17, 2024 •
edited
Loading

Merged. Yes it would be great to get a backport of the merged commit open against the 2.2.3-staging branch.

We are also still seeing a couple BRT failures in the test suite. Likely minor portability issues with the tests.

mmatuska pushed a commit to mmatuska/zfs that referenced this pull request Jan 17, 2024
@pjd @mmatuska
Fix cloning into mmaped and cached file.
1dc69d3 
If the destination file is mmaped and the mmaped region was already
read, so it is cached, we need to update mmaped pages after successful
clone using update_pages().

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Pointed out by: Ka Ho Ng <khng@freebsd.org>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
Closes openzfs#15772
@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
@pjd @mmatuska
Fix cloning into mmaped and cached file.
913ebda 
If the destination file is mmaped and the mmaped region was already
read, so it is cached, we need to update mmaped pages after successful
clone using update_pages().

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Pointed out by: Ka Ho Ng <khng@freebsd.org>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
Closes openzfs#15772
mmatuska pushed a commit to mmatuska/zfs that referenced this pull request Jan 18, 2024
@pjd @mmatuska
Fix cloning into mmaped and cached file.
9a736d1 
If the destination file is mmaped and the mmaped region was already
read, so it is cached, we need to update mmaped pages after successful
clone using update_pages().

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Pointed out by: Ka Ho Ng <khng@freebsd.org>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
Closes openzfs#15772
behlendorf pushed a commit that referenced this pull request Jan 19, 2024
@pjd @behlendorf
Fix cloning into mmaped and cached file.
ef52795 
If the destination file is mmaped and the mmaped region was already
read, so it is cached, we need to update mmaped pages after successful
clone using update_pages().

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Pointed out by: Ka Ho Ng <khng@freebsd.org>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
Closes #15772
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@pjd @lundman
Fix cloning into mmaped and cached file.
a885a98 
If the destination file is mmaped and the mmaped region was already
read, so it is cached, we need to update mmaped pages after successful
clone using update_pages().

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Pointed out by: Ka Ho Ng <khng@freebsd.org>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
Closes openzfs#15772
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@pjd @lundman
Fix cloning into mmaped and cached file.
da29d6b 
If the destination file is mmaped and the mmaped region was already
read, so it is cached, we need to update mmaped pages after successful
clone using update_pages().

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Pointed out by: Ka Ho Ng <khng@freebsd.org>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
Closes openzfs#15772
@rincebrain rincebrain mentioned this pull request Mar 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amotin amotin amotin 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.
4 participants
@pjd @behlendorf @mmatuska @amotin