Re: [PATCH v3 18/21] xfs: cross-reference the rmapbt data with the refcountbt

[Dave Chinner <david@fromorbit.com>]

On Tue, Jan 16, 2018 at 03:26:28PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Cross reference the refcount data with the rmap data to check that the
> number of rmaps for a given block match the refcount of that block, and
> that CoW blocks (which are owned entirely by the refcountbt) are tracked
> as well.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v3: fix indenting and standardize helper naming, refactor helpers per dave'sr
>     suggestion and add clarifying comments
> v2: streamline scrubber arguments, remove stack allocated objects
> ---
>  fs/xfs/scrub/refcount.c |  340 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 338 insertions(+), 2 deletions(-)

One minor thing:

> +	while (!list_empty(&refchk->fragments)) {
> +		/* Discard any fragments ending at rbno from the worklist. */
> +		nr = 0;
> +		next_rbno = NULLAGBLOCK;
> +		list_for_each_entry_safe(frag, n, &worklist, list) {
> +			bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> +			if (bno != rbno) {
> +				if (bno < next_rbno)
> +					next_rbno = bno;
> +				continue;
> +			}
> +			list_del(&frag->list);
> +			kmem_free(frag);
> +			nr++;
> +		}
> +
> +		/* Empty list?  We're done. */
> +		if (list_empty(&refchk->fragments))
> +			break;

We haven't modified the &refchk->fragments list since the check at
the start of the loop, so this seems redundant and could be removed?

Other than that, I found this much easier to understand the second
time through. A bit of time for it to sink in and a few small
changes to logic and comments has done wonders :)

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com