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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> > + }
> > +
> > + /*
> > + * 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
