Linux 6.6 compat: use inode_get/set_ctime*(...) functions, pass request_mask to generic_fillattr(), and use sync_blockdev() #15263

ckane · 2023-09-12T03:38:23Z

Motivation and Context

In Linux commit 13bc24457850583a2e7203ded05b7209ab4bc5ef, direct access to the i_ctime member of struct inode was removed. This results in the build failures documented in Issue #15257.

Description

inode_get/set_ctime*() calls

The fix is to use accessor methods that exclusively handle passing the timestamp around by value. This change adds new tests for each of these functions and introduces zpl_* equivalents in include/os/linux/zfs/sys/zpl.h. In where the inode_get/set_ctime*() functions exist, these zpl_* calls will be mapped to the new functions. On older kernels, these macros just wrap direct-access calls. The code that operated on an address of ip->i_ctime to call ZFS_TIME_DECODE() now will take a local copy using zpl_inode_get_ctime(), and then pass the address of the local copy when performing the ZFS_TIME_DECODE() call, in all cases, rather than directly accessing the member.

Not 100% certain if this is a problem too, but ZFS_TIME_DECODE performs a complex sequence of operations while operating on an un-owned pointer (in the old code). This potentially exposes a race-condition risk, if any other thread of execution is allowed to modify the data pointed at by the struct inode * and its i_ctime member.

request_mask in generic_fillattr(...)

In linux commit 0d72b92883c651a11059d93335f33d65c6eb653b added a new-ish argument to the generic_fillattr call, and updated everywhere else that function is called. It now accepts, as its second argument (pushing the rest to the right), the u32 request_mask that we already accept into the latest revision of the zpl_getattr_impl function. This change makes sure to chase that upstream kernel change, and ensure that the request_mask is passed into the necessary places in order to get it to generic_fillattr().

sync_blockdev replaces fsync_bdev

In Linux commit 560e20e4bf6484a0c12f9f3c7a1aa55056948e1e the fsync_bdev function is removed, and the preferred approach is to call sync_blockdev(), which accepts the same argument as input (a blkdev). The appropriate configure tests and conditional-compilation parser rules have been added.

How Has This Been Tested?

I have compile tested against both 6.5.3 and 6.6rc1, and the compile finished successfully on both.

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.

ckane · 2023-09-12T10:52:20Z

Looks like there's a ztest regression in ctime/ctime_001_pos and nopwrite/nopwrite_mtime so I'll take a look into those

ckane · 2023-09-13T18:49:27Z

Looking closer I see what the discrepancy is here:

 #define ZFS_TIME_DECODE(tp, stmp)               \
 do {                                            \
         (tp)->tv_sec = (time64_t)(stmp)[0];     \
         (tp)->tv_nsec = (long)(stmp)[1];        \
 } while (0)

This needs to be treated as an assignment operation, not a retrieval operation, against tp

ckane · 2023-09-15T03:13:07Z

Force-pushing this w/out the zpl_inode_set_ctime_current commit to see if the original commit (with the corrected use of ZFS_TIME_DECODE passes all the buildbot tests.

ckane · 2023-09-15T05:19:27Z

@behlendorf - this should now address the outstanding breakages introduced with 6.6rc1. This PR contains 3 distinct patches (one per commit) for 3 issues that were discovered. Since the 2nd and 3rd regressions were relatively small, I rolled them into this PR instead of making 2 more independent PRs. Barring any issue revealed by CI testing, these should be read to go.

PS: I have noticed that buildbot/FreeBSD can't get the sources, and checkstyle seems to be failing because of some breakage that is causing all of the symbols to be dumped into the output from the compiled binary objects, or something.

satmandu · 2023-09-15T17:50:03Z

This was what I needed to get 6.6-rc1 built and working (on top of the 6.5 fixes I used to patch 2.2.)
Fixes #15257

ckane · 2023-09-16T04:15:26Z

I am also running this with 6.6rc1 on my main desktop with ZFS root & KDE, and it is working well

ckane · 2023-09-19T01:38:18Z

Working on 6.6rc2

In Linux commit 13bc24457850583a2e7203ded05b7209ab4bc5ef, direct access to the i_ctime member of struct inode was removed. The new approach is to use accessor methods that exclusively handle passing the timestamp around by value. This change adds new tests for each of these functions and introduces zpl_* equivalents in include/os/linux/zfs/sys/zpl.h. In where the inode_get/set_ctime*() functions exist, these zpl_* calls will be mapped to the new functions. On older kernels, these macros just wrap direct-access calls. The code that operated on an address of ip->i_ctime to call ZFS_TIME_DECODE() now will take a local copy using zpl_inode_get_ctime(), and then pass the address of the local copy when performing the ZFS_TIME_DECODE() call, in all cases, rather than directly accessing the member. Signed-off-by: Coleman Kane <[email protected]>

In commit 0d72b92883c651a11059d93335f33d65c6eb653b, a new u32 argument for the request_mask was added to generic_fillattr. This is the same request_mask for statx that's present in the most recent API implemented by zpl_getattr_impl. This commit conditionally adds it to the zpl_generic_fillattr(...) macro, as well as the zfs_getattr_fast(...) implementation, when configure determines it's present in the kernel's generic_fillattr(...). Signed-off-by: Coleman Kane <[email protected]>

In Linux commit 560e20e4bf6484a0c12f9f3c7a1aa55056948e1e, the fsync_bdev() function was removed in favor of sync_blockdev() to do (roughly) the same thing, given the same input. This change conditionally attempts to call sync_blockdev() if fsync_bdev() isn't discovered during configure. Signed-off-by: Coleman Kane <[email protected]>

behlendorf

Looks good.

…t arg2 In commit 0d72b92883c651a11059d93335f33d65c6eb653b, a new u32 argument for the request_mask was added to generic_fillattr. This is the same request_mask for statx that's present in the most recent API implemented by zpl_getattr_impl. This commit conditionally adds it to the zpl_generic_fillattr(...) macro, as well as the zfs_getattr_fast(...) implementation, when configure determines it's present in the kernel's generic_fillattr(...). Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes #15263

…kdev() In Linux commit 560e20e4bf6484a0c12f9f3c7a1aa55056948e1e, the fsync_bdev() function was removed in favor of sync_blockdev() to do (roughly) the same thing, given the same input. This change conditionally attempts to call sync_blockdev() if fsync_bdev() isn't discovered during configure. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes #15263

In Linux commit 13bc24457850583a2e7203ded05b7209ab4bc5ef, direct access to the i_ctime member of struct inode was removed. The new approach is to use accessor methods that exclusively handle passing the timestamp around by value. This change adds new tests for each of these functions and introduces zpl_* equivalents in include/os/linux/zfs/sys/zpl.h. In where the inode_get/set_ctime*() functions exist, these zpl_* calls will be mapped to the new functions. On older kernels, these macros just wrap direct-access calls. The code that operated on an address of ip->i_ctime to call ZFS_TIME_DECODE() now will take a local copy using zpl_inode_get_ctime(), and then pass the address of the local copy when performing the ZFS_TIME_DECODE() call, in all cases, rather than directly accessing the member. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes openzfs#15263 Closes openzfs#15257

…t arg2 In commit 0d72b92883c651a11059d93335f33d65c6eb653b, a new u32 argument for the request_mask was added to generic_fillattr. This is the same request_mask for statx that's present in the most recent API implemented by zpl_getattr_impl. This commit conditionally adds it to the zpl_generic_fillattr(...) macro, as well as the zfs_getattr_fast(...) implementation, when configure determines it's present in the kernel's generic_fillattr(...). Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes openzfs#15263

…kdev() In Linux commit 560e20e4bf6484a0c12f9f3c7a1aa55056948e1e, the fsync_bdev() function was removed in favor of sync_blockdev() to do (roughly) the same thing, given the same input. This change conditionally attempts to call sync_blockdev() if fsync_bdev() isn't discovered during configure. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes openzfs#15263