Remaining Linux 5.16 fixes #12819

ckane · 2021-12-03T04:03:56Z

Motivation and Context

Multiple regressions introduced by API changes introduced by Linux 5.16. This PR will include the following

Update for submit_bio getting void return type
struct iov_iter had its type member renamed to iter_type
bio_set_dev(...) is now a static inline function, and no longet a CPP macro

Description

Conditionally compile the function implementation for zvol_submit_bio() as void or blk_qc_t return type, depending upon what configure finds. Look for the iov_iter_type(struct iov_iter*) accessor function, and use it if it exists to access iov_iter->type or iov_iter->iter_type depending upon the version. Fall back to directly access iov_iter->type if function doesn't exist.

How Has This Been Tested?

Compile tested & running on my desktop with Kernel 5.16 rc3/4 @ commit 12119cfa1052d512a92524e90ebee85029a918f8

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 · 2021-12-03T04:31:37Z

I just have one more fix for the macro->static change for bio_set_dev. I have a pending commit for this, and I am just working through the M4 macros to detect the macro->static inline change, and plan to push to this branch tomorrow (Friday)

behlendorf

Looks good. It looks like we'll need to add one more (in addition to the bio_set_dev() change) to fix this builtin conflict:

lib/zstd/common/zstd_common.o: In function `ZSTD_isError':
zstd_common.c:(.text+0x20): multiple definition of `ZSTD_isError'
fs/zfs/zstd/lib/zstd.o:zstd.c:(.text+0x25d90): first defined here
Makefile:1161: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 1

Adding the conflicting symbol to the wrapper should do the trick.

diff --git a/module/zstd/include/zstd_compat_wrapper.h b/module/zstd/include/zstd_compat_wrapper.h
index 71adc7804..339713590 100644
--- a/module/zstd/include/zstd_compat_wrapper.h
+++ b/module/zstd/include/zstd_compat_wrapper.h
@@ -406,6 +406,7 @@
        zfs_ZSTD_insertAndFindFirstIndex_internal
 #define        ZSTD_insertBlock zfs_ZSTD_insertBlock
 #define        ZSTD_invalidateRepCodes zfs_ZSTD_invalidateRepCodes
+#define        ZSTD_isError zfs_ZSTD_isError
 #define        ZSTD_isFrame zfs_ZSTD_isFrame
 #define        ZSTD_ldm_adjustParameters zfs_ZSTD_ldm_adjustParameters
 #define        ZSTD_ldm_blockCompress zfs_ZSTD_ldm_blockCompress

config/kernel-bio.m4

ckane · 2021-12-04T03:33:01Z

@behlendorf for your suggested module/zstd/include/zstd_compat_wrapper.h change to ZSTD_isError, I am getting the following error:

ERROR: modpost: "zfs_ZSTD_isError" [/home/......../src/zfs-test/module/zstd/zzstd.ko] undefined!

Looking at module/zstd/Makefile.in it seems like the zstd_compat_wrapper.h is included on the command line when compiling both zfs_zstd.o and zstd.o...so I don't know why this isn't compiling. FWIW on the version of the Linux kernel I've been testing, ZSTD_isError doesn't seem to have been introduced yet (but I haven't pulled down Linus' latest merges for a few weeks).

ckane · 2021-12-04T03:39:10Z

@behlendorf as I've added some more commits to this PR you might want to re-review those - in particular my approach to the bio_set_dev() change, in case you'd prefer a different approach.

ckane · 2021-12-04T15:58:05Z

Looks like the #include <linux/blk-cgroup.h> causes a failure on CentOS 7 - which I guess uses a super old kernel. I'll work on a way to amend that last commit to only include the header if it is found.

Other than that, I am running this "live" right now on my Arch system using the latest kernel from Linus' branch pulled down last night, and it is stable.

ckane · 2021-12-05T00:04:44Z

Ok, I think that the following lines in/around 7347 in modules/zstd/lib/zstd.c are causing the missing symbol issue (the macro gets #undef'd and then the conflicting function is declared again:

/*-****************************************
*  ZSTD Error Management
******************************************/
#undef ZSTD_isError   /* defined within zstd_internal.h */
/*! ZSTD_isError() :
 *  tells if a return value is an error code
 *  symbol is required for external callers */
unsigned ZSTD_isError(size_t code) { return ERR_isError(code); }

/*! ZSTD_getErrorName() :
 *  provides error code string from function result (useful for debugging) */
const char* ZSTD_getErrorName(size_t code) { return ERR_getErrorName(code); }

/*! ZSTD_getError() :
 *  convert a `size_t` function result into a proper ZSTD_errorCode enum */
ZSTD_ErrorCode ZSTD_getErrorCode(size_t code) { return ERR_getErrorCode(code); }

/*! ZSTD_getErrorString() :
 *  provides error code string from enum */
const char* ZSTD_getErrorString(ZSTD_ErrorCode code) { return ERR_getErrorString(code); }

As well as the following at line 6341 in the same file:

/* ---- static assert (debug) --- */
#define ZSTD_STATIC_ASSERT(c) DEBUG_STATIC_ASSERT(c)
#define ZSTD_isError ERR_isError   /* for inlining */
#define FSE_isError  ERR_isError
#define HUF_isError  ERR_isError

ckane · 2021-12-05T00:13:02Z

If I perform the following changes, it seems I can get it to build - but I don't know if these are desirable changes or not (looking for your thoughts):

index acdd4d9da..987e55bbb 100644
--- a/module/zstd/lib/zstd.c
+++ b/module/zstd/lib/zstd.c
@@ -6340,7 +6340,6 @@ extern "C" {

 /* ---- static assert (debug) --- */
 #define ZSTD_STATIC_ASSERT(c) DEBUG_STATIC_ASSERT(c)
-#define ZSTD_isError ERR_isError   /* for inlining */
 #define FSE_isError  ERR_isError
 #define HUF_isError  ERR_isError

@@ -7347,7 +7346,6 @@ const char* ZSTD_versionString(void) { return ZSTD_VERSION_STRING; }
 /*-****************************************
 *  ZSTD Error Management
 ******************************************/
-#undef ZSTD_isError   /* defined within zstd_internal.h */
 /*! ZSTD_isError() :
  *  tells if a return value is an error code
  *  symbol is required for external callers */

ckane · 2021-12-05T20:32:05Z

I've gone ahead and pushed the aforementioned modification as my fix proposal to module/zstd/lib/zstd.c to this branch as well, so it's available for anyone to test, and also for the automated tests. Please test and provide any feedback on it.

That said, module/zstd/README.md suggests trying to find an alternative way to fix this problem that doesn't involve editing the ZSTD library code directly. Unfortunately, I'm at a loss for how to resolve this conflict outside of modifying the zstd.c code as mentioned above, because both the #define and the #undef above seem to conflict with what we're trying to accomplish with include/zstd_compat_wrapper.h.

behlendorf

@ckane the approach you've taken here looks good to me. This does seem like the best way to handle these new compatibility issues.

Regarding the zstd changes while it would have been ideal to confine the changes to the wrapper, I think we can live with these minimal changes to the original source.

behlendorf · 2021-12-06T19:53:58Z

@ckane would you mind doing one last rebase of this so we can get a clean test run.

The return type for the submit_bio member of struct block_device_operations was changed to no longer return a value. Signed-off-by: Coleman Kane <[email protected]>

The iov_iter->type member was renamed iov_iter->iter_type. However, while looking into this, realized that in 2018 a iov_iter_type(*iov) accessor function was introduced. So if that is present, use it, otherwise fall back to trying the existing behavior of directly accessing type from iov_iter. Signed-off-by: Coleman Kane <[email protected]>

This change adds a confiugre check to determine if bio_set_dev is a helper macro or not. If not, then the attempt to override its internal call to bio_associate_blkg(), with a macro definition to our own version, is no longer possible, as the compiler won't use it when compiling the new inline function replacement implemented in the header. This change also creates a new vdev_bio_set_dev() function that performs the same work, and also performs the work implemented in vdev_bio_associate_blkg(), as it is the only thing calling that function in our code. Our custom vdev_bio_associate_blkg() is now only compiled if the bio_set_dev() is a macro in the Linux headers. Signed-off-by: Coleman Kane <[email protected]>

The definition of struct blkcg_gq was moved into blk-cgroup.h, which is a header that's been in Linux since 2015. This is used by vdev_blkg_tryget() in module/os/linux/zfs/vdev_disk.c. Since the kernel for CentOS 7 and similar-generation releases doesn't have this header, its inclusion is guarded by a configure test. Signed-off-by: Coleman Kane <[email protected]>

Newer zstd code introduced in the main kernel tree now creates a symbol collision with ZSTD_isError in our ZSTD code. This change relabels our implementation with a ZFS-specific symbol name, and undoes some macro-based micro-optimizations that conflict with the attempt to rename our internal-use version. Signed-off-by: Coleman Kane <[email protected]>

ckane · 2021-12-06T23:16:48Z

@ckane would you mind doing one last rebase of this so we can get a clean test run.

Thanks @behlendorf! Just rebased and re-pushed

The iov_iter->type member was renamed iov_iter->iter_type. However, while looking into this, realized that in 2018 a iov_iter_type(*iov) accessor function was introduced. So if that is present, use it, otherwise fall back to trying the existing behavior of directly accessing type from iov_iter. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes #12819

This change adds a confiugre check to determine if bio_set_dev is a helper macro or not. If not, then the attempt to override its internal call to bio_associate_blkg(), with a macro definition to our own version, is no longer possible, as the compiler won't use it when compiling the new inline function replacement implemented in the header. This change also creates a new vdev_bio_set_dev() function that performs the same work, and also performs the work implemented in vdev_bio_associate_blkg(), as it is the only thing calling that function in our code. Our custom vdev_bio_associate_blkg() is now only compiled if the bio_set_dev() is a macro in the Linux headers. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes #12819

The definition of struct blkcg_gq was moved into blk-cgroup.h, which is a header that's been in Linux since 2015. This is used by vdev_blkg_tryget() in module/os/linux/zfs/vdev_disk.c. Since the kernel for CentOS 7 and similar-generation releases doesn't have this header, its inclusion is guarded by a configure test. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes #12819

Newer zstd code introduced in the main kernel tree now creates a symbol collision with ZSTD_isError in our ZSTD code. This change relabels our implementation with a ZFS-specific symbol name, and undoes some macro-based micro-optimizations that conflict with the attempt to rename our internal-use version. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes #12819

The return type for the submit_bio member of struct block_device_operations was changed to no longer return a value. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes openzfs#12819

The iov_iter->type member was renamed iov_iter->iter_type. However, while looking into this, realized that in 2018 a iov_iter_type(*iov) accessor function was introduced. So if that is present, use it, otherwise fall back to trying the existing behavior of directly accessing type from iov_iter. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes openzfs#12819

This change adds a confiugre check to determine if bio_set_dev is a helper macro or not. If not, then the attempt to override its internal call to bio_associate_blkg(), with a macro definition to our own version, is no longer possible, as the compiler won't use it when compiling the new inline function replacement implemented in the header. This change also creates a new vdev_bio_set_dev() function that performs the same work, and also performs the work implemented in vdev_bio_associate_blkg(), as it is the only thing calling that function in our code. Our custom vdev_bio_associate_blkg() is now only compiled if the bio_set_dev() is a macro in the Linux headers. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes openzfs#12819

The definition of struct blkcg_gq was moved into blk-cgroup.h, which is a header that's been in Linux since 2015. This is used by vdev_blkg_tryget() in module/os/linux/zfs/vdev_disk.c. Since the kernel for CentOS 7 and similar-generation releases doesn't have this header, its inclusion is guarded by a configure test. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes openzfs#12819

Newer zstd code introduced in the main kernel tree now creates a symbol collision with ZSTD_isError in our ZSTD code. This change relabels our implementation with a ZFS-specific symbol name, and undoes some macro-based micro-optimizations that conflict with the attempt to rename our internal-use version. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes openzfs#12819