Cleanup: Specify unsignedness on things that should not be signed #13875

ryao · 2022-09-12T01:34:34Z

Motivation and Context

In #13871, zfs_vdev_aggregation_limit_non_rotating and zfs_vdev_aggregation_limit being signed was pointed out as a possible reason not to eliminate an unnecessary MAX(unsigned, 0) since the unsigned value was assigned from them.

There is no reason for these module parameters to be signed and upon inspection, it was found that there are a number of other module parameters that are signed, but should not be, so we make them unsigned. Making them unsigned made it clear that some other variables in the code should also be unsigned, so we also make those unsigned. This prevents users from setting negative values that could potentially cause bad behaviors. It also makes the code slightly easier to understand.

Description

Mostly module parameters that deal with timeouts, limits, bitshifts and percentages are made unsigned by this. Any that are boolean are left signed, since whether booleans should be considered signed or unsigned does not matter.

Making zfs_arc_lotsfree_percent unsigned caused a zfs_arc_lotsfree_percent >= 0 check to become redundant, so it was removed. Removing the check was also necessary to prevent a compiler error from -Werror=type-limits.

Several end of line comments had to be moved to their own lines because replacing int with uint32_t caused us to exceed the 80 character limit enforced by cstyle.pl.

The following were kept signed because they are passed to taskq_create(), which expects signed values and modifying the OpenSolaris/Illumos DDI is out of scope of this patch:

* metaslab_load_pct
* zfs_sync_taskq_batch_pct
* zfs_zil_clean_taskq_nthr_pct
* zfs_zil_clean_taskq_minalloc
* zfs_zil_clean_taskq_maxalloc
* zfs_arc_prune_task_threads

Also, negative values in those parameters was found to be harmless.

The following were left signed because either negative values make sense, or more analysis was needed to determine whether negative values should be disallowed:

* zfs_metaslab_switch_threshold
* zfs_pd_bytes_max
* zfs_livelist_min_percent_shared

zfs_multihost_history was made static to be consistent with other parameters.

A number of module parameters were marked as signed, but in reality referenced unsigned variables. upgrade_errlog_limit is one of the numerous examples. In the case of zfs_vdev_async_read_max_active, it was already uint32_t, but zdb had an extern int declaration for it.

Interestingly, the documentation in zfs.4 was right for upgrade_errlog_limit despite the module parameter being wrongly marked, while the documentation for zfs_vdev_async_read_max_active (and friends) was wrong. It was also wrong for zstd_abort_size, which was unsigned, but was documented as signed.

Also, the documentation in zfs.4 incorrectly described the following parameters as ulong when they were int:

* zfs_arc_meta_adjust_restarts
* zfs_override_estimate_recordsize

They are now uint32_t as of this patch and thus the man page has been updated to describe them as uint.

dbuf_state_index was left alone since it does nothing and perhaps should be removed in another patch.

If any module parameters were missed, they were not found by grep -r 'ZFS_MODULE_PARAM' | grep ', INT'. I did find a few that grep missed, but only because they were in files that had hits.

This patch intentionally did not attempt to address whether some of these module parameters should be elevated to 64-bit parameters, because the length of a long on 32-bit is 32-bit.

How Has This Been Tested?

Build testing has been done on Gentoo Linux.

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 · 2022-09-12T01:37:45Z

I just noticed that metaslab_load_pct is undocumented. That might be because it is only available for modification on FreeBSD. I guess further cleanup could expose it on Linux and document it.

ryao · 2022-09-12T01:38:51Z

Additionally, even though this is just cleanup and a documentation change, I also labelled it as a bug fix since allowing negative values to be injected into the kernel module via module parameters could cause bad behavior in some circumstances.

behlendorf · 2022-09-12T20:23:52Z

module/os/freebsd/zfs/zfs_debug.c

 static kmutex_t zfs_dbgmsgs_lock;
-int zfs_dbgmsg_maxsize = 4<<20; /* 4MB */
+uint32_t zfs_dbgmsg_maxsize = 4<<20; /* 4MB */


As nice as it would be to use a fixed size type here, the Linux kernel module interfaces expect either an int, unsigned int, long or unsigned long type. Internally, we can cast these values as we need too, but since we're registering them with the kernel it seemed prudent to stick to the expected types to avoid any unexpected behavior across architectures.

unsigned int is the same as uint32_t across all architectures. cstyle.pl does not permit using uint and most of the code already uses uint32_t, so I used uint32_t to be consistent.

That said, the same nice consistency is not true for long, which is why I did not make any module parameters use uint64_t.

unsigned int is the same as uint32_t across all architectures.

Well, it's true for architectures which use the ILP32, LP64, or LLP64 data model, and realistically we only really support LP64 with OpenZFS (ILP32 builds but has some known issues). It's not true for LP32, but that's ancient and not a concern. So, I think your point is fair and this should be fine.

When reviewing openzfs#13875, I noticed that our FreeBSD code has an issue where it converts from `int64_t` to `int` when calling `vnlru_free{,_vfsops}()`. The result is that if the int64_t is `1 << 36`, the int will be 0, since the low bits are 0. Even with some low bits are set, a value such as `(1 << 36) + 1)` would truncate to 1, which is wrong. There is protection against this on 32-bit platforms, but on 64-bit platforms, there is no check to protect us, so we add a check. Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>

When reviewing openzfs#13875, I noticed that our FreeBSD code has an issue where it converts from `int64_t` to `int` when calling `vnlru_free{,_vfsops}()`. The result is that if the int64_t is `1 << 36`, the int will be 0, since the low bits are 0. Even when some low bits are set, a value such as `(1 << 36) + 1)` would truncate to 1, which is wrong. There is protection against this on 32-bit platforms, but on 64-bit platforms, there is no check to protect us, so we add a check. Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>

ryao · 2022-09-13T15:20:40Z

After talking with @amotin about #13874, I have decided to modify this to use uint_t on module parameters instead of uint32_t. It will take some time before I have reworked this, so I am marking this as a draft.

Also, I need to make changes to this to take into account the finding in #13882, which I found today during self review.

When reviewing openzfs#13875, I noticed that our FreeBSD code has an issue where it converts from `int64_t` to `int` when calling `vnlru_free{,_vfsops}()`. The result is that if the int64_t is `1 << 36`, the int will be 0, since the low bits are 0. Even when some low bits are set, a value such as `((1 << 36) + 1)` would truncate to 1, which is wrong. There is protection against this on 32-bit platforms, but on 64-bit platforms, there is no check to protect us, so we add a check. Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>

When reviewing #13875, I noticed that our FreeBSD code has an issue where it converts from `int64_t` to `int` when calling `vnlru_free{,_vfsops}()`. The result is that if the int64_t is `1 << 36`, the int will be 0, since the low bits are 0. Even when some low bits are set, a value such as `((1 << 36) + 1)` would truncate to 1, which is wrong. There is protection against this on 32-bit platforms, but on 64-bit platforms, there is no check to protect us, so we add a check. Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes #13882

ryao · 2022-09-17T20:56:11Z

@amotin All module parameters are now uint_t as we previously discussed. I kept some of the int -> uint32_t in places where we already use uint64_t where the signed int was inappropriate, but those are not module parameters and they are few.

amotin

I am not getting sense of some uint32_t.

amotin · 2022-09-18T13:07:56Z

include/sys/arc_impl.h

@@ -995,7 +995,7 @@ extern void arc_wait_for_eviction(uint64_t, boolean_t);

 extern void arc_lowmem_init(void);
 extern void arc_lowmem_fini(void);
-extern void arc_prune_async(int64_t);
+extern void arc_prune_async(uint64_t);


It seems arc_evict_state() may have a race when this argument may become negative,

That would imply that there is currently an issue on Linux since this is assigned to an unsigned variable in the Linux version at ap->p_adjust = adjust;. I will study this more after I look at your other remarks.

Are you referring to aggsum_compare() saying that the value is bigger than arc_dnode_size_limit, but then the value changing under us when we call aggsum_upper_bound() to something smaller than arc_dnode_size_limit?

aggsum_upper_bound() returns a uint64_t and arc_dnode_size_limit is a uint64_t, so if this happens, we should have integer underflow in the existing code. This patch does not make things any worse.

Also, while I imagine we should address this like we did in ccec88f, I am not seeing a way of addressing this that I particularly like. Suggestions would be appreciated. :)

module/zfs/fm.c

module/zfs/metaslab.c

module/zfs/multilist.c

module/zfs/spa_misc.c

When reviewing openzfs#13875, I noticed that our FreeBSD code has an issue where it converts from `int64_t` to `int` when calling `vnlru_free{,_vfsops}()`. The result is that if the int64_t is `1 << 36`, the int will be 0, since the low bits are 0. Even when some low bits are set, a value such as `((1 << 36) + 1)` would truncate to 1, which is wrong. There is protection against this on 32-bit platforms, but on 64-bit platforms, there is no check to protect us, so we add a check. Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13882

module/zfs/arc.c

module/zfs/vdev_queue.c

ryao · 2022-09-27T16:24:49Z

@amotin I have redone this. Now everything that this patch touches uses uint_t, except for ->spa_sync_pass/spa_sync_pass(). That one does not need to be made a larger width in future memory models and even could stand to be shrunk.

In openzfs#13871, zfs_vdev_aggregation_limit_non_rotating and zfs_vdev_aggregation_limit being signed was pointed out as a possible reason not to eliminate an unnecessary MAX(unsigned, 0) since the unsigned value was assigned from them. There is no reason for these module parameters to be signed and upon inspection, it was found that there are a number of other module parameters that are signed, but should not be, so we make them unsigned. Making them unsigned made it clear that some other variables in the code should also be unsigned, so we also make those unsigned. This prevents users from setting negative values that could potentially cause bad behaviors. It also makes the code slightly easier to understand. Mostly module parameters that deal with timeouts, limits, bitshifts and percentages are made unsigned by this. Any that are boolean are left signed, since whether booleans should be considered signed or unsigned does not matter. Making zfs_arc_lotsfree_percent unsigned caused a `zfs_arc_lotsfree_percent >= 0` check to become redundant, so it was removed. Removing the check was also necessary to prevent a compiler error from -Werror=type-limits. Several end of line comments had to be moved to their own lines because replacing int with uint_t caused us to exceed the 80 character limit enforced by cstyle.pl. The following were kept signed because they are passed to taskq_create(), which expects signed values and modifying the OpenSolaris/Illumos DDI is out of scope of this patch: * metaslab_load_pct * zfs_sync_taskq_batch_pct * zfs_zil_clean_taskq_nthr_pct * zfs_zil_clean_taskq_minalloc * zfs_zil_clean_taskq_maxalloc * zfs_arc_prune_task_threads Also, negative values in those parameters was found to be harmless. The following were left signed because either negative values make sense, or more analysis was needed to determine whether negative values should be disallowed: * zfs_metaslab_switch_threshold * zfs_pd_bytes_max * zfs_livelist_min_percent_shared zfs_multihost_history was made static to be consistent with other parameters. A number of module parameters were marked as signed, but in reality referenced unsigned variables. upgrade_errlog_limit is one of the numerous examples. In the case of zfs_vdev_async_read_max_active, it was already uint32_t, but zdb had an extern int declaration for it. Interestingly, the documentation in zfs.4 was right for upgrade_errlog_limit despite the module parameter being wrongly marked, while the documentation for zfs_vdev_async_read_max_active (and friends) was wrong. It was also wrong for zstd_abort_size, which was unsigned, but was documented as signed. Also, the documentation in zfs.4 incorrectly described the following parameters as ulong when they were int: * zfs_arc_meta_adjust_restarts * zfs_override_estimate_recordsize They are now uint_t as of this patch and thus the man page has been updated to describe them as uint. dbuf_state_index was left alone since it does nothing and perhaps should be removed in another patch. If any module parameters were missed, they were not found by `grep -r 'ZFS_MODULE_PARAM' | grep ', INT'`. I did find a few that grep missed, but only because they were in files that had hits. This patch intentionally did not attempt to address whether some of these module parameters should be elevated to 64-bit parameters, because the length of a long on 32-bit is 32-bit. Lastly, it was pointed out during review that uint_t is a better match for these variables than uint32_t because FreeBSD kernel parameter definitions are designed for uint_t, whose bit width can change in future memory models. As a result, we change the existing parameters that are uint32_t to use uint_t. Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>

amotin

Thanks you. I haven't looked down to every specific variable for possible artifacts, but it looks good to me. I am just thinking whether size_t would be more appropriate for zdm_size and everywhere around zfs_debug.c.

ryao · 2022-09-27T20:51:22Z

@amotin I had intentionally avoided making changes to the bit widths to avoid scope creep.

@behlendorf All outstanding blockers to merging this are now resolved.

When reviewing openzfs#13875, I noticed that our FreeBSD code has an issue where it converts from `int64_t` to `int` when calling `vnlru_free{,_vfsops}()`. The result is that if the int64_t is `1 << 36`, the int will be 0, since the low bits are 0. Even when some low bits are set, a value such as `((1 << 36) + 1)` would truncate to 1, which is wrong. There is protection against this on 32-bit platforms, but on 64-bit platforms, there is no check to protect us, so we add a check. Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13882

In openzfs#13871, zfs_vdev_aggregation_limit_non_rotating and zfs_vdev_aggregation_limit being signed was pointed out as a possible reason not to eliminate an unnecessary MAX(unsigned, 0) since the unsigned value was assigned from them. There is no reason for these module parameters to be signed and upon inspection, it was found that there are a number of other module parameters that are signed, but should not be, so we make them unsigned. Making them unsigned made it clear that some other variables in the code should also be unsigned, so we also make those unsigned. This prevents users from setting negative values that could potentially cause bad behaviors. It also makes the code slightly easier to understand. Mostly module parameters that deal with timeouts, limits, bitshifts and percentages are made unsigned by this. Any that are boolean are left signed, since whether booleans should be considered signed or unsigned does not matter. Making zfs_arc_lotsfree_percent unsigned caused a `zfs_arc_lotsfree_percent >= 0` check to become redundant, so it was removed. Removing the check was also necessary to prevent a compiler error from -Werror=type-limits. Several end of line comments had to be moved to their own lines because replacing int with uint_t caused us to exceed the 80 character limit enforced by cstyle.pl. The following were kept signed because they are passed to taskq_create(), which expects signed values and modifying the OpenSolaris/Illumos DDI is out of scope of this patch: * metaslab_load_pct * zfs_sync_taskq_batch_pct * zfs_zil_clean_taskq_nthr_pct * zfs_zil_clean_taskq_minalloc * zfs_zil_clean_taskq_maxalloc * zfs_arc_prune_task_threads Also, negative values in those parameters was found to be harmless. The following were left signed because either negative values make sense, or more analysis was needed to determine whether negative values should be disallowed: * zfs_metaslab_switch_threshold * zfs_pd_bytes_max * zfs_livelist_min_percent_shared zfs_multihost_history was made static to be consistent with other parameters. A number of module parameters were marked as signed, but in reality referenced unsigned variables. upgrade_errlog_limit is one of the numerous examples. In the case of zfs_vdev_async_read_max_active, it was already uint32_t, but zdb had an extern int declaration for it. Interestingly, the documentation in zfs.4 was right for upgrade_errlog_limit despite the module parameter being wrongly marked, while the documentation for zfs_vdev_async_read_max_active (and friends) was wrong. It was also wrong for zstd_abort_size, which was unsigned, but was documented as signed. Also, the documentation in zfs.4 incorrectly described the following parameters as ulong when they were int: * zfs_arc_meta_adjust_restarts * zfs_override_estimate_recordsize They are now uint_t as of this patch and thus the man page has been updated to describe them as uint. dbuf_state_index was left alone since it does nothing and perhaps should be removed in another patch. If any module parameters were missed, they were not found by `grep -r 'ZFS_MODULE_PARAM' | grep ', INT'`. I did find a few that grep missed, but only because they were in files that had hits. This patch intentionally did not attempt to address whether some of these module parameters should be elevated to 64-bit parameters, because the length of a long on 32-bit is 32-bit. Lastly, it was pointed out during review that uint_t is a better match for these variables than uint32_t because FreeBSD kernel parameter definitions are designed for uint_t, whose bit width can change in future memory models. As a result, we change the existing parameters that are uint32_t to use uint_t. Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Neal Gompa <ngompa@datto.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13875

ryao force-pushed the unsigned-cleanup branch from 036043f to 445f0f7 Compare September 12, 2022 01:40

ryao mentioned this pull request Sep 12, 2022

Cleanup: Remove ineffective unsigned comparisons against 0 #13871

Merged

13 tasks

ryao force-pushed the unsigned-cleanup branch 3 times, most recently from 1a12958 to 6fd9fae Compare September 12, 2022 01:57

Conan-Kudo approved these changes Sep 12, 2022

View reviewed changes

ryao force-pushed the unsigned-cleanup branch from 6fd9fae to b139ab4 Compare September 12, 2022 05:57

behlendorf reviewed Sep 12, 2022

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 12, 2022

ryao force-pushed the unsigned-cleanup branch from b139ab4 to e467375 Compare September 13, 2022 14:22

ryao mentioned this pull request Sep 13, 2022

Add zfs_btree_verify_intensity kernel module parameter #13874

Merged

13 tasks

ryao mentioned this pull request Sep 13, 2022

FreeBSD: Fix integer conversion for vnlru_free{,_vfsops}() #13882

Merged

13 tasks

ryao marked this pull request as draft September 13, 2022 15:20

ryao force-pushed the unsigned-cleanup branch 2 times, most recently from 011d59d to 9208c20 Compare September 17, 2022 20:50

ryao marked this pull request as ready for review September 17, 2022 20:55

amotin reviewed Sep 18, 2022

View reviewed changes

ryao force-pushed the unsigned-cleanup branch from 9208c20 to 374fee7 Compare September 18, 2022 22:33

ryao force-pushed the unsigned-cleanup branch from 374fee7 to 4d402b6 Compare September 26, 2022 01:24

ryao requested review from amotin and behlendorf September 27, 2022 01:51

amotin reviewed Sep 27, 2022

View reviewed changes

module/zfs/arc.c Outdated Show resolved Hide resolved

module/zfs/arc.c Outdated Show resolved Hide resolved

module/zfs/arc.c Outdated Show resolved Hide resolved

module/zfs/vdev_queue.c Outdated Show resolved Hide resolved

module/zfs/vdev_queue.c Show resolved Hide resolved

ryao force-pushed the unsigned-cleanup branch 4 times, most recently from 34622aa to e5023c2 Compare September 27, 2022 16:23

ryao requested a review from amotin September 27, 2022 16:24

ryao force-pushed the unsigned-cleanup branch from e5023c2 to c18bc4f Compare September 27, 2022 16:29

ryao force-pushed the unsigned-cleanup branch from c18bc4f to 87f49b3 Compare September 27, 2022 16:33

amotin approved these changes Sep 27, 2022

View reviewed changes

behlendorf approved these changes Sep 27, 2022

View reviewed changes

behlendorf merged commit fdc2d30 into openzfs:master Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup: Specify unsignedness on things that should not be signed #13875

Cleanup: Specify unsignedness on things that should not be signed #13875

ryao commented Sep 12, 2022 •

edited

Loading

ryao commented Sep 12, 2022

ryao commented Sep 12, 2022

behlendorf Sep 12, 2022

ryao Sep 12, 2022

behlendorf Sep 12, 2022 •

edited

Loading

ryao commented Sep 13, 2022

ryao commented Sep 17, 2022

amotin left a comment

amotin Sep 18, 2022

ryao Sep 18, 2022

ryao Sep 18, 2022

ryao Sep 18, 2022

ryao commented Sep 27, 2022

amotin left a comment

ryao commented Sep 27, 2022

Cleanup: Specify unsignedness on things that should not be signed #13875

Cleanup: Specify unsignedness on things that should not be signed #13875

Conversation

ryao commented Sep 12, 2022 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ryao commented Sep 12, 2022

ryao commented Sep 12, 2022

behlendorf Sep 12, 2022

Choose a reason for hiding this comment

ryao Sep 12, 2022

Choose a reason for hiding this comment

behlendorf Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

ryao commented Sep 13, 2022

ryao commented Sep 17, 2022

amotin left a comment

Choose a reason for hiding this comment

amotin Sep 18, 2022

Choose a reason for hiding this comment

ryao Sep 18, 2022

Choose a reason for hiding this comment

ryao Sep 18, 2022

Choose a reason for hiding this comment

ryao Sep 18, 2022

Choose a reason for hiding this comment

ryao commented Sep 27, 2022

amotin left a comment

Choose a reason for hiding this comment

ryao commented Sep 27, 2022

ryao commented Sep 12, 2022 •

edited

Loading

behlendorf Sep 12, 2022 •

edited

Loading