Skip to content

Commit

Permalink
Cleanup: Specify unsignedness on things that should not be signed
Browse files Browse the repository at this point in the history
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.

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.

Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
  • Loading branch information
ryao committed Sep 13, 2022
1 parent 710fd1d commit e467375
Show file tree
Hide file tree
Showing 47 changed files with 442 additions and 419 deletions.
4 changes: 2 additions & 2 deletions cmd/zdb/zdb.c
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,10 @@ zdb_ot_name(dmu_object_type_t type)
extern int reference_tracking_enable;
extern int zfs_recover;
extern unsigned long zfs_arc_meta_min, zfs_arc_meta_limit;
extern int zfs_vdev_async_read_max_active;
extern uint32_t zfs_vdev_async_read_max_active;
extern boolean_t spa_load_verify_dryrun;
extern boolean_t spa_mode_readable_spacemaps;
extern int zfs_reconstruct_indirect_combinations_max;
extern uint32_t zfs_reconstruct_indirect_combinations_max;
extern int zfs_btree_verify_intensity;

static const char cmdname[] = "zdb";
Expand Down
6 changes: 3 additions & 3 deletions cmd/ztest.c
Original file line number Diff line number Diff line change
Expand Up @@ -253,10 +253,10 @@ static const ztest_shared_opts_t ztest_opts_defaults = {
extern uint64_t metaslab_force_ganging;
extern uint64_t metaslab_df_alloc_threshold;
extern unsigned long zfs_deadman_synctime_ms;
extern int metaslab_preload_limit;
extern uint32_t metaslab_preload_limit;
extern int zfs_compressed_arc_enabled;
extern int zfs_abd_scatter_enabled;
extern int dmu_object_alloc_chunk_shift;
extern uint32_t dmu_object_alloc_chunk_shift;
extern boolean_t zfs_force_some_double_word_sm_entries;
extern unsigned long zio_decompress_fail_fraction;
extern unsigned long zfs_reconstruct_indirect_damage_fraction;
Expand Down Expand Up @@ -4638,7 +4638,7 @@ ztest_dmu_object_next_chunk(ztest_ds_t *zd, uint64_t id)
{
(void) id;
objset_t *os = zd->zd_os;
int dnodes_per_chunk = 1 << dmu_object_alloc_chunk_shift;
uint32_t dnodes_per_chunk = 1 << dmu_object_alloc_chunk_shift;
uint64_t object;

/*
Expand Down
2 changes: 1 addition & 1 deletion include/sys/arc.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ typedef void arc_write_done_func_t(zio_t *zio, arc_buf_t *buf, void *priv);
typedef void arc_prune_func_t(int64_t bytes, void *priv);

/* Shared module parameters */
extern int zfs_arc_average_blocksize;
extern uint32_t zfs_arc_average_blocksize;
extern int l2arc_exclude_special;

/* generic arc_done_func_t's which you can use */
Expand Down
10 changes: 5 additions & 5 deletions include/sys/arc_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -976,15 +976,15 @@ extern arc_stats_t arc_stats;
extern arc_sums_t arc_sums;
extern hrtime_t arc_growtime;
extern boolean_t arc_warm;
extern int arc_grow_retry;
extern int arc_no_grow_shift;
extern int arc_shrink_shift;
extern uint32_t arc_grow_retry;
extern uint32_t arc_no_grow_shift;
extern uint32_t arc_shrink_shift;
extern kmutex_t arc_prune_mtx;
extern list_t arc_prune_list;
extern arc_state_t ARC_mfu;
extern arc_state_t ARC_mru;
extern uint_t zfs_arc_pc_percent;
extern int arc_lotsfree_percent;
extern uint32_t arc_lotsfree_percent;
extern unsigned long zfs_arc_min;
extern unsigned long zfs_arc_max;

Expand All @@ -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);
extern int arc_memory_throttle(spa_t *spa, uint64_t reserve, uint64_t txg);
extern uint64_t arc_free_memory(void);
extern int64_t arc_available_memory(void);
Expand Down
4 changes: 2 additions & 2 deletions include/sys/dmu.h
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ int dmu_assign_arcbuf_by_dnode(dnode_t *dn, uint64_t offset,
int dmu_assign_arcbuf_by_dbuf(dmu_buf_t *handle, uint64_t offset,
struct arc_buf *buf, dmu_tx_t *tx);
#define dmu_assign_arcbuf dmu_assign_arcbuf_by_dbuf
extern int zfs_max_recordsize;
extern uint32_t zfs_max_recordsize;

/*
* Asynchronously try to read in the data.
Expand Down Expand Up @@ -1070,7 +1070,7 @@ int dmu_diff(const char *tosnap_name, const char *fromsnap_name,
#define ZFS_CRC64_POLY 0xC96C5795D7870F42ULL /* ECMA-182, reflected form */
extern uint64_t zfs_crc64_table[256];

extern int dmu_prefetch_max;
extern uint32_t dmu_prefetch_max;

#ifdef __cplusplus
}
Expand Down
6 changes: 3 additions & 3 deletions include/sys/dsl_pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ struct dsl_deadlist;
extern unsigned long zfs_dirty_data_max;
extern unsigned long zfs_dirty_data_max_max;
extern unsigned long zfs_wrlog_data_max;
extern int zfs_dirty_data_max_percent;
extern int zfs_dirty_data_max_max_percent;
extern int zfs_delay_min_dirty_percent;
extern uint32_t zfs_dirty_data_max_percent;
extern uint32_t zfs_dirty_data_max_max_percent;
extern uint32_t zfs_delay_min_dirty_percent;
extern unsigned long zfs_delay_scale;

/* These macros are for indexing into the zfs_all_blkstats_t. */
Expand Down
2 changes: 1 addition & 1 deletion include/sys/fm/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ extern void fm_init(void);
extern void fm_fini(void);
extern void zfs_zevent_post_cb(nvlist_t *nvl, nvlist_t *detector);
extern int zfs_zevent_post(nvlist_t *, nvlist_t *, zevent_cb_t *);
extern void zfs_zevent_drain_all(int *);
extern void zfs_zevent_drain_all(uint32_t *);
extern zfs_file_t *zfs_zevent_fd_hold(int, minor_t *, zfs_zevent_t **);
extern void zfs_zevent_fd_rele(zfs_file_t *);
extern int zfs_zevent_next(zfs_zevent_t *, nvlist_t **, uint64_t *, uint64_t *);
Expand Down
4 changes: 2 additions & 2 deletions include/sys/spa.h
Original file line number Diff line number Diff line change
Expand Up @@ -826,7 +826,7 @@ extern int spa_scrub_pause_resume(spa_t *spa, pool_scrub_cmd_t flag);
extern void spa_sync(spa_t *spa, uint64_t txg); /* only for DMU use */
extern void spa_sync_allpools(void);

extern int zfs_sync_pass_deferred_free;
extern uint32_t zfs_sync_pass_deferred_free;

/* spa namespace global mutex */
extern kmutex_t spa_namespace_lock;
Expand Down Expand Up @@ -1013,7 +1013,7 @@ extern boolean_t spa_indirect_vdevs_loaded(spa_t *spa);
extern blkptr_t *spa_get_rootblkptr(spa_t *spa);
extern void spa_set_rootblkptr(spa_t *spa, const blkptr_t *bp);
extern void spa_altroot(spa_t *, char *, size_t);
extern int spa_sync_pass(spa_t *spa);
extern uint32_t spa_sync_pass(spa_t *spa);
extern char *spa_name(spa_t *spa);
extern uint64_t spa_guid(spa_t *spa);
extern uint64_t spa_load_guid(spa_t *spa);
Expand Down
4 changes: 2 additions & 2 deletions include/sys/spa_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ struct spa {
nvlist_t *spa_config_splitting; /* config for splitting */
nvlist_t *spa_load_info; /* info and errors from load */
uint64_t spa_config_txg; /* txg of last config change */
int spa_sync_pass; /* iterate-to-convergence */
uint32_t spa_sync_pass; /* iterate-to-convergence */
pool_state_t spa_state; /* pool state */
int spa_inject_ref; /* injection references */
uint8_t spa_sync_on; /* sync threads are running */
Expand Down Expand Up @@ -446,7 +446,7 @@ struct spa {

extern char *spa_config_path;
extern const char *zfs_deadman_failmode;
extern int spa_slop_shift;
extern uint32_t spa_slop_shift;
extern void spa_taskq_dispatch_ent(spa_t *spa, zio_type_t t, zio_taskq_type_t q,
task_func_t *func, void *arg, uint_t flags, taskq_ent_t *ent);
extern void spa_taskq_dispatch_sync(spa_t *, zio_type_t t, zio_taskq_type_t q,
Expand Down
2 changes: 1 addition & 1 deletion include/sys/txg.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ extern void *txg_list_head(txg_list_t *tl, uint64_t txg);
extern void *txg_list_next(txg_list_t *tl, void *p, uint64_t txg);

/* Global tuning */
extern int zfs_txg_timeout;
extern uint32_t zfs_txg_timeout;


#ifdef ZFS_DEBUG
Expand Down
4 changes: 2 additions & 2 deletions include/sys/vdev_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ typedef struct vdev_cache vdev_cache_t;
typedef struct vdev_cache_entry vdev_cache_entry_t;
struct abd;

extern int zfs_vdev_queue_depth_pct;
extern int zfs_vdev_def_queue_depth;
extern uint32_t zfs_vdev_queue_depth_pct;
extern uint32_t zfs_vdev_def_queue_depth;
extern uint32_t zfs_vdev_async_write_max_active;

/*
Expand Down
2 changes: 1 addition & 1 deletion include/sys/vdev_removal.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ extern int spa_vdev_remove_cancel(spa_t *);
extern void spa_vdev_removal_destroy(spa_vdev_removal_t *);
extern uint64_t spa_remove_max_segment(spa_t *);

extern int vdev_removal_max_span;
extern uint32_t vdev_removal_max_span;

#ifdef __cplusplus
}
Expand Down
Loading

0 comments on commit e467375

Please sign in to comment.