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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The commit's core justification is false. Changing the types to unsigned does not enable compiler right-shift optimizations for variable divisions.
- [Low] Unupdated printf-style format specifiers passing unsigned fields to '%d' cause compiler warnings (-Wformat) and break builds configured with -Werror.

Pre-existing issues:
- [Critical] The macro `round_down` is incorrectly used with a non-power-of-two divisor, causing silent data corruption in bitmap sector calculations.
--

commit 86528a8b22367a5ee16a7cb19a1d56bf58f6aa67
Author: Hiroshi Nishida <[email protected]>

md: change chunk_sectors and stripe cache counts to unsigned int

This commit changes several structural fields and local variables like
chunk_sectors and max_nr_stripes from signed to unsigned int. The stated
rationale is to prevent negative values and enable right-shift compiler
optimizations for power-of-two division in the hot I/O path.

>     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.

[Severity: Medium]
Is this optimization rationale accurate? The hot I/O path uses sector_div(),
which expands to do_div(). The do_div() macro explicitly casts the divisor
to a 32-bit unsigned integer, so the division was already operating on an
unsigned value.

Additionally, because sectors_per_chunk is a runtime variable rather than a
compile-time constant, would the compiler be able to optimize it into a
right shift without a conditional branch?

> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index d8daf0f75cbbe..b9ad26844799d 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;

[Severity: Low]
Does changing this field to unsigned int cause compiler warnings with
existing format strings?

For example, chunk_size_show() in drivers/md/md.c appears to use %d to
format mddev->chunk_sectors:

chunk_size_show() {
    ...
    return sprintf(page, "%d (%d)\n",
                   mddev->new_chunk_sectors << 9,
                   mddev->chunk_sectors << 9);
}

Will this break builds configured with -Werror?

> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index cb5feae04db27..5cd9d0f36b6e0 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;

[Severity: Low]
Will the changes to min_nr_stripes and max_nr_stripes also trigger format
warnings?

Places like raid5_end_read_request() and raid5_set_cache_size() still use
%d to print these fields. For instance, in raid5_set_cache_size():

pr_err("md/raid:%s: Cannot increase cache size, ret=%d, new_size=%d, min_nr_stripes=%d, max_nr_stripes=%d\n",
       ...
       conf->min_nr_stripes,
       conf->max_nr_stripes);

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 0c5c9fb0606ee..28828e083c2b3 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;

[Severity: Critical]
This is a pre-existing issue, but could the use of sectors_per_chunk
cause problems when passed to rounding macros?

Looking at raid5_bitmap_sector():

raid5_bitmap_sector() {
    ...
    sectors_per_chunk = conf->chunk_sectors *
        (conf->raid_disks - conf->max_degraded);
    start = round_down(start, sectors_per_chunk);
    end = round_up(end, sectors_per_chunk);
    ...
}

The round_down() and round_up() macros are designed specifically for
power-of-two divisors. If the number of data disks is not a power of two,
sectors_per_chunk will not be a power of two, which would yield incorrect
sector boundaries.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1