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

[PATCH V2] 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, without
specifying the suffix to be used.

This patch tries to make user's life easier by outputing the option as
it should be used by the mkfs, so users can just copy/paste it.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---

V2:
	- Keep printing it in FSBs, just add the agsize= and the 'b' suffix at
	  the end

 mkfs/xfs_mkfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index d3a15cf44..b61934e57 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3179,9 +3179,9 @@ _("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, for example: agsize=%llub\n"),
 				(unsigned long long)cfg->agsize - dsunit);
 			fflush(stdout);
 			goto validate;
-- 
2.39.2

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

[Bill O'Donnell]

On Fri, Sep 15, 2023 at 12:22:46PM +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, without
> specifying the suffix to be used.
> 
> This patch tries to make user's life easier by outputing the option as
> it should be used by the mkfs, so users can just copy/paste it.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

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

> ---
> 
> V2:
> 	- Keep printing it in FSBs, just add the agsize= and the 'b' suffix at
> 	  the end
> 
>  mkfs/xfs_mkfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index d3a15cf44..b61934e57 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3179,9 +3179,9 @@ _("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, for example: agsize=%llub\n"),
>  				(unsigned long long)cfg->agsize - dsunit);
>  			fflush(stdout);
>  			goto validate;
> -- 
> 2.39.2
> 

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

[Dave Chinner]

On Fri, Sep 15, 2023 at 12:22:46PM +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, without
> specifying the suffix to be used.
> 
> This patch tries to make user's life easier by outputing the option as
> it should be used by the mkfs, so users can just copy/paste it.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

I'm fine with this but, as I said in my previous comment, a man page
update for agsize is also needed. Something like this:

	agsize=value
		This is an alternative to using the agcount
		suboption. The value is the desired size of the
-		allocation group expressed in bytes (usually using
-		the m or g suffixes).  This value must be a multiple
-		of the filesystem block size, and must be at  least
+		allocation group expressed as a multiple of the
+		filesystem block size.  It must be at  least
		16MiB, and no more than 1TiB, and may be
		automatically adjusted to properly align with the
		stripe geometry.  The agcount and agsize suboptions
		are mutually exclusive.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Carlos Maiolino]

On Mon, Sep 18, 2023 at 08:52:49AM +1000, Dave Chinner wrote:
> On Fri, Sep 15, 2023 at 12:22:46PM +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, without
> > specifying the suffix to be used.
> >
> > This patch tries to make user's life easier by outputing the option as
> > it should be used by the mkfs, so users can just copy/paste it.
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
> I'm fine with this but, as I said in my previous comment, a man page
> update for agsize is also needed. Something like this:

Thanks, yes I am planning for the manpage update, I just wanted to send the
message fix first before going into the discussion of the manpage, I'll try to
prepare a patch for it today.

> 
> 	agsize=value
> 		This is an alternative to using the agcount
> 		suboption. The value is the desired size of the
> -		allocation group expressed in bytes (usually using
> -		the m or g suffixes).  This value must be a multiple
> -		of the filesystem block size, and must be at  least
> +		allocation group expressed as a multiple of the
> +		filesystem block size.  It must be at  least
> 		16MiB, and no more than 1TiB, and may be
> 		automatically adjusted to properly align with the
> 		stripe geometry.  The agcount and agsize suboptions
> 		are mutually exclusive.

Thanks for the suggestion.

Carlos

> 
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com