Use pin_user_pages API for Direct I/O requests #16856

bwatkinson · 2024-12-11T23:55:45Z

Updated the Direct I/O code to use pin_user_pages* API that was introduced in kernel v5.8 and is the preferred way of doing Direct I/O in the kernel. The pin_user_pages* API use the FOLL_PIN flag.

I also have an additional commit that removes dead Direct I/O code. This is just a little bit of cleanup since the ZFS minimal kernel version is now at v4.18.

Motivation and Context

This gets ZFS uptodate with the latest kernel API's for doing Direct I/O. This also allows for FOLL_PCI_P2PDMA to be passed in the future to the pin_user_pages_unlocked() call. This allows for the pages to be used for P2P operations. The FOLL_PCI_P2PDMA flag was added in v6.2.

Description

Added new configure check for pin_user_pages_unlocked() so it can be used for all Direct I/O request. I also added a check for iter_is_ubuf() as the ITER_UBUF iov_iter type was added in v6.0. There is a little different handling that has to happen in the case of ITER_UBUF and ITER_IOVEC when calling pin_user_pages_unlocked(). The ITER_UBUF contains only a single struct iovec. With this updates, the Linux UIO code now uses the current pin_user_pages* API.

There are also different pin_user_pages* API calls that are more optimized, but they are currently GPL only. Also, in kernel v6.3 there is the iov_iter_extract_pages() which uses the pin_user_pages* API, but it also is GPL only. Unfortunately, this means the iov_iter's must be manually iterated though in order to call pin_user_pages_unlocked().

This PR should also get rid of the issue reported in #16689 and #16642. By using the pin_user_pages* API's, only unpin_user_pages() needs to be called. Also, starting in v6.5, the pin_user_pages* API's do not place a pin on the zero page at all. Both of those issues should try this PR out before we merge it.

How Has This Been Tested?

Tested by running the direct ZTS tests on:

4.18.0-240.15.1.el8_3.x86_64 -> pin_user_pages_unlocked() not available, iter_is_ubuf() not available
4.18.0-553.6.1.el8.x86_64 -> pin_user_pages_unlocked() available, iter_is_ubuf() not available
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:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

There were checks still in place to verify we could completely use iov_iter's on the Linux side. All interfaces are available as of kernel 4.18, so there is no reason to check whether we should use that interface at this point. This PR completely removes the UIO_USERSPACE type. It also removes the check for the direct_IO interface checks. Signed-off-by: Brian Atkinson <[email protected]>

config/kernel-vfs-iov_iter.m4

module/os/linux/zfs/zfs_uio.c

amotin · 2024-12-13T20:14:04Z

module/os/linux/zfs/zfs_uio.c

+		nr_pages = DIV_ROUND_UP(len, PAGE_SIZE);
+		addr = (unsigned long)uio->uio_iter->ubuf + skip;
+		res = pin_user_pages_unlocked(addr, nr_pages,
+		    &uio->uio_dio.pages[uio->uio_dio.npages], gup_flags);


I wonder if this code is supposed to handle page-misaligned buffers? If yes, then I guess nr_pages might need correction for misaligned address, similar to what you seem to add below in zfs_uio_get_dio_pages_iov_iter(). But then the len != (res * PAGE_SIZE) condition below will never be true.

So, I think you are referring to how I was using start to the npage calculation in zfs_uio_get_duio_upages_iov_iter(). So this was a some clean up on my part and my misunderstanding of last parameter sent to iov_iter_get_pages(). So there can be not page misalignment for Direct I/O as we make sure to do checks with zfs_uio_page_aligned() beforehand. I changed this to have an ASSERT() instead. There is nothing saying in the future we couldn't possibly change the semantics for ZFS to allow for sub-page alignment with Direct I/O, but the ASSERT added makes sure that can be caught with changes. It is not required though to do the addition of start at this point for calculating the npages.

I was not actually arguing against accounting start in zfs_uio_get_duio_upages_iov_iter(), it seemed to make the code slightly more universal. BTW, if you leave the assertion instead of what was there originally, I think it could be just an ASSERT0(start). I was thinking about the lack of it in zfs_uio_pin_user_pages(). But whatever.

As of kernel v5.8, pin_user_pages* interfaced were introduced. These interfaces use the FOLL_PIN flag. This is preferred interface now for Direct I/O requests in the kernel. The reasoning for using this new interface for Direct I/O requests is explained in the kernel documenetation: Documentation/core-api/pin_user_pages.rst If pin_user_pages_unlocked is available, the all Direct I/O requests will use this new API to stay uptodate with the kernel API requirements. Signed-off-by: Brian Atkinson <[email protected]>

amotin

Seems OK as much as can speak about the area.

amotin · 2024-12-15T00:05:01Z

module/os/linux/zfs/zfs_uio.c

+		nr_pages = DIV_ROUND_UP(len, PAGE_SIZE);
+		addr = (unsigned long)uio->uio_iter->ubuf + skip;
+		res = pin_user_pages_unlocked(addr, nr_pages,
+		    &uio->uio_dio.pages[uio->uio_dio.npages], gup_flags);


I was not actually arguing against accounting start in zfs_uio_get_duio_upages_iov_iter(), it seemed to make the code slightly more universal. BTW, if you leave the assertion instead of what was there originally, I think it could be just an ASSERT0(start). I was thinking about the lack of it in zfs_uio_pin_user_pages(). But whatever.

module/os/linux/zfs/zfs_uio.c

As of kernel v5.8, pin_user_pages* interfaced were introduced. These interfaces use the FOLL_PIN flag. This is preferred interface now for Direct I/O requests in the kernel. The reasoning for using this new interface for Direct I/O requests is explained in the kernel documenetation: Documentation/core-api/pin_user_pages.rst If pin_user_pages_unlocked is available, the all Direct I/O requests will use this new API to stay uptodate with the kernel API requirements. Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Brian Atkinson <[email protected]> Closes #16856

There were checks still in place to verify we could completely use iov_iter's on the Linux side. All interfaces are available as of kernel 4.18, so there is no reason to check whether we should use that interface at this point. This PR completely removes the UIO_USERSPACE type. It also removes the check for the direct_IO interface checks. Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Brian Atkinson <[email protected]> Closes openzfs#16856

As of kernel v5.8, pin_user_pages* interfaced were introduced. These interfaces use the FOLL_PIN flag. This is preferred interface now for Direct I/O requests in the kernel. The reasoning for using this new interface for Direct I/O requests is explained in the kernel documenetation: Documentation/core-api/pin_user_pages.rst If pin_user_pages_unlocked is available, the all Direct I/O requests will use this new API to stay uptodate with the kernel API requirements. Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Brian Atkinson <[email protected]> Closes openzfs#16856

There were checks still in place to verify we could completely use iov_iter's on the Linux side. All interfaces are available as of kernel 4.18, so there is no reason to check whether we should use that interface at this point. This PR completely removes the UIO_USERSPACE type. It also removes the check for the direct_IO interface checks. Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Brian Atkinson <[email protected]> Closes openzfs#16856

As of kernel v5.8, pin_user_pages* interfaced were introduced. These interfaces use the FOLL_PIN flag. This is preferred interface now for Direct I/O requests in the kernel. The reasoning for using this new interface for Direct I/O requests is explained in the kernel documenetation: Documentation/core-api/pin_user_pages.rst If pin_user_pages_unlocked is available, the all Direct I/O requests will use this new API to stay uptodate with the kernel API requirements. Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Brian Atkinson <[email protected]> Closes openzfs#16856

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 <[email protected]> Closes openzfs#16956

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. Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Ameer Hamza <[email protected]> Signed-off-by: Brian Atkinson <[email protected]> Closes #16956 Closes #17006

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. Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Ameer Hamza <[email protected]> Signed-off-by: Brian Atkinson <[email protected]> Closes openzfs#16956 Closes openzfs#17006

bwatkinson requested review from robn, behlendorf and amotin December 11, 2024 23:55

bwatkinson force-pushed the iov_iter_dio_updates branch from e477778 to 4604935 Compare December 13, 2024 18:13

behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 13, 2024

behlendorf approved these changes Dec 13, 2024

View reviewed changes

config/kernel-vfs-iov_iter.m4 Outdated Show resolved Hide resolved

amotin reviewed Dec 13, 2024

View reviewed changes

bwatkinson force-pushed the iov_iter_dio_updates branch from 4604935 to 791c02d Compare December 14, 2024 02:54

amotin approved these changes Dec 15, 2024

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 16, 2024

behlendorf closed this in c6442bd Dec 16, 2024

bwatkinson mentioned this pull request Jan 29, 2025

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

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pin_user_pages API for Direct I/O requests #16856

Use pin_user_pages API for Direct I/O requests #16856

bwatkinson commented Dec 11, 2024 •

edited

Loading

amotin Dec 13, 2024

bwatkinson Dec 14, 2024 •

edited

Loading

amotin Dec 15, 2024

amotin left a comment

amotin Dec 15, 2024

Use pin_user_pages API for Direct I/O requests #16856

Use pin_user_pages API for Direct I/O requests #16856

Conversation

bwatkinson commented Dec 11, 2024 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

amotin Dec 13, 2024

Choose a reason for hiding this comment

bwatkinson Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

amotin Dec 15, 2024

Choose a reason for hiding this comment

amotin left a comment

Choose a reason for hiding this comment

amotin Dec 15, 2024

Choose a reason for hiding this comment

bwatkinson commented Dec 11, 2024 •

edited

Loading

bwatkinson Dec 14, 2024 •

edited

Loading