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

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

[Eric Sandeen]

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.

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);
 
 	if (!error) {
 		if (bufidx) {

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

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

[Dave Chinner]

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