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

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

[xiakaixu1987]

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.

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);
-
 	/*
 	 * Before anything else, verify that we are correctly operating on
 	 * files or block devices and set the control parameters correctly.
@@ -1730,6 +1722,7 @@ validate_sectorsize(
 	memset(ft, 0, sizeof(*ft));
 	get_topology(cli->xi, ft, force_overwrite);
 
+	/* set configured sector sizes in preparation for checks */
 	if (!cli->sectorsize) {
 		/*
 		 * Unless specified manually on the command line use the
@@ -1759,9 +1752,10 @@ _("specified blocksize %d is less than device physical sector size %d\n"
 				ft->lsectorsize);
 			cfg->sectorsize = ft->lsectorsize;
 		}
+	} else
+		cfg->sectorsize = cli->sectorsize;
 
-		cfg->sectorlog = libxfs_highbit32(cfg->sectorsize);
-	}
+	cfg->sectorlog = libxfs_highbit32(cfg->sectorsize);
 
 	/* validate specified/probed sector size */
 	if (cfg->sectorsize < XFS_MIN_SECTORSIZE ||
-- 
2.20.0

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

[Chaitanya Kulkarni]

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):-

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 3ed7d844..6cbd5d9d 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1690,7 +1690,6 @@ static void
  validate_sectorsize(
         struct mkfs_params      *cfg,
         struct cli_params       *cli,
-       struct mkfs_default_params *dft,
         struct fs_topology      *ft,
         char                    *dfile,
         int                     dry_run,
@@ -3696,8 +3695,7 @@ main(
          * before opening the libxfs devices.
          */
         validate_blocksize(&cfg, &cli, &dft);
-       validate_sectorsize(&cfg, &cli, &dft, &ft, dfile, dry_run,
-                           force_overwrite);
+       validate_sectorsize(&cfg, &cli, &ft, dfile, dry_run, 
force_overwrite);

         /*
          * XXX: we still need to set block size and sector size global 
variables

>   	/*
>   	 * Before anything else, verify that we are correctly operating on
>   	 * files or block devices and set the control parameters correctly.
> @@ -1730,6 +1722,7 @@ validate_sectorsize(
>   	memset(ft, 0, sizeof(*ft));
>   	get_topology(cli->xi, ft, force_overwrite);
>   
> +	/* set configured sector sizes in preparation for checks */
>   	if (!cli->sectorsize) {
>   		/*
>   		 * Unless specified manually on the command line use the
> @@ -1759,9 +1752,10 @@ _("specified blocksize %d is less than device physical sector size %d\n"
>   				ft->lsectorsize);
>   			cfg->sectorsize = ft->lsectorsize;
>   		}
> +	} else
> +		cfg->sectorsize = cli->sectorsize;
>   
> -		cfg->sectorlog = libxfs_highbit32(cfg->sectorsize);
> -	}
> +	cfg->sectorlog = libxfs_highbit32(cfg->sectorsize);
>   
>   	/* validate specified/probed sector size */
>   	if (cfg->sectorsize < XFS_MIN_SECTORSIZE ||
> 

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

[Eric Sandeen]

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.

-Eric
 

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

[kaixuxia]

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). 
Secondly, for the !cli->sectorsize case, the first if statement set cfg->sectorsize
to dft->sectorsize, but the cfg->sectorsize value would be overwrote and set to
ft->psectorsize in the next if statement, so the first if statement is meaningless
and the two if statements can be combined.

> 
> -Eric
>  
> 

-- 
kaixuxia

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

[Dave Chinner]

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