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@xxxxxxxxx wrote:
>>> From: Kaixu Xia <kaixuxia@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxx>
>>> ---
>>> 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
