| Message ID | 20241014060516.245606-7-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [1/6] xfs: pass the exact range to initialize to xfs_initialize_perag | expand |
On Mon, Oct 14, 2024 at 08:04:55AM +0200, Christoph Hellwig wrote: > Currently log recovery never updates the in-core perag values for the > last allocation group when they were grown by growfs. This leads to > btree record validation failures for the alloc, ialloc or finotbt > trees if a transaction references this new space. > > Found by Brian's new growfs recovery stress test. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Thanks for tracking this down. The test now passes here as well. I'll try to get it polished up and posted soon. > fs/xfs/libxfs/xfs_ag.c | 17 +++++++++++++++++ > fs/xfs/libxfs/xfs_ag.h | 1 + > fs/xfs/xfs_buf_item_recover.c | 19 ++++++++++++++++--- > 3 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c > index 25cec9dc10c941..5ca8d01068273d 100644 > --- a/fs/xfs/libxfs/xfs_ag.c > +++ b/fs/xfs/libxfs/xfs_ag.c > @@ -273,6 +273,23 @@ xfs_agino_range( > return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last); > } > Comment please. I.e., /* * Update the perag of the previous tail AG if it has been changed * during recovery (i.e. recovery of a growfs). */ > +int > +xfs_update_last_ag_size( > + struct xfs_mount *mp, > + xfs_agnumber_t prev_agcount) > +{ > + struct xfs_perag *pag = xfs_perag_grab(mp, prev_agcount - 1); > + > + if (!pag) > + return -EFSCORRUPTED; > + pag->block_count = __xfs_ag_block_count(mp, prev_agcount - 1, > + mp->m_sb.sb_agcount, mp->m_sb.sb_dblocks); > + __xfs_agino_range(mp, pag->block_count, &pag->agino_min, > + &pag->agino_max); > + xfs_perag_rele(pag); > + return 0; > +} > + > int > xfs_initialize_perag( > struct xfs_mount *mp, > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h > index 6e68d6a3161a0f..9edfe0e9643964 100644 > --- a/fs/xfs/libxfs/xfs_ag.h > +++ b/fs/xfs/libxfs/xfs_ag.h > @@ -150,6 +150,7 @@ int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t old_agcount, > void xfs_free_perag_range(struct xfs_mount *mp, xfs_agnumber_t first_agno, > xfs_agnumber_t end_agno); > int xfs_initialize_perag_data(struct xfs_mount *mp, xfs_agnumber_t agno); > +int xfs_update_last_ag_size(struct xfs_mount *mp, xfs_agnumber_t prev_agcount); > > /* Passive AG references */ > struct xfs_perag *xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno); > diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c > index a839ff5dcaa908..5180cbf5a90b4b 100644 > --- a/fs/xfs/xfs_buf_item_recover.c > +++ b/fs/xfs/xfs_buf_item_recover.c > @@ -708,6 +708,11 @@ xlog_recover_do_primary_sb_buffer( > > xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn); > > + if (orig_agcount == 0) { > + xfs_alert(mp, "Trying to grow file system without AGs"); > + return -EFSCORRUPTED; > + } > + > /* > * Update the in-core super block from the freshly recovered on-disk one. > */ > @@ -718,15 +723,23 @@ xlog_recover_do_primary_sb_buffer( > return -EFSCORRUPTED; > } > > + /* > + * Growfs can also grow the last existing AG. In this case we also need It can shrink the last AG as well, FWIW. > + * to update the length in the in-core perag structure and values > + * depending on it. > + */ > + error = xfs_update_last_ag_size(mp, orig_agcount); > + if (error) > + return error; > + > /* > * Initialize the new perags, and also update various block and inode > * allocator setting based off the number of AGs or total blocks. > * Because of the latter this also needs to happen if the agcount did > * not change. > */ > - error = xfs_initialize_perag(mp, orig_agcount, > - mp->m_sb.sb_agcount, mp->m_sb.sb_dblocks, > - &mp->m_maxagi); > + error = xfs_initialize_perag(mp, orig_agcount, mp->m_sb.sb_agcount, > + mp->m_sb.sb_dblocks, &mp->m_maxagi); Seems like this should be folded into an earlier patch? With the nits addressed: Reviewed-by: Brian Foster <bfoster@redhat.com> > if (error) { > xfs_warn(mp, "Failed recovery per-ag init: %d", error); > return error; > -- > 2.45.2 > >
On Tue, Oct 15, 2024 at 09:11:41AM -0400, Brian Foster wrote: > > index 25cec9dc10c941..5ca8d01068273d 100644 > > --- a/fs/xfs/libxfs/xfs_ag.c > > +++ b/fs/xfs/libxfs/xfs_ag.c > > @@ -273,6 +273,23 @@ xfs_agino_range( > > return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last); > > } > > > > Comment please. I.e., > > /* > * Update the perag of the previous tail AG if it has been changed > * during recovery (i.e. recovery of a growfs). > */ Sure. > > + /* > > + * Growfs can also grow the last existing AG. In this case we also need > > It can shrink the last AG as well, FWIW. Indeed, I keep forgetting about the weird special case partial shrink that we support. > > - error = xfs_initialize_perag(mp, orig_agcount, > > - mp->m_sb.sb_agcount, mp->m_sb.sb_dblocks, > > - &mp->m_maxagi); > > + error = xfs_initialize_perag(mp, orig_agcount, mp->m_sb.sb_agcount, > > + mp->m_sb.sb_dblocks, &mp->m_maxagi); > > Seems like this should be folded into an earlier patch? Yes.
diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c index 25cec9dc10c941..5ca8d01068273d 100644 --- a/fs/xfs/libxfs/xfs_ag.c +++ b/fs/xfs/libxfs/xfs_ag.c @@ -273,6 +273,23 @@ xfs_agino_range( return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last); } +int +xfs_update_last_ag_size( + struct xfs_mount *mp, + xfs_agnumber_t prev_agcount) +{ + struct xfs_perag *pag = xfs_perag_grab(mp, prev_agcount - 1); + + if (!pag) + return -EFSCORRUPTED; + pag->block_count = __xfs_ag_block_count(mp, prev_agcount - 1, + mp->m_sb.sb_agcount, mp->m_sb.sb_dblocks); + __xfs_agino_range(mp, pag->block_count, &pag->agino_min, + &pag->agino_max); + xfs_perag_rele(pag); + return 0; +} + int xfs_initialize_perag( struct xfs_mount *mp, diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h index 6e68d6a3161a0f..9edfe0e9643964 100644 --- a/fs/xfs/libxfs/xfs_ag.h +++ b/fs/xfs/libxfs/xfs_ag.h @@ -150,6 +150,7 @@ int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t old_agcount, void xfs_free_perag_range(struct xfs_mount *mp, xfs_agnumber_t first_agno, xfs_agnumber_t end_agno); int xfs_initialize_perag_data(struct xfs_mount *mp, xfs_agnumber_t agno); +int xfs_update_last_ag_size(struct xfs_mount *mp, xfs_agnumber_t prev_agcount); /* Passive AG references */ struct xfs_perag *xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno); diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c index a839ff5dcaa908..5180cbf5a90b4b 100644 --- a/fs/xfs/xfs_buf_item_recover.c +++ b/fs/xfs/xfs_buf_item_recover.c @@ -708,6 +708,11 @@ xlog_recover_do_primary_sb_buffer( xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn); + if (orig_agcount == 0) { + xfs_alert(mp, "Trying to grow file system without AGs"); + return -EFSCORRUPTED; + } + /* * Update the in-core super block from the freshly recovered on-disk one. */ @@ -718,15 +723,23 @@ xlog_recover_do_primary_sb_buffer( return -EFSCORRUPTED; } + /* + * Growfs can also grow the last existing AG. In this case we also need + * to update the length in the in-core perag structure and values + * depending on it. + */ + error = xfs_update_last_ag_size(mp, orig_agcount); + if (error) + return error; + /* * Initialize the new perags, and also update various block and inode * allocator setting based off the number of AGs or total blocks. * Because of the latter this also needs to happen if the agcount did * not change. */ - error = xfs_initialize_perag(mp, orig_agcount, - mp->m_sb.sb_agcount, mp->m_sb.sb_dblocks, - &mp->m_maxagi); + error = xfs_initialize_perag(mp, orig_agcount, mp->m_sb.sb_agcount, + mp->m_sb.sb_dblocks, &mp->m_maxagi); if (error) { xfs_warn(mp, "Failed recovery per-ag init: %d", error); return error;
Currently log recovery never updates the in-core perag values for the last allocation group when they were grown by growfs. This leads to btree record validation failures for the alloc, ialloc or finotbt trees if a transaction references this new space. Found by Brian's new growfs recovery stress test. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_ag.c | 17 +++++++++++++++++ fs/xfs/libxfs/xfs_ag.h | 1 + fs/xfs/xfs_buf_item_recover.c | 19 ++++++++++++++++--- 3 files changed, 34 insertions(+), 3 deletions(-)