| Message ID | 158864115522.182683.9248036319539577559.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: refactor log recovery | expand |
On Tuesday 5 May 2020 6:42:35 AM IST Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Hoist the unlinked inode processing logic out of the AG loop and into > its own function. No functional changes. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_unlink_recover.c | 91 +++++++++++++++++++++++++------------------ > 1 file changed, 52 insertions(+), 39 deletions(-) > > > diff --git a/fs/xfs/xfs_unlink_recover.c b/fs/xfs/xfs_unlink_recover.c > index 2a19d096e88d..413b34085640 100644 > --- a/fs/xfs/xfs_unlink_recover.c > +++ b/fs/xfs/xfs_unlink_recover.c > @@ -145,54 +145,67 @@ xlog_recover_process_one_iunlink( > * scheduled on this CPU to ensure other scheduled work can run without undue > * latency. > */ > -void > -xlog_recover_process_unlinked( > - struct xlog *log) > +STATIC int > +xlog_recover_process_iunlinked( > + struct xfs_mount *mp, > + xfs_agnumber_t agno) > { > - struct xfs_mount *mp; > struct xfs_agi *agi; > struct xfs_buf *agibp; > - xfs_agnumber_t agno; > xfs_agino_t agino; > int bucket; > int error; > > - mp = log->l_mp; > - > - for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > - /* > - * Find the agi for this ag. > - */ > - error = xfs_read_agi(mp, NULL, 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.... > - */ > - continue; > - } > + /* > + * Find the agi for this ag. > + */ > + error = xfs_read_agi(mp, NULL, agno, &agibp); > + if (error) { > /* > - * 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 is b0rked. Don't process it. > + * > + * We should probably mark the filesystem as corrupt > + * after we've recovered all the ag's we can.... > */ > - 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, > - agno, agino, bucket); > - cond_resched(); > - } > + return error; This causes a change in behaviour i.e. an error return from here would cause xlog_recover_process_unlinked() to break "loop on all AGs". Before this change, XFS would continue to process all the remaining AGs as described by the above comment. > + } > + > + /* > + * 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, > + agno, agino, bucket); > + cond_resched(); > } > - xfs_buf_rele(agibp); > + } > + xfs_buf_rele(agibp); > + > + return 0; > +} > + > +void > +xlog_recover_process_unlinked( > + struct xlog *log) > +{ > + struct xfs_mount *mp = log->l_mp; > + xfs_agnumber_t agno; > + int error; > + > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > + error = xlog_recover_process_iunlinked(mp, agno); > + if (error) > + break; > } > } > >
On Tuesday 5 May 2020 6:49:17 PM IST Chandan Babu R wrote: > On Tuesday 5 May 2020 6:42:35 AM IST Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Hoist the unlinked inode processing logic out of the AG loop and into > > its own function. No functional changes. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_unlink_recover.c | 91 +++++++++++++++++++++++++------------------ > > 1 file changed, 52 insertions(+), 39 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_unlink_recover.c b/fs/xfs/xfs_unlink_recover.c > > index 2a19d096e88d..413b34085640 100644 > > --- a/fs/xfs/xfs_unlink_recover.c > > +++ b/fs/xfs/xfs_unlink_recover.c > > @@ -145,54 +145,67 @@ xlog_recover_process_one_iunlink( > > * scheduled on this CPU to ensure other scheduled work can run without undue > > * latency. > > */ > > -void > > -xlog_recover_process_unlinked( > > - struct xlog *log) > > +STATIC int > > +xlog_recover_process_iunlinked( > > + struct xfs_mount *mp, > > + xfs_agnumber_t agno) > > { > > - struct xfs_mount *mp; > > struct xfs_agi *agi; > > struct xfs_buf *agibp; > > - xfs_agnumber_t agno; > > xfs_agino_t agino; > > int bucket; > > int error; > > > > - mp = log->l_mp; > > - > > - for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > > - /* > > - * Find the agi for this ag. > > - */ > > - error = xfs_read_agi(mp, NULL, 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.... > > - */ > > - continue; > > - } > > + /* > > + * Find the agi for this ag. > > + */ > > + error = xfs_read_agi(mp, NULL, agno, &agibp); > > + if (error) { > > /* > > - * 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 is b0rked. Don't process it. > > + * > > + * We should probably mark the filesystem as corrupt > > + * after we've recovered all the ag's we can.... > > */ > > - 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, > > - agno, agino, bucket); > > - cond_resched(); > > - } > > + return error; > > > This causes a change in behaviour i.e. an error return from here would cause > xlog_recover_process_unlinked() to break "loop on all AGs". Before this > change, XFS would continue to process all the remaining AGs as described by > the above comment. > I noticed that in the next patch the error code is percolated to the calling functions and it is done with the intention that since the agi[s] is already corrupt the code will most likely hit this corruption during a normal fs operation. Hence, Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > > > + } > > + > > + /* > > + * 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, > > + agno, agino, bucket); > > + cond_resched(); > > } > > - xfs_buf_rele(agibp); > > + } > > + xfs_buf_rele(agibp); > > + > > + return 0; > > +} > > + > > +void > > +xlog_recover_process_unlinked( > > + struct xlog *log) > > +{ > > + struct xfs_mount *mp = log->l_mp; > > + xfs_agnumber_t agno; > > + int error; > > + > > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > > + error = xlog_recover_process_iunlinked(mp, agno); > > + if (error) > > + break; > > } > > } > > > > > > >
On Mon, May 04, 2020 at 06:12:35PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Hoist the unlinked inode processing logic out of the AG loop and into > its own function. No functional changes. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_unlink_recover.c | 91 +++++++++++++++++++++++++------------------ > 1 file changed, 52 insertions(+), 39 deletions(-) > > > diff --git a/fs/xfs/xfs_unlink_recover.c b/fs/xfs/xfs_unlink_recover.c > index 2a19d096e88d..413b34085640 100644 > --- a/fs/xfs/xfs_unlink_recover.c > +++ b/fs/xfs/xfs_unlink_recover.c > @@ -145,54 +145,67 @@ xlog_recover_process_one_iunlink( > * scheduled on this CPU to ensure other scheduled work can run without undue > * latency. > */ > -void > -xlog_recover_process_unlinked( > - struct xlog *log) > +STATIC int > +xlog_recover_process_iunlinked( xlog_recover_process_ag_iunlinked?
On Tue, May 05, 2020 at 06:49:17PM +0530, Chandan Babu R wrote: > On Tuesday 5 May 2020 6:42:35 AM IST Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Hoist the unlinked inode processing logic out of the AG loop and into > > its own function. No functional changes. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_unlink_recover.c | 91 +++++++++++++++++++++++++------------------ > > 1 file changed, 52 insertions(+), 39 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_unlink_recover.c b/fs/xfs/xfs_unlink_recover.c > > index 2a19d096e88d..413b34085640 100644 > > --- a/fs/xfs/xfs_unlink_recover.c > > +++ b/fs/xfs/xfs_unlink_recover.c > > @@ -145,54 +145,67 @@ xlog_recover_process_one_iunlink( > > * scheduled on this CPU to ensure other scheduled work can run without undue > > * latency. > > */ > > -void > > -xlog_recover_process_unlinked( > > - struct xlog *log) > > +STATIC int > > +xlog_recover_process_iunlinked( > > + struct xfs_mount *mp, > > + xfs_agnumber_t agno) > > { > > - struct xfs_mount *mp; > > struct xfs_agi *agi; > > struct xfs_buf *agibp; > > - xfs_agnumber_t agno; > > xfs_agino_t agino; > > int bucket; > > int error; > > > > - mp = log->l_mp; > > - > > - for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > > - /* > > - * Find the agi for this ag. > > - */ > > - error = xfs_read_agi(mp, NULL, 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.... > > - */ > > - continue; > > - } > > + /* > > + * Find the agi for this ag. > > + */ > > + error = xfs_read_agi(mp, NULL, agno, &agibp); > > + if (error) { > > /* > > - * 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 is b0rked. Don't process it. > > + * > > + * We should probably mark the filesystem as corrupt > > + * after we've recovered all the ag's we can.... > > */ > > - 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, > > - agno, agino, bucket); > > - cond_resched(); > > - } > > + return error; > > > This causes a change in behaviour i.e. an error return from here would cause > xlog_recover_process_unlinked() to break "loop on all AGs". Before this > change, XFS would continue to process all the remaining AGs as described by > the above comment. Hm, you're right. I'll make this function return void and then mess with the return values and whatnot later. --D > > > + } > > + > > + /* > > + * 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, > > + agno, agino, bucket); > > + cond_resched(); > > } > > - xfs_buf_rele(agibp); > > + } > > + xfs_buf_rele(agibp); > > + > > + return 0; > > +} > > + > > +void > > +xlog_recover_process_unlinked( > > + struct xlog *log) > > +{ > > + struct xfs_mount *mp = log->l_mp; > > + xfs_agnumber_t agno; > > + int error; > > + > > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > > + error = xlog_recover_process_iunlinked(mp, agno); > > + if (error) > > + break; > > } > > } > > > > > > > -- > chandan > > >
diff --git a/fs/xfs/xfs_unlink_recover.c b/fs/xfs/xfs_unlink_recover.c index 2a19d096e88d..413b34085640 100644 --- a/fs/xfs/xfs_unlink_recover.c +++ b/fs/xfs/xfs_unlink_recover.c @@ -145,54 +145,67 @@ xlog_recover_process_one_iunlink( * scheduled on this CPU to ensure other scheduled work can run without undue * latency. */ -void -xlog_recover_process_unlinked( - struct xlog *log) +STATIC int +xlog_recover_process_iunlinked( + struct xfs_mount *mp, + xfs_agnumber_t agno) { - struct xfs_mount *mp; struct xfs_agi *agi; struct xfs_buf *agibp; - xfs_agnumber_t agno; xfs_agino_t agino; int bucket; int error; - mp = log->l_mp; - - for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { - /* - * Find the agi for this ag. - */ - error = xfs_read_agi(mp, NULL, 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.... - */ - continue; - } + /* + * Find the agi for this ag. + */ + error = xfs_read_agi(mp, NULL, agno, &agibp); + if (error) { /* - * 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 is b0rked. Don't process it. + * + * We should probably mark the filesystem as corrupt + * after we've recovered all the ag's we can.... */ - 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, - agno, agino, bucket); - cond_resched(); - } + return error; + } + + /* + * 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, + agno, agino, bucket); + cond_resched(); } - xfs_buf_rele(agibp); + } + xfs_buf_rele(agibp); + + return 0; +} + +void +xlog_recover_process_unlinked( + struct xlog *log) +{ + struct xfs_mount *mp = log->l_mp; + xfs_agnumber_t agno; + int error; + + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { + error = xlog_recover_process_iunlinked(mp, agno); + if (error) + break; } }