| Message ID | 20240930164211.2357358-4-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/7] xfs: pass the exact range to initialize to xfs_initialize_perag | expand |
On Mon, Sep 30, 2024 at 06:41:44PM +0200, Christoph Hellwig wrote: > Primary superblock buffers that change the file system geometry after a > growfs operation can affect the operation of later CIL checkpoints that > make use of the newly added space and allocation groups. > > Apply the changes to the in-memory structures as part of recovery pass 2, > to ensure recovery works fine for such cases. > > In the future we should apply the logic to other updates such as features > bits as well. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_log_recover.h | 2 ++ > fs/xfs/xfs_buf_item_recover.c | 27 +++++++++++++++++++++++++++ > fs/xfs/xfs_log_recover.c | 27 +++++++++++++++++++-------- > 3 files changed, 48 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h > index 521d327e4c89ed..d0e13c84422d0a 100644 > --- a/fs/xfs/libxfs/xfs_log_recover.h > +++ b/fs/xfs/libxfs/xfs_log_recover.h > @@ -165,4 +165,6 @@ void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip, > int xlog_recover_finish_intent(struct xfs_trans *tp, > struct xfs_defer_pending *dfp); > > +int xlog_recover_update_agcount(struct xfs_mount *mp, struct xfs_dsb *dsb); > + > #endif /* __XFS_LOG_RECOVER_H__ */ > diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c > index 09e893cf563cb9..08c129022304a8 100644 > --- a/fs/xfs/xfs_buf_item_recover.c > +++ b/fs/xfs/xfs_buf_item_recover.c > @@ -684,6 +684,28 @@ xlog_recover_do_inode_buffer( > return 0; > } > > +static int > +xlog_recover_do_sb_buffer( > + struct xfs_mount *mp, > + struct xlog_recover_item *item, > + struct xfs_buf *bp, > + struct xfs_buf_log_format *buf_f, > + xfs_lsn_t current_lsn) > +{ > + xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn); > + > + /* > + * Update the in-memory superblock and perag structures from the > + * primary SB buffer. > + * > + * This is required because transactions running after growfs may require > + * the updated values to be set in a previous fully commit transaction. > + */ > + if (xfs_buf_daddr(bp) != 0) > + return 0; > + return xlog_recover_update_agcount(mp, bp->b_addr); > +} > + > /* > * V5 filesystems know the age of the buffer on disk being recovered. We can > * have newer objects on disk than we are replaying, and so for these cases we > @@ -967,6 +989,11 @@ xlog_recover_buf_commit_pass2( > dirty = xlog_recover_do_dquot_buffer(mp, log, item, bp, buf_f); > if (!dirty) > goto out_release; > + } else if (xfs_blft_from_flags(buf_f) & XFS_BLFT_SB_BUF) { > + error = xlog_recover_do_sb_buffer(mp, item, bp, buf_f, > + current_lsn); > + if (error) > + goto out_release; > } else { > xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn); > } > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 6a165ca55da1a8..03701409c7dcd6 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -3334,6 +3334,25 @@ xlog_do_log_recovery( > return error; > } > > +int > +xlog_recover_update_agcount( > + struct xfs_mount *mp, > + struct xfs_dsb *dsb) > +{ > + xfs_agnumber_t old_agcount = mp->m_sb.sb_agcount; > + int error; > + > + xfs_sb_from_disk(&mp->m_sb, dsb); If I'm understanding this correctly, the incore superblock gets updated every time recovery finds a logged primary superblock buffer now, instead of once at the end of log recovery, right? Shouldn't this conversion be done in the caller? Some day we're going to want to do the same with xfs_initialize_rtgroups(), right? --D > + error = xfs_initialize_perag(mp, old_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; > + } > + mp->m_alloc_set_aside = xfs_alloc_set_aside(mp); > + return 0; > +} > + > /* > * Do the actual recovery > */ > @@ -3346,7 +3365,6 @@ xlog_do_recover( > struct xfs_mount *mp = log->l_mp; > struct xfs_buf *bp = mp->m_sb_bp; > struct xfs_sb *sbp = &mp->m_sb; > - xfs_agnumber_t old_agcount = sbp->sb_agcount; > int error; > > trace_xfs_log_recover(log, head_blk, tail_blk); > @@ -3394,13 +3412,6 @@ xlog_do_recover( > /* re-initialise in-core superblock and geometry structures */ > mp->m_features |= xfs_sb_version_to_features(sbp); > xfs_reinit_percpu_counters(mp); > - error = xfs_initialize_perag(mp, old_agcount, sbp->sb_agcount, > - sbp->sb_dblocks, &mp->m_maxagi); > - if (error) { > - xfs_warn(mp, "Failed post-recovery per-ag init: %d", error); > - return error; > - } > - mp->m_alloc_set_aside = xfs_alloc_set_aside(mp); > > /* Normal transactions can now occur */ > clear_bit(XLOG_ACTIVE_RECOVERY, &log->l_opstate); > -- > 2.45.2 > >
On Mon, Sep 30, 2024 at 09:50:19AM -0700, Darrick J. Wong wrote: > > +int > > +xlog_recover_update_agcount( > > + struct xfs_mount *mp, > > + struct xfs_dsb *dsb) > > +{ > > + xfs_agnumber_t old_agcount = mp->m_sb.sb_agcount; > > + int error; > > + > > + xfs_sb_from_disk(&mp->m_sb, dsb); > > If I'm understanding this correctly, the incore superblock gets updated > every time recovery finds a logged primary superblock buffer now, > instead of once at the end of log recovery, right? Yes. > Shouldn't this conversion be done in the caller? Some day we're going > to want to do the same with xfs_initialize_rtgroups(), right? Yeah. But the right "fix" for that is probably to just rename this function :) Probably even for the next repost instead of waiting for more features.
On Mon, Sep 30, 2024 at 06:41:44PM +0200, Christoph Hellwig wrote: > Primary superblock buffers that change the file system geometry after a > growfs operation can affect the operation of later CIL checkpoints that > make use of the newly added space and allocation groups. > > Apply the changes to the in-memory structures as part of recovery pass 2, > to ensure recovery works fine for such cases. > > In the future we should apply the logic to other updates such as features > bits as well. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_log_recover.h | 2 ++ > fs/xfs/xfs_buf_item_recover.c | 27 +++++++++++++++++++++++++++ > fs/xfs/xfs_log_recover.c | 27 +++++++++++++++++++-------- > 3 files changed, 48 insertions(+), 8 deletions(-) > ... > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 6a165ca55da1a8..03701409c7dcd6 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -3334,6 +3334,25 @@ xlog_do_log_recovery( > return error; > } > > +int > +xlog_recover_update_agcount( > + struct xfs_mount *mp, > + struct xfs_dsb *dsb) > +{ > + xfs_agnumber_t old_agcount = mp->m_sb.sb_agcount; > + int error; > + > + xfs_sb_from_disk(&mp->m_sb, dsb); > + error = xfs_initialize_perag(mp, old_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; > + } > + mp->m_alloc_set_aside = xfs_alloc_set_aside(mp); Re: my comments on patch 1, it looks like this also doesn't need to change unless the superblock update actually changed the AG count. Otherwise seems Ok in terms of the context change. Brian > + return 0; > +} > + > /* > * Do the actual recovery > */ > @@ -3346,7 +3365,6 @@ xlog_do_recover( > struct xfs_mount *mp = log->l_mp; > struct xfs_buf *bp = mp->m_sb_bp; > struct xfs_sb *sbp = &mp->m_sb; > - xfs_agnumber_t old_agcount = sbp->sb_agcount; > int error; > > trace_xfs_log_recover(log, head_blk, tail_blk); > @@ -3394,13 +3412,6 @@ xlog_do_recover( > /* re-initialise in-core superblock and geometry structures */ > mp->m_features |= xfs_sb_version_to_features(sbp); > xfs_reinit_percpu_counters(mp); > - error = xfs_initialize_perag(mp, old_agcount, sbp->sb_agcount, > - sbp->sb_dblocks, &mp->m_maxagi); > - if (error) { > - xfs_warn(mp, "Failed post-recovery per-ag init: %d", error); > - return error; > - } > - mp->m_alloc_set_aside = xfs_alloc_set_aside(mp); > > /* Normal transactions can now occur */ > clear_bit(XLOG_ACTIVE_RECOVERY, &log->l_opstate); > -- > 2.45.2 > >
On Tue, Oct 01, 2024 at 10:49:18AM +0200, Christoph Hellwig wrote: > On Mon, Sep 30, 2024 at 09:50:19AM -0700, Darrick J. Wong wrote: > > > +int > > > +xlog_recover_update_agcount( > > > + struct xfs_mount *mp, > > > + struct xfs_dsb *dsb) > > > +{ > > > + xfs_agnumber_t old_agcount = mp->m_sb.sb_agcount; > > > + int error; > > > + > > > + xfs_sb_from_disk(&mp->m_sb, dsb); > > > > If I'm understanding this correctly, the incore superblock gets updated > > every time recovery finds a logged primary superblock buffer now, > > instead of once at the end of log recovery, right? > > Yes. > > > Shouldn't this conversion be done in the caller? Some day we're going > > to want to do the same with xfs_initialize_rtgroups(), right? > > Yeah. But the right "fix" for that is probably to just rename > this function :) Probably even for the next repost instead of > waiting for more features. Forgot to reply to this, but yes, how about xlog_recover_update_group_count? --D
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h index 521d327e4c89ed..d0e13c84422d0a 100644 --- a/fs/xfs/libxfs/xfs_log_recover.h +++ b/fs/xfs/libxfs/xfs_log_recover.h @@ -165,4 +165,6 @@ void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip, int xlog_recover_finish_intent(struct xfs_trans *tp, struct xfs_defer_pending *dfp); +int xlog_recover_update_agcount(struct xfs_mount *mp, struct xfs_dsb *dsb); + #endif /* __XFS_LOG_RECOVER_H__ */ diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c index 09e893cf563cb9..08c129022304a8 100644 --- a/fs/xfs/xfs_buf_item_recover.c +++ b/fs/xfs/xfs_buf_item_recover.c @@ -684,6 +684,28 @@ xlog_recover_do_inode_buffer( return 0; } +static int +xlog_recover_do_sb_buffer( + struct xfs_mount *mp, + struct xlog_recover_item *item, + struct xfs_buf *bp, + struct xfs_buf_log_format *buf_f, + xfs_lsn_t current_lsn) +{ + xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn); + + /* + * Update the in-memory superblock and perag structures from the + * primary SB buffer. + * + * This is required because transactions running after growfs may require + * the updated values to be set in a previous fully commit transaction. + */ + if (xfs_buf_daddr(bp) != 0) + return 0; + return xlog_recover_update_agcount(mp, bp->b_addr); +} + /* * V5 filesystems know the age of the buffer on disk being recovered. We can * have newer objects on disk than we are replaying, and so for these cases we @@ -967,6 +989,11 @@ xlog_recover_buf_commit_pass2( dirty = xlog_recover_do_dquot_buffer(mp, log, item, bp, buf_f); if (!dirty) goto out_release; + } else if (xfs_blft_from_flags(buf_f) & XFS_BLFT_SB_BUF) { + error = xlog_recover_do_sb_buffer(mp, item, bp, buf_f, + current_lsn); + if (error) + goto out_release; } else { xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn); } diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 6a165ca55da1a8..03701409c7dcd6 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -3334,6 +3334,25 @@ xlog_do_log_recovery( return error; } +int +xlog_recover_update_agcount( + struct xfs_mount *mp, + struct xfs_dsb *dsb) +{ + xfs_agnumber_t old_agcount = mp->m_sb.sb_agcount; + int error; + + xfs_sb_from_disk(&mp->m_sb, dsb); + error = xfs_initialize_perag(mp, old_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; + } + mp->m_alloc_set_aside = xfs_alloc_set_aside(mp); + return 0; +} + /* * Do the actual recovery */ @@ -3346,7 +3365,6 @@ xlog_do_recover( struct xfs_mount *mp = log->l_mp; struct xfs_buf *bp = mp->m_sb_bp; struct xfs_sb *sbp = &mp->m_sb; - xfs_agnumber_t old_agcount = sbp->sb_agcount; int error; trace_xfs_log_recover(log, head_blk, tail_blk); @@ -3394,13 +3412,6 @@ xlog_do_recover( /* re-initialise in-core superblock and geometry structures */ mp->m_features |= xfs_sb_version_to_features(sbp); xfs_reinit_percpu_counters(mp); - error = xfs_initialize_perag(mp, old_agcount, sbp->sb_agcount, - sbp->sb_dblocks, &mp->m_maxagi); - if (error) { - xfs_warn(mp, "Failed post-recovery per-ag init: %d", error); - return error; - } - mp->m_alloc_set_aside = xfs_alloc_set_aside(mp); /* Normal transactions can now occur */ clear_bit(XLOG_ACTIVE_RECOVERY, &log->l_opstate);
Primary superblock buffers that change the file system geometry after a growfs operation can affect the operation of later CIL checkpoints that make use of the newly added space and allocation groups. Apply the changes to the in-memory structures as part of recovery pass 2, to ensure recovery works fine for such cases. In the future we should apply the logic to other updates such as features bits as well. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_log_recover.h | 2 ++ fs/xfs/xfs_buf_item_recover.c | 27 +++++++++++++++++++++++++++ fs/xfs/xfs_log_recover.c | 27 +++++++++++++++++++-------- 3 files changed, 48 insertions(+), 8 deletions(-)