[PATCHSET 0/5] xfs_repair: fix corruption messaging with verbose mode

[PATCHSET 0/5] xfs_repair: fix corruption messaging with verbose mode

[Darrick J. Wong]

Hi all,

While I was running xfs/155 and fuzz testing of the parent pointers
patchset, I noticed that there were quite a few places in xfs_repair
where adding the -v flag caused repair either not to say that it was
correcting something, or that it would spit out the wrong action message
entirely.  This series resolves all of those problems.

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

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=repair-fix-corruption-messaging
---
 repair/dino_chunks.c |    8 +++-----
 repair/phase6.c      |   43 ++++++++++++++++++-------------------------
 2 files changed, 21 insertions(+), 30 deletions(-)

[PATCH 1/5] xfs_repair: don't spray correcting imap all by itself

[Darrick J. Wong]

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

In xfs/155, I occasionally see this in the xfs_repair output:

correcting imap
correcting imap
correcting imap
...

This is completely useless, since we don't actually log which inode
prompted this message.  This logic has been here for a really long time,
but ... I can't make heads nor tails of it.  If we're running in verbose
or dry-run mode, then print the inode number, but not if we're running
in fixit mode?

A few lines later, if we're running in verbose dry-run mode, we print
"correcting imap" even though we're not going to write anything.

If we get here, the inode looks like it's in use, but the inode index
says it isn't.  This is a corruption, so either we fix it or we say that
we would fix it.

Fixes: 6c39a3cbda3 ("Don't trash lost+found in phase 4 Merge of master-melb:xfs-cmds:29144a by kenmcd.")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/dino_chunks.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)


diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index cf6a5e399d4..33008853789 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -871,13 +871,11 @@ process_inode_chunk(
 		 */
 		if (is_used)  {
 			if (is_inode_free(ino_rec, irec_offset))  {
-				if (verbose || no_modify)  {
-					do_warn(
+				do_warn(
 	_("imap claims in-use inode %" PRIu64 " is free, "),
 						ino);
-				}
 
-				if (verbose || !no_modify)
+				if (!no_modify)
 					do_warn(_("correcting imap\n"));
 				else
 					do_warn(_("would correct imap\n"));

Re: [PATCH 1/5] xfs_repair: don't spray correcting imap all by itself

[Carlos Maiolino]

On Mon, Jun 05, 2023 at 08:37:39AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In xfs/155, I occasionally see this in the xfs_repair output:
> 
> correcting imap
> correcting imap
> correcting imap
> ...
> 
> This is completely useless, since we don't actually log which inode
> prompted this message.  This logic has been here for a really long time,
> but ... I can't make heads nor tails of it.  If we're running in verbose
> or dry-run mode, then print the inode number, but not if we're running
> in fixit mode?
> 
> A few lines later, if we're running in verbose dry-run mode, we print
> "correcting imap" even though we're not going to write anything.
> 
> If we get here, the inode looks like it's in use, but the inode index
> says it isn't.  This is a corruption, so either we fix it or we say that
> we would fix it.

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


> 
> Fixes: 6c39a3cbda3 ("Don't trash lost+found in phase 4 Merge of master-melb:xfs-cmds:29144a by kenmcd.")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/dino_chunks.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> index cf6a5e399d4..33008853789 100644
> --- a/repair/dino_chunks.c
> +++ b/repair/dino_chunks.c
> @@ -871,13 +871,11 @@ process_inode_chunk(
>  		 */
>  		if (is_used)  {
>  			if (is_inode_free(ino_rec, irec_offset))  {
> -				if (verbose || no_modify)  {
> -					do_warn(
> +				do_warn(
>  	_("imap claims in-use inode %" PRIu64 " is free, "),
>  						ino);
> -				}
> 
> -				if (verbose || !no_modify)
> +				if (!no_modify)
>  					do_warn(_("correcting imap\n"));
>  				else
>  					do_warn(_("would correct imap\n"));
> 

[PATCH 2/5] xfs_repair: don't log inode problems without printing resolution

[Darrick J. Wong]

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

If we're running in repair mode without the verbose flag, I see a bunch
of stuff like this:

entry "FOO" in directory inode XXX points to non-existent inode YYY

This output is less than helpful, since it doesn't tell us that repair
is actually fixing the problem.  We're fixing a corruption, so we should
always say that we're going to fix it.

Fixes: 6c39a3cbda3 ("Don't trash lost+found in phase 4 Merge of master-melb:xfs-cmds:29144a by kenmcd.")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/phase6.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)


diff --git a/repair/phase6.c b/repair/phase6.c
index 37573b4301b..39470185ea4 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1176,13 +1176,10 @@ entry_junked(
 	xfs_ino_t	ino2)
 {
 	do_warn(msg, iname, ino1, ino2);
-	if (!no_modify) {
-		if (verbose)
-			do_warn(_(", marking entry to be junked\n"));
-		else
-			do_warn("\n");
-	} else
-		do_warn(_(", would junk entry\n"));
+	if (!no_modify)
+		do_warn(_("junking entry\n"));
+	else
+		do_warn(_("would junk entry\n"));
 	return !no_modify;
 }
 
@@ -1682,7 +1679,7 @@ longform_dir2_entry_check_data(
 		if (irec == NULL)  {
 			nbad++;
 			if (entry_junked(
-	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ""),
+	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ", "),
 					fname, ip->i_ino, inum)) {
 				dep->name[0] = '/';
 				libxfs_dir2_data_log_entry(&da, bp, dep);
@@ -1699,7 +1696,7 @@ longform_dir2_entry_check_data(
 		if (is_inode_free(irec, ino_offset))  {
 			nbad++;
 			if (entry_junked(
-	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64),
+	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64 ", "),
 					fname, ip->i_ino, inum)) {
 				dep->name[0] = '/';
 				libxfs_dir2_data_log_entry(&da, bp, dep);
@@ -1717,7 +1714,7 @@ longform_dir2_entry_check_data(
 			if (!inode_isadir(irec, ino_offset)) {
 				nbad++;
 				if (entry_junked(
-	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory"),
+	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory, "),
 						ORPHANAGE, inum, ip->i_ino)) {
 					dep->name[0] = '/';
 					libxfs_dir2_data_log_entry(&da, bp, dep);
@@ -1739,7 +1736,7 @@ longform_dir2_entry_check_data(
 				dep->name, libxfs_dir2_data_get_ftype(mp, dep))) {
 			nbad++;
 			if (entry_junked(
-	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
+	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name, "),
 					fname, inum, ip->i_ino)) {
 				dep->name[0] = '/';
 				libxfs_dir2_data_log_entry(&da, bp, dep);
@@ -1770,7 +1767,7 @@ longform_dir2_entry_check_data(
 				/* ".." should be in the first block */
 				nbad++;
 				if (entry_junked(
-	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block"), fname,
+	_("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] = '/';
@@ -1803,7 +1800,7 @@ longform_dir2_entry_check_data(
 				/* "." should be the first entry */
 				nbad++;
 				if (entry_junked(
-	_("entry \"%s\" in dir %" PRIu64 " is not the first entry"),
+	_("entry \"%s\" in dir %" PRIu64 " is not the first entry, "),
 						fname, inum, ip->i_ino)) {
 					dir_hash_junkit(hashtab, addr);
 					dep->name[0] = '/';

Re: [PATCH 2/5] xfs_repair: don't log inode problems without printing resolution

[Carlos Maiolino]

On Mon, Jun 05, 2023 at 08:37:44AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If we're running in repair mode without the verbose flag, I see a bunch
> of stuff like this:
> 
> entry "FOO" in directory inode XXX points to non-existent inode YYY
> 
> This output is less than helpful, since it doesn't tell us that repair
> is actually fixing the problem.  We're fixing a corruption, so we should
> always say that we're going to fix it.
> 
> Fixes: 6c39a3cbda3 ("Don't trash lost+found in phase 4 Merge of master-melb:xfs-cmds:29144a by kenmcd.")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/phase6.c |   23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)

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

> 
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 37573b4301b..39470185ea4 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1176,13 +1176,10 @@ entry_junked(
>  	xfs_ino_t	ino2)
>  {
>  	do_warn(msg, iname, ino1, ino2);
> -	if (!no_modify) {
> -		if (verbose)
> -			do_warn(_(", marking entry to be junked\n"));
> -		else
> -			do_warn("\n");
> -	} else
> -		do_warn(_(", would junk entry\n"));
> +	if (!no_modify)
> +		do_warn(_("junking entry\n"));
> +	else
> +		do_warn(_("would junk entry\n"));
>  	return !no_modify;
>  }
> 
> @@ -1682,7 +1679,7 @@ longform_dir2_entry_check_data(
>  		if (irec == NULL)  {
>  			nbad++;
>  			if (entry_junked(
> -	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ""),
> +	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ", "),
>  					fname, ip->i_ino, inum)) {
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
> @@ -1699,7 +1696,7 @@ longform_dir2_entry_check_data(
>  		if (is_inode_free(irec, ino_offset))  {
>  			nbad++;
>  			if (entry_junked(
> -	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64),
> +	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64 ", "),
>  					fname, ip->i_ino, inum)) {
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
> @@ -1717,7 +1714,7 @@ longform_dir2_entry_check_data(
>  			if (!inode_isadir(irec, ino_offset)) {
>  				nbad++;
>  				if (entry_junked(
> -	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory"),
> +	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory, "),
>  						ORPHANAGE, inum, ip->i_ino)) {
>  					dep->name[0] = '/';
>  					libxfs_dir2_data_log_entry(&da, bp, dep);
> @@ -1739,7 +1736,7 @@ longform_dir2_entry_check_data(
>  				dep->name, libxfs_dir2_data_get_ftype(mp, dep))) {
>  			nbad++;
>  			if (entry_junked(
> -	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
> +	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name, "),
>  					fname, inum, ip->i_ino)) {
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
> @@ -1770,7 +1767,7 @@ longform_dir2_entry_check_data(
>  				/* ".." should be in the first block */
>  				nbad++;
>  				if (entry_junked(
> -	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block"), fname,
> +	_("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] = '/';
> @@ -1803,7 +1800,7 @@ longform_dir2_entry_check_data(
>  				/* "." should be the first entry */
>  				nbad++;
>  				if (entry_junked(
> -	_("entry \"%s\" in dir %" PRIu64 " is not the first entry"),
> +	_("entry \"%s\" in dir %" PRIu64 " is not the first entry, "),
>  						fname, inum, ip->i_ino)) {
>  					dir_hash_junkit(hashtab, addr);
>  					dep->name[0] = '/';
> 

[PATCH 3/5] xfs_repair: fix messaging when shortform_dir2_junk is called

[Darrick J. Wong]

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

This function is called when we've decide to junk a shortform directory
entry.  This is obviously corruption of some kind, so we should always
say something, particularly if we're in !verbose repair mode.
Otherwise, if we're in non-verbose repair mode, we print things like:

entry "FOO" in shortform directory XXX references non-existent inode YYY

Without telling the sysadmin that we're removing the dirent.

Fixes: aaca101b1ae ("xfs_repair: add support for validating dirent ftype field")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/phase6.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)


diff --git a/repair/phase6.c b/repair/phase6.c
index 39470185ea4..a457429b3c6 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2441,10 +2441,7 @@ shortform_dir2_junk(
 	 */
 	(*index)--;
 
-	if (verbose)
-		do_warn(_("junking entry\n"));
-	else
-		do_warn("\n");
+	do_warn(_("junking entry\n"));
 	return sfep;
 }
 
@@ -2593,7 +2590,7 @@ shortform_dir2_entry_check(
 
 		if (irec == NULL)  {
 			do_warn(
-	_("entry \"%s\" in shortform directory %" PRIu64 " references non-existent inode %" PRIu64 "\n"),
+	_("entry \"%s\" in shortform directory %" PRIu64 " references non-existent inode %" PRIu64 ", "),
 				fname, ino, lino);
 			next_sfep = shortform_dir2_junk(mp, sfp, sfep, lino,
 						&max_size, &i, &bytes_deleted,
@@ -2610,7 +2607,7 @@ shortform_dir2_entry_check(
 		 */
 		if (is_inode_free(irec, ino_offset))  {
 			do_warn(
-	_("entry \"%s\" in shortform directory inode %" PRIu64 " points to free inode %" PRIu64 "\n"),
+	_("entry \"%s\" in shortform directory inode %" PRIu64 " points to free inode %" PRIu64 ", "),
 				fname, ino, lino);
 			next_sfep = shortform_dir2_junk(mp, sfp, sfep, lino,
 						&max_size, &i, &bytes_deleted,
@@ -2626,7 +2623,7 @@ shortform_dir2_entry_check(
 			 */
 			if (!inode_isadir(irec, ino_offset)) {
 				do_warn(
-	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory"),
+	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory, "),
 					ORPHANAGE, lino, ino);
 				next_sfep = shortform_dir2_junk(mp, sfp, sfep,
 						lino, &max_size, &i,
@@ -2648,7 +2645,7 @@ shortform_dir2_entry_check(
 				lino, sfep->namelen, sfep->name,
 				libxfs_dir2_sf_get_ftype(mp, sfep))) {
 			do_warn(
-_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
+_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name, "),
 				fname, lino, ino);
 			next_sfep = shortform_dir2_junk(mp, sfp, sfep, lino,
 						&max_size, &i, &bytes_deleted,
@@ -2673,7 +2670,7 @@ _("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
 			if (is_inode_reached(irec, ino_offset))  {
 				do_warn(
 	_("entry \"%s\" in directory inode %" PRIu64
-	  " references already connected inode %" PRIu64 ".\n"),
+	  " references already connected inode %" PRIu64 ", "),
 					fname, ino, lino);
 				next_sfep = shortform_dir2_junk(mp, sfp, sfep,
 						lino, &max_size, &i,
@@ -2697,7 +2694,7 @@ _("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
 				do_warn(
 	_("entry \"%s\" in directory inode %" PRIu64
 	  " not consistent with .. value (%" PRIu64
-	  ") in inode %" PRIu64 ",\n"),
+	  ") in inode %" PRIu64 ", "),
 					fname, ino, parent, lino);
 				next_sfep = shortform_dir2_junk(mp, sfp, sfep,
 						lino, &max_size, &i,

Re: [PATCH 3/5] xfs_repair: fix messaging when shortform_dir2_junk is called

[Carlos Maiolino]

On Mon, Jun 05, 2023 at 08:37:50AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> This function is called when we've decide to junk a shortform directory
> entry.  This is obviously corruption of some kind, so we should always
> say something, particularly if we're in !verbose repair mode.
> Otherwise, if we're in non-verbose repair mode, we print things like:
> 
> entry "FOO" in shortform directory XXX references non-existent inode YYY
> 
> Without telling the sysadmin that we're removing the dirent.
> 
> Fixes: aaca101b1ae ("xfs_repair: add support for validating dirent ftype field")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/phase6.c |   17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 

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

> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 39470185ea4..a457429b3c6 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -2441,10 +2441,7 @@ shortform_dir2_junk(
>  	 */
>  	(*index)--;
> 
> -	if (verbose)
> -		do_warn(_("junking entry\n"));
> -	else
> -		do_warn("\n");
> +	do_warn(_("junking entry\n"));
>  	return sfep;
>  }
> 
> @@ -2593,7 +2590,7 @@ shortform_dir2_entry_check(
> 
>  		if (irec == NULL)  {
>  			do_warn(
> -	_("entry \"%s\" in shortform directory %" PRIu64 " references non-existent inode %" PRIu64 "\n"),
> +	_("entry \"%s\" in shortform directory %" PRIu64 " references non-existent inode %" PRIu64 ", "),
>  				fname, ino, lino);
>  			next_sfep = shortform_dir2_junk(mp, sfp, sfep, lino,
>  						&max_size, &i, &bytes_deleted,
> @@ -2610,7 +2607,7 @@ shortform_dir2_entry_check(
>  		 */
>  		if (is_inode_free(irec, ino_offset))  {
>  			do_warn(
> -	_("entry \"%s\" in shortform directory inode %" PRIu64 " points to free inode %" PRIu64 "\n"),
> +	_("entry \"%s\" in shortform directory inode %" PRIu64 " points to free inode %" PRIu64 ", "),
>  				fname, ino, lino);
>  			next_sfep = shortform_dir2_junk(mp, sfp, sfep, lino,
>  						&max_size, &i, &bytes_deleted,
> @@ -2626,7 +2623,7 @@ shortform_dir2_entry_check(
>  			 */
>  			if (!inode_isadir(irec, ino_offset)) {
>  				do_warn(
> -	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory"),
> +	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory, "),
>  					ORPHANAGE, lino, ino);
>  				next_sfep = shortform_dir2_junk(mp, sfp, sfep,
>  						lino, &max_size, &i,
> @@ -2648,7 +2645,7 @@ shortform_dir2_entry_check(
>  				lino, sfep->namelen, sfep->name,
>  				libxfs_dir2_sf_get_ftype(mp, sfep))) {
>  			do_warn(
> -_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
> +_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name, "),
>  				fname, lino, ino);
>  			next_sfep = shortform_dir2_junk(mp, sfp, sfep, lino,
>  						&max_size, &i, &bytes_deleted,
> @@ -2673,7 +2670,7 @@ _("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
>  			if (is_inode_reached(irec, ino_offset))  {
>  				do_warn(
>  	_("entry \"%s\" in directory inode %" PRIu64
> -	  " references already connected inode %" PRIu64 ".\n"),
> +	  " references already connected inode %" PRIu64 ", "),
>  					fname, ino, lino);
>  				next_sfep = shortform_dir2_junk(mp, sfp, sfep,
>  						lino, &max_size, &i,
> @@ -2697,7 +2694,7 @@ _("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
>  				do_warn(
>  	_("entry \"%s\" in directory inode %" PRIu64
>  	  " not consistent with .. value (%" PRIu64
> -	  ") in inode %" PRIu64 ",\n"),
> +	  ") in inode %" PRIu64 ", "),
>  					fname, ino, parent, lino);
>  				next_sfep = shortform_dir2_junk(mp, sfp, sfep,
>  						lino, &max_size, &i,
> 

[PATCH 4/5] xfs_repair: fix messaging in longform_dir2_entry_check_data

[Darrick J. Wong]

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

Always log when we're junking a dirent from a non-shortform directory,
because we're fixing corruptions.  Even if we're in !verbose repair
mode.  Otherwise, we print things like:

entry "FOO" in dir inode XXX inconsistent with .. value (YYY) in ino ZZZ

Without telling the user that we're clearing the entry.

Fixes: 6c39a3cbda3 ("Don't trash lost+found in phase 4 Merge of master-melb:xfs-cmds:29144a by kenmcd.")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/phase6.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


diff --git a/repair/phase6.c b/repair/phase6.c
index a457429b3c6..3870c5c933a 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1883,8 +1883,7 @@ _("entry \"%s\" in dir inode %" PRIu64 " inconsistent with .. value (%" PRIu64 "
 				dir_hash_junkit(hashtab, addr);
 				dep->name[0] = '/';
 				libxfs_dir2_data_log_entry(&da, bp, dep);
-				if (verbose)
-					do_warn(
+				do_warn(
 					_("\twill clear entry \"%s\"\n"),
 						fname);
 			} else  {

Re: [PATCH 4/5] xfs_repair: fix messaging in longform_dir2_entry_check_data

[Carlos Maiolino]

On Mon, Jun 05, 2023 at 08:37:56AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Always log when we're junking a dirent from a non-shortform directory,
> because we're fixing corruptions.  Even if we're in !verbose repair
> mode.  Otherwise, we print things like:
> 
> entry "FOO" in dir inode XXX inconsistent with .. value (YYY) in ino ZZZ
> 
> Without telling the user that we're clearing the entry.
> 
> Fixes: 6c39a3cbda3 ("Don't trash lost+found in phase 4 Merge of master-melb:xfs-cmds:29144a by kenmcd.")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/phase6.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

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

> 
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index a457429b3c6..3870c5c933a 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1883,8 +1883,7 @@ _("entry \"%s\" in dir inode %" PRIu64 " inconsistent with .. value (%" PRIu64 "
>  				dir_hash_junkit(hashtab, addr);
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
> -				if (verbose)
> -					do_warn(
> +				do_warn(
>  					_("\twill clear entry \"%s\"\n"),
>  						fname);
>  			} else  {
> 

[PATCH 5/5] xfs_repair: fix messaging when fixing imap due to sparse cluster

[Darrick J. Wong]

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

This logic is wrong -- if we're in verbose dry-run mode, we do NOT want
to say that we're correcting the imap.  Otherwise, we print things like:

imap claims inode XXX is present, but inode cluster is sparse,

But then we can erroneously tell the user that we would correct the
imap when in fact we /are/ correcting it.

Fixes: f4ff8086586 ("xfs_repair: don't crash on partially sparse inode clusters")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/dino_chunks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 33008853789..171756818a6 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -834,7 +834,7 @@ process_inode_chunk(
 			do_warn(
 	_("imap claims inode %" PRIu64 " is present, but inode cluster is sparse, "),
 						ino);
-			if (verbose || !no_modify)
+			if (!no_modify)
 				do_warn(_("correcting imap\n"));
 			else
 				do_warn(_("would correct imap\n"));

Re: [PATCH 5/5] xfs_repair: fix messaging when fixing imap due to sparse cluster

[Carlos Maiolino]

On Mon, Jun 05, 2023 at 08:38:01AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> This logic is wrong -- if we're in verbose dry-run mode, we do NOT want
> to say that we're correcting the imap.  Otherwise, we print things like:
> 
> imap claims inode XXX is present, but inode cluster is sparse,
> 
> But then we can erroneously tell the user that we would correct the
> imap when in fact we /are/ correcting it.
> 
> Fixes: f4ff8086586 ("xfs_repair: don't crash on partially sparse inode clusters")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

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

> ---
>  repair/dino_chunks.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> index 33008853789..171756818a6 100644
> --- a/repair/dino_chunks.c
> +++ b/repair/dino_chunks.c
> @@ -834,7 +834,7 @@ process_inode_chunk(
>  			do_warn(
>  	_("imap claims inode %" PRIu64 " is present, but inode cluster is sparse, "),
>  						ino);
> -			if (verbose || !no_modify)
> +			if (!no_modify)
>  				do_warn(_("correcting imap\n"));
>  			else
>  				do_warn(_("would correct imap\n"));
> 

Re: [PATCHSET 0/5] xfs_repair: fix corruption messaging with verbose mode

[Pavel Reichl]

Whole patchset LGTM & Builds

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