[PATCH] mkfs: Improve warning when AG size is a multiple of stripe width

[PATCH] mkfs: Improve warning when AG size is a multiple of stripe width

[cem]

From: Carlos Maiolino <cmaiolino@redhat.com>

The current output message prints out a suggestion of an AG size to be
used in lieu of the user-defined one.
The problem is this suggestion is printed in filesystem blocks, while
agsize= option receives a size in bytes (or m, g).

This patch tries to make user's life easier by outputing the suggesting
in bytes directly.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 mkfs/xfs_mkfs.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index d3a15cf44..827d5b656 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3179,9 +3179,11 @@ _("agsize rounded to %lld, sunit = %d\n"),
 		if (cli_opt_set(&dopts, D_AGCOUNT) ||
 		    cli_opt_set(&dopts, D_AGSIZE)) {
 			printf(_(
-"Warning: AG size is a multiple of stripe width.  This can cause performance\n\
-problems by aligning all AGs on the same disk.  To avoid this, run mkfs with\n\
-an AG size that is one stripe unit smaller or larger, for example %llu.\n"),
+"Warning: AG size is a multiple of stripe width. This can cause performance\n\
+problems by aligning all AGs on the same disk. To avoid this, run mkfs with\n\
+an AG size that is one stripe unit smaller or larger,\n\
+for example: agsize=%llu (%llu blks).\n"),
+				(unsigned long long)((cfg->agsize - dsunit) * cfg->blocksize),
 				(unsigned long long)cfg->agsize - dsunit);
 			fflush(stdout);
 			goto validate;
-- 
2.39.2

Re: [PATCH] mkfs: Improve warning when AG size is a multiple of stripe width

[Pavel Reichl]

LGTM

Signed-off-by: Pavel Reichl <preichl@redhat.com>


On Thu, Sep 14, 2023 at 2:38 PM <cem@kernel.org> wrote:
>
> From: Carlos Maiolino <cmaiolino@redhat.com>
>
> The current output message prints out a suggestion of an AG size to be
> used in lieu of the user-defined one.
> The problem is this suggestion is printed in filesystem blocks, while
> agsize= option receives a size in bytes (or m, g).
>
> This patch tries to make user's life easier by outputing the suggesting
> in bytes directly.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index d3a15cf44..827d5b656 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3179,9 +3179,11 @@ _("agsize rounded to %lld, sunit = %d\n"),
>                 if (cli_opt_set(&dopts, D_AGCOUNT) ||
>                     cli_opt_set(&dopts, D_AGSIZE)) {
>                         printf(_(
> -"Warning: AG size is a multiple of stripe width.  This can cause performance\n\
> -problems by aligning all AGs on the same disk.  To avoid this, run mkfs with\n\
> -an AG size that is one stripe unit smaller or larger, for example %llu.\n"),
> +"Warning: AG size is a multiple of stripe width. This can cause performance\n\
> +problems by aligning all AGs on the same disk. To avoid this, run mkfs with\n\
> +an AG size that is one stripe unit smaller or larger,\n\
> +for example: agsize=%llu (%llu blks).\n"),
> +                               (unsigned long long)((cfg->agsize - dsunit) * cfg->blocksize),
>                                 (unsigned long long)cfg->agsize - dsunit);
>                         fflush(stdout);
>                         goto validate;
> --
> 2.39.2
>

Re: [PATCH] mkfs: Improve warning when AG size is a multiple of stripe width

[Bill O'Donnell]

On Thu, Sep 14, 2023 at 02:36:40PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cmaiolino@redhat.com>
> 
> The current output message prints out a suggestion of an AG size to be
> used in lieu of the user-defined one.
> The problem is this suggestion is printed in filesystem blocks, while
> agsize= option receives a size in bytes (or m, g).
> 
> This patch tries to make user's life easier by outputing the suggesting
> in bytes directly.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>

> ---
>  mkfs/xfs_mkfs.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index d3a15cf44..827d5b656 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3179,9 +3179,11 @@ _("agsize rounded to %lld, sunit = %d\n"),
>  		if (cli_opt_set(&dopts, D_AGCOUNT) ||
>  		    cli_opt_set(&dopts, D_AGSIZE)) {
>  			printf(_(
> -"Warning: AG size is a multiple of stripe width.  This can cause performance\n\
> -problems by aligning all AGs on the same disk.  To avoid this, run mkfs with\n\
> -an AG size that is one stripe unit smaller or larger, for example %llu.\n"),
> +"Warning: AG size is a multiple of stripe width. This can cause performance\n\
> +problems by aligning all AGs on the same disk. To avoid this, run mkfs with\n\
> +an AG size that is one stripe unit smaller or larger,\n\
> +for example: agsize=%llu (%llu blks).\n"),
> +				(unsigned long long)((cfg->agsize - dsunit) * cfg->blocksize),
>  				(unsigned long long)cfg->agsize - dsunit);
>  			fflush(stdout);
>  			goto validate;
> -- 
> 2.39.2
> 

Re: [PATCH] mkfs: Improve warning when AG size is a multiple of stripe width

[Dave Chinner]

On Thu, Sep 14, 2023 at 02:36:40PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cmaiolino@redhat.com>
> 
> The current output message prints out a suggestion of an AG size to be
> used in lieu of the user-defined one.
> The problem is this suggestion is printed in filesystem blocks, while
> agsize= option receives a size in bytes (or m, g).

Hmmm. The actual definition of the parameter in mkfs.xfs has
".convert = true", which means it will take a value in filesystem
blocks by appending "b" to the number.

i.e. anything that is defined as a "value" with that supports a
suffix like "m" or "g" requires conversion via cvtnum() to turn it
into a byte-based units will also support suffixes for block and
sector size units.

Hence "-d agsize=10000b" will make an AG of 10000 filesystem blocks
in size, or 40000kB in size if the block size 4kB.... 

# mkfs.xfs -f -dagsize=100000b /dev/vdb
meta-data=/dev/vdb               isize=512    agcount=42, agsize=100000 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=1    bigtime=1 inobtcount=1 nrext64=0
data     =                       bsize=4096   blocks=4194304, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
Discarding blocks...Done.
#

> This patch tries to make user's life easier by outputing the suggesting
> in bytes directly.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index d3a15cf44..827d5b656 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3179,9 +3179,11 @@ _("agsize rounded to %lld, sunit = %d\n"),
>  		if (cli_opt_set(&dopts, D_AGCOUNT) ||
>  		    cli_opt_set(&dopts, D_AGSIZE)) {
>  			printf(_(
> -"Warning: AG size is a multiple of stripe width.  This can cause performance\n\
> -problems by aligning all AGs on the same disk.  To avoid this, run mkfs with\n\
> -an AG size that is one stripe unit smaller or larger, for example %llu.\n"),
> +"Warning: AG size is a multiple of stripe width. This can cause performance\n\
> +problems by aligning all AGs on the same disk. To avoid this, run mkfs with\n\
> +an AG size that is one stripe unit smaller or larger,\n\
> +for example: agsize=%llu (%llu blks).\n"),
> +				(unsigned long long)((cfg->agsize - dsunit) * cfg->blocksize),
>  				(unsigned long long)cfg->agsize - dsunit);
>  			fflush(stdout);
>  			goto validate;

I have no problem with the change, though I think the man page needs
updating to remove the "takes a value in bytes" because it has taken
a value that supports all the suffix types the man page defines
for quite a long time....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] mkfs: Improve warning when AG size is a multiple of stripe width

[Carlos Maiolino]

On Fri, Sep 15, 2023 at 07:45:11AM +1000, Dave Chinner wrote:
> On Thu, Sep 14, 2023 at 02:36:40PM +0200, cem@kernel.org wrote:
> > From: Carlos Maiolino <cmaiolino@redhat.com>
> >
> > The current output message prints out a suggestion of an AG size to be
> > used in lieu of the user-defined one.
> > The problem is this suggestion is printed in filesystem blocks, while
> > agsize= option receives a size in bytes (or m, g).
> 
> Hmmm. The actual definition of the parameter in mkfs.xfs has
> ".convert = true", which means it will take a value in filesystem
> blocks by appending "b" to the number.
> 
> i.e. anything that is defined as a "value" with that supports a
> suffix like "m" or "g" requires conversion via cvtnum() to turn it
> into a byte-based units will also support suffixes for block and
> sector size units.
> 
> Hence "-d agsize=10000b" will make an AG of 10000 filesystem blocks
> in size, or 40000kB in size if the block size 4kB....

Hah, I actually didn't realize the b suffix, knowing that now, I believe
we can just replace the message by:

"for example: agsize=%llub\n")".

I'd rather have it printed in FSblocks other than bytes anyway, giving the size
of the final number. I just opted for bytes originally, to avoid adding more
code just for calculating the representation on a bigger unit.

I'll send a V2.

Carlos

> 
> # mkfs.xfs -f -dagsize=100000b /dev/vdb
> meta-data=/dev/vdb               isize=512    agcount=42, agsize=100000 blks
>          =                       sectsz=512   attr=2, projid32bit=1
>          =                       crc=1        finobt=1, sparse=1, rmapbt=0
>          =                       reflink=1    bigtime=1 inobtcount=1 nrext64=0
> data     =                       bsize=4096   blocks=4194304, imaxpct=25
>          =                       sunit=0      swidth=0 blks
> naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
> log      =internal log           bsize=4096   blocks=16384, version=2
>          =                       sectsz=512   sunit=0 blks, lazy-count=1
> realtime =none                   extsz=4096   blocks=0, rtextents=0
> Discarding blocks...Done.
> #
> 
> > This patch tries to make user's life easier by outputing the suggesting
> > in bytes directly.
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  mkfs/xfs_mkfs.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index d3a15cf44..827d5b656 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3179,9 +3179,11 @@ _("agsize rounded to %lld, sunit = %d\n"),
> >  		if (cli_opt_set(&dopts, D_AGCOUNT) ||
> >  		    cli_opt_set(&dopts, D_AGSIZE)) {
> >  			printf(_(
> > -"Warning: AG size is a multiple of stripe width.  This can cause performance\n\
> > -problems by aligning all AGs on the same disk.  To avoid this, run mkfs with\n\
> > -an AG size that is one stripe unit smaller or larger, for example %llu.\n"),
> > +"Warning: AG size is a multiple of stripe width. This can cause performance\n\
> > +problems by aligning all AGs on the same disk. To avoid this, run mkfs with\n\
> > +an AG size that is one stripe unit smaller or larger,\n\
> > +for example: agsize=%llu (%llu blks).\n"),
> > +				(unsigned long long)((cfg->agsize - dsunit) * cfg->blocksize),
> >  				(unsigned long long)cfg->agsize - dsunit);
> >  			fflush(stdout);
> >  			goto validate;
> 
> I have no problem with the change, though I think the man page needs
> updating to remove the "takes a value in bytes" because it has taken
> a value that supports all the suffix types the man page defines
> for quite a long time....
> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com