[PATCH 0/5] xfsprogs: random fixes

[PATCH 0/5] xfsprogs: random fixes

[Darrick J. Wong]

Hi all,

Here are a handful of fixes that have come in over the past month.  The
first was found via code inspection, and the rest are fixes for various
Coverity complaints.

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=random-fixes

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

[PATCH 1/5] xfs_spaceman: always report sick metadata, checked or not

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If the kernel thinks a piece of metadata is bad, we must always report
it.  This will happen with an upcoming series to mark things sick
whenever we return EFSCORRUPTED at runtime.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 spaceman/health.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/spaceman/health.c b/spaceman/health.c
index 8fd985a2..0d3aa243 100644
--- a/spaceman/health.c
+++ b/spaceman/health.c
@@ -171,10 +171,10 @@ report_sick(
 	for (f = maps; f->mask != 0; f++) {
 		if (f->has_fn && !f->has_fn(&file->xfd.fsgeom))
 			continue;
-		if (!(checked & f->mask))
+		bad = sick & f->mask;
+		if (!bad && !(checked & f->mask))
 			continue;
 		reported++;
-		bad = sick & f->mask;
 		if (!bad && quiet)
 			continue;
 		printf("%s %s: %s\n", descr, _(f->descr),

[PATCH 2/5] xfs_db: btheight should check geometry more carefully

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The btheight command needs to check user-supplied geometry more
carefully so that we don't hit floating point exceptions.

Coverity-id: 1453661, 1453659
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/btheight.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 6 deletions(-)


diff --git a/db/btheight.c b/db/btheight.c
index 289e5d84..8aa17c89 100644
--- a/db/btheight.c
+++ b/db/btheight.c
@@ -138,6 +138,10 @@ construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (record_size == 0) {
+		fprintf(stderr, _("%s: record size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -149,6 +153,10 @@ construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (key_size == 0) {
+		fprintf(stderr, _("%s: key size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -160,6 +168,10 @@ construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (ptr_size == 0) {
+		fprintf(stderr, _("%s: pointer size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -180,6 +192,27 @@ construct_records_per_block(
 		goto out;
 	}
 
+	if (record_size > blocksize) {
+		fprintf(stderr,
+_("%s: record size must be less than selected block size (%u bytes).\n"),
+			tag, blocksize);
+		goto out;
+	}
+
+	if (key_size > blocksize) {
+		fprintf(stderr,
+_("%s: key size must be less than selected block size (%u bytes).\n"),
+			tag, blocksize);
+		goto out;
+	}
+
+	if (ptr_size > blocksize) {
+		fprintf(stderr,
+_("%s: pointer size must be less than selected block size (%u bytes).\n"),
+			tag, blocksize);
+		goto out;
+	}
+
 	p = strtok(NULL, ":");
 	if (p) {
 		fprintf(stderr,
@@ -211,13 +244,24 @@ report(
 	int			ret;
 
 	ret = construct_records_per_block(tag, blocksize, records_per_block);
-	if (ret) {
-		printf(_("%s: Unable to determine records per block.\n"),
-				tag);
+	if (ret)
 		return;
-	}
 
 	if (report_what & REPORT_MAX) {
+		if (records_per_block[0] < 2) {
+			fprintf(stderr,
+_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
+				tag);
+			return;
+		}
+
+		if (records_per_block[1] < 4) {
+			fprintf(stderr,
+_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
+				tag);
+			return;
+		}
+
 		printf(
 _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 				tag, blocksize, records_per_block[0],
@@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 		records_per_block[0] /= 2;
 		records_per_block[1] /= 2;
 
+		if (records_per_block[0] < 1) {
+			fprintf(stderr,
+_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
+				tag);
+			return;
+		}
+
+		if (records_per_block[1] < 2) {
+			fprintf(stderr,
+_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
+				tag);
+			return;
+		}
+
 		printf(
 _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 				tag, blocksize, records_per_block[0],
@@ -284,8 +342,26 @@ btheight_f(
 		}
 	}
 
-	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
-	    nr_records == 0) {
+	if (nr_records == 0) {
+		fprintf(stderr,
+_("Number of records must be greater than zero.\n"));
+		return 0;
+	}
+
+	if (blocksize > INT_MAX) {
+		fprintf(stderr,
+_("The largest block size this command will consider is %u bytes.\n"),
+			INT_MAX);
+		return 0;
+	}
+
+	if (blocksize < 128) {
+		fprintf(stderr,
+_("The smallest block size this command will consider is 128 bytes.\n"));
+		return 0;
+	}
+
+	if (argc == optind) {
 		btheight_help();
 		return 0;
 	}

[PATCH 3/5] xfs_scrub: report repair activities on stdout, not stderr

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Reduce the severity of reports about successful metadata repairs.  We
fixed the problem, so there's no action necessary on the part of the
system admin.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/common.c |    2 +-
 scrub/common.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/scrub/common.c b/scrub/common.c
index b41f443d..7632a8d8 100644
--- a/scrub/common.c
+++ b/scrub/common.c
@@ -48,7 +48,7 @@ static struct {
 } err_levels[] = {
 	[S_ERROR]  = { .string = "Error",	.loglevel = LOG_ERR },
 	[S_WARN]   = { .string = "Warning",	.loglevel = LOG_WARNING },
-	[S_REPAIR] = { .string = "Repaired",	.loglevel = LOG_WARNING },
+	[S_REPAIR] = { .string = "Repaired",	.loglevel = LOG_INFO },
 	[S_INFO]   = { .string = "Info",	.loglevel = LOG_INFO },
 	[S_PREEN]  = { .string = "Optimized",	.loglevel = LOG_INFO }
 };
diff --git a/scrub/common.h b/scrub/common.h
index 9a37e9ed..ef4cf439 100644
--- a/scrub/common.h
+++ b/scrub/common.h
@@ -18,8 +18,8 @@ bool xfs_scrub_excessive_errors(struct scrub_ctx *ctx);
 enum error_level {
 	S_ERROR	= 0,
 	S_WARN,
-	S_REPAIR,
 	S_INFO,
+	S_REPAIR,
 	S_PREEN,
 };
 

[PATCH 4/5] xfs_scrub: don't allow error or negative error injection interval

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Don't allow zero or negative values from XFS_SCRUB_DISK_ERROR_INTERVAL
to slip into the system.  This is a debugging knob so we don't need to
be rigorous, but we can at least take care of obvious garbage values.

Coverity-id: 1454842
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/disk.c |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/scrub/disk.c b/scrub/disk.c
index 214a5346..8a8a411b 100644
--- a/scrub/disk.c
+++ b/scrub/disk.c
@@ -303,6 +303,10 @@ disk_simulate_read_error(
 		interval = strtoull(p, NULL, 10);
 		interval &= ~((1U << disk->d_lbalog) - 1);
 	}
+	if (interval <= 0) {
+		interval = -1;
+		return 0;
+	}
 
 	/*
 	 * We simulate disk errors by pretending that there are media errors at

[PATCH 5/5] libfrog: fix workqueue_add error out

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Don't forget to unlock before erroring out.

Coverity-id: 1454843
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libfrog/workqueue.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
index 07f11a7b..a93bba3d 100644
--- a/libfrog/workqueue.c
+++ b/libfrog/workqueue.c
@@ -142,8 +142,11 @@ workqueue_add(
 	if (wq->next_item == NULL) {
 		assert(wq->item_count == 0);
 		ret = pthread_cond_signal(&wq->wakeup);
-		if (ret)
-			goto out_item;
+		if (ret) {
+			pthread_mutex_unlock(&wq->lock);
+			free(wi);
+			return ret;
+		}
 		wq->next_item = wi;
 	} else {
 		wq->last_item->next = wi;
@@ -153,9 +156,6 @@ workqueue_add(
 	pthread_mutex_unlock(&wq->lock);
 
 	return 0;
-out_item:
-	free(wi);
-	return ret;
 }
 
 /*

[PATCH 6/5] xfs_repair: print better information when metadata updates fail

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If a metadata update fails during phase 6, we should print an error
message that can be traced back to a specific line of code.  Also,
res_failed spits out a general message about "xfs_trans_reserve failed",
which is probably not where the failure happened.  Fix two incorrect
call sites.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/phase6.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index 28e633de..91d208a6 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1354,7 +1354,8 @@ longform_dir2_rebuild(
 
 	error = dir_binval(tp, ip, XFS_DATA_FORK);
 	if (error)
-		res_failed(error);
+		do_error(_("error %d invalidating directory %llu blocks\n"),
+				error, (unsigned long long)ip->i_ino);
 
 	if ((error = -libxfs_bmap_last_offset(ip, &lastblock, XFS_DATA_FORK)))
 		do_error(_("xfs_bmap_last_offset failed -- error - %d\n"),
@@ -2972,7 +2973,10 @@ process_dir_inode(
 					XFS_ILOG_CORE | XFS_ILOG_DDATA);
 				error = -libxfs_trans_commit(tp);
 				if (error)
-					res_failed(error);
+					do_error(
+_("error %d fixing shortform directory %llu\n"),
+						error,
+						(unsigned long long)ip->i_ino);
 			} else  {
 				libxfs_trans_cancel(tp);
 			}

[PATCH 7/5] libxfs: fix typo in message about write verifier

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Fix a silly typo.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/rdwr.c |    2 +-
 po/pl.po      |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 0d3e6089..7080cd9c 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1117,7 +1117,7 @@ libxfs_writebufr(xfs_buf_t *bp)
 		bp->b_ops->verify_write(bp);
 		if (bp->b_error) {
 			fprintf(stderr,
-	_("%s: write verifer failed on %s bno 0x%llx/0x%x\n"),
+	_("%s: write verifier failed on %s bno 0x%llx/0x%x\n"),
 				__func__, bp->b_ops->name,
 				(long long)bp->b_bn, bp->b_bcount);
 			return bp->b_error;
diff --git a/po/pl.po b/po/pl.po
index ab5b11da..87109f6b 100644
--- a/po/pl.po
+++ b/po/pl.po
@@ -7466,7 +7466,7 @@ msgstr "%s: błąd - wykonano pwrite tylko %d z %d bajtów\n"
 
 #: .././libxfs/rdwr.c:1138
 #, c-format
-msgid "%s: write verifer failed on %s bno 0x%llx/0x%x\n"
+msgid "%s: write verifier failed on %s bno 0x%llx/0x%x\n"
 msgstr "%s: weryfikacja zapisu nie powiodła się na %s bno 0x%llx/0x%x\n"
 
 #: .././libxfs/trans.c:733

Re: [PATCH 1/5] xfs_spaceman: always report sick metadata, checked or not

[Eric Sandeen]

On 10/22/19 1:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If the kernel thinks a piece of metadata is bad, we must always report
> it.  This will happen with an upcoming series to mark things sick
> whenever we return EFSCORRUPTED at runtime.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I gotta say, I find this all really hard to read - something I should
have commented on earlier.  Masks, maps, and functions oh my.  bad and
sick and checked ... reported++ with no actual reporting ....

I'll try to think about what would make my poor brain happier later.
Comments, for one I think.  Maybe some bikeshedding over variable names.

I guess the upshot here is that if it's marked sick due to the kernel
sumbling over corruption, report it whether or not we ever explicitly
/asked/ for a check via the scrub interfaces?

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

> ---
>  spaceman/health.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/spaceman/health.c b/spaceman/health.c
> index 8fd985a2..0d3aa243 100644
> --- a/spaceman/health.c
> +++ b/spaceman/health.c
> @@ -171,10 +171,10 @@ report_sick(
>  	for (f = maps; f->mask != 0; f++) {
>  		if (f->has_fn && !f->has_fn(&file->xfd.fsgeom))
>  			continue;
> -		if (!(checked & f->mask))
> +		bad = sick & f->mask;
> +		if (!bad && !(checked & f->mask))
>  			continue;
>  		reported++;
> -		bad = sick & f->mask;
>  		if (!bad && quiet)
>  			continue;
>  		printf("%s %s: %s\n", descr, _(f->descr),
> 

Re: [PATCH 2/5] xfs_db: btheight should check geometry more carefully

[Eric Sandeen]

On 10/22/19 1:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The btheight command needs to check user-supplied geometry more
> carefully so that we don't hit floating point exceptions.

ok

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

> Coverity-id: 1453661, 1453659
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Re: [PATCH 3/5] xfs_scrub: report repair activities on stdout, not stderr

[Eric Sandeen]

On 10/22/19 1:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reduce the severity of reports about successful metadata repairs.  We
> fixed the problem, so there's no action necessary on the part of the
> system admin.

Hm, ok.  "we found corruption" seems quite important, but I guess it's
not an operational error of the utility.  *shrug*

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  scrub/common.c |    2 +-
>  scrub/common.h |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/scrub/common.c b/scrub/common.c
> index b41f443d..7632a8d8 100644
> --- a/scrub/common.c
> +++ b/scrub/common.c
> @@ -48,7 +48,7 @@ static struct {
>  } err_levels[] = {
>  	[S_ERROR]  = { .string = "Error",	.loglevel = LOG_ERR },
>  	[S_WARN]   = { .string = "Warning",	.loglevel = LOG_WARNING },
> -	[S_REPAIR] = { .string = "Repaired",	.loglevel = LOG_WARNING },
> +	[S_REPAIR] = { .string = "Repaired",	.loglevel = LOG_INFO },
>  	[S_INFO]   = { .string = "Info",	.loglevel = LOG_INFO },
>  	[S_PREEN]  = { .string = "Optimized",	.loglevel = LOG_INFO }

My OCD wants this in the same order as error_level, I'll change that
on commit if it's ok w/ you.  And if I remember.

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

>  };
> diff --git a/scrub/common.h b/scrub/common.h
> index 9a37e9ed..ef4cf439 100644
> --- a/scrub/common.h
> +++ b/scrub/common.h
> @@ -18,8 +18,8 @@ bool xfs_scrub_excessive_errors(struct scrub_ctx *ctx);
>  enum error_level {
>  	S_ERROR	= 0,
>  	S_WARN,
> -	S_REPAIR,
>  	S_INFO,
> +	S_REPAIR,
>  	S_PREEN,
>  };
>  
> 

Re: [PATCH 4/5] xfs_scrub: don't allow error or negative error injection interval

[Eric Sandeen]

On 10/22/19 1:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't allow zero or negative values from XFS_SCRUB_DISK_ERROR_INTERVAL
> to slip into the system.  This is a debugging knob so we don't need to
> be rigorous, but we can at least take care of obvious garbage values.
> 
> Coverity-id: 1454842
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

so we can't /set/ it to 0 or -1, and if it is, set it to -1. (!)

but ok, -1 means disabled, so invalid value -> disabled.

dbg_printf might be nice intead of silently ignoring but ... it's just
a debug knob so

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

> ---
>  scrub/disk.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> 
> diff --git a/scrub/disk.c b/scrub/disk.c
> index 214a5346..8a8a411b 100644
> --- a/scrub/disk.c
> +++ b/scrub/disk.c
> @@ -303,6 +303,10 @@ disk_simulate_read_error(
>  		interval = strtoull(p, NULL, 10);
>  		interval &= ~((1U << disk->d_lbalog) - 1);
>  	}
> +	if (interval <= 0) {
> +		interval = -1;
> +		return 0;
> +	}
>  
>  	/*
>  	 * We simulate disk errors by pretending that there are media errors at
> 

Re: [PATCH 5/5] libfrog: fix workqueue_add error out

[Eric Sandeen]

On 10/22/19 1:47 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't forget to unlock before erroring out.
> 
> Coverity-id: 1454843
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

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

Re: [PATCH 1/5] xfs_spaceman: always report sick metadata, checked or not

[Darrick J. Wong]

On Fri, Nov 01, 2019 at 01:17:11PM -0500, Eric Sandeen wrote:
> On 10/22/19 1:46 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > If the kernel thinks a piece of metadata is bad, we must always report
> > it.  This will happen with an upcoming series to mark things sick
> > whenever we return EFSCORRUPTED at runtime.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I gotta say, I find this all really hard to read - something I should
> have commented on earlier.  Masks, maps, and functions oh my.  bad and
> sick and checked ... reported++ with no actual reporting ....

Hm, yeah, the "reported" variable here counts the number of things the
kernel reported to us as having been checked or marked sick at some point.
The whole point of that is that if the kernel hasn't marked anything
checked or sick then we really can't say much about the state of the
filesystem and maybe the user should run xfs_scrub.

> I'll try to think about what would make my poor brain happier later.
> Comments, for one I think.  Maybe some bikeshedding over variable names.
> 
> I guess the upshot here is that if it's marked sick due to the kernel
> sumbling over corruption, report it whether or not we ever explicitly
> /asked/ for a check via the scrub interfaces?

Correct.

--D

> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > ---
> >  spaceman/health.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/spaceman/health.c b/spaceman/health.c
> > index 8fd985a2..0d3aa243 100644
> > --- a/spaceman/health.c
> > +++ b/spaceman/health.c
> > @@ -171,10 +171,10 @@ report_sick(
> >  	for (f = maps; f->mask != 0; f++) {
> >  		if (f->has_fn && !f->has_fn(&file->xfd.fsgeom))
> >  			continue;
> > -		if (!(checked & f->mask))
> > +		bad = sick & f->mask;
> > +		if (!bad && !(checked & f->mask))
> >  			continue;
> >  		reported++;
> > -		bad = sick & f->mask;
> >  		if (!bad && quiet)
> >  			continue;
> >  		printf("%s %s: %s\n", descr, _(f->descr),
> > 

Re: [PATCH 3/5] xfs_scrub: report repair activities on stdout, not stderr

[Darrick J. Wong]

On Fri, Nov 01, 2019 at 01:26:49PM -0500, Eric Sandeen wrote:
> On 10/22/19 1:46 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Reduce the severity of reports about successful metadata repairs.  We
> > fixed the problem, so there's no action necessary on the part of the
> > system admin.
> 
> Hm, ok.  "we found corruption" seems quite important, but I guess it's
> not an operational error of the utility.  *shrug*

We *fixed* corruption, meaning that the fs is ok now.

If we left corruption behind (either because the user gave us "-n" or
the repair failed) then we still yell about it on stderr.

> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  scrub/common.c |    2 +-
> >  scrub/common.h |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/scrub/common.c b/scrub/common.c
> > index b41f443d..7632a8d8 100644
> > --- a/scrub/common.c
> > +++ b/scrub/common.c
> > @@ -48,7 +48,7 @@ static struct {
> >  } err_levels[] = {
> >  	[S_ERROR]  = { .string = "Error",	.loglevel = LOG_ERR },
> >  	[S_WARN]   = { .string = "Warning",	.loglevel = LOG_WARNING },
> > -	[S_REPAIR] = { .string = "Repaired",	.loglevel = LOG_WARNING },
> > +	[S_REPAIR] = { .string = "Repaired",	.loglevel = LOG_INFO },
> >  	[S_INFO]   = { .string = "Info",	.loglevel = LOG_INFO },
> >  	[S_PREEN]  = { .string = "Optimized",	.loglevel = LOG_INFO }
> 
> My OCD wants this in the same order as error_level, I'll change that
> on commit if it's ok w/ you.  And if I remember.

<nod>

--D

> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> >  };
> > diff --git a/scrub/common.h b/scrub/common.h
> > index 9a37e9ed..ef4cf439 100644
> > --- a/scrub/common.h
> > +++ b/scrub/common.h
> > @@ -18,8 +18,8 @@ bool xfs_scrub_excessive_errors(struct scrub_ctx *ctx);
> >  enum error_level {
> >  	S_ERROR	= 0,
> >  	S_WARN,
> > -	S_REPAIR,
> >  	S_INFO,
> > +	S_REPAIR,
> >  	S_PREEN,
> >  };
> >  
> > 

Re: [PATCH 6/5] xfs_repair: print better information when metadata updates fail

[Eric Sandeen]

On 10/30/19 12:52 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If a metadata update fails during phase 6, we should print an error
> message that can be traced back to a specific line of code.  Also,
> res_failed spits out a general message about "xfs_trans_reserve failed",
> which is probably not where the failure happened.  Fix two incorrect
> call sites.

looks like mkfs could use a fix too,

        c = -libxfs_trans_commit(tp);
        if (c)
                res_failed(c);

but for this,

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

not sure if we care, but:

Fixes: f9c559f4e ("xfs_repair: invalidate dirty dir buffers when we zap a  directory")
Fixes: f2279d8de ("libxfs: check libxfs_trans_commit return values")

Re: [PATCH 7/5] libxfs: fix typo in message about write verifier

[Eric Sandeen]

On 10/30/19 12:53 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix a silly typo.

I have no complaints about this.  :)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>
 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Re: [PATCH 1/5] xfs_spaceman: always report sick metadata, checked or not

[Darrick J. Wong]

On Fri, Nov 01, 2019 at 11:40:10AM -0700, Darrick J. Wong wrote:
> On Fri, Nov 01, 2019 at 01:17:11PM -0500, Eric Sandeen wrote:
> > On 10/22/19 1:46 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > If the kernel thinks a piece of metadata is bad, we must always report
> > > it.  This will happen with an upcoming series to mark things sick
> > > whenever we return EFSCORRUPTED at runtime.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > I gotta say, I find this all really hard to read - something I should
> > have commented on earlier.  Masks, maps, and functions oh my.  bad and
> > sick and checked ... reported++ with no actual reporting ....
> 
> Hm, yeah, the "reported" variable here counts the number of things the
> kernel reported to us as having been checked or marked sick at some point.
> The whole point of that is that if the kernel hasn't marked anything
> checked or sick then we really can't say much about the state of the
> filesystem and maybe the user should run xfs_scrub.
> 
> > I'll try to think about what would make my poor brain happier later.
> > Comments, for one I think.  Maybe some bikeshedding over variable names.
> > 
> > I guess the upshot here is that if it's marked sick due to the kernel
> > sumbling over corruption, report it whether or not we ever explicitly
> > /asked/ for a check via the scrub interfaces?
> 
> Correct.

Sigh.  Change that to:

Correct, and soon I will send a kernel series that amends all of our
EFSCORRUPTED returns in libxfs to set the appropriate sick bit.  Then
spaceman will be able to pull reports about past corruption that were
found in the course of normal operations.

--D

> 
> --D
> 
> > Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> > 
> > > ---
> > >  spaceman/health.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > 
> > > diff --git a/spaceman/health.c b/spaceman/health.c
> > > index 8fd985a2..0d3aa243 100644
> > > --- a/spaceman/health.c
> > > +++ b/spaceman/health.c
> > > @@ -171,10 +171,10 @@ report_sick(
> > >  	for (f = maps; f->mask != 0; f++) {
> > >  		if (f->has_fn && !f->has_fn(&file->xfd.fsgeom))
> > >  			continue;
> > > -		if (!(checked & f->mask))
> > > +		bad = sick & f->mask;
> > > +		if (!bad && !(checked & f->mask))
> > >  			continue;
> > >  		reported++;
> > > -		bad = sick & f->mask;
> > >  		if (!bad && quiet)
> > >  			continue;
> > >  		printf("%s %s: %s\n", descr, _(f->descr),
> > > 

[PATCH 8/5] mkfs: fix incorrect error message

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If we encounter a failure while fixing the freelist during mkfs, we
shouldn't print a misleading message about space reservation.  Fix it so
that we print something about what we were trying to do when the error
happened.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 mkfs/xfs_mkfs.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 10a40cd4..18338a61 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3481,8 +3481,11 @@ initialise_ag_freespace(
 	libxfs_alloc_fix_freelist(&args, 0);
 	libxfs_perag_put(args.pag);
 	c = -libxfs_trans_commit(tp);
-	if (c)
-		res_failed(c);
+	if (c) {
+		errno = c;
+		perror(_("initializing AG free space list"));
+		exit(1);
+	}
 }
 
 /*

Re: [PATCH 8/5] mkfs: fix incorrect error message

[Eric Sandeen]

On 11/1/19 1:52 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If we encounter a failure while fixing the freelist during mkfs, we
> shouldn't print a misleading message about space reservation.  Fix it so
> that we print something about what we were trying to do when the error
> happened.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

8 going once going twice... sold, to the man in the front row,
8/5 is our final winning bid for this series. ;)

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