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 lseek(SEEK_DATA/SEEK_HOLE) mmap consistency #12724

Merged
merged 1 commit into from
Nov 7, 2021

Conversation

behlendorf
Copy link
Contributor

Motivation and Context

Issue #11900. When a file is written using mmap holes will not be
correctly reported until an msync() is run and those pages go through
writeback.

Description

When using lseek(2) to report data/holes memory mapped regions of
the file were ignored. This could result in incorrect results.
To handle this zfs_holey_common() was updated to asynchronously
writeback any dirty mmap(2) regions prior to reporting holes.

Additionally, while not strictly required the dn_struct_rwlock is
now held over the dirty check to prevent the dnode structure from
changing. This ensures that a clean dnode can't be dirtied before
the data/hole is located. The range lock is now also taken to
ensure the call cannot race with zfs_write().

Furthermore, the code was refactored to provide a dnode_is_dirty()
helper function which checks the dnode for any dirty records to
determine its dirtiness.

A test case was added which does some basic verification that
data/holes are reported correctly when writing a file with mmap.

How Has This Been Tested?

Added a new mmap_seek_001_pos test case which verifies
the basic data/hole reporting for files written with mmap(2).
Independently verified that with the patch applied the issue
can no longer be recreated with the original reproducer.

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:

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 3, 2021
@behlendorf
Copy link
Contributor Author

@asomers I believe this is a general fix for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256205, would you mind reviewing it. Particularly the FreeBSD specific bits to make sure we're using the right interface.

Copy link
Contributor

@rincebrain rincebrain left a comment

Choose a reason for hiding this comment

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

LGTM

tests/zfs-tests/cmd/mmap_seek/mmap_seek.c Outdated Show resolved Hide resolved
@behlendorf behlendorf force-pushed the zfs-holey branch 2 times, most recently from f23a7e5 to 05ad282 Compare November 3, 2021 20:59
Copy link
Member

@ahrens ahrens left a comment

Choose a reason for hiding this comment

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

Great find! Does this supersede #12710, or do we think there may be additional problems with SEEK_HOLE?

module/zfs/dmu.c Show resolved Hide resolved
@asomers
Copy link
Contributor

asomers commented Nov 3, 2021

@asomers I believe this is a general fix for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256205, would you mind reviewing it. Particularly the FreeBSD specific bits to make sure we're using the right interface.

@kostikbel could you review this one?

@behlendorf
Copy link
Contributor Author

behlendorf commented Nov 3, 2021

Does this supersede #12710, or do we think there may be additional problems with SEEK_HOLE?

Yes, I believe this removes the need for #12710. After applying the patch the original reported issue is no longer reproducible, and we did confirm that it was doing its writes with mmap. So that explains it nicely.

Copy link
Contributor

@tonyhutter tonyhutter 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 see any surface level issues with this (although I'm unfamiliar with the dmu/dnode code).

@kostikbel
Copy link

@asomers I believe this is a general fix for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256205, would you mind reviewing it. Particularly the FreeBSD specific bits to make sure we're using the right interface.

@kostikbel could you review this one?

It looks fine, as far as the vnode is locked around calls to vn_flush_cached_data()

@behlendorf
Copy link
Contributor Author

According to the CI it appears vm_object_mightbedirty() wasn't yet available in FreeBSD 12. I've gone ahead and updated the code to use vp->v_object->flags & OBJ_MIGHTBEDIRTY for the older releases which is what was done before.

include/os/freebsd/spl/sys/vnode.h Outdated Show resolved Hide resolved
include/os/freebsd/spl/sys/vnode.h Outdated Show resolved Hide resolved
When using lseek(2) to report data/holes memory mapped regions of
the file were ignored.  This could result in incorrect results.
To handle this zfs_holey_common() was updated to asynchronously
writeback any dirty mmap(2) regions prior to reporting holes.

Additionally, while not strictly required the dn_struct_rwlock is
now held over the dirty check to prevent the dnode structure from
changing.  This ensures that a clean dnode can't be dirtied before
the data/hole is located.  The range lock is now also taken to
ensure the call cannot race with zfs_write().

Furthermore, the code was refactored to provide a dnode_is_dirty()
helper function which checks the dnode for any dirty records to
determine its dirtiness.

A test case was added which does some basic verification that
data/holes are reported correctly when writing a file with mmap.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#11900
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 5, 2021
@tonyhutter tonyhutter mentioned this pull request Nov 5, 2021
13 tasks
@mmaybee mmaybee merged commit de198f2 into openzfs:master Nov 7, 2021
gyakovlev added a commit to gyakovlev/gentoo that referenced this pull request Nov 7, 2021
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Nov 7, 2021
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 10, 2021
When using lseek(2) to report data/holes memory mapped regions of
the file were ignored.  This could result in incorrect results.
To handle this zfs_holey_common() was updated to asynchronously
writeback any dirty mmap(2) regions prior to reporting holes.

Additionally, while not strictly required, the dn_struct_rwlock is
now held over the dirty check to prevent the dnode structure from
changing.  This ensures that a clean dnode can't be dirtied before
the data/hole is located.  The range lock is now also taken to
ensure the call cannot race with zfs_write().

Furthermore, the code was refactored to provide a dnode_is_dirty()
helper function which checks the dnode for any dirty records to
determine its dirtiness.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Rich Ercolani <rincebrain@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#11900
Closes openzfs#12724
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 10, 2021
When using lseek(2) to report data/holes memory mapped regions of
the file were ignored.  This could result in incorrect results.
To handle this zfs_holey_common() was updated to asynchronously
writeback any dirty mmap(2) regions prior to reporting holes.

Additionally, while not strictly required, the dn_struct_rwlock is
now held over the dirty check to prevent the dnode structure from
changing.  This ensures that a clean dnode can't be dirtied before
the data/hole is located.  The range lock is now also taken to
ensure the call cannot race with zfs_write().

Furthermore, the code was refactored to provide a dnode_is_dirty()
helper function which checks the dnode for any dirty records to
determine its dirtiness.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Rich Ercolani <rincebrain@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#11900
Closes openzfs#12724
pcd1193182 pushed a commit to pcd1193182/zfs that referenced this pull request Nov 12, 2021
When using lseek(2) to report data/holes memory mapped regions of
the file were ignored.  This could result in incorrect results.
To handle this zfs_holey_common() was updated to asynchronously
writeback any dirty mmap(2) regions prior to reporting holes.

Additionally, while not strictly required, the dn_struct_rwlock is
now held over the dirty check to prevent the dnode structure from
changing.  This ensures that a clean dnode can't be dirtied before
the data/hole is located.  The range lock is now also taken to
ensure the call cannot race with zfs_write().

Furthermore, the code was refactored to provide a dnode_is_dirty()
helper function which checks the dnode for any dirty records to
determine its dirtiness.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Rich Ercolani <rincebrain@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#11900
Closes openzfs#12724
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 13, 2021
When using lseek(2) to report data/holes memory mapped regions of
the file were ignored.  This could result in incorrect results.
To handle this zfs_holey_common() was updated to asynchronously
writeback any dirty mmap(2) regions prior to reporting holes.

Additionally, while not strictly required, the dn_struct_rwlock is
now held over the dirty check to prevent the dnode structure from
changing.  This ensures that a clean dnode can't be dirtied before
the data/hole is located.  The range lock is now also taken to
ensure the call cannot race with zfs_write().

Furthermore, the code was refactored to provide a dnode_is_dirty()
helper function which checks the dnode for any dirty records to
determine its dirtiness.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Rich Ercolani <rincebrain@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#11900
Closes openzfs#12724
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 13, 2021
When using lseek(2) to report data/holes memory mapped regions of
the file were ignored.  This could result in incorrect results.
To handle this zfs_holey_common() was updated to asynchronously
writeback any dirty mmap(2) regions prior to reporting holes.

Additionally, while not strictly required, the dn_struct_rwlock is
now held over the dirty check to prevent the dnode structure from
changing.  This ensures that a clean dnode can't be dirtied before
the data/hole is located.  The range lock is now also taken to
ensure the call cannot race with zfs_write().

Furthermore, the code was refactored to provide a dnode_is_dirty()
helper function which checks the dnode for any dirty records to
determine its dirtiness.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Rich Ercolani <rincebrain@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#11900
Closes openzfs#12724
ofaaland pushed a commit to LLNL/zfs that referenced this pull request Mar 9, 2023
When using lseek(2) to report data/holes memory mapped regions of
the file were ignored.  This could result in incorrect results.
To handle this zfs_holey_common() was updated to asynchronously
writeback any dirty mmap(2) regions prior to reporting holes.

Additionally, while not strictly required, the dn_struct_rwlock is
now held over the dirty check to prevent the dnode structure from
changing.  This ensures that a clean dnode can't be dirtied before
the data/hole is located.  The range lock is now also taken to
ensure the call cannot race with zfs_write().

Furthermore, the code was refactored to provide a dnode_is_dirty()
helper function which checks the dnode for any dirty records to
determine its dirtiness.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Rich Ercolani <rincebrain@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#11900
Closes openzfs#12724
gentoo-repo-qa-bot pushed a commit to gentoo-mirror/linux-be that referenced this pull request Jul 2, 2023
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.

7 participants