* [PATCH] mkfs: simplify the configured sector sizes setting in validate_sectorsize @ 2020-05-19 6:38 xiakaixu1987 2020-05-19 8:38 ` Chaitanya Kulkarni 0 siblings, 1 reply; 5+ messages in thread From: xiakaixu1987 @ 2020-05-19 6:38 UTC (permalink / raw) To: sandeen; +Cc: linux-xfs, Kaixu Xia 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mkfs: simplify the configured sector sizes setting in validate_sectorsize 2020-05-19 6:38 [PATCH] mkfs: simplify the configured sector sizes setting in validate_sectorsize xiakaixu1987 @ 2020-05-19 8:38 ` Chaitanya Kulkarni 2020-05-19 13:03 ` Eric Sandeen 0 siblings, 1 reply; 5+ messages in thread From: Chaitanya Kulkarni @ 2020-05-19 8:38 UTC (permalink / raw) To: xiakaixu1987@gmail.com, sandeen@sandeen.net Cc: linux-xfs@vger.kernel.org, Kaixu Xia 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 || > ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mkfs: simplify the configured sector sizes setting in validate_sectorsize 2020-05-19 8:38 ` Chaitanya Kulkarni @ 2020-05-19 13:03 ` Eric Sandeen 2020-05-19 14:56 ` kaixuxia 0 siblings, 1 reply; 5+ messages in thread From: Eric Sandeen @ 2020-05-19 13:03 UTC (permalink / raw) To: Chaitanya Kulkarni, xiakaixu1987@gmail.com Cc: linux-xfs@vger.kernel.org, Kaixu Xia 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mkfs: simplify the configured sector sizes setting in validate_sectorsize 2020-05-19 13:03 ` Eric Sandeen @ 2020-05-19 14:56 ` kaixuxia 2020-05-19 21:15 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: kaixuxia @ 2020-05-19 14:56 UTC (permalink / raw) To: Eric Sandeen, Chaitanya Kulkarni; +Cc: linux-xfs@vger.kernel.org, Kaixu Xia 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mkfs: simplify the configured sector sizes setting in validate_sectorsize 2020-05-19 14:56 ` kaixuxia @ 2020-05-19 21:15 ` Dave Chinner 0 siblings, 0 replies; 5+ messages in thread From: Dave Chinner @ 2020-05-19 21:15 UTC (permalink / raw) To: kaixuxia Cc: Eric Sandeen, Chaitanya Kulkarni, linux-xfs@vger.kernel.org, Kaixu Xia 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-05-19 21:15 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-05-19 6:38 [PATCH] mkfs: simplify the configured sector sizes setting in validate_sectorsize xiakaixu1987 2020-05-19 8:38 ` Chaitanya Kulkarni 2020-05-19 13:03 ` Eric Sandeen 2020-05-19 14:56 ` kaixuxia 2020-05-19 21:15 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox