Re: [PATCH] xfs: fix agno increment in xfs_inumbers() loop

[Dave Chinner <david@fromorbit.com>]

On Sun, Oct 12, 2014 at 01:26:17PM -0500, Eric Sandeen wrote:
> commit c7cb51d xfs: fix error handling at xfs_inumbers
> caused a regression in xfs_inumbers, which in turn broke
> xfsdump, causing incomplete dumps.
> 
> The loop in xfs_inumbers() needs to fill the user-supplied
> buffers, and iterates via xfs_btree_increment, reading new
> ags as needed.
> 
> But the first time through the loop, if xfs_btree_increment()
> succeeds, we continue, which triggers the ++agno at the bottom
> of the loop, and we skip to soon to the next ag - without
> the proper setup under next_ag to read the next ag.
> 
> Fix this by removing the agno increment from the loop conditional,
> and only increment agno if we have actually hit the code under
> the next_ag: target.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> p.s. it's alarming that apparently no test in the dump group
> detects this problem!  I'll look into it.

The dump tests must not create more than an inode chunks worth
of files in each AG. Yup, see common/dump::_mk_fillconfig1() - maybe
35 files?

> So I can say that I've tested this with xfstests, but I guess
> that doesn't matter much, yet.
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index f71be9c..f1deb96 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -639,7 +639,8 @@ next_ag:
>  		xfs_buf_relse(agbp);
>  		agbp = NULL;
>  		agino = 0;
> -	} while (++agno < mp->m_sb.sb_agcount);
> +		agno++;
> +	} while (agno < mp->m_sb.sb_agcount);

Yeah, it fixes the bug, but I don't like the structure of the code
that leads to it. What we actually have it two loops in one:

	do {
		while (inobt records exist) {
			/* format record */
		}

	} while (++agno < mp->m_sb.sb_agcount)

But the inner loop is not implemented as a loop, it's implemented
split across two iterations on the outer loop (i.e.  increment in
one iteration, formatting in the next). Hence the bug.

The code should really be further factored to:

	do {

		/* read agi */
		error = xfs_ialloc_read_agi(agno, &agbp)
		if (error)
			break;

		error = xfs_inumbers_ag(...);
		if (error)
			break;

		xfs_buf_relse(agbp);
		agino = 0;
		agno++;
	} while (agno < mp->m_sb.sb_agcount)

So that the AG is traversed and records processed within it's own
cursor-based traversal loop inside xfs_inumbers_ag(), not split
across the outer ag scope loop.

I'll take the first patch right away, but can you follow up with
the above refactoring, Eric?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs