Re: [PATCH 1/2] md: change chunk_sectors and stripe cache counts to unsigned int

[yu kuai]

Hi,

在 2026/7/10 21:23, Hiroshi Nishida 写道:
> chunk_sectors, new_chunk_sectors, prev_chunk_sectors, max_nr_stripes,
> and min_nr_stripes are never negative. Using signed int is semantically
> wrong and prevents the compiler from optimizing division/modulo by
> power-of-two chunk sizes to right shifts in the hot I/O path.
>
> Change all struct fields and derived local variables to unsigned int:
>    mddev->chunk_sectors
>    mddev->new_chunk_sectors
>    r5conf->chunk_sectors
>    r5conf->prev_chunk_sectors
>    r5conf->max_nr_stripes
>    r5conf->min_nr_stripes
>    Local: sectors_per_chunk, new_chunk, chunk_sectors
>
> The min() in r5c_check_cached_full_stripe() required both operands to
> match signedness; this is now satisfied with max_nr_stripes unsigned.
>
> Signed-off-by: Hiroshi Nishida <nishidafmly@xxxxxxxxx>
> ---
>   drivers/md/md.h    |  4 ++--
>   drivers/md/raid5.c | 14 +++++++-------
>   drivers/md/raid5.h |  8 ++++----
>   3 files changed, 13 insertions(+), 13 deletions(-)

Patch looks fine, but you're missing some places like raid5_show_stripe_cache_size(),
where %d is used to print min_nr_stripes.

>
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index d8daf0f75cbb..b9ad26844799 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -437,7 +437,7 @@ struct mddev {
>   	int				external;	/* metadata is
>   							 * managed externally */
>   	char				metadata_type[17]; /* externally set*/
> -	int				chunk_sectors;
> +	unsigned int			chunk_sectors;
>   	time64_t			ctime, utime;
>   	int				level, layout;
>   	char				clevel[16];
> @@ -466,7 +466,7 @@ struct mddev {
>   	 */
>   	sector_t			reshape_position;
>   	int				delta_disks, new_level, new_layout;
> -	int				new_chunk_sectors;
> +	unsigned int			new_chunk_sectors;
>   	int				reshape_backwards;
>   
>   	struct md_thread __rcu		*thread;	/* management thread */
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 0c5c9fb0606e..28828e083c2b 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2970,7 +2970,7 @@ sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
>   	sector_t new_sector;
>   	int algorithm = previous ? conf->prev_algo
>   				 : conf->algorithm;
> -	int sectors_per_chunk = previous ? conf->prev_chunk_sectors
> +	unsigned int sectors_per_chunk = previous ? conf->prev_chunk_sectors
>   					 : conf->chunk_sectors;
>   	int raid_disks = previous ? conf->previous_raid_disks
>   				  : conf->raid_disks;
> @@ -3166,7 +3166,7 @@ sector_t raid5_compute_blocknr(struct stripe_head *sh, int i, int previous)
>   	int raid_disks = sh->disks;
>   	int data_disks = raid_disks - conf->max_degraded;
>   	sector_t new_sector = sh->sector, check;
> -	int sectors_per_chunk = previous ? conf->prev_chunk_sectors
> +	unsigned int sectors_per_chunk = previous ? conf->prev_chunk_sectors
>   					 : conf->chunk_sectors;
>   	int algorithm = previous ? conf->prev_algo
>   				 : conf->algorithm;
> @@ -3584,7 +3584,7 @@ static void end_reshape(struct r5conf *conf);
>   static void stripe_set_idx(sector_t stripe, struct r5conf *conf, int previous,
>   			    struct stripe_head *sh)
>   {
> -	int sectors_per_chunk =
> +	unsigned int sectors_per_chunk =
>   		previous ? conf->prev_chunk_sectors : conf->chunk_sectors;
>   	int dd_idx;
>   	int chunk_offset = sector_div(stripe, sectors_per_chunk);
> @@ -6103,7 +6103,7 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
>   static sector_t raid5_bio_lowest_chunk_sector(struct r5conf *conf,
>   					      struct bio *bi)
>   {
> -	int sectors_per_chunk = conf->chunk_sectors;
> +	unsigned int sectors_per_chunk = conf->chunk_sectors;
>   	int raid_disks = conf->raid_disks;
>   	int dd_idx;
>   	struct stripe_head sh;
> @@ -7930,7 +7930,7 @@ static int raid5_run(struct mddev *mddev)
>   		sector_t here_new, here_old;
>   		int old_disks;
>   		int max_degraded = (mddev->level == 6 ? 2 : 1);
> -		int chunk_sectors;
> +		unsigned int chunk_sectors;
>   		int new_data_disks;
>   
>   		if (journal_dev) {
> @@ -8832,7 +8832,7 @@ static int raid5_check_reshape(struct mddev *mddev)
>   	 * to be used by a reshape pass.
>   	 */
>   	struct r5conf *conf = mddev->private;
> -	int new_chunk = mddev->new_chunk_sectors;
> +	unsigned int new_chunk = mddev->new_chunk_sectors;
>   
>   	if (mddev->new_layout >= 0 && !algorithm_valid_raid5(mddev->new_layout))
>   		return -EINVAL;
> @@ -8866,7 +8866,7 @@ static int raid5_check_reshape(struct mddev *mddev)
>   
>   static int raid6_check_reshape(struct mddev *mddev)
>   {
> -	int new_chunk = mddev->new_chunk_sectors;
> +	unsigned int new_chunk = mddev->new_chunk_sectors;
>   
>   	if (mddev->new_layout >= 0 && !algorithm_valid_raid6(mddev->new_layout))
>   		return -EINVAL;
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index cb5feae04db2..5cd9d0f36b6e 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -572,12 +572,12 @@ struct r5conf {
>   	/* only protect corresponding hash list and inactive_list */
>   	spinlock_t		hash_locks[NR_STRIPE_HASH_LOCKS];
>   	struct mddev		*mddev;
> -	int			chunk_sectors;
> +	unsigned int		chunk_sectors;
>   	int			level, algorithm, rmw_level;
>   	int			max_degraded;
>   	int			raid_disks;
> -	int			max_nr_stripes;
> -	int			min_nr_stripes;
> +	unsigned int		max_nr_stripes;
> +	unsigned int		min_nr_stripes;
>   #if PAGE_SIZE != DEFAULT_STRIPE_SIZE
>   	unsigned long	stripe_size;
>   	unsigned int	stripe_shift;
> @@ -595,7 +595,7 @@ struct r5conf {
>   	 */
>   	sector_t		reshape_safe;
>   	int			previous_raid_disks;
> -	int			prev_chunk_sectors;
> +	unsigned int		prev_chunk_sectors;
>   	int			prev_algo;
>   	short			generation; /* increments with every reshape */
>   	seqcount_spinlock_t	gen_lock;	/* lock against generation changes */

-- 
Thanks,
Kuai