* [PATCH] xfs: flush inode gc workqueue before clearing agi bucket @ 2022-07-11 14:41 Zhang Yi 2022-07-11 22:06 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: Zhang Yi @ 2022-07-11 14:41 UTC (permalink / raw) To: linux-xfs; +Cc: djwong, yi.zhang, yukuai3 In the procedure of recover AGI unlinked lists, if something bad happenes on one of the unlinked inode in the bucket list, we would call xlog_recover_clear_agi_bucket() to clear the whole unlinked bucket list, not the unlinked inodes after the bad one. If we have already added some inodes to the gc workqueue before the bad inode in the list, we could get below error when freeing those inodes, and finaly fail to complete the log recover procedure. XFS (ram0): Internal error xfs_iunlink_remove at line 2456 of file fs/xfs/xfs_inode.c. Caller xfs_ifree+0xb0/0x360 [xfs] The problem is xlog_recover_clear_agi_bucket() clear the bucket list, so the gc worker fail to check the agino in xfs_verify_agino(). Fix this by flush workqueue before clearing the bucket. Signed-off-by: Zhang Yi <yi.zhang@huawei.com> --- fs/xfs/xfs_log_recover.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 5f7e4e6e33ce..2f655ef4364e 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -2714,6 +2714,7 @@ xlog_recover_process_one_iunlink( * Call xlog_recover_clear_agi_bucket() to perform a transaction to * clear the inode pointer in the bucket. */ + xfs_inodegc_flush(mp); xlog_recover_clear_agi_bucket(mp, agno, bucket); return NULLAGINO; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: flush inode gc workqueue before clearing agi bucket 2022-07-11 14:41 [PATCH] xfs: flush inode gc workqueue before clearing agi bucket Zhang Yi @ 2022-07-11 22:06 ` Dave Chinner 2022-07-12 18:32 ` Darrick J. Wong 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2022-07-11 22:06 UTC (permalink / raw) To: Zhang Yi; +Cc: linux-xfs, djwong, yukuai3 On Mon, Jul 11, 2022 at 10:41:34PM +0800, Zhang Yi wrote: > In the procedure of recover AGI unlinked lists, if something bad > happenes on one of the unlinked inode in the bucket list, we would call > xlog_recover_clear_agi_bucket() to clear the whole unlinked bucket list, > not the unlinked inodes after the bad one. If we have already added some > inodes to the gc workqueue before the bad inode in the list, we could > get below error when freeing those inodes, and finaly fail to complete > the log recover procedure. > > XFS (ram0): Internal error xfs_iunlink_remove at line 2456 of file > fs/xfs/xfs_inode.c. Caller xfs_ifree+0xb0/0x360 [xfs] > > The problem is xlog_recover_clear_agi_bucket() clear the bucket list, so > the gc worker fail to check the agino in xfs_verify_agino(). Fix this by > flush workqueue before clearing the bucket. > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > --- > fs/xfs/xfs_log_recover.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 5f7e4e6e33ce..2f655ef4364e 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -2714,6 +2714,7 @@ xlog_recover_process_one_iunlink( > * Call xlog_recover_clear_agi_bucket() to perform a transaction to > * clear the inode pointer in the bucket. > */ > + xfs_inodegc_flush(mp); > xlog_recover_clear_agi_bucket(mp, agno, bucket); > return NULLAGINO; > } Looks good. Reviewed-by: Dave Chinner <dchinner@redhat.com> Darrick, FYI, I actually tripped over this and fixed it in the inode iunlink series as part of double linking the unlinked inode list in this patch: https://lore.kernel.org/linux-xfs/20220707234345.1097095-6-david@fromorbit.com/ I didn't realise at the time I was forward porting this code that it was a pre-existing bug..... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: flush inode gc workqueue before clearing agi bucket 2022-07-11 22:06 ` Dave Chinner @ 2022-07-12 18:32 ` Darrick J. Wong 2022-07-12 21:49 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: Darrick J. Wong @ 2022-07-12 18:32 UTC (permalink / raw) To: Dave Chinner; +Cc: Zhang Yi, linux-xfs, yukuai3 On Tue, Jul 12, 2022 at 08:06:42AM +1000, Dave Chinner wrote: > On Mon, Jul 11, 2022 at 10:41:34PM +0800, Zhang Yi wrote: > > In the procedure of recover AGI unlinked lists, if something bad > > happenes on one of the unlinked inode in the bucket list, we would call > > xlog_recover_clear_agi_bucket() to clear the whole unlinked bucket list, > > not the unlinked inodes after the bad one. If we have already added some > > inodes to the gc workqueue before the bad inode in the list, we could > > get below error when freeing those inodes, and finaly fail to complete > > the log recover procedure. > > > > XFS (ram0): Internal error xfs_iunlink_remove at line 2456 of file > > fs/xfs/xfs_inode.c. Caller xfs_ifree+0xb0/0x360 [xfs] > > > > The problem is xlog_recover_clear_agi_bucket() clear the bucket list, so > > the gc worker fail to check the agino in xfs_verify_agino(). Fix this by > > flush workqueue before clearing the bucket. > > > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > > --- > > fs/xfs/xfs_log_recover.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > > index 5f7e4e6e33ce..2f655ef4364e 100644 > > --- a/fs/xfs/xfs_log_recover.c > > +++ b/fs/xfs/xfs_log_recover.c > > @@ -2714,6 +2714,7 @@ xlog_recover_process_one_iunlink( > > * Call xlog_recover_clear_agi_bucket() to perform a transaction to > > * clear the inode pointer in the bucket. > > */ > > + xfs_inodegc_flush(mp); > > xlog_recover_clear_agi_bucket(mp, agno, bucket); > > return NULLAGINO; > > } > > Looks good. > > Reviewed-by: Dave Chinner <dchinner@redhat.com> I propose adding: Fixes: ab23a7768739 ("xfs: per-cpu deferred inode inactivation queues") Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > Darrick, FYI, I actually tripped over this and fixed it in the inode > iunlink series as part of double linking the unlinked inode list in > this patch: > > https://lore.kernel.org/linux-xfs/20220707234345.1097095-6-david@fromorbit.com/ > > I didn't realise at the time I was forward porting this code that it > was a pre-existing bug..... Yep. I'll merge this into the tree for easier porting with 5.15, and fix up whatever merge conflicts result, if you're still interested in merging the incore iunlinks for 5.20. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: flush inode gc workqueue before clearing agi bucket 2022-07-12 18:32 ` Darrick J. Wong @ 2022-07-12 21:49 ` Dave Chinner 2022-07-12 23:09 ` Darrick J. Wong 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2022-07-12 21:49 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Zhang Yi, linux-xfs, yukuai3 On Tue, Jul 12, 2022 at 11:32:19AM -0700, Darrick J. Wong wrote: > On Tue, Jul 12, 2022 at 08:06:42AM +1000, Dave Chinner wrote: > > On Mon, Jul 11, 2022 at 10:41:34PM +0800, Zhang Yi wrote: > > > In the procedure of recover AGI unlinked lists, if something bad > > > happenes on one of the unlinked inode in the bucket list, we would call > > > xlog_recover_clear_agi_bucket() to clear the whole unlinked bucket list, > > > not the unlinked inodes after the bad one. If we have already added some > > > inodes to the gc workqueue before the bad inode in the list, we could > > > get below error when freeing those inodes, and finaly fail to complete > > > the log recover procedure. > > > > > > XFS (ram0): Internal error xfs_iunlink_remove at line 2456 of file > > > fs/xfs/xfs_inode.c. Caller xfs_ifree+0xb0/0x360 [xfs] > > > > > > The problem is xlog_recover_clear_agi_bucket() clear the bucket list, so > > > the gc worker fail to check the agino in xfs_verify_agino(). Fix this by > > > flush workqueue before clearing the bucket. > > > > > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > > > --- > > > fs/xfs/xfs_log_recover.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > > > index 5f7e4e6e33ce..2f655ef4364e 100644 > > > --- a/fs/xfs/xfs_log_recover.c > > > +++ b/fs/xfs/xfs_log_recover.c > > > @@ -2714,6 +2714,7 @@ xlog_recover_process_one_iunlink( > > > * Call xlog_recover_clear_agi_bucket() to perform a transaction to > > > * clear the inode pointer in the bucket. > > > */ > > > + xfs_inodegc_flush(mp); > > > xlog_recover_clear_agi_bucket(mp, agno, bucket); > > > return NULLAGINO; > > > } > > > > Looks good. > > > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > I propose adding: > Fixes: ab23a7768739 ("xfs: per-cpu deferred inode inactivation queues") > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > > > > Darrick, FYI, I actually tripped over this and fixed it in the inode > > iunlink series as part of double linking the unlinked inode list in > > this patch: > > > > https://lore.kernel.org/linux-xfs/20220707234345.1097095-6-david@fromorbit.com/ > > > > I didn't realise at the time I was forward porting this code that it > > was a pre-existing bug..... > > Yep. I'll merge this into the tree for easier porting with 5.15, and > fix up whatever merge conflicts result, if you're still interested in > merging the incore iunlinks for 5.20. Yes, I'll send you a pull request for it soon now that all reviews have been done. If you want, I'll include this fix first and rebase the commits that is causes conflicts with on top of it cleanly.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: flush inode gc workqueue before clearing agi bucket 2022-07-12 21:49 ` Dave Chinner @ 2022-07-12 23:09 ` Darrick J. Wong 0 siblings, 0 replies; 5+ messages in thread From: Darrick J. Wong @ 2022-07-12 23:09 UTC (permalink / raw) To: Dave Chinner; +Cc: Zhang Yi, linux-xfs, yukuai3 On Wed, Jul 13, 2022 at 07:49:40AM +1000, Dave Chinner wrote: > On Tue, Jul 12, 2022 at 11:32:19AM -0700, Darrick J. Wong wrote: > > On Tue, Jul 12, 2022 at 08:06:42AM +1000, Dave Chinner wrote: > > > On Mon, Jul 11, 2022 at 10:41:34PM +0800, Zhang Yi wrote: > > > > In the procedure of recover AGI unlinked lists, if something bad > > > > happenes on one of the unlinked inode in the bucket list, we would call > > > > xlog_recover_clear_agi_bucket() to clear the whole unlinked bucket list, > > > > not the unlinked inodes after the bad one. If we have already added some > > > > inodes to the gc workqueue before the bad inode in the list, we could > > > > get below error when freeing those inodes, and finaly fail to complete > > > > the log recover procedure. > > > > > > > > XFS (ram0): Internal error xfs_iunlink_remove at line 2456 of file > > > > fs/xfs/xfs_inode.c. Caller xfs_ifree+0xb0/0x360 [xfs] > > > > > > > > The problem is xlog_recover_clear_agi_bucket() clear the bucket list, so > > > > the gc worker fail to check the agino in xfs_verify_agino(). Fix this by > > > > flush workqueue before clearing the bucket. > > > > > > > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > > > > --- > > > > fs/xfs/xfs_log_recover.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > > > > index 5f7e4e6e33ce..2f655ef4364e 100644 > > > > --- a/fs/xfs/xfs_log_recover.c > > > > +++ b/fs/xfs/xfs_log_recover.c > > > > @@ -2714,6 +2714,7 @@ xlog_recover_process_one_iunlink( > > > > * Call xlog_recover_clear_agi_bucket() to perform a transaction to > > > > * clear the inode pointer in the bucket. > > > > */ > > > > + xfs_inodegc_flush(mp); > > > > xlog_recover_clear_agi_bucket(mp, agno, bucket); > > > > return NULLAGINO; > > > > } > > > > > > Looks good. > > > > > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > > > I propose adding: > > Fixes: ab23a7768739 ("xfs: per-cpu deferred inode inactivation queues") > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > > > > > > > Darrick, FYI, I actually tripped over this and fixed it in the inode > > > iunlink series as part of double linking the unlinked inode list in > > > this patch: > > > > > > https://lore.kernel.org/linux-xfs/20220707234345.1097095-6-david@fromorbit.com/ > > > > > > I didn't realise at the time I was forward porting this code that it > > > was a pre-existing bug..... > > > > Yep. I'll merge this into the tree for easier porting with 5.15, and > > fix up whatever merge conflicts result, if you're still interested in > > merging the incore iunlinks for 5.20. > > Yes, I'll send you a pull request for it soon now that all reviews > have been done. If you want, I'll include this fix first and rebase > the commits that is causes conflicts with on top of it cleanly.... Yes, please include it in the rebase. :) --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-12 23:09 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-11 14:41 [PATCH] xfs: flush inode gc workqueue before clearing agi bucket Zhang Yi 2022-07-11 22:06 ` Dave Chinner 2022-07-12 18:32 ` Darrick J. Wong 2022-07-12 21:49 ` Dave Chinner 2022-07-12 23:09 ` Darrick J. Wong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox