[PATCHSET 0/2] xfsprogs: random fixes

[PATCHSET 0/2] xfsprogs: random fixes

[Darrick J. Wong]

Hi all,

Here are a couple more random fixes.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
 quota/state.c   |    3 ++-
 repair/phase6.c |   18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

[PATCH 1/2] xfs_repair: invalidate dirhash entry when junking dirent

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In longform_dir2_entry_check_data, we add the directory entries we find
to the incore dirent hash table after we've validated the name but
before we're totally done checking the entry.  This sequence is
necessary to detect all duplicated names in the directory.

Unfortunately, if we later decide to junk the ondisk dirent, we neglect
to mark the dirhash entry, so if the directory gets rebuilt, it will get
rebuilt with the entry that we rejected.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 repair/phase6.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)


diff --git a/repair/phase6.c b/repair/phase6.c
index 0929dcdf..696a6427 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -237,6 +237,21 @@ dir_hash_add(
 	return !dup;
 }
 
+/* Mark an existing directory hashtable entry as junk. */
+static void
+dir_hash_junkit(
+	struct dir_hash_tab	*hashtab,
+	xfs_dir2_dataptr_t	addr)
+{
+	struct dir_hash_ent	*p;
+
+	p = radix_tree_lookup(&hashtab->byaddr, addr);
+	assert(p != NULL);
+
+	p->junkit = 1;
+	p->namebuf[0] = '/';
+}
+
 static int
 dir_hash_check(
 	struct dir_hash_tab	*hashtab,
@@ -1729,6 +1744,7 @@ longform_dir2_entry_check_data(
 				if (entry_junked(
 	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block"), fname,
 						inum, ip->i_ino)) {
+					dir_hash_junkit(hashtab, addr);
 					dep->name[0] = '/';
 					libxfs_dir2_data_log_entry(&da, bp, dep);
 				}
@@ -1756,6 +1772,7 @@ longform_dir2_entry_check_data(
 				if (entry_junked(
 	_("entry \"%s\" in dir %" PRIu64 " is not the first entry"),
 						fname, inum, ip->i_ino)) {
+					dir_hash_junkit(hashtab, addr);
 					dep->name[0] = '/';
 					libxfs_dir2_data_log_entry(&da, bp, dep);
 				}
@@ -1844,6 +1861,7 @@ _("entry \"%s\" in dir inode %" PRIu64 " inconsistent with .. value (%" PRIu64 "
 				orphanage_ino = 0;
 			nbad++;
 			if (!no_modify)  {
+				dir_hash_junkit(hashtab, addr);
 				dep->name[0] = '/';
 				libxfs_dir2_data_log_entry(&da, bp, dep);
 				if (verbose)

[PATCH 2/2] xfs_quota: allow users to truncate group and project quota files

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In commit 79ac1ae4, I /think/ xfsprogs gained the ability to deal with
project or group quotas.  For some reason, the quota remove command was
structured so that if the user passes both -g and -p, it will only ask
the kernel truncate the group quota file.  This is a strange behavior
since -ug results in truncation requests for both user and group quota
files, and the kernel is smart enough to return 0 if asked to truncate a
quota file that doesn't exist.

In other words, this is a seemingly arbitrary limitation of the command.
It's an unexpected behavior since we don't do any sort of parameter
validation to warn users when -p is silently ignored.  Modern V5
filesystems support both group and project quotas, so it's all the more
surprising that you can't do group and project all at once.  Remove this
pointless restriction.

Found while triaging xfs/007 regressions.

Fixes: 79ac1ae4 ("Fix xfs_quota disable, enable, off and remove commands Merge of master-melb:xfs-cmds:29395a by kenmcd.")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 quota/state.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/quota/state.c b/quota/state.c
index 19d34ed0..260ef51d 100644
--- a/quota/state.c
+++ b/quota/state.c
@@ -462,7 +462,8 @@ remove_extents(
 	if (type & XFS_GROUP_QUOTA) {
 		if (remove_qtype_extents(dir, XFS_GROUP_QUOTA) < 0)
 			return;
-	} else if (type & XFS_PROJ_QUOTA) {
+	}
+	if (type & XFS_PROJ_QUOTA) {
 		if (remove_qtype_extents(dir, XFS_PROJ_QUOTA) < 0)
 			return;
 	}

Re: [PATCH 2/2] xfs_quota: allow users to truncate group and project quota files

[Eric Sandeen]

On 7/28/21 2:16 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In commit 79ac1ae4, I /think/ xfsprogs gained the ability to deal with
> project or group quotas.  For some reason, the quota remove command was
> structured so that if the user passes both -g and -p, it will only ask
> the kernel truncate the group quota file.

Probably because group & project used to be mutually exclusive. I wonder
if this is the last remnant of that relic. ;)

>  This is a strange behavior
> since -ug results in truncation requests for both user and group quota
> files, and the kernel is smart enough to return 0 if asked to truncate a
> quota file that doesn't exist.
> 
> In other words, this is a seemingly arbitrary limitation of the command.
> It's an unexpected behavior since we don't do any sort of parameter
> validation to warn users when -p is silently ignored.  Modern V5
> filesystems support both group and project quotas, so it's all the more
> surprising that you can't do group and project all at once.  Remove this
> pointless restriction.
> 
> Found while triaging xfs/007 regressions.
> 
> Fixes: 79ac1ae4 ("Fix xfs_quota disable, enable, off and remove commands Merge of master-melb:xfs-cmds:29395a by kenmcd.")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  quota/state.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/quota/state.c b/quota/state.c
> index 19d34ed0..260ef51d 100644
> --- a/quota/state.c
> +++ b/quota/state.c
> @@ -462,7 +462,8 @@ remove_extents(
>  	if (type & XFS_GROUP_QUOTA) {
>  		if (remove_qtype_extents(dir, XFS_GROUP_QUOTA) < 0)
>  			return;
> -	} else if (type & XFS_PROJ_QUOTA) {
> +	}
> +	if (type & XFS_PROJ_QUOTA) {
>  		if (remove_qtype_extents(dir, XFS_PROJ_QUOTA) < 0)
>  			return;
>  	}
> 

Re: [PATCH 1/2] xfs_repair: invalidate dirhash entry when junking dirent

[Carlos Maiolino]

On Wed, Jul 28, 2021 at 02:16:26PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In longform_dir2_entry_check_data, we add the directory entries we find
> to the incore dirent hash table after we've validated the name but
> before we're totally done checking the entry.  This sequence is
> necessary to detect all duplicated names in the directory.
> 
> Unfortunately, if we later decide to junk the ondisk dirent, we neglect
> to mark the dirhash entry, so if the directory gets rebuilt, it will get
> rebuilt with the entry that we rejected.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

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

> ---
>  repair/phase6.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 0929dcdf..696a6427 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -237,6 +237,21 @@ dir_hash_add(
>  	return !dup;
>  }
>  
> +/* Mark an existing directory hashtable entry as junk. */
> +static void
> +dir_hash_junkit(
> +	struct dir_hash_tab	*hashtab,
> +	xfs_dir2_dataptr_t	addr)
> +{
> +	struct dir_hash_ent	*p;
> +
> +	p = radix_tree_lookup(&hashtab->byaddr, addr);
> +	assert(p != NULL);
> +
> +	p->junkit = 1;
> +	p->namebuf[0] = '/';
> +}
> +
>  static int
>  dir_hash_check(
>  	struct dir_hash_tab	*hashtab,
> @@ -1729,6 +1744,7 @@ longform_dir2_entry_check_data(
>  				if (entry_junked(
>  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block"), fname,
>  						inum, ip->i_ino)) {
> +					dir_hash_junkit(hashtab, addr);
>  					dep->name[0] = '/';
>  					libxfs_dir2_data_log_entry(&da, bp, dep);
>  				}
> @@ -1756,6 +1772,7 @@ longform_dir2_entry_check_data(
>  				if (entry_junked(
>  	_("entry \"%s\" in dir %" PRIu64 " is not the first entry"),
>  						fname, inum, ip->i_ino)) {
> +					dir_hash_junkit(hashtab, addr);
>  					dep->name[0] = '/';
>  					libxfs_dir2_data_log_entry(&da, bp, dep);
>  				}
> @@ -1844,6 +1861,7 @@ _("entry \"%s\" in dir inode %" PRIu64 " inconsistent with .. value (%" PRIu64 "
>  				orphanage_ino = 0;
>  			nbad++;
>  			if (!no_modify)  {
> +				dir_hash_junkit(hashtab, addr);
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
>  				if (verbose)
> 

-- 
Carlos