| Message ID | 20220627004336.217366-4-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: in-memory iunlink items | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, Jun 27, 2022 at 10:43:30AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > For upcoming changes to the way inode unlinked list processing is > done, the structure of recovery needs to change slightly. We also > really need to untangle the messy error handling in list recovery > so that actions like emptying the bucket on inode lookup failure > are associated with the bucket list walk failing, not failing > to look up the inode. > > Refactor the recovery code now to keep the re-organisation seperate > to the algorithm changes. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_log_recover.c | 162 ++++++++++++++++++++------------------- > 1 file changed, 84 insertions(+), 78 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index f360b46533a6..7d0f530d7e3c 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -2627,23 +2627,23 @@ xlog_recover_cancel_intents( > * This routine performs a transaction to null out a bad inode pointer > * in an agi unlinked inode hash bucket. > */ > -STATIC void > +static void > xlog_recover_clear_agi_bucket( > - xfs_mount_t *mp, > - xfs_agnumber_t agno, > - int bucket) > + struct xfs_mount *mp, > + struct xfs_perag *pag, Why even pass in *mp when you've got *pag? > + int bucket) > { > - xfs_trans_t *tp; > - xfs_agi_t *agi; > - struct xfs_buf *agibp; > - int offset; > - int error; > + struct xfs_trans *tp; > + struct xfs_agi *agi; > + struct xfs_buf *agibp; > + int offset; > + int error; > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_clearagi, 0, 0, 0, &tp); > if (error) > goto out_error; > > - error = xfs_read_agi(mp, tp, agno, &agibp); > + error = xfs_read_agi(mp, tp, pag->pag_agno, &agibp); > if (error) > goto out_abort; > > @@ -2662,46 +2662,40 @@ xlog_recover_clear_agi_bucket( > out_abort: > xfs_trans_cancel(tp); > out_error: > - xfs_warn(mp, "%s: failed to clear agi %d. Continuing.", __func__, agno); > + xfs_warn(mp, "%s: failed to clear agi %d. Continuing.", > + __func__, pag->pag_agno); > return; Nit: Don't need the return here. Otherwise looks good. --D > } > > -STATIC xfs_agino_t > -xlog_recover_process_one_iunlink( > - struct xfs_mount *mp, > - xfs_agnumber_t agno, > - xfs_agino_t agino, > - int bucket) > +static int > +xlog_recover_iunlink_bucket( > + struct xfs_mount *mp, > + struct xfs_perag *pag, > + struct xfs_agi *agi, > + int bucket) > { > - struct xfs_inode *ip; > - xfs_ino_t ino; > - int error; > + struct xfs_inode *ip; > + xfs_agino_t agino; > > - ino = XFS_AGINO_TO_INO(mp, agno, agino); > - error = xfs_iget(mp, NULL, ino, 0, 0, &ip); > - if (error) > - goto fail; > + agino = be32_to_cpu(agi->agi_unlinked[bucket]); > + while (agino != NULLAGINO) { > + int error; > > - xfs_iflags_clear(ip, XFS_IRECOVERY); > - ASSERT(VFS_I(ip)->i_nlink == 0); > - ASSERT(VFS_I(ip)->i_mode != 0); > + error = xfs_iget(mp, NULL, > + XFS_AGINO_TO_INO(mp, pag->pag_agno, agino), > + 0, 0, &ip); > + if (error) > + return error;; > > - /* setup for the next pass */ > - agino = ip->i_next_unlinked; > - xfs_irele(ip); > - return agino; > + ASSERT(VFS_I(ip)->i_nlink == 0); > + ASSERT(VFS_I(ip)->i_mode != 0); > + xfs_iflags_clear(ip, XFS_IRECOVERY); > + agino = ip->i_next_unlinked; > > - fail: > - /* > - * We can't read in the inode this bucket points to, or this inode > - * is messed up. Just ditch this bucket of inodes. We will lose > - * some inodes and space, but at least we won't hang. > - * > - * Call xlog_recover_clear_agi_bucket() to perform a transaction to > - * clear the inode pointer in the bucket. > - */ > - xlog_recover_clear_agi_bucket(mp, agno, bucket); > - return NULLAGINO; > + xfs_irele(ip); > + cond_resched(); > + } > + return 0; > } > > /* > @@ -2727,51 +2721,63 @@ xlog_recover_process_one_iunlink( > * scheduled on this CPU to ensure other scheduled work can run without undue > * latency. > */ > -STATIC void > -xlog_recover_process_iunlinks( > - struct xlog *log) > +static void > +xlog_recover_iunlink_ag( > + struct xfs_mount *mp, > + struct xfs_perag *pag) > { > - struct xfs_mount *mp = log->l_mp; > - struct xfs_perag *pag; > - xfs_agnumber_t agno; > struct xfs_agi *agi; > struct xfs_buf *agibp; > - xfs_agino_t agino; > int bucket; > int error; > > - for_each_perag(mp, agno, pag) { > - error = xfs_read_agi(mp, NULL, pag->pag_agno, &agibp); > + error = xfs_read_agi(mp, NULL, pag->pag_agno, &agibp); > + if (error) { > + /* > + * AGI is b0rked. Don't process it. > + * > + * We should probably mark the filesystem as corrupt after we've > + * recovered all the ag's we can.... > + */ > + return; > + } > + > + /* > + * Unlock the buffer so that it can be acquired in the normal course of > + * the transaction to truncate and free each inode. Because we are not > + * racing with anyone else here for the AGI buffer, we don't even need > + * to hold it locked to read the initial unlinked bucket entries out of > + * the buffer. We keep buffer reference though, so that it stays pinned > + * in memory while we need the buffer. > + */ > + agi = agibp->b_addr; > + xfs_buf_unlock(agibp); > + > + for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) { > + error = xlog_recover_iunlink_bucket(mp, pag, agi, bucket); > if (error) { > /* > - * AGI is b0rked. Don't process it. > - * > - * We should probably mark the filesystem as corrupt > - * after we've recovered all the ag's we can.... > + * Bucket is unrecoverable, so only a repair scan can > + * free the remaining unlinked inodes. Just empty the > + * bucket and remaining inodes on it unreferenced and > + * unfreeable. > */ > - continue; > + xlog_recover_clear_agi_bucket(mp, pag, bucket); > } > - /* > - * Unlock the buffer so that it can be acquired in the normal > - * course of the transaction to truncate and free each inode. > - * Because we are not racing with anyone else here for the AGI > - * buffer, we don't even need to hold it locked to read the > - * initial unlinked bucket entries out of the buffer. We keep > - * buffer reference though, so that it stays pinned in memory > - * while we need the buffer. > - */ > - agi = agibp->b_addr; > - xfs_buf_unlock(agibp); > - > - for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) { > - agino = be32_to_cpu(agi->agi_unlinked[bucket]); > - while (agino != NULLAGINO) { > - agino = xlog_recover_process_one_iunlink(mp, > - pag->pag_agno, agino, bucket); > - cond_resched(); > - } > - } > - xfs_buf_rele(agibp); > + } > + > + xfs_buf_rele(agibp); > +} > + > +static void > +xlog_recover_process_iunlinks( > + struct xlog *log) > +{ > + struct xfs_perag *pag; > + xfs_agnumber_t agno; > + > + for_each_perag(log->l_mp, agno, pag) { > + xlog_recover_iunlink_ag(log->l_mp, pag); > } > > /* > @@ -2779,7 +2785,7 @@ xlog_recover_process_iunlinks( > * are fully completed on disk and the incore inodes can be reclaimed > * before we signal that recovery is complete. > */ > - xfs_inodegc_flush(mp); > + xfs_inodegc_flush(log->l_mp); > } > > STATIC void > -- > 2.36.1 >
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index f360b46533a6..7d0f530d7e3c 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -2627,23 +2627,23 @@ xlog_recover_cancel_intents( * This routine performs a transaction to null out a bad inode pointer * in an agi unlinked inode hash bucket. */ -STATIC void +static void xlog_recover_clear_agi_bucket( - xfs_mount_t *mp, - xfs_agnumber_t agno, - int bucket) + struct xfs_mount *mp, + struct xfs_perag *pag, + int bucket) { - xfs_trans_t *tp; - xfs_agi_t *agi; - struct xfs_buf *agibp; - int offset; - int error; + struct xfs_trans *tp; + struct xfs_agi *agi; + struct xfs_buf *agibp; + int offset; + int error; error = xfs_trans_alloc(mp, &M_RES(mp)->tr_clearagi, 0, 0, 0, &tp); if (error) goto out_error; - error = xfs_read_agi(mp, tp, agno, &agibp); + error = xfs_read_agi(mp, tp, pag->pag_agno, &agibp); if (error) goto out_abort; @@ -2662,46 +2662,40 @@ xlog_recover_clear_agi_bucket( out_abort: xfs_trans_cancel(tp); out_error: - xfs_warn(mp, "%s: failed to clear agi %d. Continuing.", __func__, agno); + xfs_warn(mp, "%s: failed to clear agi %d. Continuing.", + __func__, pag->pag_agno); return; } -STATIC xfs_agino_t -xlog_recover_process_one_iunlink( - struct xfs_mount *mp, - xfs_agnumber_t agno, - xfs_agino_t agino, - int bucket) +static int +xlog_recover_iunlink_bucket( + struct xfs_mount *mp, + struct xfs_perag *pag, + struct xfs_agi *agi, + int bucket) { - struct xfs_inode *ip; - xfs_ino_t ino; - int error; + struct xfs_inode *ip; + xfs_agino_t agino; - ino = XFS_AGINO_TO_INO(mp, agno, agino); - error = xfs_iget(mp, NULL, ino, 0, 0, &ip); - if (error) - goto fail; + agino = be32_to_cpu(agi->agi_unlinked[bucket]); + while (agino != NULLAGINO) { + int error; - xfs_iflags_clear(ip, XFS_IRECOVERY); - ASSERT(VFS_I(ip)->i_nlink == 0); - ASSERT(VFS_I(ip)->i_mode != 0); + error = xfs_iget(mp, NULL, + XFS_AGINO_TO_INO(mp, pag->pag_agno, agino), + 0, 0, &ip); + if (error) + return error;; - /* setup for the next pass */ - agino = ip->i_next_unlinked; - xfs_irele(ip); - return agino; + ASSERT(VFS_I(ip)->i_nlink == 0); + ASSERT(VFS_I(ip)->i_mode != 0); + xfs_iflags_clear(ip, XFS_IRECOVERY); + agino = ip->i_next_unlinked; - fail: - /* - * We can't read in the inode this bucket points to, or this inode - * is messed up. Just ditch this bucket of inodes. We will lose - * some inodes and space, but at least we won't hang. - * - * Call xlog_recover_clear_agi_bucket() to perform a transaction to - * clear the inode pointer in the bucket. - */ - xlog_recover_clear_agi_bucket(mp, agno, bucket); - return NULLAGINO; + xfs_irele(ip); + cond_resched(); + } + return 0; } /* @@ -2727,51 +2721,63 @@ xlog_recover_process_one_iunlink( * scheduled on this CPU to ensure other scheduled work can run without undue * latency. */ -STATIC void -xlog_recover_process_iunlinks( - struct xlog *log) +static void +xlog_recover_iunlink_ag( + struct xfs_mount *mp, + struct xfs_perag *pag) { - struct xfs_mount *mp = log->l_mp; - struct xfs_perag *pag; - xfs_agnumber_t agno; struct xfs_agi *agi; struct xfs_buf *agibp; - xfs_agino_t agino; int bucket; int error; - for_each_perag(mp, agno, pag) { - error = xfs_read_agi(mp, NULL, pag->pag_agno, &agibp); + error = xfs_read_agi(mp, NULL, pag->pag_agno, &agibp); + if (error) { + /* + * AGI is b0rked. Don't process it. + * + * We should probably mark the filesystem as corrupt after we've + * recovered all the ag's we can.... + */ + return; + } + + /* + * Unlock the buffer so that it can be acquired in the normal course of + * the transaction to truncate and free each inode. Because we are not + * racing with anyone else here for the AGI buffer, we don't even need + * to hold it locked to read the initial unlinked bucket entries out of + * the buffer. We keep buffer reference though, so that it stays pinned + * in memory while we need the buffer. + */ + agi = agibp->b_addr; + xfs_buf_unlock(agibp); + + for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) { + error = xlog_recover_iunlink_bucket(mp, pag, agi, bucket); if (error) { /* - * AGI is b0rked. Don't process it. - * - * We should probably mark the filesystem as corrupt - * after we've recovered all the ag's we can.... + * Bucket is unrecoverable, so only a repair scan can + * free the remaining unlinked inodes. Just empty the + * bucket and remaining inodes on it unreferenced and + * unfreeable. */ - continue; + xlog_recover_clear_agi_bucket(mp, pag, bucket); } - /* - * Unlock the buffer so that it can be acquired in the normal - * course of the transaction to truncate and free each inode. - * Because we are not racing with anyone else here for the AGI - * buffer, we don't even need to hold it locked to read the - * initial unlinked bucket entries out of the buffer. We keep - * buffer reference though, so that it stays pinned in memory - * while we need the buffer. - */ - agi = agibp->b_addr; - xfs_buf_unlock(agibp); - - for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) { - agino = be32_to_cpu(agi->agi_unlinked[bucket]); - while (agino != NULLAGINO) { - agino = xlog_recover_process_one_iunlink(mp, - pag->pag_agno, agino, bucket); - cond_resched(); - } - } - xfs_buf_rele(agibp); + } + + xfs_buf_rele(agibp); +} + +static void +xlog_recover_process_iunlinks( + struct xlog *log) +{ + struct xfs_perag *pag; + xfs_agnumber_t agno; + + for_each_perag(log->l_mp, agno, pag) { + xlog_recover_iunlink_ag(log->l_mp, pag); } /* @@ -2779,7 +2785,7 @@ xlog_recover_process_iunlinks( * are fully completed on disk and the incore inodes can be reclaimed * before we signal that recovery is complete. */ - xfs_inodegc_flush(mp); + xfs_inodegc_flush(log->l_mp); } STATIC void