On Thu, Jun 29, 2023 at 07:42:36AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 29, 2023 at 09:17:24PM +0800, Long Li wrote:
> > When recovery intents, it may capture some deferred ops and commit the new
> > intent items, if recovery intents fails, there will be no done item drop
> > the reference to the new intent item. New intent items will left in AIL
> > and caused mount thread hung all the time as fllows:
>
> Er... let me try rewriting this a bit:
>
I have tried my best to express it clearly, but there are still some
language barriers. Thank you very much for making the commit message
clearer.
> "When recovering intents, we capture newly created intent items as part
> of committing recovered intent items. If intent recovery fails at a
> later point, we forget to remove those newly created intent items from
> the AIL and hang:
>
> >
> > [root@localhost ~]# cat /proc/539/stack
> > [<0>] xfs_ail_push_all_sync+0x174/0x230
> > [<0>] xfs_unmount_flush_inodes+0x8d/0xd0
> > [<0>] xfs_mountfs+0x15f7/0x1e70
> > [<0>] xfs_fs_fill_super+0x10ec/0x1b20
> > [<0>] get_tree_bdev+0x3c8/0x730
> > [<0>] vfs_get_tree+0x89/0x2c0
> > [<0>] path_mount+0xecf/0x1800
> > [<0>] do_mount+0xf3/0x110
> > [<0>] __x64_sys_mount+0x154/0x1f0
> > [<0>] do_syscall_64+0x39/0x80
> > [<0>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >
> > During intent item recover, if transaction that have deferred ops commmit
> > fails in xfs_defer_ops_capture_and_commit(), defer capture would not added
> > to capture list, so matching done items would not commit when finish defer
> > operations, this will cause intent item leaks:
>
> "Intent recovery hasn't created done items for these newly created
> intent items, so the capture structure is the sole owner of the captured
> intent items. We must release them explicitly or else they leak:
>
> > unreferenced object 0xffff888016719108 (size 432):
> > comm "mount", pid 529, jiffies 4294706839 (age 144.463s)
> > hex dump (first 32 bytes):
> > 08 91 71 16 80 88 ff ff 08 91 71 16 80 88 ff ff ..q.......q.....
> > 18 91 71 16 80 88 ff ff 18 91 71 16 80 88 ff ff ..q.......q.....
> > backtrace:
> > [<ffffffff8230c68f>] xfs_efi_init+0x18f/0x1d0
> > [<ffffffff8230c720>] xfs_extent_free_create_intent+0x50/0x150
> > [<ffffffff821b671a>] xfs_defer_create_intents+0x16a/0x340
> > [<ffffffff821bac3e>] xfs_defer_ops_capture_and_commit+0x8e/0xad0
> > [<ffffffff82322bb9>] xfs_cui_item_recover+0x819/0x980
> > [<ffffffff823289b6>] xlog_recover_process_intents+0x246/0xb70
> > [<ffffffff8233249a>] xlog_recover_finish+0x8a/0x9a0
> > [<ffffffff822eeafb>] xfs_log_mount_finish+0x2bb/0x4a0
> > [<ffffffff822c0f4f>] xfs_mountfs+0x14bf/0x1e70
> > [<ffffffff822d1f80>] xfs_fs_fill_super+0x10d0/0x1b20
> > [<ffffffff81a21fa2>] get_tree_bdev+0x3d2/0x6d0
> > [<ffffffff81a1ee09>] vfs_get_tree+0x89/0x2c0
> > [<ffffffff81a9f35f>] path_mount+0xecf/0x1800
> > [<ffffffff81a9fd83>] do_mount+0xf3/0x110
> > [<ffffffff81aa00e4>] __x64_sys_mount+0x154/0x1f0
> > [<ffffffff83968739>] do_syscall_64+0x39/0x80
> >
> > Fix the problem above by abort intent items that don't have a done item
> > when recovery intents fail.
> >
> > Fixes: e6fff81e4870 ("xfs: proper replay of deferred ops queued during log recovery")
> > Signed-off-by: Long Li <leo.lilong@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_defer.c | 1 +
> > fs/xfs/libxfs/xfs_defer.h | 1 +
> > fs/xfs/xfs_log_recover.c | 1 +
> > 3 files changed, 3 insertions(+)
> >
> > diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> > index 7ec6812fa625..b2b46d142281 100644
> > --- a/fs/xfs/libxfs/xfs_defer.c
> > +++ b/fs/xfs/libxfs/xfs_defer.c
> > @@ -809,6 +809,7 @@ xfs_defer_ops_capture_and_commit(
> > /* Commit the transaction and add the capture structure to the list. */
> > error = xfs_trans_commit(tp);
> > if (error) {
> > + xfs_defer_pending_abort(mp, &dfc->dfc_dfops);
> > xfs_defer_ops_capture_free(mp, dfc);
>
> I prefer that we not go extern'ing two functions that mess around with
> internal state. Could you instead add the xfs_defer_pending_abort call
> to the start of xfs_defer_ops_capture_free, and rename
> xfs_defer_ops_capture_free to xfs_defer_ops_capture_abort?
>
Thanks for your suggestion, it seems reasonable and clean enough, it will
change in the next version.
> Other than those two things, I /think/ this looks correct. Assuming my
> understanding of the problem is reflected in the tweaks I made to the
> commit message. ;)
>
> --D
Thanks for your review again, the commit message you wrote is exactly
what I want to describe, so I will send a new version later.
ddThanks,
Long Li
>
> > return error;
> > }
> > diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> > index 114a3a4930a3..c3775014f7ab 100644
> > --- a/fs/xfs/libxfs/xfs_defer.h
> > +++ b/fs/xfs/libxfs/xfs_defer.h
> > @@ -37,6 +37,7 @@ struct xfs_defer_pending {
> > enum xfs_defer_ops_type dfp_type;
> > };
> >
> > +void xfs_defer_pending_abort(struct xfs_mount *mp, struct list_head *dop_list);
> > void xfs_defer_add(struct xfs_trans *tp, enum xfs_defer_ops_type type,
> > struct list_head *h);
> > int xfs_defer_finish_noroll(struct xfs_trans **tp);
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 82c81d20459d..924beecf07bb 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -2511,6 +2511,7 @@ xlog_abort_defer_ops(
> >
> > list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
> > list_del_init(&dfc->dfc_list);
> > + xfs_defer_pending_abort(mp, &dfc->dfc_dfops);
> > xfs_defer_ops_capture_free(mp, dfc);
> > }
> > }
> > --
> > 2.31.1
> >
