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

> [Medium] unsigned types don't enable right-shift optimization for variable divisions.

You're right -- the hot-path divisions go through
sector_div()/do_div() (already u32),
and sectors_per_chunk is a runtime value, so signedness changes no codegen.
The rationale is wrong and the change has no perf benefit, so I'll
drop this patch
from v2 rather than reword a claim that doesn't hold.

> [Low] unupdated %d specifiers for the now-unsigned fields.
Another reason to drop it -- avoids the %d/%u churn. (They don't warn
in a default
build, only under -Wformat-signedness, but they are inconsistent.)

 > [Critical, pre-existing] round_down/round_up with a non-power-of-two divisor.
Good catch, and independent of this series.
sectors_per_chunk = chunk_sectors * (raid_disks - max_degraded)
isn't a power of two whenever the data-disk count isn't (raid5 4-disk
-> 3 data),
and round_down/round_up are mask-based.
I'll send a separate fix switching to rounddown()/roundup() with a Fixes: tag.

Thanks,

2026年6月24日(水) 9:16 <[email protected]>:
>
> 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



-- 
Hiroshi Nishida
[email protected]