On Wed, Apr 03, 2019 at 10:30:05AM -0400, Brian Foster wrote:
> On Mon, Apr 01, 2019 at 10:10:52AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Use the AG geometry info ioctl to report health status too.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_fs.h | 12 +++++++++++-
> > fs/xfs/libxfs/xfs_health.h | 2 ++
> > fs/xfs/xfs_health.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > fs/xfs/xfs_ioctl.c | 2 ++
> > 4 files changed, 55 insertions(+), 1 deletion(-)
> >
> >
> ...
> > diff --git a/fs/xfs/xfs_health.c b/fs/xfs/xfs_health.c
> > index 151c98693bef..5ca471bd41ad 100644
> > --- a/fs/xfs/xfs_health.c
> > +++ b/fs/xfs/xfs_health.c
> > @@ -276,3 +276,43 @@ xfs_fsop_geom_health(
> > if (sick & XFS_HEALTH_RT_SUMMARY)
> > geo->health |= XFS_FSOP_GEOM_HEALTH_RT_SUMMARY;
> > }
> > +
> > +/* Fill out ag geometry health info. */
> > +void
> > +xfs_ag_geom_health(
> > + struct xfs_mount *mp,
> > + xfs_agnumber_t agno,
> > + struct xfs_ag_geometry *ageo)
> > +{
> > + struct xfs_perag *pag;
> > + unsigned int sick;
> > +
> > + if (agno >= mp->m_sb.sb_agcount)
> > + return;
>
> The call to xfs_ag_get_geometry() would have already returned an error
> in the ioctl path for the above scenario. It might still make sense to
> check here, but perhaps we could let this function also return an int
> and return an error for consistency?
Or maybe just ASSERT on the agno and add a note that the caller is
required to pass in a valid ag number.
> > +
> > + ageo->ag_health = 0;
> > +
> > + pag = xfs_perag_get(mp, agno);
> > + sick = xfs_ag_measure_sickness(pag);
> > + if (sick & XFS_HEALTH_AG_SB)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_SB;
>
> I'm starting to wonder whether "health" is the best term to use for the
> interface bits just because it reads a little weird to measure
> "sickness" and then apply all the sick state to something called
> "health." I don't have a better suggestion off the top of my head,
> though. Just something to think about a bit more from an API
> standpoint..
I had the same conundrum. I guess we could start the bitset with -1 and
clear bits when scrub says they've gone bad? That would be much clearer
with regards to the names, but technically we don't know the health of a
structure until we scan it, so I wouldn't want to represent the fs as
being "healthy" having not actually looked for problems.
What we /really/ need is a tri-state bitset[1]:
enum Bool
{
True,
False,
FileNotFound
};
But maybe I will try renaming all this to "sick" again.
if (sick & XFS_SICK_AG_AGF)
ageo->ag_sick |= XFS_AG_GEOM_SICK_AG_AGF;
Gosh. That second name is gross. XFS_AG_GEOM_SICK_AGF.
Sick sick sick sick sick. Ok, I've convinced myself of the name change. :P
--D
[1] https://thedailywtf.com/articles/What_Is_Truth_0x3f_
> Brian
>
> > + if (sick & XFS_HEALTH_AG_AGF)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_AGF;
> > + if (sick & XFS_HEALTH_AG_AGFL)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_AGFL;
> > + if (sick & XFS_HEALTH_AG_AGI)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_AGI;
> > + if (sick & XFS_HEALTH_AG_BNOBT)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_BNOBT;
> > + if (sick & XFS_HEALTH_AG_CNTBT)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_CNTBT;
> > + if (sick & XFS_HEALTH_AG_INOBT)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_INOBT;
> > + if (sick & XFS_HEALTH_AG_FINOBT)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_FINOBT;
> > + if (sick & XFS_HEALTH_AG_RMAPBT)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_RMAPBT;
> > + if (sick & XFS_HEALTH_AG_REFCNTBT)
> > + ageo->ag_health |= XFS_AG_GEOM_HEALTH_AG_REFCNTBT;
> > + xfs_perag_put(pag);
> > +}
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index f9bf11b6a055..f1fc5e53cfc1 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -853,6 +853,8 @@ xfs_ioc_ag_geometry(
> > if (error)
> > return error;
> >
> > + xfs_ag_geom_health(mp, ageo.ag_number, &ageo);
> > +
> > if (copy_to_user(arg, &ageo, sizeof(ageo)))
> > return -EFAULT;
> > return 0;
> >
