[dm-devel] [PATCH] md: change mddev 'chunk_sectors' from int to unsigned

[Song Liu]

> On Dec 12, 2020, at 8:55 AM, Mike Snitzer <snitzer at redhat.com> wrote:
> 
> Commit e2782f560c29 ("Revert "dm raid: remove unnecessary discard
> limits for raid10"") exposed compiler warnings introduced by commit
> e0910c8e4f87 ("dm raid: fix discard limits for raid1 and raid10"):
> 
> In file included from ./include/linux/kernel.h:14,
>                 from ./include/asm-generic/bug.h:20,
>                 from ./arch/x86/include/asm/bug.h:93,
>                 from ./include/linux/bug.h:5,
>                 from ./include/linux/mmdebug.h:5,
>                 from ./include/linux/gfp.h:5,
>                 from ./include/linux/slab.h:15,
>                 from drivers/md/dm-raid.c:8:
> drivers/md/dm-raid.c: In function ‘raid_io_hints’:
> ./include/linux/minmax.h:18:28: warning: comparison of distinct pointer types lacks a cast
>  (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>                            ^~
> ./include/linux/minmax.h:32:4: note: in expansion of macro ‘__typecheck’
>   (__typecheck(x, y) && __no_side_effects(x, y))
>    ^~~~~~~~~~~
> ./include/linux/minmax.h:42:24: note: in expansion of macro ‘__safe_cmp’
>  __builtin_choose_expr(__safe_cmp(x, y), \
>                        ^~~~~~~~~~
> ./include/linux/minmax.h:51:19: note: in expansion of macro ‘__careful_cmp’
> #define min(x, y) __careful_cmp(x, y, <)
>                   ^~~~~~~~~~~~~
> ./include/linux/minmax.h:84:39: note: in expansion of macro ‘min’
>  __x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
>                                       ^~~
> drivers/md/dm-raid.c:3739:33: note: in expansion of macro ‘min_not_zero’
>   limits->max_discard_sectors = min_not_zero(rs->md.chunk_sectors,
>                                 ^~~~~~~~~~~~
> 
> Fix this by changing the chunk_sectors member of 'struct mddev' from
> int to 'unsigned int' to match the type used for the 'chunk_sectors'
> member of 'struct queue_limits'.  Various MD code still uses 'int' but
> none of it appears to ever make use of signed int; and storing
> positive signed int in unsigned is perfectly safe.

Thanks for the quick fix and thorough analysis. I also checked MD code 
and didn't see any use of negative chunk_sectors, so this change is safe.
I will convert the rest use of signed chunk_sectors in 5.11. 

> 
> Reported-by: Song Liu <songliubraving at fb.com>
> Fixes: e2782f560c29 ("Revert "dm raid: remove unnecessary discard limits for raid10"")
> Fixes: e0910c8e4f87 ("dm raid: fix discard limits for raid1 and raid10")
> Cc: stable at vger,kernel.org # e0910c8e4f87 was marked for stable@
> Signed-off-by: Mike Snitzer <snitzer at redhat.com>

Reviewed-by: Song Liu <song at kernel.org>

> ---
> drivers/md/md.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 2175a5ac4f7c..bb645bc3ba6d 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -311,7 +311,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];
> @@ -339,7 +339,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		*thread;	/* management thread */
> -- 
> 2.15.0
>