Re: [PATCH 04/25] xfs: create helpers to record and deal with scrub problems

[Dave Chinner <david@fromorbit.com>]

On Tue, Oct 03, 2017 at 01:41:14PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create helper functions to record crc and corruption problems, and
> deal with any other runtime errors that arise.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
....
> +/*
> + * Handling scrub corruption/optimization/warning checks.
> + *
> + * The *_set_{corrupt,preen,warning}() family of functions are used to
> + * record the presence of metadata that is incorrect (corrupt), could be
> + * optimized somehow (preen), or should be flagged for administrative
> + * review but is not incorrect (warn).
> + *
> + * ftrace can be used to record the precise metadata location and
> + * approximate code location of the failed check.
> + */
> +
> +/* Record a block which could be optimized. */
> +void
> +xfs_scrub_block_set_preen(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_buf			*bp)
> +{
> +	struct xfs_mount		*mp = sc->mp;
> +	xfs_fsblock_t			fsbno;
> +	xfs_agnumber_t			agno;
> +	xfs_agblock_t			bno;
> +
> +	fsbno = XFS_DADDR_TO_FSB(mp, bp->b_bn);
> +	agno = XFS_FSB_TO_AGNO(mp, fsbno);
> +	bno = XFS_FSB_TO_AGBNO(mp, fsbno);
> +
> +	sc->sm->sm_flags |= XFS_SCRUB_OFLAG_PREEN;
> +	trace_xfs_scrub_block_preen(sc, agno, bno, __return_address);

Is agno/agbno used here only for the trace point? if
so, the tracepoint call could simply be:

	trace_xfs_scrub_block_preen(sc, bp->b_bn, __return_address);

and the tracepoint internal implementation can split that into
agno/agbno.


> +}
> +
> +/*
> + * Record an inode which could be optimized.  The trace data will
> + * include the block given by bp if bp is given; otherwise it will use
> + * the block location of the inode record itself.
> + */
> +void
> +xfs_scrub_ino_set_preen(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_buf			*bp)
> +{
> +	struct xfs_inode		*ip = sc->ip;
> +	struct xfs_mount		*mp = sc->mp;
> +	xfs_fsblock_t			fsbno;
> +	xfs_agnumber_t			agno;
> +	xfs_agblock_t			bno;
> +
> +	if (bp) {
> +		fsbno = XFS_DADDR_TO_FSB(mp, bp->b_bn);
> +		agno = XFS_FSB_TO_AGNO(mp, fsbno);
> +		bno = XFS_FSB_TO_AGBNO(mp, fsbno);
> +	} else {
> +		agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
> +		bno = XFS_INO_TO_AGINO(mp, ip->i_ino);

That's not a block number. Going to be mighty confusing because the
trace output isn't going to tell you whether bno is an agbno or an
agino....

> +	}
> +
> +	sc->sm->sm_flags |= XFS_SCRUB_OFLAG_PREEN;
> +	trace_xfs_scrub_ino_preen(sc, ip->i_ino, agno, bno, __return_address);
> +}
> +
> +/* Record a corrupt block. */
> +void
> +xfs_scrub_block_set_corrupt(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_buf			*bp)
> +{
> +	struct xfs_mount		*mp = sc->mp;
> +	xfs_fsblock_t			fsbno;
> +	xfs_agnumber_t			agno;
> +	xfs_agblock_t			bno;
> +
> +	fsbno = XFS_DADDR_TO_FSB(mp, bp->b_bn);
> +	agno = XFS_FSB_TO_AGNO(mp, fsbno);
> +	bno = XFS_FSB_TO_AGBNO(mp, fsbno);
> +
> +	sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
> +	trace_xfs_scrub_block_error(sc, agno, bno, __return_address);
> +}
> +
> +/*
> + * Record a corrupt inode.  The trace data will include the block given
> + * by bp if bp is given; otherwise it will use the block location of the
> + * inode record itself.
> + */
> +void
> +xfs_scrub_ino_set_corrupt(
> +	struct xfs_scrub_context	*sc,
> +	xfs_ino_t			ino,
> +	struct xfs_buf			*bp)
> +{
> +	struct xfs_inode		*ip = sc->ip;
> +	struct xfs_mount		*mp = sc->mp;
> +	xfs_fsblock_t			fsbno;
> +	xfs_agnumber_t			agno;
> +	xfs_agblock_t			bno;
> +
> +	if (bp) {
> +		fsbno = XFS_DADDR_TO_FSB(mp, bp->b_bn);
> +		agno = XFS_FSB_TO_AGNO(mp, fsbno);
> +		bno = XFS_FSB_TO_AGBNO(mp, fsbno);
> +	} else {
> +		agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
> +		bno = XFS_INO_TO_AGINO(mp, ip->i_ino);
> +	}
> +
> +	sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
> +	trace_xfs_scrub_ino_error(sc, ino, agno, bno, __return_address);

Pattern seems to be repeated...

>  DEFINE_SCRUB_EVENT(xfs_scrub_start);
>  DEFINE_SCRUB_EVENT(xfs_scrub_done);
> +DEFINE_SCRUB_EVENT(xfs_scrub_deadlock_retry);
> +
> +TRACE_EVENT(xfs_scrub_op_error,
> +	TP_PROTO(struct xfs_scrub_context *sc, xfs_agnumber_t agno,
> +		 xfs_agblock_t bno, int error, void *ret_ip),
> +	TP_ARGS(sc, agno, bno, error, ret_ip),
> +	TP_STRUCT__entry(
> +		__field(dev_t, dev)
> +		__field(unsigned int, type)
> +		__field(xfs_agnumber_t, agno)
> +		__field(xfs_agblock_t, bno)
> +		__field(int, error)
> +		__field(void *, ret_ip)
> +	),
> +	TP_fast_assign(
> +		__entry->dev = sc->mp->m_super->s_dev;
> +		__entry->type = sc->sm->sm_type;
> +		__entry->agno = agno;
> +		__entry->bno = bno;
> +		__entry->error = error;
> +		__entry->ret_ip = ret_ip;

i.e. we can put the decoding in here, which makes it all conditional
on the tracepoint being enabled....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com