Replace 'long' with os defined ZFS_MODULE_LONG to support LLP64 used by windows #13984

andrewc12 · 2022-10-02T11:56:50Z

Unortunately, Windows defines 'long' as 32-bit even on x64 compiles. We create two new macros ZFS_MODULE_LONG and ZFS_MODULE_ULONG. These two will be 'long' on Unix, and let the toolchain handle the size of it.

On Windows the two macros are defined as 'int64_t'/'uint64_t'.

Signed-off-by: Andrew Innes andrew.c12@gmail.com
Co-Authored-By: Jorgen Lundman lundman@lundman.net

Motivation and Context

Description

How Has This Been Tested?

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.

ryao

I am fine with the code changes, but there are two things that look off to me.

First, why are there two commits? Usually, something like this would be squashed into one commit unless the changes are useful to keep separate, but I do not see a purpose to that here. I would like to see this squashed into one commit unless having two commits is somehow beneficial to people looking at the commit history. However, I just do not see any utility to that here.

Second, the commit message needs revision. There have been times when I use git log to see what has been done previously and when I do that, descriptive commit messages are important. The first line of the commit message is even more important, since it should attempt to summarize the commit. The first line of the commit message would make no sense to me when looking back at the commit history. I am not going to prescribe a specific message unless I am asked for a suggestion, but I ask that you revise it to be more descriptive. Prefixing it with upstream is nonsensical at upstream and the rest of the line could be a little better.

ryao · 2022-10-02T17:23:11Z

After thinking about this some more, it occurs to me that this is the first of various changes needed to support 64-bit Windows' LLP64 memory model, since at this time, the codebase only supports the ILP32 and LP64 memory models.

Despite being for C++, the following reference does a good job of explaining the differences between the three memory models:

https://en.cppreference.com/w/cpp/language/types

A more generic documentation source would be:

https://sourceforge.net/p/predef/wiki/DataModels/

A quick summary is that while both Windows and the modern UNIX world agree on ILP32 for 32-bit, they disagree on the memory model for 64-bit, but the two models are the same with one difference. That difference is that 64-bit Windows' LLP64 memory model defines long/unsigned long as 32-bit while the 64-bit UNIX world's LP64 memory model defines long/unsigned long as 64-bit.

That said, it is not hard to find what needs to be changed to support LLP64 correctly.

First, we can grep the source code for LP64 and ILP32. Anything that changes behavior based on that needs review to see if it is LLP64 compatible. For example, lib/libspl/include/sys/isa_defs.h will need to change. Some research will probably need to be done to see what macros the relevant compilers emit when building C against the LLP64 memory model since it is absent from the documentation that I linked.

Second, we can grep the source code for "long" (and filter "long long" and "longlong" from the results). Basically, all uses of it need to be reviewed and any that create a problem when 32-bit on a 64-bit machine will need to be changed. In the case of kernel module parameters, FreeBSD is very insistent on using the long type for module parameters that could potentially be 64-bit, so what is done here is a good solution. For everything else, we probably should consider what the bit width should be and switch it either to int32_t/uint32_t/int_t/uint_t or int64_t/uint64_t/longlong_t/ulonglong_t.

andrewc12 · 2022-10-02T22:15:51Z

First, why are there two commits? Usually, something like this would be squashed into one commit unless the changes are useful to keep separate, but I do not see a purpose to that here. I would like to see this squashed into one commit unless having two commits is somehow beneficial to people looking at the commit history. However, I just do not see any utility to that here.

Yeah, it was originally one commit but I noticed I moved some variables above another block and quickly fixed it in the web editor.
I will squash.

lundman · 2022-10-02T23:52:30Z

Hiya!

This isn't so much about ILP32 and LP64, as it is about tunables and that ZFS uses the Linux kernel API MODULE_PARAM() which uses int and long (among others) in the sources of ZFS, to define the types of the tunables.

On Linux, the size of long changes between 32-bit and 64-bit, mostly due to legacy reasons, and when I last spoke to Brian, he preferred to keep support of 32-bit for Linux.

On Windows, long is always 32-bit. To take one specific tunable as an example, it would be nice if zfs_arc_max could be set to more than 4GB on Windows.

In ZFS, we always specify the size wanted with int32_t and int64_t, so that is not an issue, it is only the use of long with Kernel module params. Which is presumably a legacy thing. Even if I were to do 32-bit Windows build, there is absolutely no reason to do tunables 32-bit on Windows. They would still be 64-bit tunables, there is no legacy for me to support here.

So, how do we would around this? ZFS shouldn't be using a Linux API really, and a good start away from that is ZFS_MODULE_PARAM instead of MODULE_PARAM, but that doesn't handle the copious amounts of long tunableA; scattered around the source files. It is amusing that we replaced one specific (illumos) /etc/system tunable API for another specific (Linux) tunable API - presumably because it was a poor-fit for the other platform.

Nor can I change the size of long on Windows, as all the Windows headers love to use long in structures used to talk to the kernel.

The cleanest approach seemed to be to make the type for MODULE_PARAMS be a Macro, which stays at long for Linux, and int64_t for Windows. Technically, ALL types should probably be defined this way (int, char *) but the shock of that might be too much.

Any better ideas?

ryao · 2022-10-02T23:59:36Z

I am fine with this approach. I just feel that it might not go far enough due to the codebase not being written to expect 32-bit long on 64-bit systems.

andrewc12 · 2022-10-03T00:44:47Z

At the moment we have commits that change some things that touch these from long to int64_t for windows.
I'll look at changing them to zfs_module_long aswell.

ryao · 2022-10-03T00:50:24Z

At the moment we have commits that change some things that touch these from long to int64_t for windows.
I'll look at changing them to zfs_module_long aswell.

Int64_t is fine with me, although I suspect that @amotin would feel differently.

ryao · 2022-10-03T01:11:02Z

Another option is to switch them to long_t and then use a preprocessor definition to redefine them to int64_t on Windows. Using ulong_t and having the preprocessor change it to uint64_t should also work for the unsigned versions. That might be cleaner and would hopefully make everyone happy.

lundman · 2022-10-03T05:13:31Z

Another option is to switch them to long_t and then use a preprocessor definition
I mean, that is what this does, we are just arguing over the name of the macro. Honestly,I have no attachment to the name, pick anything as long as it's unique.
ulong_t seems risky, you have to be sure none of the platforms define that anywhere, hence the ZFS_ namespace.
Not looking very hard at all MacOSX10.15.sdk/usr/include/sys/dtrace.h:typedef unsigned long ulong_t;

Unortunately, Windows defines 'long' as 32-bit even on x64 compiles. We create two new macros ZFS_MODULE_LONG and ZFS_MODULE_ULONG. These two will be 'long' on Unix, and let the toolchain handle the size of it. On Windows the two macros are defined as 'int64_t'/'uint64_t'. Signed-off-by: Andrew Innes <andrew.c12@gmail.com> Co-Authored-By: Jorgen Lundman <lundman@lundman.net>

andrewc12 · 2022-10-03T07:25:37Z

In general I'd prefer something descriptive, not something that could be mistaken for something else like ulong_t

ryao · 2022-10-03T14:14:36Z

I think we are going on a tangent with our discussion. I already approved this patch because I like what was done in it. I am going to reserve further comments until there is another patch so this does not become a long protracted discussion about something on which it should be easy to come to a consensus when the next PR is opened.

amotin · 2022-10-03T16:16:47Z

At the moment we have commits that change some things that touch these from long to int64_t for windows.
I'll look at changing them to zfs_module_long aswell.

Int64_t is fine with me, although I suspect that @amotin would feel differently.

Comparing to this patch as alternative I'd actually prefer uint64_t in cases where 32bits is known not enough. I don't like this patch, it looks dirty to me. Excessive macros and random re-definitions of left into right are evil.

ryao · 2022-10-03T16:49:51Z

At the moment we have commits that change some things that touch these from long to int64_t for windows.
I'll look at changing them to zfs_module_long aswell.

Int64_t is fine with me, although I suspect that @amotin would feel differently.

Comparing to this patch as alternative I'd actually prefer uint64_t in cases where 32bits is known not enough. I don't like this patch, it looks dirty to me. Excessive macros and random re-definitions of left into right are evil.

It sounds like we can agree on upgrading long in the code to int64_t where it needs to be bigger. I imagine that long could be downgraded to int where it does not need to be bigger than 32-bit. That way we could retire long with the exception of system interfaces that require it and we would not have any ambiguity of whether something is a long because it is okay to make it 32-bit on LLP64 or because someone did not consider LLP64.

However, module parameters are a special case. If I recall, we cannot unconditionally use uint64_t/int64_t for module parameters because it makes no sense on 32-bit systems. If we tried, we would not be able to export them to the module parameter interface on 32-bit systems without having to handle endianness from shrinking the width since Linux and FreeBSD do not support 64-bit module parameters on 32-bit systems. There is also the issue of the signed bit being in the wrong place when trying to convert a pointer to a 64-bit signed value to pointer to a 32-bit signed value for the kernel to read and write to the value. Fixing that would require that we do special get and set functions. I do not see a cleaner way of doing this than what is done in this patch.

amotin · 2022-10-03T17:07:23Z

If we tried, we would not be able to export them to the module parameter interface on 32-bit systems without having to handle endianness from shrinking the width since Linux and FreeBSD do not support 64-bit module parameters on 32-bit systems.

FreeBSD has native S64/U64 support for sysctls/tunables, so there should be no problems on any arch.

ryao · 2022-10-03T19:04:55Z

After doing an investigation, I learned three things:

The module parameters such as zfs_arc_max were changed from uint64_t to unsigned long for Linux compatibility in the first place.
Linux's kernel module parameter API supports kernel modules defining their own types.
Linux v3.17 implemented ullong to allow 32-bit kernels to use 64-bit module parameters in b4210b810e5040f10a30ba56de6c3faab5c49345.

Undoing the Linux specific change would make everything nicer. Then we have two choices for what to do on Linux:

Use the Linux v3.17 functionality and fill in the missing gaps in the SPL. In specific, we need llong support on all kernels as well as to backport ullong support to pre-3.17 kernels.
Implement our own.

I am in favor of implementing our own. It would spare us some autotools checks and would not require much more code.

ryao · 2022-10-03T19:11:06Z

I have an initial implementation of 64-bit parameter support in a git branch:

https://github.com/ryao/zfs/tree/linux-module-param

I plan to finish it later this week. I will convert the existing parameters back to uint64_t/int64_t and hook them up to FreeBSD's u64/s64 when I do. That should render this PR obsolete and make everyone happy.

@amotin How does this sound?

lundman · 2022-10-03T22:30:25Z

Carry-on, this sounds good.

ryao · 2022-10-08T21:23:28Z

module/zfs/dbuf.c

@@ -227,8 +227,8 @@ typedef struct dbuf_cache {
 dbuf_cache_t dbuf_caches[DB_CACHE_MAX];

 /* Size limits for the caches */
-static unsigned long dbuf_cache_max_bytes = ULONG_MAX;
-static unsigned long dbuf_metadata_cache_max_bytes = ULONG_MAX;
+static ZFS_MODULE_ULONG dbuf_cache_max_bytes = ULONG_MAX;


It is probably a moot point now, but I noticed when working on the patch to supercede this that these values need to be changed from ULONG_MAX to UINT64_MAX.

ryao · 2022-10-08T21:59:50Z

module/zfs/arc.c

@@ -421,10 +421,10 @@ boolean_t arc_warm;
 */
 unsigned long zfs_arc_max = 0;
 unsigned long zfs_arc_min = 0;


While it is a moot point, these two were missed.

ryao · 2022-10-08T23:26:46Z

I have opened #14004. It is expected to supersede this PR.

ryao · 2022-10-10T19:27:16Z

module/zfs/spa_log_spacemap.c


 /*
 * If the log becomes too big, the import time of the pool can take a hit in
 * terms of performance. Thus we have a hard limit in the size of the log in
 * terms of blocks.
 */
-static unsigned long zfs_unflushed_log_block_max = (1ULL << 17);
+static ZFS_MODULE_ULONG zfs_unflushed_log_block_max = (1ULL << 18);


It is a moot point now, but this tunable should not have been changed.

No it should not.
I will make sure I look for changes like this in the future.

Various module parameters such as `zfs_arc_max` were originally `uint64_t` on OpenSolaris/Illumos, but were changed to `unsigned long` for Linux compatibility because Linux's kernel default module parameter implementation did not support 64-bit types on 32-bit platforms. This caused problems when porting OpenZFS to Windows because its LLP64 memory model made `unsigned long` a 32-bit type on 64-bit, which created the undesireable situation that parameters that should accept 64-bit values could not on 64-bit Windows. Upon inspection, it turns out that the Linux kernel module parameter interface is extensible, such that we are allowed to define our own types. Rather than maintaining the original type change via hacks to to continue shrinking module parameters on 32-bit Linux, we implement support for 64-bit module parameters on Linux. After doing a review of all 64-bit kernel parameters (found via the man page and also proposed changes by Andrew Innes), the kernel module parameters fell into a few groups: Parameters that were originally 64-bit on Illumos: * dbuf_cache_max_bytes * dbuf_metadata_cache_max_bytes * l2arc_feed_min_ms * l2arc_feed_secs * l2arc_headroom * l2arc_headroom_boost * l2arc_write_boost * l2arc_write_max * metaslab_aliquot * metaslab_force_ganging * zfetch_array_rd_sz * zfs_arc_max * zfs_arc_meta_limit * zfs_arc_meta_min * zfs_arc_min * zfs_async_block_max_blocks * zfs_condense_max_obsolete_bytes * zfs_condense_min_mapping_bytes * zfs_deadman_checktime_ms * zfs_deadman_synctime_ms * zfs_initialize_chunk_size * zfs_initialize_value * zfs_lua_max_instrlimit * zfs_lua_max_memlimit * zil_slog_bulk Parameters that were originally 32-bit on Illumos: * zfs_per_txg_dirty_frees_percent Parameters that were originally `ssize_t` on Illumos: * zfs_immediate_write_sz Note that `ssize_t` is `int32_t` on 32-bit and `int64_t` on 64-bit. It has been upgraded to 64-bit. Parameters that were `long`/`unsigned long` because of Linux/FreeBSD influence: * l2arc_rebuild_blocks_min_l2size * zfs_key_max_salt_uses * zfs_max_log_walking * zfs_max_logsm_summary_length * zfs_metaslab_max_size_cache_sec * zfs_min_metaslabs_to_flush * zfs_multihost_interval * zfs_unflushed_log_block_max * zfs_unflushed_log_block_min * zfs_unflushed_log_block_pct * zfs_unflushed_max_mem_amt * zfs_unflushed_max_mem_ppm New parameters that do not exist in Illumos: * l2arc_trim_ahead * vdev_file_logical_ashift * vdev_file_physical_ashift * zfs_arc_dnode_limit * zfs_arc_dnode_limit_percent * zfs_arc_dnode_reduce_percent * zfs_arc_meta_limit_percent * zfs_arc_sys_free * zfs_deadman_ziotime_ms * zfs_delete_blocks * zfs_history_output_max * zfs_livelist_max_entries * zfs_max_async_dedup_frees * zfs_max_nvlist_src_size * zfs_rebuild_max_segment * zfs_rebuild_vdev_limit * zfs_unflushed_log_txg_max * zfs_vdev_max_auto_ashift * zfs_vdev_min_auto_ashift * zfs_vnops_read_chunk_size * zvol_max_discard_blocks Rather than clutter the lists with commentary, the module parameters that need comments are repeated below. A few parameters were defined in Linux/FreeBSD specific code, where the use of ulong/long is not an issue for portability, so we leave them alone: * zfs_delete_blocks * zfs_key_max_salt_uses * zvol_max_discard_blocks The documentation for a few parameters was found to be incorrect: * zfs_deadman_checktime_ms - incorrectly documented as int * zfs_delete_blocks - not documented as Linux only * zfs_history_output_max - incorrectly documented as int * zfs_vnops_read_chunk_size - incorrectly documented as long * zvol_max_discard_blocks - incorrectly documented as ulong The documentation for these has been fixed, alongside the changes to document the switch to fixed width types. In addition, several kernel module parameters were percentages or held ashift values, so being 64-bit never made sense for them. They have been downgraded to 32-bit: * vdev_file_logical_ashift * vdev_file_physical_ashift * zfs_arc_dnode_limit_percent * zfs_arc_dnode_reduce_percent * zfs_arc_meta_limit_percent * zfs_per_txg_dirty_frees_percent * zfs_unflushed_log_block_pct * zfs_vdev_max_auto_ashift * zfs_vdev_min_auto_ashift Of special note are `zfs_vdev_max_auto_ashift` and `zfs_vdev_min_auto_ashift`, which were already defined as `uint64_t`, and passed to the kernel as `ulong`. This is inherently buggy on big endian 32-bit Linux, since the values would not be written to the correct locations. 32-bit FreeBSD was unaffected because its sysctl code correctly treated this as a `uint64_t`. Lastly, a code comment suggests that `zfs_arc_sys_free` is Linux-specific, but there is nothing to indicate to me that it is Linux-specific. Nothing was done about that. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Jorgen Lundman <lundman@lundman.net> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Original-patch-by: Andrew Innes <andrew.c12@gmail.com> Original-patch-by: Jorgen Lundman <lundman@lundman.net> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes #13984 Closes #14004

andrewc12 · 2022-10-13T22:36:11Z

Thanks. This is excellent news.

On older kernels, the definition for `module_param_call()` typecasts function pointers to `(void *)`, which triggers -Werror, causing the check to return false when it should return true. Fixing this breaks the build process on some older kernels because they define a `__check_old_set_param()` function in their headers that checks for a non-constified `->set()`. We workaround that through the c preprocessor by defining `__check_old_set_param(set)` to `(set)`, which prevents the build failures. However, it is now apparent that all kernels that we support have adopted the GRSecurity change, so there is no need to have an explicit autotools check for it anymore. We therefore remove the autotools check, while adding the workaround to our headers for the build time non-constified `->set()` check done by older kernel headers. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Jorgen Lundman <lundman@lundman.net> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13984 Closes openzfs#14004

Various module parameters such as `zfs_arc_max` were originally `uint64_t` on OpenSolaris/Illumos, but were changed to `unsigned long` for Linux compatibility because Linux's kernel default module parameter implementation did not support 64-bit types on 32-bit platforms. This caused problems when porting OpenZFS to Windows because its LLP64 memory model made `unsigned long` a 32-bit type on 64-bit, which created the undesireable situation that parameters that should accept 64-bit values could not on 64-bit Windows. Upon inspection, it turns out that the Linux kernel module parameter interface is extensible, such that we are allowed to define our own types. Rather than maintaining the original type change via hacks to to continue shrinking module parameters on 32-bit Linux, we implement support for 64-bit module parameters on Linux. After doing a review of all 64-bit kernel parameters (found via the man page and also proposed changes by Andrew Innes), the kernel module parameters fell into a few groups: Parameters that were originally 64-bit on Illumos: * dbuf_cache_max_bytes * dbuf_metadata_cache_max_bytes * l2arc_feed_min_ms * l2arc_feed_secs * l2arc_headroom * l2arc_headroom_boost * l2arc_write_boost * l2arc_write_max * metaslab_aliquot * metaslab_force_ganging * zfetch_array_rd_sz * zfs_arc_max * zfs_arc_meta_limit * zfs_arc_meta_min * zfs_arc_min * zfs_async_block_max_blocks * zfs_condense_max_obsolete_bytes * zfs_condense_min_mapping_bytes * zfs_deadman_checktime_ms * zfs_deadman_synctime_ms * zfs_initialize_chunk_size * zfs_initialize_value * zfs_lua_max_instrlimit * zfs_lua_max_memlimit * zil_slog_bulk Parameters that were originally 32-bit on Illumos: * zfs_per_txg_dirty_frees_percent Parameters that were originally `ssize_t` on Illumos: * zfs_immediate_write_sz Note that `ssize_t` is `int32_t` on 32-bit and `int64_t` on 64-bit. It has been upgraded to 64-bit. Parameters that were `long`/`unsigned long` because of Linux/FreeBSD influence: * l2arc_rebuild_blocks_min_l2size * zfs_key_max_salt_uses * zfs_max_log_walking * zfs_max_logsm_summary_length * zfs_metaslab_max_size_cache_sec * zfs_min_metaslabs_to_flush * zfs_multihost_interval * zfs_unflushed_log_block_max * zfs_unflushed_log_block_min * zfs_unflushed_log_block_pct * zfs_unflushed_max_mem_amt * zfs_unflushed_max_mem_ppm New parameters that do not exist in Illumos: * l2arc_trim_ahead * vdev_file_logical_ashift * vdev_file_physical_ashift * zfs_arc_dnode_limit * zfs_arc_dnode_limit_percent * zfs_arc_dnode_reduce_percent * zfs_arc_meta_limit_percent * zfs_arc_sys_free * zfs_deadman_ziotime_ms * zfs_delete_blocks * zfs_history_output_max * zfs_livelist_max_entries * zfs_max_async_dedup_frees * zfs_max_nvlist_src_size * zfs_rebuild_max_segment * zfs_rebuild_vdev_limit * zfs_unflushed_log_txg_max * zfs_vdev_max_auto_ashift * zfs_vdev_min_auto_ashift * zfs_vnops_read_chunk_size * zvol_max_discard_blocks Rather than clutter the lists with commentary, the module parameters that need comments are repeated below. A few parameters were defined in Linux/FreeBSD specific code, where the use of ulong/long is not an issue for portability, so we leave them alone: * zfs_delete_blocks * zfs_key_max_salt_uses * zvol_max_discard_blocks The documentation for a few parameters was found to be incorrect: * zfs_deadman_checktime_ms - incorrectly documented as int * zfs_delete_blocks - not documented as Linux only * zfs_history_output_max - incorrectly documented as int * zfs_vnops_read_chunk_size - incorrectly documented as long * zvol_max_discard_blocks - incorrectly documented as ulong The documentation for these has been fixed, alongside the changes to document the switch to fixed width types. In addition, several kernel module parameters were percentages or held ashift values, so being 64-bit never made sense for them. They have been downgraded to 32-bit: * vdev_file_logical_ashift * vdev_file_physical_ashift * zfs_arc_dnode_limit_percent * zfs_arc_dnode_reduce_percent * zfs_arc_meta_limit_percent * zfs_per_txg_dirty_frees_percent * zfs_unflushed_log_block_pct * zfs_vdev_max_auto_ashift * zfs_vdev_min_auto_ashift Of special note are `zfs_vdev_max_auto_ashift` and `zfs_vdev_min_auto_ashift`, which were already defined as `uint64_t`, and passed to the kernel as `ulong`. This is inherently buggy on big endian 32-bit Linux, since the values would not be written to the correct locations. 32-bit FreeBSD was unaffected because its sysctl code correctly treated this as a `uint64_t`. Lastly, a code comment suggests that `zfs_arc_sys_free` is Linux-specific, but there is nothing to indicate to me that it is Linux-specific. Nothing was done about that. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Jorgen Lundman <lundman@lundman.net> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Original-patch-by: Andrew Innes <andrew.c12@gmail.com> Original-patch-by: Jorgen Lundman <lundman@lundman.net> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13984 Closes openzfs#14004

On older kernels, the definition for `module_param_call()` typecasts function pointers to `(void *)`, which triggers -Werror, causing the check to return false when it should return true. Fixing this breaks the build process on some older kernels because they define a `__check_old_set_param()` function in their headers that checks for a non-constified `->set()`. We workaround that through the c preprocessor by defining `__check_old_set_param(set)` to `(set)`, which prevents the build failures. However, it is now apparent that all kernels that we support have adopted the GRSecurity change, so there is no need to have an explicit autotools check for it anymore. We therefore remove the autotools check, while adding the workaround to our headers for the build time non-constified `->set()` check done by older kernel headers. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Jorgen Lundman <lundman@lundman.net> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes #13984 Closes #14004

* etc: mask zfs-load-key.service Otherwise, systemd-sysv-generator will generate a service equivalent that breaks the boot: under systemd this is covered by zfs-mount-generator We already do this for zfs-import.service, and other init scripts are suppressed automatically by the "actual" .service files Fixes: commit f04b976 ("Add init script to load keys") Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#14010 Closes openzfs#14019 * Linux: Remove ZFS_AC_KERNEL_SRC_MODULE_PARAM_CALL_CONST autotools check On older kernels, the definition for `module_param_call()` typecasts function pointers to `(void *)`, which triggers -Werror, causing the check to return false when it should return true. Fixing this breaks the build process on some older kernels because they define a `__check_old_set_param()` function in their headers that checks for a non-constified `->set()`. We workaround that through the c preprocessor by defining `__check_old_set_param(set)` to `(set)`, which prevents the build failures. However, it is now apparent that all kernels that we support have adopted the GRSecurity change, so there is no need to have an explicit autotools check for it anymore. We therefore remove the autotools check, while adding the workaround to our headers for the build time non-constified `->set()` check done by older kernel headers. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Jorgen Lundman <lundman@lundman.net> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13984 Closes openzfs#14004 * Cleanup: 64-bit kernel module parameters should use fixed width types Various module parameters such as `zfs_arc_max` were originally `uint64_t` on OpenSolaris/Illumos, but were changed to `unsigned long` for Linux compatibility because Linux's kernel default module parameter implementation did not support 64-bit types on 32-bit platforms. This caused problems when porting OpenZFS to Windows because its LLP64 memory model made `unsigned long` a 32-bit type on 64-bit, which created the undesireable situation that parameters that should accept 64-bit values could not on 64-bit Windows. Upon inspection, it turns out that the Linux kernel module parameter interface is extensible, such that we are allowed to define our own types. Rather than maintaining the original type change via hacks to to continue shrinking module parameters on 32-bit Linux, we implement support for 64-bit module parameters on Linux. After doing a review of all 64-bit kernel parameters (found via the man page and also proposed changes by Andrew Innes), the kernel module parameters fell into a few groups: Parameters that were originally 64-bit on Illumos: * dbuf_cache_max_bytes * dbuf_metadata_cache_max_bytes * l2arc_feed_min_ms * l2arc_feed_secs * l2arc_headroom * l2arc_headroom_boost * l2arc_write_boost * l2arc_write_max * metaslab_aliquot * metaslab_force_ganging * zfetch_array_rd_sz * zfs_arc_max * zfs_arc_meta_limit * zfs_arc_meta_min * zfs_arc_min * zfs_async_block_max_blocks * zfs_condense_max_obsolete_bytes * zfs_condense_min_mapping_bytes * zfs_deadman_checktime_ms * zfs_deadman_synctime_ms * zfs_initialize_chunk_size * zfs_initialize_value * zfs_lua_max_instrlimit * zfs_lua_max_memlimit * zil_slog_bulk Parameters that were originally 32-bit on Illumos: * zfs_per_txg_dirty_frees_percent Parameters that were originally `ssize_t` on Illumos: * zfs_immediate_write_sz Note that `ssize_t` is `int32_t` on 32-bit and `int64_t` on 64-bit. It has been upgraded to 64-bit. Parameters that were `long`/`unsigned long` because of Linux/FreeBSD influence: * l2arc_rebuild_blocks_min_l2size * zfs_key_max_salt_uses * zfs_max_log_walking * zfs_max_logsm_summary_length * zfs_metaslab_max_size_cache_sec * zfs_min_metaslabs_to_flush * zfs_multihost_interval * zfs_unflushed_log_block_max * zfs_unflushed_log_block_min * zfs_unflushed_log_block_pct * zfs_unflushed_max_mem_amt * zfs_unflushed_max_mem_ppm New parameters that do not exist in Illumos: * l2arc_trim_ahead * vdev_file_logical_ashift * vdev_file_physical_ashift * zfs_arc_dnode_limit * zfs_arc_dnode_limit_percent * zfs_arc_dnode_reduce_percent * zfs_arc_meta_limit_percent * zfs_arc_sys_free * zfs_deadman_ziotime_ms * zfs_delete_blocks * zfs_history_output_max * zfs_livelist_max_entries * zfs_max_async_dedup_frees * zfs_max_nvlist_src_size * zfs_rebuild_max_segment * zfs_rebuild_vdev_limit * zfs_unflushed_log_txg_max * zfs_vdev_max_auto_ashift * zfs_vdev_min_auto_ashift * zfs_vnops_read_chunk_size * zvol_max_discard_blocks Rather than clutter the lists with commentary, the module parameters that need comments are repeated below. A few parameters were defined in Linux/FreeBSD specific code, where the use of ulong/long is not an issue for portability, so we leave them alone: * zfs_delete_blocks * zfs_key_max_salt_uses * zvol_max_discard_blocks The documentation for a few parameters was found to be incorrect: * zfs_deadman_checktime_ms - incorrectly documented as int * zfs_delete_blocks - not documented as Linux only * zfs_history_output_max - incorrectly documented as int * zfs_vnops_read_chunk_size - incorrectly documented as long * zvol_max_discard_blocks - incorrectly documented as ulong The documentation for these has been fixed, alongside the changes to document the switch to fixed width types. In addition, several kernel module parameters were percentages or held ashift values, so being 64-bit never made sense for them. They have been downgraded to 32-bit: * vdev_file_logical_ashift * vdev_file_physical_ashift * zfs_arc_dnode_limit_percent * zfs_arc_dnode_reduce_percent * zfs_arc_meta_limit_percent * zfs_per_txg_dirty_frees_percent * zfs_unflushed_log_block_pct * zfs_vdev_max_auto_ashift * zfs_vdev_min_auto_ashift Of special note are `zfs_vdev_max_auto_ashift` and `zfs_vdev_min_auto_ashift`, which were already defined as `uint64_t`, and passed to the kernel as `ulong`. This is inherently buggy on big endian 32-bit Linux, since the values would not be written to the correct locations. 32-bit FreeBSD was unaffected because its sysctl code correctly treated this as a `uint64_t`. Lastly, a code comment suggests that `zfs_arc_sys_free` is Linux-specific, but there is nothing to indicate to me that it is Linux-specific. Nothing was done about that. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Jorgen Lundman <lundman@lundman.net> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Original-patch-by: Andrew Innes <andrew.c12@gmail.com> Original-patch-by: Jorgen Lundman <lundman@lundman.net> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13984 Closes openzfs#14004 * cstyle: Allow URLs in C++ comments If a C++ comment contained a URL, the `://` part of the URL would trigger an error because there was no trailing blank, but trailing blanks make for an invalid URL. Modify the check to ignore text within the C++ comment. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com> Closes openzfs#13987 * zfs_domount: fix double-disown of dataset / double-free of zfsvfs_t Before this patch, in zfs_domount, if zfs_root or d_make_root fails, we leave zfsvfs != NULL. This will lead to execution of the error handling `if` statement at the `out` label, and hence to a call to dmu_objset_disown and zfsvfs_free. However, zfs_umount, which we call upon failure of zfs_root and d_make_root already does dmu_objset_disown and zfsvfs_free. I suppose this patch rather adds to the brittleness of this part of the code base, but I don't want to invest more time in this right now. To add a regression test, we'd need some kind of fault injection facility for zfs_root or d_make_root, which doesn't exist right now. And even then, I think that regression test would be too closely tied to the implementation. To repro the double-disown / double-free, do the following: 1. patch zfs_root to always return an error 2. mount a ZFS filesystem Here's the stack trace you would see then: VERIFY3(ds->ds_owner == tag) failed (0000000000000000 == ffff9142361e8000) PANIC at dsl_dataset.c:1003:dsl_dataset_disown() Showing stack for process 28332 CPU: 2 PID: 28332 Comm: zpool Tainted: G O 5.10.103-1.nutanix.el7.x86_64 #1 Call Trace: dump_stack+0x74/0x92 spl_dumpstack+0x29/0x2b [spl] spl_panic+0xd4/0xfc [spl] dsl_dataset_disown+0xe9/0x150 [zfs] dmu_objset_disown+0xd6/0x150 [zfs] zfs_domount+0x17b/0x4b0 [zfs] zpl_mount+0x174/0x220 [zfs] legacy_get_tree+0x2b/0x50 vfs_get_tree+0x2a/0xc0 path_mount+0x2fa/0xa70 do_mount+0x7c/0xa0 __x64_sys_mount+0x8b/0xe0 do_syscall_64+0x38/0x50 entry_SYSCALL_64_after_hwframe+0x44/0xa9 Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Co-authored-by: Christian Schwarz <christian.schwarz@nutanix.com> Signed-off-by: Christian Schwarz <christian.schwarz@nutanix.com> Closes openzfs#14025 * Fix potential NULL pointer dereference in lzc_ioctl() Users are allowed to pass NULL to resultp, but we unconditionally assume that they never do. When an external user does pass NULL to resultp, we dereference a NULL pointer. Clang's static analyzer complained about this. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14008 * Cleanup: Address Clang's static analyzer's unused code complaints These were categorized as the following: * Dead assignment 23 * Dead increment 4 * Dead initialization 6 * Dead nested assignment 18 Most of these are harmless, but since actual issues can hide among them, we correct them. That said, there were a few return values that were being ignored that appeared to merit some correction: * `destroy_callback()` in `cmd/zfs/zfs_main.c` ignored the error from `destroy_batched()`. We handle it by returning -1 if there is an error. * `zfs_do_upgrade()` in `cmd/zfs/zfs_main.c` ignored the error from `zfs_for_each()`. We handle it by doing a binary OR of the error value from the subsequent `zfs_for_each()` call to the existing value. This is how errors are mostly handled inside `zfs_for_each()`. The error value here is passed to exit from the zfs command, so doing a binary or on it is better than what we did previously. * `get_zap_prop()` in `module/zfs/zcp_get.c` ignored the error from `dsl_prop_get_ds()` when the property is not of type string. We return an error when it does. There is a small concern that the `zfs_get_temporary_prop()` call would handle things, but in the case that it does not, we would be pushing an uninitialized numval onto the lua stack. It is expected that `dsl_prop_get_ds()` will succeed anytime that `zfs_get_temporary_prop()` does, so that not giving it a chance to fix things is not a problem. * `draid_merge_impl()` in `tests/zfs-tests/cmd/draid.c` used `nvlist_add_nvlist()` twice in ways in which errors are expected to be impossible, so we switch to `fnvlist_add_nvlist()`. A few notable ones did not merit use of the return value, so we suppressed it with `(void)`: * `write_free_diffs()` in `lib/libzfs/libzfs_diff.c` ignored the error value from `describe_free()`. A look through the commit history revealed that this was intentional. * `arc_evict_hdr()` in `module/zfs/arc.c` did not need to use the returned handle from `arc_hdr_realloc()` because it is already referenced in lists. * `spa_vdev_detach()` in `module/zfs/spa.c` has a comment explicitly saying not to use the error from `vdev_label_init()` because whatever causes the error could be the reason why a detach is being done. Unfortunately, I am not presently able to analyze the kernel modules with Clang's static analyzer, so I could have missed some cases of this. In cases where reports were present in code that is duplicated between Linux and FreeBSD, I made a conscious effort to fix the FreeBSD version too. After this commit is merged, regressions like dee8934 should become extremely obvious with Clang's static analyzer since a regression would appear in the results as the only instance of unused code. That assumes that Coverity does not catch the issue first. My local branch with fixes from all of my outstanding non-draft pull requests shows 118 reports from Clang's static anlayzer after this patch. That is down by 51 from 169. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Cedric Berger <cedric@precidata.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13986 * zstream: allow decompress to fix metadata for uncompressed records If a record is uncompressed on-disk but the block pointer insists otherwise, reading it will return EIO. This commit adds an "off" type to the "zstream decompress" command. Using it will set the compression field in a zfs stream to "off" without changing the record's data. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: George Melikov <mail@gmelikov.ru> Signed-off-by: Alan Somers <asomers@FreeBSD.org> Sponsored by: Axcient Closes openzfs#13997 * Fix theoretical array overflow in lua_typename() Out of the 12 defects in lua that coverity reports, 5 of them involve `lua_typename()` and out of the dozens of defects in ZFS that lua reports, 3 of them involve `lua_typename()` due to the ZCP code. Given all of the uses of `lua_typename()` in the ZCP code, I was surprised that there were not more. It appears that only 2 were reported because only 3 called `lua_type()`, which does a defective sanity check that allows invalid types to be passed. lua/lua@d4fb848 addressed this in upstream lua 5.3. Unfortunately, we did not get that fix since we use lua 5.2 and we do not have assertions enabled in lua, so the upstream solution would not do anything. While we could adopt the upstream solution and enable assertions, a simpler solution is to fix the issue by making `lua_typename()` return `internal_type_error` whenever it is called with an invalid type. This avoids the array overflow and if we ever see it appear somewhere, we will know there is a problem with the lua interpreter. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13947 * Linux compat: fix DECLARE_EVENT_CLASS() test when ZFS is built-in ZFS_LINUX_TRY_COMPILE_HEADER macro doesn't take CONFIG_ZFS=y into account. As a result, on several latest Linux versions, configure script marks DECLARE_EVENT_CLASS() available for non-GPL when ZFS is being built as a module, but marks it unavailable when ZFS is built-in. Follow the logic of the neighbor macros and adjust ZFS_LINUX_TRY_COMPILE_HEADER accordingly, so that it doesn't try to look for a .ko when ZFS is built-in. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Signed-off-by: Alexander Lobakin <alobakin@pm.me> Closes openzfs#14006 * Fix declarations of non-global variables This patch inserts the `static` keyword to non-global variables, which where found by the analysis tool smatch. Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tino Reichardt <milky-zfs@mcmilk.de> Closes openzfs#13970 * Coverity model file update Upon review, it was found that the model for malloc() was incorrect. In addition, several general purpose memory allocation functions were missing models: * kmem_vasprintf() * kmem_asprintf() * kmem_strdup() * kmem_strfree() * spl_vmem_alloc() * spl_vmem_zalloc() * spl_vmem_free() * calloc() As an experiment to try to find more bugs, some less than general purpose memory allocation functions were also given models: * zfsvfs_create() * zfsvfs_free() * nvlist_alloc() * nvlist_dup() * nvlist_free() * nvlist_pack() * nvlist_unpack() Finally, the models were improved using additional coverity primitives: * __coverity_negative_sink__() * __coverity_writeall0__() * __coverity_mark_as_uninitialized_buffer__() * __coverity_mark_as_afm_allocated__() In addition, an attempt to inform coverity that certain modelled functions read entire buffers was used by adding the following to certain models: int first = buf[0]; int last = buf[buflen-1]; It was inspired by the QEMU model file. No additional false positives were found by this, but it is believed that the more accurate model file will help to catch false positives in the future. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14048 * Linux 6.1 compat: change order of sys/mutex.h includes After Linux 6.1-rc1 came out, the build started failing to build a couple of the files in the linux spl code due to the mutex_init redefinition. Moving the sys/mutex.h include to a lower position within these two files appears to fix the problem. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#14040 * ZED: Fix uninitialized value reads Coverity complained about a couple of uninitialized value reads in ZED. * zfs_deliver_dle() can pass an uninitialized string to zed_log_msg() * An uninitialized sev.sigev_signo is passed to timer_create() The former would log garbage while the latter is not a real issue, but we might as well suppress it by initializing the field to 0 for consistency's sake. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14047 * Fix NULL pointer dereference in zdb Clang's static analyzer complained that we dereference a NULL pointer in dump_path() if we return 0 when there is an error. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14044 * fm_fmri_hc_create() must call va_end() before returning clang-tidy caught this. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14044 * Fix NULL pointer passed to strlcpy from zap_lookup_impl() Clang's static analyzer pointed out that whenever zap_lookup_by_dnode() is called, we have the following stack where strlcpy() is passed a NULL pointer for realname from zap_lookup_by_dnode(): strlcpy() zap_lookup_impl() zap_lookup_norm_by_dnode() zap_lookup_by_dnode() Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14044 * Fix NULL pointer dereference in spa_open_common() Calling spa_open() will pass a NULL pointer to spa_open_common()'s config parameter. Under the right circumstances, we will dereference the config parameter without doing a NULL check. Clang's static analyzer found this. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14044 * set_global_var() should not pass NULL pointers to dlclose() Both Coverity and Clang's static analyzer caught this. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14044 * Fix possible NULL pointer dereference in sha2_mac_init() If mechanism->cm_param is NULL, passing mechanism to PROV_SHA2_GET_DIGEST_LEN() will dereference a NULL pointer. Coverity reported this. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14044 * Cleanup: Simplify userspace abd_free_chunks() Clang's static analyzer complained that we could use after free here if the inner loop ever iterated. That is a false positive, but upon inspection, the userland abd_alloc_chunks() function never will put multiple consecutive pages into a `struct scatterlist`, so there is no need to loop. We delete the inner loop. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14042 * Cleanup: Delete unnecessary pointer check from vdev_to_nvlist_iter() This confused Clang's static analyzer, making it think there was a possible NULL pointer dereference. There is no NULL pointer dereference. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14042 * Cleanup: metaslab_alloc_dva() should not NULL check mg->mg_next This is a circularly linked list. mg->mg_next can never be NULL. This caused 3 defect reports in Coverity. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14042 * Cleanup: zvol_add_clones() should not NULL check dp It is never NULL because we return early if dsl_pool_hold() fails. This caused Coverity to complain. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14042 * Cleanup: Delete dead code from send_merge_thread() range is always deferenced before it reaches this check, such that the kmem_zalloc() call is never executed. There is also no need to set `range->eos_marker = B_TRUE` because it is already set. Coverity incorrectly complained about a potential NULL pointer dereference because of this. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14042 * Cleanup: Remove NULL pointer check from dmu_send_impl() The pointer is to a structure member, so it is never NULL. Coverity complained about this. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14042 * Fix memory leaks in dmu_send()/dmu_send_obj() If we encounter an EXDEV error when using the redacted snapshots feature, the memory used by dspp.fromredactsnaps is leaked. Clang's static analyzer caught this during an experiment in which I had annotated various headers in an attempt to improve the results of static analysis. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13973 * Support idmapped mount Adds support for idmapped mounts. Supported as of Linux 5.12 this functionality allows user and group IDs to be remapped without changing their state on disk. This can be useful for portable home directories and a variety of container related use cases. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Youzhong Yang <yyang@mathworks.com> Closes openzfs#12923 Closes openzfs#13671 * Fix sequential resilver drive failure race condition This patch handles the race condition on simultaneous failure of 2 drives, which misses the vdev_rebuild_reset_wanted signal in vdev_rebuild_thread. We retry to catch this inside the vdev_rebuild_complete_sync function. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Reviewed-by: Dipak Ghosh <dipak.ghosh@hpe.com> Reviewed-by: Akash B <akash-b@hpe.com> Signed-off-by: Samuel Wycliffe J <samwyc@hpe.com> Closes openzfs#14041 Closes openzfs#14050 * Add options to zfs redundant_metadata property Currently, additional/extra copies are created for metadata in addition to the redundancy provided by the pool(mirror/raidz/draid), due to this 2 times more space is utilized per inode and this decreases the total number of inodes that can be created in the filesystem. By setting redundant_metadata to none, no additional copies of metadata are created, hence can reduce the space consumed by the additional metadata copies and increase the total number of inodes that can be created in the filesystem. Additionally, this can improve file create performance due to the reduced amount of metadata which needs to be written. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Dipak Ghosh <dipak.ghosh@hpe.com> Signed-off-by: Akash B <akash-b@hpe.com> Closes openzfs#13680 * Fix userland memory leak in zfs_do_send() Clang 15's static analyzer caught this. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14045 * Fix theoretical use of uninitialized values Clang's static analyzer complains about this. In get_configs(), if we have an invalid configuration that has no top level vdevs, we can read a couple of uninitialized variables. Aborting upon seeing this would break the userland tools for healthy pools, so we instead initialize the two variables to 0 to allow the userland tools to continue functioning for the pools with valid configurations. In zfs_do_wait(), if no wait activities are enabled, we read an uninitialized error variable. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14043 * Silence static analyzer warnings about spa_sync_props() Both Coverity and Clang's static analyzer complain about reading an uninitialized intval if the property is not passed as DATA_TYPE_UINT64 in the nvlist. This is impossible becuase spa_prop_validate() already checked this, but they are unlikely to be the last static analyzers to complain about this, so lets just refactor the code to suppress the warnings. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14043 * crypto_get_ptrs() should always write to *out_data_2 Callers will check if it has been set to NULL before trying to access it, but never initialize it themselves. Whenever "one block spans two iovecs", `crypto_get_ptrs()` will return, without ever setting `*out_data_2 = NULL`. The caller will then do a NULL check against the uninitailized pointer and if it is not zero, pass it to `memcpy()`. The only reason this has not caused horrible runtime issues is because `memcpy()` should be told to copy zero bytes when this happens. That said, this is technically undefined behavior, so we should correct it so that future changes to the code cannot trigger it. Clang's static analyzer found this with the help of CodeChecker's CTU analysis. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14043 * abd_return_buf() should call zfs_refcount_remove_many() early Calling zfs_refcount_remove_many() after freeing memory means we pass a reference to freed memory as the holder. This is not believed to be able to cause a problem, but there is a bit of a tradition of fixing these issues when they appear so that they do not obscure more serious issues in static analyzer output, so we fix this one too. Clang's static analyzer found this with the help of CodeChecker's CTU analysis. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14043 * Add defensive assertion to vdev_queue_aggregate() a6ccb36 had been intended to include this to silence Coverity reports, but this one was missed by mistake. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#14043 * Fix build failures Co-authored-by: наб <nabijaczleweli@nabijaczleweli.xyz> Co-authored-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Co-authored-by: ColMelvin <chris.lindee+github@gmail.com> Co-authored-by: Christian Schwarz <me@cschwarz.com> Co-authored-by: Alan Somers <asomers@FreeBSD.org> Co-authored-by: Alexander <solbjorn@users.noreply.github.com> Co-authored-by: Tino Reichardt <milky-zfs@mcmilk.de> Co-authored-by: Coleman Kane <ckane@colemankane.org> Co-authored-by: youzhongyang <youzhong@gmail.com> Co-authored-by: samwyc <115969550+samwyc@users.noreply.github.com> Co-authored-by: Akash B <akash-b@hpe.com>

ryao suggested changes Oct 2, 2022

View reviewed changes

andrewc12 changed the title ~~Upstream: Replace ZFS_MODULE_PARAM 'long' usage~~ Replace 'long' with os defined ZFS_MODULE_LONG to support LLP64 used by windows Oct 2, 2022

andrewc12 force-pushed the andrew_module-02 branch from ac09be5 to ef5637f Compare October 2, 2022 22:27

ryao approved these changes Oct 2, 2022

View reviewed changes

andrewc12 force-pushed the andrew_module-02 branch from ef5637f to d95ec0d Compare October 3, 2022 06:09

ryao self-requested a review October 4, 2022 04:40

ryao reviewed Oct 8, 2022

View reviewed changes

ryao mentioned this pull request Oct 9, 2022

Cleanup: 64-bit kernel module parameters should use fixed width types #14004

Closed

13 tasks

ryao reviewed Oct 10, 2022

View reviewed changes

behlendorf closed this in ff7a0a1 Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace 'long' with os defined ZFS_MODULE_LONG to support LLP64 used by windows #13984

Replace 'long' with os defined ZFS_MODULE_LONG to support LLP64 used by windows #13984

andrewc12 commented Oct 2, 2022 •

edited

Loading

ryao left a comment •

edited

Loading

ryao commented Oct 2, 2022 •

edited

Loading

andrewc12 commented Oct 2, 2022

lundman commented Oct 2, 2022

ryao commented Oct 2, 2022

andrewc12 commented Oct 3, 2022

ryao commented Oct 3, 2022

ryao commented Oct 3, 2022 •

edited

Loading

lundman commented Oct 3, 2022

andrewc12 commented Oct 3, 2022

ryao commented Oct 3, 2022

amotin commented Oct 3, 2022

ryao commented Oct 3, 2022 •

edited

Loading

amotin commented Oct 3, 2022 •

edited

Loading

ryao commented Oct 3, 2022 •

edited

Loading

ryao commented Oct 3, 2022

lundman commented Oct 3, 2022

ryao Oct 8, 2022

ryao Oct 8, 2022

ryao commented Oct 8, 2022

ryao Oct 10, 2022

andrewc12 Oct 10, 2022

andrewc12 commented Oct 13, 2022

Replace 'long' with os defined ZFS_MODULE_LONG to support LLP64 used by windows #13984

Replace 'long' with os defined ZFS_MODULE_LONG to support LLP64 used by windows #13984

Conversation

andrewc12 commented Oct 2, 2022 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ryao left a comment • edited Loading

Choose a reason for hiding this comment

ryao commented Oct 2, 2022 • edited Loading

andrewc12 commented Oct 2, 2022

lundman commented Oct 2, 2022

ryao commented Oct 2, 2022

andrewc12 commented Oct 3, 2022

ryao commented Oct 3, 2022

ryao commented Oct 3, 2022 • edited Loading

lundman commented Oct 3, 2022

andrewc12 commented Oct 3, 2022

ryao commented Oct 3, 2022

amotin commented Oct 3, 2022

ryao commented Oct 3, 2022 • edited Loading

amotin commented Oct 3, 2022 • edited Loading

ryao commented Oct 3, 2022 • edited Loading

ryao commented Oct 3, 2022

lundman commented Oct 3, 2022

ryao Oct 8, 2022

Choose a reason for hiding this comment

ryao Oct 8, 2022

Choose a reason for hiding this comment

ryao commented Oct 8, 2022

ryao Oct 10, 2022

Choose a reason for hiding this comment

andrewc12 Oct 10, 2022

Choose a reason for hiding this comment

andrewc12 commented Oct 13, 2022

andrewc12 commented Oct 2, 2022 •

edited

Loading

ryao left a comment •

edited

Loading

ryao commented Oct 2, 2022 •

edited

Loading

ryao commented Oct 3, 2022 •

edited

Loading

ryao commented Oct 3, 2022 •

edited

Loading

amotin commented Oct 3, 2022 •

edited

Loading

ryao commented Oct 3, 2022 •

edited

Loading