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

[Linux 5.18+] Fix detection of filemap_range_has_page #16034

Merged
merged 1 commit into from
Mar 30, 2024

Conversation

rrevans
Copy link
Contributor

@rrevans rrevans commented Mar 28, 2024

Motivation and Context

While testing #16031, I noted that HAVE_FILEMAP_RANGE_HAS_PAGE is not being defined for Linux 6.5 or 6.6.

Turns out the symbol moved in v5.18 from fs.h to pagemap.h, and the fallback path has been in use for all versions of Linux since then.

For the history of the symbol, run from linux git repo:

$ git grep -e filemap_range_has_page $(git rev-parse --tags=v*) -- include/linux/pagemap.h | git name-rev --annotate-stdin --tags

I also verified this by compiling the test src at all major releases from v5.18 ... v6.8 and v6.9-rc1

$ cat /tmp/x.c
#include <linux/module.h>
#include <linux/fs.h>

int main(void) {
                struct address_space *mapping = NULL;
                loff_t lstart = 0;
                loff_t lend = 0;
                bool ret __attribute__ ((unused));

                ret = filemap_range_has_page(mapping, lstart, lend);
                return 0;
}
$ gcc -Wp,-MMD,arch/x86/realmode/.init.o.d -nostdinc -I./arch/x86/include -I./arch/x86/include/generated  -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -std=gnu11 -m64 -c -o /tmp/x.o /tmp/x.c
... pass for v5.17, fail for all versions after ...

In the fallback path, ZFS calls mappedread and update_pages for every read or write (respectively) after the first mmap call to the file. This is unnecessary for unmapped ranges and has a slight performance cost.

Edit: The cost maybe isn't so slight. The fallback code within mappedread only reads in page-sized increments which is somewhat unfortunate. See #16031 (comment)

Description

In v5.18 filemap_range_has_page moved to pagemap.h

pagemap.h has been around since 3.10 so just include both

How Has This Been Tested?

Local ZTS passes (linux.run, common.run, and sanity.run) on 6.6.8-200.fc39.x86_64 w/ debug build

This is not without risk since the non-fallback path has been disabled for a long while

I don't have a test setup that can build against older kernels.

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:

In v5.18 `filemap_range_has_page` moved to `pagemap.h`

`pagemap.h` has been around since 3.10 so just include both

Signed-off-by: Robert Evans <evansr@google.com>
Copy link
Member

@robn robn left a comment

Choose a reason for hiding this comment

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

Well that stinks. Good find.

@tonyhutter tonyhutter added the Status: Accepted Ready to integrate (reviewed, tested) label Mar 28, 2024
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.

Well that's unfortunate, thanks for the fix.

@behlendorf behlendorf merged commit 39be46f into openzfs:master Mar 30, 2024
23 of 27 checks passed
@ssergiienko
Copy link

Given the noticeable performance improvement and visual safety of the change, wouldn't it be nice to cherry-pick it to v2.2.4?

@rrevans
Copy link
Contributor Author

rrevans commented Apr 3, 2024

Seems reasonable to backport with the caveat that this code path hasn't been exercised in recent kernels.

@rrevans rrevans mentioned this pull request Apr 18, 2024
13 tasks
tonyhutter pushed a commit that referenced this pull request May 2, 2024
In v5.18 `filemap_range_has_page` moved to `pagemap.h`

`pagemap.h` has been around since 3.10 so just include both

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Rob Norris <robn@despairlabs.com>
Signed-off-by: Robert Evans <evansr@google.com>
Closes #16034
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
In v5.18 `filemap_range_has_page` moved to `pagemap.h`

`pagemap.h` has been around since 3.10 so just include both

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Rob Norris <robn@despairlabs.com>
Signed-off-by: Robert Evans <evansr@google.com>
Closes openzfs#16034
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.

5 participants