Re: [PATCH] mkfs: simplify the configured sector sizes setting in validate_sectorsize

[Dave Chinner <david@fromorbit.com>]

On Tue, May 19, 2020 at 10:56:01PM +0800, kaixuxia wrote:
> On 2020/5/19 21:03, Eric Sandeen wrote:
> > On 5/19/20 3:38 AM, Chaitanya Kulkarni wrote:
> >> On 5/18/20 11:39 PM, xiakaixu1987@gmail.com wrote:
> >>> From: Kaixu Xia <kaixuxia@tencent.com>
> >>>
> >>> There are two places that set the configured sector sizes in validate_sectorsize,
> >>> actually we can simplify them and combine into one if statement.
> >>> Is it me or patch description seems to be longer than what is in the
> >> tree ?
> >>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> >>> ---
> >>>   mkfs/xfs_mkfs.c | 14 ++++----------
> >>>   1 file changed, 4 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> >>> index 039b1dcc..e1904d57 100644
> >>> --- a/mkfs/xfs_mkfs.c
> >>> +++ b/mkfs/xfs_mkfs.c
> >>> @@ -1696,14 +1696,6 @@ validate_sectorsize(
> >>>   	int			dry_run,
> >>>   	int			force_overwrite)
> >>>   {
> >>> -	/* set configured sector sizes in preparation for checks */
> >>> -	if (!cli->sectorsize) {
> >>> -		cfg->sectorsize = dft->sectorsize;
> >>> -	} else {
> >>> -		cfg->sectorsize = cli->sectorsize;
> >>> -	}
> >>> -	cfg->sectorlog = libxfs_highbit32(cfg->sectorsize);
> >>> -
> >>
> >> If above logic is correct which I've not looked into it, then dft is
> >> not used in validate_sectorsize(), how about something like this on
> >> the top of this this patch (totally untested):-
> > 
> > Honestly if not set via commandline, and probing fails, we should fall
> > back to dft->sectorsize so that all the defaults are still set in one place,
> > i.e. the defaults structure mkfs_default_params.
> 
> The original logic in validate_sectorsize() is:
> 
>   static void 
>   validate_sectorsize(
>     ...
>     if (!cli->sectorsize) {
> 	cfg->sectorsize = dft->sectorsize;
>     } else {
> 	cfg->sectorsize = cli->sectorsize;
>     }
>     ...
>     if (!cli->sectorsize) {
> 	if (!ft->lsectorsize)
> 	   ft->lsectorsize = XFS_MIN_SECTORSIZE;
> 	...
> 	cfg->sectorsize = ft->psectorsize;
> 	...
>     } 
>     ...
>   }
> 
> Firstly, if not set via commandline and probing fails, we will use the
> XFS_MIN_SECTORSIZE (actually equal to dft->sectorsize). 

Which is incorrect - this code should be using dft->sectorsize, not
hard coding a default value. Defaults are set in the default value
structure so they are all configured in one place, not strewn
randomly through the code and hence are impossible to find and
review.

With the added change to use dft->sectorsize, the rest of the patch
is fine.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com