[PATCH 0/4] xfsprogs: misc small fixes

[PATCH 0/4] xfsprogs: misc small fixes

[Eric Sandeen]

A handful of minor updates after encountering poor documentation
and minor bugs while dogfooding and reading user questions.

[PATCH 1/4] xfs_quota: document unit multipliers used in limit command

[Eric Sandeen]

From: Eric Sandeen <sandeen@redhat.com>

The units used to set limits are never specified in the xfs_quota
man page, and in fact for block limits, the standard k/m/g/...
units are accepted. Document all of this.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---
 man/man8/xfs_quota.8 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/man/man8/xfs_quota.8 b/man/man8/xfs_quota.8
index 59e603f..f841e3f 100644
--- a/man/man8/xfs_quota.8
+++ b/man/man8/xfs_quota.8
@@ -446,7 +446,13 @@ option reports state on all filesystems and not just the current path.
 .I name
 .br
 Set quota block limits (bhard/bsoft), inode count limits (ihard/isoft)
-and/or realtime block limits (rtbhard/rtbsoft). The
+and/or realtime block limits (rtbhard/rtbsoft) to N, where N is a bare
+number representing bytes or inodes.
+For block limits, a number with a s/b/k/m/g/t/p/e multiplication suffix
+as described in
+.BR mkfs.xfs (8)
+is also accepted.
+The
 .B \-d
 option (defaults) can be used to set the default value
 that will be used, otherwise a specific
-- 
1.8.3.1

[PATCH 2/4] mkfs.xfs(8): remove incorrect default inode allocator description

[Eric Sandeen]

From: Eric Sandeen <sandeen@redhat.com>

The "maxpct" section of the mkfs.xfs manpage has a gratuitous and
incorrect description of the default inode allocator mode.

inode64 has been the default since 2012, as of

08bf540412ed xfs: make inode64 as the default allocation mode

so the description is wrong. In addition, imaxpct is only
tangentially related to inode allocator behavior, so this section
of the man page is really the wrong place for discussion.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---
 man/man8/mkfs.xfs.8 | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index a7f7028..a67532a 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -568,22 +568,15 @@ can be allocated to inodes. The default
 is 25% for filesystems under 1TB, 5% for filesystems under 50TB and 1%
 for filesystems over 50TB.
 .IP
-In the default inode allocation mode, inode blocks are chosen such
-that inode numbers will not exceed 32 bits, which restricts the inode
-blocks to the lower portion of the filesystem. The data block
-allocator will avoid these low blocks to accommodate the specified
-maxpct, so a high value may result in a filesystem with nothing but
-inodes in a significant portion of the lower blocks of the filesystem.
-(This restriction is not present when the filesystem is mounted with
-the
-.I "inode64"
-option on 64-bit platforms).
-.IP
 Setting the value to 0 means that essentially all of the filesystem
-can become inode blocks, subject to inode32 restrictions.
+can become inode blocks (subject to possible
+.B inode32
+mount option restrictions, see
+.BR xfs (5)
+for details.)
 .IP
 This value can be modified with
-.IR xfs_growfs(8) .
+.BR xfs_growfs (8).
 .TP
 .BI align[= value ]
 This is used to specify that inode allocation is or is not aligned. The
-- 
1.8.3.1

[PATCH 3/4] xfs_quota: don't exit on fs_table_insert_project_path failure

[Eric Sandeen]

From: Eric Sandeen <sandeen@redhat.com>

If "project -p" fails in fs_table_insert_project_path, it
calls exit() today which is quite unfriendly. Return an error
and return to the command prompt as expected.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---
 libfrog/paths.c | 7 +++----
 libfrog/paths.h | 2 +-
 quota/project.c | 4 +++-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/libfrog/paths.c b/libfrog/paths.c
index d679376..6c0fee2 100644
--- a/libfrog/paths.c
+++ b/libfrog/paths.c
@@ -546,7 +546,7 @@ out_error:
 		progname, strerror(error));
 }
 
-void
+int
 fs_table_insert_project_path(
 	char		*dir,
 	prid_t		prid)
@@ -561,9 +561,8 @@ fs_table_insert_project_path(
 	else
 		error = ENOENT;
 
-	if (error) {
+	if (error)
 		fprintf(stderr, _("%s: cannot setup path for project dir %s: %s\n"),
 				progname, dir, strerror(error));
-		exit(1);
-	}
+	return error;
 }
diff --git a/libfrog/paths.h b/libfrog/paths.h
index c08e373..f20a2c3 100644
--- a/libfrog/paths.h
+++ b/libfrog/paths.h
@@ -40,7 +40,7 @@ extern char *mtab_file;
 extern void fs_table_initialise(int, char *[], int, char *[]);
 extern void fs_table_destroy(void);
 
-extern void fs_table_insert_project_path(char *__dir, uint __projid);
+extern int fs_table_insert_project_path(char *__dir, uint __projid);
 
 
 extern fs_path_t *fs_table_lookup(const char *__dir, uint __flags);
diff --git a/quota/project.c b/quota/project.c
index 03ae10d..bed0dc5 100644
--- a/quota/project.c
+++ b/quota/project.c
@@ -281,7 +281,9 @@ project_f(
 			break;
 		case 'p':
 			ispath = 1;
-			fs_table_insert_project_path(optarg, -1);
+			/* fs_table_insert_project_path prints the failure */
+			if (fs_table_insert_project_path(optarg, -1))
+				return 0;
 			break;
 		case 's':
 			type = SETUP_PROJECT;
-- 
1.8.3.1

[PATCH 4/4] xfs_repair: don't guess about failure reason in phase6

[Eric Sandeen]

From: Eric Sandeen <sandeen@redhat.com>

There are many error messages in phase 6 which say
"filesystem may be out of space," when in reality the failure could
have been corruption or some other issue.  Rather than guessing, and
emitting a confusing and possibly-wrong message, use the existing
res_failed() for any xfs_trans_alloc failures, and simply print the
error number in the other cases.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---
 repair/phase6.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index 696a642..df22daa 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1067,9 +1067,7 @@ mv_orphanage(
 			err = -libxfs_trans_alloc(mp, &M_RES(mp)->tr_rename,
 						  nres, 0, 0, &tp);
 			if (err)
-				do_error(
-	_("space reservation failed (%d), filesystem may be out of space\n"),
-					err);
+				res_failed(err);
 
 			libxfs_trans_ijoin(tp, orphanage_ip, 0);
 			libxfs_trans_ijoin(tp, ino_p, 0);
@@ -1078,8 +1076,7 @@ mv_orphanage(
 						ino, nres);
 			if (err)
 				do_error(
-	_("name create failed in %s (%d), filesystem may be out of space\n"),
-					ORPHANAGE, err);
+	_("name create failed in %s (%d)\n"), ORPHANAGE, err);
 
 			if (irec)
 				add_inode_ref(irec, ino_offset);
@@ -1091,8 +1088,7 @@ mv_orphanage(
 					orphanage_ino, nres);
 			if (err)
 				do_error(
-	_("creation of .. entry failed (%d), filesystem may be out of space\n"),
-					err);
+	_("creation of .. entry failed (%d)\n"), err);
 
 			inc_nlink(VFS_I(ino_p));
 			libxfs_trans_log_inode(tp, ino_p, XFS_ILOG_CORE);
@@ -1104,9 +1100,7 @@ mv_orphanage(
 			err = -libxfs_trans_alloc(mp, &M_RES(mp)->tr_rename,
 						  nres, 0, 0, &tp);
 			if (err)
-				do_error(
-	_("space reservation failed (%d), filesystem may be out of space\n"),
-					err);
+				res_failed(err);
 
 			libxfs_trans_ijoin(tp, orphanage_ip, 0);
 			libxfs_trans_ijoin(tp, ino_p, 0);
@@ -1116,8 +1110,7 @@ mv_orphanage(
 						ino, nres);
 			if (err)
 				do_error(
-	_("name create failed in %s (%d), filesystem may be out of space\n"),
-					ORPHANAGE, err);
+	_("name create failed in %s (%d)\n"), ORPHANAGE, err);
 
 			if (irec)
 				add_inode_ref(irec, ino_offset);
@@ -1135,8 +1128,7 @@ mv_orphanage(
 						nres);
 				if (err)
 					do_error(
-	_("name replace op failed (%d), filesystem may be out of space\n"),
-						err);
+	_("name replace op failed (%d)\n"), err);
 			}
 
 			err = -libxfs_trans_commit(tp);
@@ -1156,9 +1148,7 @@ mv_orphanage(
 		err = -libxfs_trans_alloc(mp, &M_RES(mp)->tr_remove,
 					  nres, 0, 0, &tp);
 		if (err)
-			do_error(
-	_("space reservation failed (%d), filesystem may be out of space\n"),
-				err);
+			res_failed(err);
 
 		libxfs_trans_ijoin(tp, orphanage_ip, 0);
 		libxfs_trans_ijoin(tp, ino_p, 0);
@@ -1167,8 +1157,7 @@ mv_orphanage(
 						nres);
 		if (err)
 			do_error(
-	_("name create failed in %s (%d), filesystem may be out of space\n"),
-				ORPHANAGE, err);
+	_("name create failed in %s (%d)\n"), ORPHANAGE, err);
 		ASSERT(err == 0);
 
 		set_nlink(VFS_I(ino_p), 1);
@@ -1351,8 +1340,7 @@ longform_dir2_rebuild(
 						nres);
 		if (error) {
 			do_warn(
-_("name create failed in ino %" PRIu64 " (%d), filesystem may be out of space\n"),
-				ino, error);
+_("name create failed in ino %" PRIu64 " (%d)\n"), ino, error);
 			goto out_bmap_cancel;
 		}
 
-- 
1.8.3.1

Re: [PATCH 1/4] xfs_quota: document unit multipliers used in limit command

[Darrick J. Wong]

On Fri, Dec 10, 2021 at 02:21:34PM -0600, Eric Sandeen wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> The units used to set limits are never specified in the xfs_quota
> man page, and in fact for block limits, the standard k/m/g/...
> units are accepted. Document all of this.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> ---
>  man/man8/xfs_quota.8 | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/man/man8/xfs_quota.8 b/man/man8/xfs_quota.8
> index 59e603f..f841e3f 100644
> --- a/man/man8/xfs_quota.8
> +++ b/man/man8/xfs_quota.8
> @@ -446,7 +446,13 @@ option reports state on all filesystems and not just the current path.
>  .I name
>  .br
>  Set quota block limits (bhard/bsoft), inode count limits (ihard/isoft)
> -and/or realtime block limits (rtbhard/rtbsoft). The
> +and/or realtime block limits (rtbhard/rtbsoft) to N, where N is a bare

What is a 'bare' number?

How about (shortened so I don't have to retype the whole thing):

"Set quota block limits...to N.  For block limits, N is a number
with a s/b/k/m/g/t/p/e multiplication suffix..."

"For inode limits, N is a bare number; no suffixes are allowed."

?

--D

> +number representing bytes or inodes.
> +For block limits, a number with a s/b/k/m/g/t/p/e multiplication suffix
> +as described in
> +.BR mkfs.xfs (8)
> +is also accepted.
> +The
>  .B \-d
>  option (defaults) can be used to set the default value
>  that will be used, otherwise a specific
> -- 
> 1.8.3.1
> 

Re: [PATCH 2/4] mkfs.xfs(8): remove incorrect default inode allocator description

[Darrick J. Wong]

On Fri, Dec 10, 2021 at 02:21:35PM -0600, Eric Sandeen wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> The "maxpct" section of the mkfs.xfs manpage has a gratuitous and
> incorrect description of the default inode allocator mode.
> 
> inode64 has been the default since 2012, as of
> 
> 08bf540412ed xfs: make inode64 as the default allocation mode
> 
> so the description is wrong. In addition, imaxpct is only
> tangentially related to inode allocator behavior, so this section
> of the man page is really the wrong place for discussion.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

Looks ok,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  man/man8/mkfs.xfs.8 | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index a7f7028..a67532a 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -568,22 +568,15 @@ can be allocated to inodes. The default
>  is 25% for filesystems under 1TB, 5% for filesystems under 50TB and 1%
>  for filesystems over 50TB.
>  .IP
> -In the default inode allocation mode, inode blocks are chosen such
> -that inode numbers will not exceed 32 bits, which restricts the inode
> -blocks to the lower portion of the filesystem. The data block
> -allocator will avoid these low blocks to accommodate the specified
> -maxpct, so a high value may result in a filesystem with nothing but
> -inodes in a significant portion of the lower blocks of the filesystem.
> -(This restriction is not present when the filesystem is mounted with
> -the
> -.I "inode64"
> -option on 64-bit platforms).
> -.IP
>  Setting the value to 0 means that essentially all of the filesystem
> -can become inode blocks, subject to inode32 restrictions.
> +can become inode blocks (subject to possible
> +.B inode32
> +mount option restrictions, see
> +.BR xfs (5)
> +for details.)
>  .IP
>  This value can be modified with
> -.IR xfs_growfs(8) .
> +.BR xfs_growfs (8).
>  .TP
>  .BI align[= value ]
>  This is used to specify that inode allocation is or is not aligned. The
> -- 
> 1.8.3.1
> 

Re: [PATCH 3/4] xfs_quota: don't exit on fs_table_insert_project_path failure

[Darrick J. Wong]

On Fri, Dec 10, 2021 at 02:21:36PM -0600, Eric Sandeen wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> If "project -p" fails in fs_table_insert_project_path, it
> calls exit() today which is quite unfriendly. Return an error
> and return to the command prompt as expected.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> ---
>  libfrog/paths.c | 7 +++----
>  libfrog/paths.h | 2 +-
>  quota/project.c | 4 +++-
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/libfrog/paths.c b/libfrog/paths.c
> index d679376..6c0fee2 100644
> --- a/libfrog/paths.c
> +++ b/libfrog/paths.c
> @@ -546,7 +546,7 @@ out_error:
>  		progname, strerror(error));
>  }
>  
> -void
> +int
>  fs_table_insert_project_path(
>  	char		*dir,
>  	prid_t		prid)
> @@ -561,9 +561,8 @@ fs_table_insert_project_path(
>  	else
>  		error = ENOENT;
>  
> -	if (error) {
> +	if (error)
>  		fprintf(stderr, _("%s: cannot setup path for project dir %s: %s\n"),
>  				progname, dir, strerror(error));

Why not move this to the (sole) caller?  Libraries (even pseudolibraries
like libfrog) usually aren't supposed to go around fprintfing things.

--D

> -		exit(1);
> -	}
> +	return error;
>  }
> diff --git a/libfrog/paths.h b/libfrog/paths.h
> index c08e373..f20a2c3 100644
> --- a/libfrog/paths.h
> +++ b/libfrog/paths.h
> @@ -40,7 +40,7 @@ extern char *mtab_file;
>  extern void fs_table_initialise(int, char *[], int, char *[]);
>  extern void fs_table_destroy(void);
>  
> -extern void fs_table_insert_project_path(char *__dir, uint __projid);
> +extern int fs_table_insert_project_path(char *__dir, uint __projid);
>  
>  
>  extern fs_path_t *fs_table_lookup(const char *__dir, uint __flags);
> diff --git a/quota/project.c b/quota/project.c
> index 03ae10d..bed0dc5 100644
> --- a/quota/project.c
> +++ b/quota/project.c
> @@ -281,7 +281,9 @@ project_f(
>  			break;
>  		case 'p':
>  			ispath = 1;
> -			fs_table_insert_project_path(optarg, -1);
> +			/* fs_table_insert_project_path prints the failure */
> +			if (fs_table_insert_project_path(optarg, -1))
> +				return 0;
>  			break;
>  		case 's':
>  			type = SETUP_PROJECT;
> -- 
> 1.8.3.1
> 

Re: [PATCH 4/4] xfs_repair: don't guess about failure reason in phase6

[Darrick J. Wong]

On Fri, Dec 10, 2021 at 02:21:37PM -0600, Eric Sandeen wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> There are many error messages in phase 6 which say
> "filesystem may be out of space," when in reality the failure could
> have been corruption or some other issue.  Rather than guessing, and
> emitting a confusing and possibly-wrong message, use the existing
> res_failed() for any xfs_trans_alloc failures, and simply print the
> error number in the other cases.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  repair/phase6.c | 30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 696a642..df22daa 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1067,9 +1067,7 @@ mv_orphanage(
>  			err = -libxfs_trans_alloc(mp, &M_RES(mp)->tr_rename,
>  						  nres, 0, 0, &tp);
>  			if (err)
> -				do_error(
> -	_("space reservation failed (%d), filesystem may be out of space\n"),
> -					err);
> +				res_failed(err);
>  
>  			libxfs_trans_ijoin(tp, orphanage_ip, 0);
>  			libxfs_trans_ijoin(tp, ino_p, 0);
> @@ -1078,8 +1076,7 @@ mv_orphanage(
>  						ino, nres);
>  			if (err)
>  				do_error(
> -	_("name create failed in %s (%d), filesystem may be out of space\n"),
> -					ORPHANAGE, err);
> +	_("name create failed in %s (%d)\n"), ORPHANAGE, err);
>  
>  			if (irec)
>  				add_inode_ref(irec, ino_offset);
> @@ -1091,8 +1088,7 @@ mv_orphanage(
>  					orphanage_ino, nres);
>  			if (err)
>  				do_error(
> -	_("creation of .. entry failed (%d), filesystem may be out of space\n"),
> -					err);
> +	_("creation of .. entry failed (%d)\n"), err);
>  
>  			inc_nlink(VFS_I(ino_p));
>  			libxfs_trans_log_inode(tp, ino_p, XFS_ILOG_CORE);
> @@ -1104,9 +1100,7 @@ mv_orphanage(
>  			err = -libxfs_trans_alloc(mp, &M_RES(mp)->tr_rename,
>  						  nres, 0, 0, &tp);
>  			if (err)
> -				do_error(
> -	_("space reservation failed (%d), filesystem may be out of space\n"),
> -					err);
> +				res_failed(err);
>  
>  			libxfs_trans_ijoin(tp, orphanage_ip, 0);
>  			libxfs_trans_ijoin(tp, ino_p, 0);
> @@ -1116,8 +1110,7 @@ mv_orphanage(
>  						ino, nres);
>  			if (err)
>  				do_error(
> -	_("name create failed in %s (%d), filesystem may be out of space\n"),
> -					ORPHANAGE, err);
> +	_("name create failed in %s (%d)\n"), ORPHANAGE, err);
>  
>  			if (irec)
>  				add_inode_ref(irec, ino_offset);
> @@ -1135,8 +1128,7 @@ mv_orphanage(
>  						nres);
>  				if (err)
>  					do_error(
> -	_("name replace op failed (%d), filesystem may be out of space\n"),
> -						err);
> +	_("name replace op failed (%d)\n"), err);
>  			}
>  
>  			err = -libxfs_trans_commit(tp);
> @@ -1156,9 +1148,7 @@ mv_orphanage(
>  		err = -libxfs_trans_alloc(mp, &M_RES(mp)->tr_remove,
>  					  nres, 0, 0, &tp);
>  		if (err)
> -			do_error(
> -	_("space reservation failed (%d), filesystem may be out of space\n"),
> -				err);
> +			res_failed(err);
>  
>  		libxfs_trans_ijoin(tp, orphanage_ip, 0);
>  		libxfs_trans_ijoin(tp, ino_p, 0);
> @@ -1167,8 +1157,7 @@ mv_orphanage(
>  						nres);
>  		if (err)
>  			do_error(
> -	_("name create failed in %s (%d), filesystem may be out of space\n"),
> -				ORPHANAGE, err);
> +	_("name create failed in %s (%d)\n"), ORPHANAGE, err);
>  		ASSERT(err == 0);
>  
>  		set_nlink(VFS_I(ino_p), 1);
> @@ -1351,8 +1340,7 @@ longform_dir2_rebuild(
>  						nres);
>  		if (error) {
>  			do_warn(
> -_("name create failed in ino %" PRIu64 " (%d), filesystem may be out of space\n"),
> -				ino, error);
> +_("name create failed in ino %" PRIu64 " (%d)\n"), ino, error);
>  			goto out_bmap_cancel;
>  		}
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH 1/4] xfs_quota: document unit multipliers used in limit command

[Eric Sandeen]

On 12/10/21 6:15 PM, Darrick J. Wong wrote:
> On Fri, Dec 10, 2021 at 02:21:34PM -0600, Eric Sandeen wrote:
>> From: Eric Sandeen <sandeen@redhat.com>
>>
>> The units used to set limits are never specified in the xfs_quota
>> man page, and in fact for block limits, the standard k/m/g/...
>> units are accepted. Document all of this.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
>> ---
>>   man/man8/xfs_quota.8 | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/man/man8/xfs_quota.8 b/man/man8/xfs_quota.8
>> index 59e603f..f841e3f 100644
>> --- a/man/man8/xfs_quota.8
>> +++ b/man/man8/xfs_quota.8
>> @@ -446,7 +446,13 @@ option reports state on all filesystems and not just the current path.
>>   .I name
>>   .br
>>   Set quota block limits (bhard/bsoft), inode count limits (ihard/isoft)
>> -and/or realtime block limits (rtbhard/rtbsoft). The
>> +and/or realtime block limits (rtbhard/rtbsoft) to N, where N is a bare
> 
> What is a 'bare' number?
> 
> How about (shortened so I don't have to retype the whole thing):
> 
> "Set quota block limits...to N.  For block limits, N is a number
> with a s/b/k/m/g/t/p/e multiplication suffix..."

it's also allowed w/o the suffix. so I propose ...

Set quota block limits (bhard/bsoft), inode count limits (ihard/isoft)
+and/or realtime block limits (rtbhard/rtbsoft) to N, where N is a
number representing bytes or inodes.
+For block limits, a number with a s/b/k/m/g/t/p/e multiplication suffix
+as described in
+.BR mkfs.xfs (8)
+is also accepted.
For inode limits, no suffixes are allowed.

(I thought about adding suffix support to inodes but meh, that's confusing,
what is 1 block's worth of inodes?)

> 
> "For inode limits, N is a bare number; no suffixes are allowed."
> 
> ?
> 
> --D
> 
>> +number representing bytes or inodes.
>> +For block limits, a number with a s/b/k/m/g/t/p/e multiplication suffix
>> +as described in
>> +.BR mkfs.xfs (8)
>> +is also accepted.
>> +The
>>   .B \-d
>>   option (defaults) can be used to set the default value
>>   that will be used, otherwise a specific
>> -- 
>> 1.8.3.1
>>
> 

Re: [PATCH 3/4] xfs_quota: don't exit on fs_table_insert_project_path failure

[Eric Sandeen]

On 12/10/21 6:21 PM, Darrick J. Wong wrote:
> On Fri, Dec 10, 2021 at 02:21:36PM -0600, Eric Sandeen wrote:
>> From: Eric Sandeen <sandeen@redhat.com>
>>
>> If "project -p" fails in fs_table_insert_project_path, it
>> calls exit() today which is quite unfriendly. Return an error
>> and return to the command prompt as expected.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
>> ---
>>   libfrog/paths.c | 7 +++----
>>   libfrog/paths.h | 2 +-
>>   quota/project.c | 4 +++-
>>   3 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/libfrog/paths.c b/libfrog/paths.c
>> index d679376..6c0fee2 100644
>> --- a/libfrog/paths.c
>> +++ b/libfrog/paths.c
>> @@ -546,7 +546,7 @@ out_error:
>>   		progname, strerror(error));
>>   }
>>   
>> -void
>> +int
>>   fs_table_insert_project_path(
>>   	char		*dir,
>>   	prid_t		prid)
>> @@ -561,9 +561,8 @@ fs_table_insert_project_path(
>>   	else
>>   		error = ENOENT;
>>   
>> -	if (error) {
>> +	if (error)
>>   		fprintf(stderr, _("%s: cannot setup path for project dir %s: %s\n"),
>>   				progname, dir, strerror(error));
> 
> Why not move this to the (sole) caller?  Libraries (even pseudolibraries
> like libfrog) usually aren't supposed to go around fprintfing things.

I mean, that's a legit goal, but

$ grep -rw "printf\|fprintf"  libfrog/ | wc -l
55

but ok, I can reduce it to 54 ;)

-Eric
  

Re: [PATCH 1/4] xfs_quota: document unit multipliers used in limit command

[Darrick J. Wong]

On Fri, Dec 10, 2021 at 11:47:07PM -0600, Eric Sandeen wrote:
> On 12/10/21 6:15 PM, Darrick J. Wong wrote:
> > On Fri, Dec 10, 2021 at 02:21:34PM -0600, Eric Sandeen wrote:
> > > From: Eric Sandeen <sandeen@redhat.com>
> > > 
> > > The units used to set limits are never specified in the xfs_quota
> > > man page, and in fact for block limits, the standard k/m/g/...
> > > units are accepted. Document all of this.
> > > 
> > > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > > Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> > > ---
> > >   man/man8/xfs_quota.8 | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/man/man8/xfs_quota.8 b/man/man8/xfs_quota.8
> > > index 59e603f..f841e3f 100644
> > > --- a/man/man8/xfs_quota.8
> > > +++ b/man/man8/xfs_quota.8
> > > @@ -446,7 +446,13 @@ option reports state on all filesystems and not just the current path.
> > >   .I name
> > >   .br
> > >   Set quota block limits (bhard/bsoft), inode count limits (ihard/isoft)
> > > -and/or realtime block limits (rtbhard/rtbsoft). The
> > > +and/or realtime block limits (rtbhard/rtbsoft) to N, where N is a bare
> > 
> > What is a 'bare' number?
> > 
> > How about (shortened so I don't have to retype the whole thing):
> > 
> > "Set quota block limits...to N.  For block limits, N is a number
> > with a s/b/k/m/g/t/p/e multiplication suffix..."
> 
> it's also allowed w/o the suffix. so I propose ...
> 
> Set quota block limits (bhard/bsoft), inode count limits (ihard/isoft)
> +and/or realtime block limits (rtbhard/rtbsoft) to N, where N is a
> number representing bytes or inodes.
> +For block limits, a number with a s/b/k/m/g/t/p/e multiplication suffix
> +as described in
> +.BR mkfs.xfs (8)
> +is also accepted.
> For inode limits, no suffixes are allowed.
> 
> (I thought about adding suffix support to inodes but meh, that's confusing,
> what is 1 block's worth of inodes?)

...or does "1g" refer to one giga-inode, or one gibi-inode?
Probably best to leave the code as it is.

As for the manpage update, with the new wording,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> > 
> > "For inode limits, N is a bare number; no suffixes are allowed."
> > 
> > ?
> > 
> > --D
> > 
> > > +number representing bytes or inodes.
> > > +For block limits, a number with a s/b/k/m/g/t/p/e multiplication suffix
> > > +as described in
> > > +.BR mkfs.xfs (8)
> > > +is also accepted.
> > > +The
> > >   .B \-d
> > >   option (defaults) can be used to set the default value
> > >   that will be used, otherwise a specific
> > > -- 
> > > 1.8.3.1
> > > 
> > 

Re: [PATCH 3/4] xfs_quota: don't exit on fs_table_insert_project_path failure

[Darrick J. Wong]

On Fri, Dec 10, 2021 at 11:52:54PM -0600, Eric Sandeen wrote:
> On 12/10/21 6:21 PM, Darrick J. Wong wrote:
> > On Fri, Dec 10, 2021 at 02:21:36PM -0600, Eric Sandeen wrote:
> > > From: Eric Sandeen <sandeen@redhat.com>
> > > 
> > > If "project -p" fails in fs_table_insert_project_path, it
> > > calls exit() today which is quite unfriendly. Return an error
> > > and return to the command prompt as expected.
> > > 
> > > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > > Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> > > ---
> > >   libfrog/paths.c | 7 +++----
> > >   libfrog/paths.h | 2 +-
> > >   quota/project.c | 4 +++-
> > >   3 files changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/libfrog/paths.c b/libfrog/paths.c
> > > index d679376..6c0fee2 100644
> > > --- a/libfrog/paths.c
> > > +++ b/libfrog/paths.c
> > > @@ -546,7 +546,7 @@ out_error:
> > >   		progname, strerror(error));
> > >   }
> > > -void
> > > +int
> > >   fs_table_insert_project_path(
> > >   	char		*dir,
> > >   	prid_t		prid)
> > > @@ -561,9 +561,8 @@ fs_table_insert_project_path(
> > >   	else
> > >   		error = ENOENT;
> > > -	if (error) {
> > > +	if (error)
> > >   		fprintf(stderr, _("%s: cannot setup path for project dir %s: %s\n"),
> > >   				progname, dir, strerror(error));
> > 
> > Why not move this to the (sole) caller?  Libraries (even pseudolibraries
> > like libfrog) usually aren't supposed to go around fprintfing things.
> 
> I mean, that's a legit goal, but
> 
> $ grep -rw "printf\|fprintf"  libfrog/ | wc -l
> 55
> 
> but ok, I can reduce it to 54 ;)
> 

With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> -Eric