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

[kaixuxia <xiakaixu1987@gmail.com>]

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