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

Update pin_user_pages() calls for Direct I/O #17006

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

bwatkinson
Copy link
Contributor

@bwatkinson bwatkinson commented Jan 29, 2025

Originally #16856 updated Linux Direct I/O requests to use the new pin_user_pages API. However, it was an oversight that this PR only handled iov_iter's of type ITER_IOVEC and ITER_UBUF. Other iov_iter types may try and use the pin_user_pages API if it is available. This can lead to panics as the iov_iter is not being iterated over correctly in zfs_uio_pin_user_pages().

Unfortunately, generic iov_iter API's that call pin_user_page_fast() are protected as GPL only. Rather than update zfs_uio_pin_user_pages() to account for all iov_iter types, we can simply just call zfs_uio_get_dio_page_iov_iter() if the iov_iter type is not ITER_IOVEC or ITER_UBUF. zfs_uio_get_dio_page_iov_iter() calls the iov_iter_get_pages() calls that can handle any iov_iter type.

In the future it might be worth using the exposed iov_iter iterator functions that are included in the header iov_iter.h since v6.7. These functions allow for any iov_iter type to be iterated over and advanced while applying a step function during iteration. This could possibly be leveraged in zfs_uio_pin_user_pages().

A new ZFS test case was added to test that a ITER_BVEC is handled correctly using this new code path. This test case was provided though issue #16956.

Signed-off-by: Brian Atkinson batkinson@lanl.gov
Closes #16956

Updated when to use pin_user_pages_unlocked() based on the iov_iter type.

Motivation and Context

00

It resolves a kernel panic that can occur in zfs_uio_pin_user_pages() when trying to iterate over a struct iov_iter t hat is not a ITER_IOVEC or ITER_UBUF.

Fixes issue #16956.

Description

Updated the check in zfs_uio_get_dio_pages_alloc() to only call zfs_uio_pin_user_pages() if the iov_iter type is ITER_IOVEC or ITER_UBUF. These two types are the only iov_iter types taken into account for iterating in zfs_uio_pin_user_pages(). While we could manually code in other iov_iter types to iterate over them, that seems ripe for error in the future if the kernel introduces another struct iov_iter type in the future. Added boolean_t variable called pinned to zfs_uio_dio_t to signal whether pin_user_pages_unlocked() was used for pages for Direct I/O.

Due to this update, I did have to add back the check for iov_iter_get_pages2() in the config checks. This will still need to used in kernels >= v6.0 if we need to call zfs_uio_get_dio_pages_iov_iter().

A ZTS test case dio_loopback_dev was added that uses the test that caused a kernel panic in #16956.

How Has This Been Tested?

Tested by running the direct ZTS tests on:

  1. 4.18.0-240.15.1.el8_3.x86_64 -> pin_user_pages_unlocked() not available, iter_is_ubuf() not available
  2. 4.18.0-553.6.1.el8.x86_64 -> pin_user_pages_unlocked() available, iter_is_ubuf() not available
  3. 6.5.12-100.fc37.x86_64 -> pin_user_pages_unlocked() available, iter_is_ubuf() available

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:

module/os/linux/zfs/zfs_uio.c Outdated Show resolved Hide resolved
module/os/linux/zfs/zfs_uio.c Outdated Show resolved Hide resolved
@amotin amotin requested a review from ixhamza January 29, 2025 03:46
@bwatkinson bwatkinson force-pushed the user_backed_iov_iter branch from 3d49053 to d593704 Compare January 29, 2025 20:26
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.

As much as I understand that code I have no objections, just few cosmetic comments.

module/os/linux/zfs/zfs_uio.c Outdated Show resolved Hide resolved
module/os/linux/zfs/zfs_uio.c Outdated Show resolved Hide resolved
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jan 29, 2025
Originally openzfs#16856 updated Linux Direct I/O requests to use the new
pin_user_pages API. However, it was an oversight that this PR only
handled iov_iter's of type ITER_IOVEC and ITER_UBUF. Other iov_iter
types may try and use the pin_user_pages API if it is available. This
can lead to panics as the iov_iter is not being iterated over correctly
in zfs_uio_pin_user_pages().

Unfortunately, generic iov_iter API's that call pin_user_page_fast() are
protected as GPL only. Rather than update zfs_uio_pin_user_pages() to
account for all iov_iter types, we can simply just call
zfs_uio_get_dio_page_iov_iter() if the iov_iter type is not ITER_IOVEC
or ITER_UBUF. zfs_uio_get_dio_page_iov_iter() calls the
iov_iter_get_pages() calls that can handle any iov_iter type.

In the future it might be worth using the exposed iov_iter iterator
functions that are included in the header iov_iter.h since v6.7. These
functions allow for any iov_iter type to be iterated over and advanced
while applying a step function during iteration. This could possibly be
leveraged in zfs_uio_pin_user_pages().

A new ZFS test case was added to test that a ITER_BVEC is handled
correctly using this new code path. This test case was provided though
issue openzfs#16956.

Signed-off-by: Brian Atkinson <batkinson@lanl.gov>
Closes openzfs#16956
@bwatkinson bwatkinson force-pushed the user_backed_iov_iter branch from d593704 to e46af9b Compare January 30, 2025 00:06
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.

Makes sense. Thanks for running this down!

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 30, 2025
@behlendorf behlendorf merged commit 1e32c57 into openzfs:master Jan 30, 2025
23 of 25 checks passed
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.

Creating a loop device over ZFS file system causes NULL pointer exception when direct=always is set
4 participants