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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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),
> >