xfs: fix agno increment in xfs_inumbers() loop
PATCH 1/1 xfs: fix agno increment in xfs_inumbers() loop
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.
---
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.
fs/xfs/xfs_itable.c +2 -1
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 17 lines · Eric Sandeen
commit c7cb51d xfs: fix error handling at
xfs_inumberscaused a regression inxfs_inumbers, which in turn broke xfsdump, causing incomplete dumps.The loop in
xfs_inumbers()needs to fill the user-supplied buffers, and iterates viaxfs_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 undernext_agto 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.
Quoted 6 lines
Signed-off-by Eric Sandeen <[email protected]>---
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?
Quoted 8 lines
So I can say that I've tested this with xfstests, but I guess that doesn't matter much, yet.
fs/xfs/xfs_itable.c +0
-0
fs/xfs/xfs_itable.c next_ag: · line 643
Quoted 6 lines
fs/xfs/xfs_itable.c +2
-1@@ -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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs