* [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state
@ 2023-12-03 19:00 Darrick J. Wong
2023-12-03 19:00 ` [PATCH 1/8] xfs: don't leak recovered attri intent items Darrick J. Wong
` (7 more replies)
0 siblings, 8 replies; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:00 UTC (permalink / raw)
To: djwong, chandanbabu, hch, leo.lilong; +Cc: linux-xfs
Hi all,
Long Li reported a KASAN report from a UAF when intent recovery fails:
==================================================================
BUG: KASAN: slab-use-after-free in xfs_cui_release+0xb7/0xc0
Read of size 4 at addr ffff888012575e60 by task kworker/u8:3/103
CPU: 3 PID: 103 Comm: kworker/u8:3 Not tainted 6.4.0-rc7-next-20230619-00003-g94543a53f9a4-dirty #166
Workqueue: xfs-cil/sda xlog_cil_push_work
Call Trace:
<TASK>
dump_stack_lvl+0x50/0x70
print_report+0xc2/0x600
kasan_report+0xb6/0xe0
xfs_cui_release+0xb7/0xc0
xfs_cud_item_release+0x3c/0x90
xfs_trans_committed_bulk+0x2d5/0x7f0
xlog_cil_committed+0xaba/0xf20
xlog_cil_push_work+0x1a60/0x2360
process_one_work+0x78e/0x1140
worker_thread+0x58b/0xf60
kthread+0x2cd/0x3c0
ret_from_fork+0x1f/0x30
</TASK>
Allocated by task 531:
kasan_save_stack+0x22/0x40
kasan_set_track+0x25/0x30
__kasan_slab_alloc+0x55/0x60
kmem_cache_alloc+0x195/0x5f0
xfs_cui_init+0x198/0x1d0
xlog_recover_cui_commit_pass2+0x133/0x5f0
xlog_recover_items_pass2+0x107/0x230
xlog_recover_commit_trans+0x3e7/0x9c0
xlog_recovery_process_trans+0x140/0x1d0
xlog_recover_process_ophdr+0x1a0/0x3d0
xlog_recover_process_data+0x108/0x2d0
xlog_recover_process+0x1f6/0x280
xlog_do_recovery_pass+0x609/0xdb0
xlog_do_log_recovery+0x84/0xe0
xlog_do_recover+0x7d/0x470
xlog_recover+0x25f/0x490
xfs_log_mount+0x2dd/0x6f0
xfs_mountfs+0x11ce/0x1e70
xfs_fs_fill_super+0x10ec/0x1b20
get_tree_bdev+0x3c8/0x730
vfs_get_tree+0x89/0x2c0
path_mount+0xecf/0x1800
do_mount+0xf3/0x110
__x64_sys_mount+0x154/0x1f0
do_syscall_64+0x39/0x80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
Freed by task 531:
kasan_save_stack+0x22/0x40
kasan_set_track+0x25/0x30
kasan_save_free_info+0x2b/0x40
__kasan_slab_free+0x114/0x1b0
kmem_cache_free+0xf8/0x510
xfs_cui_item_free+0x95/0xb0
xfs_cui_release+0x86/0xc0
xlog_recover_cancel_intents.isra.0+0xf8/0x210
xlog_recover_finish+0x7e7/0x980
xfs_log_mount_finish+0x2bb/0x4a0
xfs_mountfs+0x14bf/0x1e70
xfs_fs_fill_super+0x10ec/0x1b20
get_tree_bdev+0x3c8/0x730
vfs_get_tree+0x89/0x2c0
path_mount+0xecf/0x1800
do_mount+0xf3/0x110
__x64_sys_mount+0x154/0x1f0
do_syscall_64+0x39/0x80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
The buggy address belongs to the object at ffff888012575dc8
which belongs to the cache xfs_cui_item of size 432
The buggy address is located 152 bytes inside of
freed 432-byte region [ffff888012575dc8, ffff888012575f78)
The buggy address belongs to the physical page:
page:ffffea0000495d00 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff888012576208 pfn:0x12574
head:ffffea0000495d00 order:2 entire_mapcount:0 nr_pages_mapped:0 pincount:0
flags: 0x1fffff80010200(slab|head|node=0|zone=1|lastcpupid=0x1fffff)
page_type: 0xffffffff()
raw: 001fffff80010200 ffff888012092f40 ffff888014570150 ffff888014570150
raw: ffff888012576208 00000000001e0010 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff888012575d00: fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc fc
ffff888012575d80: fc fc fc fc fc fc fc fc fc fa fb fb fb fb fb fb
>ffff888012575e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff888012575e80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888012575f00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fc
==================================================================
"If process intents fails, intent items left in AIL will be delete
from AIL and freed in error handling, even intent items that have been
recovered and created done items. After this, uaf will be triggered when
done item committed, because at this point the released intent item will
be accessed.
xlog_recover_finish xlog_cil_push_work
---------------------------- ---------------------------
xlog_recover_process_intents
xfs_cui_item_recover//cui_refcount == 1
xfs_trans_get_cud
xfs_trans_commit
<add cud item to cil>
xfs_cui_item_recover
<error occurred and return>
xlog_recover_cancel_intents
xfs_cui_release //cui_refcount == 0
xfs_cui_item_free //free cui
<release other intent items>
xlog_force_shutdown //shutdown
<...>
<push items in cil>
xlog_cil_committed
xfs_cud_item_release
xfs_cui_release // UAF
"Intent log items are created with a reference count of 2, one for the
creator, and one for the intent done object. Log recovery explicitly
drops the creator reference after it is inserted into the AIL, but it
then processes the log item as if it also owns the intent-done reference.
"The code in ->iop_recovery should assume that it passes the reference
to the done intent, we can remove the intent item from the AIL after
creating the done-intent, but if that code fails before creating the
done-intent then it needs to release the intent reference by log recovery
itself.
"That way when we go to cancel the intent, the only intents we find in
the AIL are the ones we know have not been processed yet and hence we
can safely drop both the creator and the intent done reference from
xlog_recover_cancel_intents().
"Hence if we remove the intent from the list of intents that need to
be recovered after we have done the initial recovery, we acheive two
things:
"1. the tail of the log can be moved forward with the commit of the
done intent or new intent to continue the operation, and
"2. We avoid the problem of trying to determine how many reference
counts we need to drop from intent recovery cancelling because we
never come across intents we've actually attempted recovery on."
Restated: The cause of the UAF is that xlog_recover_cancel_intents
thinks that it owns the refcount on any intent item in the AIL, and that
it's always safe to release these intent items. This is not true after
the recovery function creates an log intent done item and points it at
the log intent item because releasing the done item always releases the
intent item.
The runtime defer ops code avoids all this by tracking both the log
intent and the intent done items, and releasing only the intent done
item if both have been created. Long Li proposed fixing this by adding
state flags, but I have a more comprehensive fix.
First, observe that the latter half of the intent _recover functions are
nearly open-coded versions of the corresponding _finish_one function
that uses an onstack deferred work item to single-step through the item.
Second, notice that the recover function is not an exact match because
of the odd behavior that unfinished recovered work items are relogged
with separate log intent items instead of a single new log intent item,
which is what the defer ops machinery does.
Dave and I have long suspected that recovery should be reconstructing
the defer work state from what's in the recovered intent item. Now we
finally have an excuse to refactor the code to do that.
This series starts by fixing a resource leak in LARP recovery. We fix
the bug that Long Li reported by switching the intent recovery code to
construct chains of xfs_defer_pending objects and then using the defer
pending objects to track the intent/done item ownership. Finally, we
clean up the code to reconstruct the exact incore state, which means we
can remove all the opencoded _recover code, which makes maintaining log
items much easier.
If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.
This has been lightly tested with fstests. Enjoy!
Comments and questions are, as always, welcome.
--D
kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=reconstruct-defer-work-6.7
---
fs/xfs/libxfs/xfs_defer.c | 127 +++++++++++++++++++++++--------
fs/xfs/libxfs/xfs_defer.h | 19 +++++
fs/xfs/libxfs/xfs_log_recover.h | 7 ++
fs/xfs/xfs_attr_item.c | 131 ++++++++++++++++----------------
fs/xfs/xfs_bmap_item.c | 89 +++++++++++-----------
fs/xfs/xfs_extfree_item.c | 127 +++++++++++++------------------
fs/xfs/xfs_log.c | 1
fs/xfs/xfs_log_priv.h | 1
fs/xfs/xfs_log_recover.c | 129 +++++++++++++++++--------------
fs/xfs/xfs_refcount_item.c | 138 +++++++++++----------------------
fs/xfs/xfs_rmap_item.c | 161 +++++++++++++++++++--------------------
fs/xfs/xfs_trans.h | 2
12 files changed, 483 insertions(+), 449 deletions(-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/8] xfs: don't leak recovered attri intent items
2023-12-03 19:00 [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
@ 2023-12-03 19:00 ` Darrick J. Wong
2023-12-05 4:31 ` Christoph Hellwig
2023-12-03 19:01 ` [PATCH 2/8] xfs: use xfs_defer_pending objects to recover " Darrick J. Wong
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:00 UTC (permalink / raw)
To: djwong, chandanbabu, hch, leo.lilong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
If recovery finds an xattr log intent item calling for the removal of an
attribute and the file doesn't even have an attr fork, we know that the
removal is trivially complete. However, we can't just exit the recovery
function without doing something about the recovered log intent item --
it's still on the AIL, and not logging an attrd item means it stays
there forever.
This has likely not been seen in practice because few people use LARP
and the runtime code won't log the attri for a no-attrfork removexattr
operation. But let's fix this anyway.
Also we shouldn't really be testing the attr fork presence until we've
taken the ILOCK, though this doesn't matter much in recovery, which is
single threaded.
Fixes: fdaf1bb3cafc ("xfs: ATTR_REPLACE algorithm with LARP enabled needs rework")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/xfs_attr_item.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 36fe2abb16e6..11e88a76a33c 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -329,6 +329,13 @@ xfs_xattri_finish_update(
goto out;
}
+ /* If an attr removal is trivially complete, we're done. */
+ if (attr->xattri_op_flags == XFS_ATTRI_OP_FLAGS_REMOVE &&
+ !xfs_inode_hasattr(args->dp)) {
+ error = 0;
+ goto out;
+ }
+
error = xfs_attr_set_iter(attr);
if (!error && attr->xattri_dela_state != XFS_DAS_DONE)
error = -EAGAIN;
@@ -608,8 +615,6 @@ xfs_attri_item_recover(
attr->xattri_dela_state = xfs_attr_init_add_state(args);
break;
case XFS_ATTRI_OP_FLAGS_REMOVE:
- if (!xfs_inode_hasattr(args->dp))
- goto out;
attr->xattri_dela_state = xfs_attr_init_remove_state(args);
break;
default:
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/8] xfs: use xfs_defer_pending objects to recover intent items
2023-12-03 19:00 [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
2023-12-03 19:00 ` [PATCH 1/8] xfs: don't leak recovered attri intent items Darrick J. Wong
@ 2023-12-03 19:01 ` Darrick J. Wong
2023-12-04 5:03 ` Christoph Hellwig
2023-12-03 19:01 ` [PATCH 3/8] xfs: pass the xfs_defer_pending object to iop_recover Darrick J. Wong
` (5 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:01 UTC (permalink / raw)
To: djwong, chandanbabu, hch, leo.lilong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
One thing I never quite got around to doing is porting the log intent
item recovery code to reconstruct the deferred pending work state. As a
result, each intent item open codes xfs_defer_finish_one in its recovery
method, because that's what the EFI code did before xfs_defer.c even
existed.
This is a gross thing to have left unfixed -- if an EFI cannot proceed
due to busy extents, we end up creating separate new EFIs for each
unfinished work item, which is a change in behavior from what runtime
would have done.
Worse yet, Long Li pointed out that there's a UAF in the recovery code.
The ->commit_pass2 function adds the intent item to the AIL and drops
the refcount. The one remaining refcount is now owned by the recovery
mechanism (aka the log intent items in the AIL) with the intent of
giving the refcount to the intent done item in the ->iop_recover
function.
However, if something fails later in recovery, xlog_recover_finish will
walk the recovered intent items in the AIL and release them. If the CIL
hasn't been pushed before that point (which is possible since we don't
force the log until later) then the intent done release will try to free
its associated intent, which has already been freed.
This patch starts to address this mess by having the ->commit_pass2
functions recreate the xfs_defer_pending state. The next few patches
will fix the recovery functions.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/libxfs/xfs_defer.c | 105 +++++++++++++++++++++++++++----------
fs/xfs/libxfs/xfs_defer.h | 5 ++
fs/xfs/libxfs/xfs_log_recover.h | 3 +
fs/xfs/xfs_attr_item.c | 10 +---
fs/xfs/xfs_bmap_item.c | 9 +--
fs/xfs/xfs_extfree_item.c | 9 +--
fs/xfs/xfs_log.c | 1
fs/xfs/xfs_log_priv.h | 1
fs/xfs/xfs_log_recover.c | 111 ++++++++++++++++++++-------------------
fs/xfs/xfs_refcount_item.c | 9 +--
fs/xfs/xfs_rmap_item.c | 9 +--
11 files changed, 157 insertions(+), 115 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index f71679ce23b9..363da37a8e7f 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -245,23 +245,53 @@ xfs_defer_create_intents(
return ret;
}
-STATIC void
+static inline void
xfs_defer_pending_abort(
+ struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp)
+{
+ const struct xfs_defer_op_type *ops = defer_op_types[dfp->dfp_type];
+
+ trace_xfs_defer_pending_abort(mp, dfp);
+
+ if (dfp->dfp_intent && !dfp->dfp_done) {
+ ops->abort_intent(dfp->dfp_intent);
+ dfp->dfp_intent = NULL;
+ }
+}
+
+static inline void
+xfs_defer_pending_cancel_work(
+ struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp)
+{
+ const struct xfs_defer_op_type *ops = defer_op_types[dfp->dfp_type];
+ struct list_head *pwi;
+ struct list_head *n;
+
+ trace_xfs_defer_cancel_list(mp, dfp);
+
+ list_del(&dfp->dfp_list);
+ list_for_each_safe(pwi, n, &dfp->dfp_work) {
+ list_del(pwi);
+ dfp->dfp_count--;
+ trace_xfs_defer_cancel_item(mp, dfp, pwi);
+ ops->cancel_item(pwi);
+ }
+ ASSERT(dfp->dfp_count == 0);
+ kmem_cache_free(xfs_defer_pending_cache, dfp);
+}
+
+STATIC void
+xfs_defer_pending_abort_list(
struct xfs_mount *mp,
struct list_head *dop_list)
{
struct xfs_defer_pending *dfp;
- const struct xfs_defer_op_type *ops;
/* Abort intent items that don't have a done item. */
- list_for_each_entry(dfp, dop_list, dfp_list) {
- ops = defer_op_types[dfp->dfp_type];
- trace_xfs_defer_pending_abort(mp, dfp);
- if (dfp->dfp_intent && !dfp->dfp_done) {
- ops->abort_intent(dfp->dfp_intent);
- dfp->dfp_intent = NULL;
- }
- }
+ list_for_each_entry(dfp, dop_list, dfp_list)
+ xfs_defer_pending_abort(mp, dfp);
}
/* Abort all the intents that were committed. */
@@ -271,7 +301,7 @@ xfs_defer_trans_abort(
struct list_head *dop_pending)
{
trace_xfs_defer_trans_abort(tp, _RET_IP_);
- xfs_defer_pending_abort(tp->t_mountp, dop_pending);
+ xfs_defer_pending_abort_list(tp->t_mountp, dop_pending);
}
/*
@@ -389,27 +419,13 @@ xfs_defer_cancel_list(
{
struct xfs_defer_pending *dfp;
struct xfs_defer_pending *pli;
- struct list_head *pwi;
- struct list_head *n;
- const struct xfs_defer_op_type *ops;
/*
* Free the pending items. Caller should already have arranged
* for the intent items to be released.
*/
- list_for_each_entry_safe(dfp, pli, dop_list, dfp_list) {
- ops = defer_op_types[dfp->dfp_type];
- trace_xfs_defer_cancel_list(mp, dfp);
- list_del(&dfp->dfp_list);
- list_for_each_safe(pwi, n, &dfp->dfp_work) {
- list_del(pwi);
- dfp->dfp_count--;
- trace_xfs_defer_cancel_item(mp, dfp, pwi);
- ops->cancel_item(pwi);
- }
- ASSERT(dfp->dfp_count == 0);
- kmem_cache_free(xfs_defer_pending_cache, dfp);
- }
+ list_for_each_entry_safe(dfp, pli, dop_list, dfp_list)
+ xfs_defer_pending_cancel_work(mp, dfp);
}
/*
@@ -665,6 +681,39 @@ xfs_defer_add(
dfp->dfp_count++;
}
+/*
+ * Create a pending deferred work item to replay the recovered intent item
+ * and add it to the list.
+ */
+void
+xfs_defer_start_recovery(
+ struct xfs_log_item *lip,
+ enum xfs_defer_ops_type dfp_type,
+ struct list_head *r_dfops)
+{
+ struct xfs_defer_pending *dfp;
+
+ dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
+ GFP_NOFS | __GFP_NOFAIL);
+ dfp->dfp_type = dfp_type;
+ dfp->dfp_intent = lip;
+ INIT_LIST_HEAD(&dfp->dfp_work);
+ list_add_tail(&dfp->dfp_list, r_dfops);
+}
+
+/*
+ * Cancel a deferred work item created to recover a log intent item. @dfp
+ * will be freed after this function returns.
+ */
+void
+xfs_defer_cancel_recovery(
+ struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp)
+{
+ xfs_defer_pending_abort(mp, dfp);
+ xfs_defer_pending_cancel_work(mp, dfp);
+}
+
/*
* Move deferred ops from one transaction to another and reset the source to
* initial state. This is primarily used to carry state forward across
@@ -769,7 +818,7 @@ xfs_defer_ops_capture_abort(
{
unsigned short i;
- xfs_defer_pending_abort(mp, &dfc->dfc_dfops);
+ xfs_defer_pending_abort_list(mp, &dfc->dfc_dfops);
xfs_defer_cancel_list(mp, &dfc->dfc_dfops);
for (i = 0; i < dfc->dfc_held.dr_bufs; i++)
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 8788ad5f6a73..5dce938ba3d5 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -125,6 +125,11 @@ void xfs_defer_ops_capture_abort(struct xfs_mount *mp,
struct xfs_defer_capture *d);
void xfs_defer_resources_rele(struct xfs_defer_resources *dres);
+void xfs_defer_start_recovery(struct xfs_log_item *lip,
+ enum xfs_defer_ops_type dfp_type, struct list_head *r_dfops);
+void xfs_defer_cancel_recovery(struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp);
+
int __init xfs_defer_init_item_caches(void);
void xfs_defer_destroy_item_caches(void);
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index a5100a11faf9..271a4ce7375c 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -153,4 +153,7 @@ xlog_recover_resv(const struct xfs_trans_res *r)
return ret;
}
+void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
+ xfs_lsn_t lsn, unsigned int dfp_type);
+
#endif /* __XFS_LOG_RECOVER_H__ */
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 11e88a76a33c..a32716b8cbbd 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -772,14 +772,8 @@ xlog_recover_attri_commit_pass2(
attrip = xfs_attri_init(mp, nv);
memcpy(&attrip->attri_format, attri_formatp, len);
- /*
- * The ATTRI has two references. One for the ATTRD and one for ATTRI to
- * ensure it makes it into the AIL. Insert the ATTRI into the AIL
- * directly and drop the ATTRI reference. Note that
- * xfs_trans_ail_update() drops the AIL lock.
- */
- xfs_trans_ail_insert(log->l_ailp, &attrip->attri_item, lsn);
- xfs_attri_release(attrip);
+ xlog_recover_intent_item(log, &attrip->attri_item, lsn,
+ XFS_DEFER_OPS_TYPE_ATTR);
xfs_attri_log_nameval_put(nv);
return 0;
}
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index e736a0844c89..6cbae4fdf43f 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -681,12 +681,9 @@ xlog_recover_bui_commit_pass2(
buip = xfs_bui_init(mp);
xfs_bui_copy_format(&buip->bui_format, bui_formatp);
atomic_set(&buip->bui_next_extent, bui_formatp->bui_nextents);
- /*
- * Insert the intent into the AIL directly and drop one reference so
- * that finishing or canceling the work will drop the other.
- */
- xfs_trans_ail_insert(log->l_ailp, &buip->bui_item, lsn);
- xfs_bui_release(buip);
+
+ xlog_recover_intent_item(log, &buip->bui_item, lsn,
+ XFS_DEFER_OPS_TYPE_BMAP);
return 0;
}
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 3fa8789820ad..cf0ddeb70580 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -820,12 +820,9 @@ xlog_recover_efi_commit_pass2(
return error;
}
atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
- /*
- * Insert the intent into the AIL directly and drop one reference so
- * that finishing or canceling the work will drop the other.
- */
- xfs_trans_ail_insert(log->l_ailp, &efip->efi_item, lsn);
- xfs_efi_release(efip);
+
+ xlog_recover_intent_item(log, &efip->efi_item, lsn,
+ XFS_DEFER_OPS_TYPE_FREE);
return 0;
}
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index ee206facf0dc..a1650fc81382 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1542,6 +1542,7 @@ xlog_alloc_log(
log->l_covered_state = XLOG_STATE_COVER_IDLE;
set_bit(XLOG_ACTIVE_RECOVERY, &log->l_opstate);
INIT_DELAYED_WORK(&log->l_work, xfs_log_worker);
+ INIT_LIST_HEAD(&log->r_dfops);
log->l_prev_block = -1;
/* log->l_tail_lsn = 0x100000000LL; cycle = 1; current block = 0 */
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index fa3ad1d7b31c..e30c06ec20e3 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -407,6 +407,7 @@ struct xlog {
long l_opstate; /* operational state */
uint l_quotaoffs_flag; /* XFS_DQ_*, for QUOTAOFFs */
struct list_head *l_buf_cancel_table;
+ struct list_head r_dfops; /* recovered log intent items */
int l_iclog_hsize; /* size of iclog header */
int l_iclog_heads; /* # of iclog header sectors */
uint l_sectBBsize; /* sector size in BBs (2^n) */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index a1e18b24971a..b9d2152a2bad 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1723,30 +1723,24 @@ xlog_clear_stale_blocks(
*/
void
xlog_recover_release_intent(
- struct xlog *log,
- unsigned short intent_type,
- uint64_t intent_id)
+ struct xlog *log,
+ unsigned short intent_type,
+ uint64_t intent_id)
{
- struct xfs_ail_cursor cur;
- struct xfs_log_item *lip;
- struct xfs_ail *ailp = log->l_ailp;
+ struct xfs_defer_pending *dfp, *n;
+
+ list_for_each_entry_safe(dfp, n, &log->r_dfops, dfp_list) {
+ struct xfs_log_item *lip = dfp->dfp_intent;
- spin_lock(&ailp->ail_lock);
- for (lip = xfs_trans_ail_cursor_first(ailp, &cur, 0); lip != NULL;
- lip = xfs_trans_ail_cursor_next(ailp, &cur)) {
if (lip->li_type != intent_type)
continue;
if (!lip->li_ops->iop_match(lip, intent_id))
continue;
- spin_unlock(&ailp->ail_lock);
- lip->li_ops->iop_release(lip);
- spin_lock(&ailp->ail_lock);
- break;
- }
+ ASSERT(xlog_item_is_intent(lip));
- xfs_trans_ail_cursor_done(&cur);
- spin_unlock(&ailp->ail_lock);
+ xfs_defer_cancel_recovery(log->l_mp, dfp);
+ }
}
int
@@ -1939,6 +1933,29 @@ xlog_buf_readahead(
xfs_buf_readahead(log->l_mp->m_ddev_targp, blkno, len, ops);
}
+/*
+ * Create a deferred work structure for resuming and tracking the progress of a
+ * log intent item that was found during recovery.
+ */
+void
+xlog_recover_intent_item(
+ struct xlog *log,
+ struct xfs_log_item *lip,
+ xfs_lsn_t lsn,
+ unsigned int dfp_type)
+{
+ ASSERT(xlog_item_is_intent(lip));
+
+ xfs_defer_start_recovery(lip, dfp_type, &log->r_dfops);
+
+ /*
+ * Insert the intent into the AIL directly and drop one reference so
+ * that finishing or canceling the work will drop the other.
+ */
+ xfs_trans_ail_insert(log->l_ailp, lip, lsn);
+ lip->li_ops->iop_unpin(lip, 0);
+}
+
STATIC int
xlog_recover_items_pass2(
struct xlog *log,
@@ -2533,29 +2550,22 @@ xlog_abort_defer_ops(
*/
STATIC int
xlog_recover_process_intents(
- struct xlog *log)
+ struct xlog *log)
{
LIST_HEAD(capture_list);
- struct xfs_ail_cursor cur;
- struct xfs_log_item *lip;
- struct xfs_ail *ailp;
- int error = 0;
+ struct xfs_defer_pending *dfp, *n;
+ int error = 0;
#if defined(DEBUG) || defined(XFS_WARN)
- xfs_lsn_t last_lsn;
-#endif
+ xfs_lsn_t last_lsn;
- ailp = log->l_ailp;
- spin_lock(&ailp->ail_lock);
-#if defined(DEBUG) || defined(XFS_WARN)
last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block);
#endif
- for (lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
- lip != NULL;
- lip = xfs_trans_ail_cursor_next(ailp, &cur)) {
- const struct xfs_item_ops *ops;
- if (!xlog_item_is_intent(lip))
- break;
+ list_for_each_entry_safe(dfp, n, &log->r_dfops, dfp_list) {
+ struct xfs_log_item *lip = dfp->dfp_intent;
+ const struct xfs_item_ops *ops = lip->li_ops;
+
+ ASSERT(xlog_item_is_intent(lip));
/*
* We should never see a redo item with a LSN higher than
@@ -2573,19 +2583,22 @@ xlog_recover_process_intents(
* The recovery function can free the log item, so we must not
* access lip after it returns.
*/
- spin_unlock(&ailp->ail_lock);
- ops = lip->li_ops;
error = ops->iop_recover(lip, &capture_list);
- spin_lock(&ailp->ail_lock);
if (error) {
trace_xlog_intent_recovery_failed(log->l_mp, error,
ops->iop_recover);
break;
}
- }
- xfs_trans_ail_cursor_done(&cur);
- spin_unlock(&ailp->ail_lock);
+ /*
+ * XXX: @lip could have been freed, so detach the log item from
+ * the pending item before freeing the pending item. This does
+ * not fix the existing UAF bug that occurs if ->iop_recover
+ * fails after creating the intent done item.
+ */
+ dfp->dfp_intent = NULL;
+ xfs_defer_cancel_recovery(log->l_mp, dfp);
+ }
if (error)
goto err;
@@ -2606,27 +2619,15 @@ xlog_recover_process_intents(
*/
STATIC void
xlog_recover_cancel_intents(
- struct xlog *log)
+ struct xlog *log)
{
- struct xfs_log_item *lip;
- struct xfs_ail_cursor cur;
- struct xfs_ail *ailp;
+ struct xfs_defer_pending *dfp, *n;
- ailp = log->l_ailp;
- spin_lock(&ailp->ail_lock);
- lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
- while (lip != NULL) {
- if (!xlog_item_is_intent(lip))
- break;
+ list_for_each_entry_safe(dfp, n, &log->r_dfops, dfp_list) {
+ ASSERT(xlog_item_is_intent(dfp->dfp_intent));
- spin_unlock(&ailp->ail_lock);
- lip->li_ops->iop_release(lip);
- spin_lock(&ailp->ail_lock);
- lip = xfs_trans_ail_cursor_next(ailp, &cur);
+ xfs_defer_cancel_recovery(log->l_mp, dfp);
}
-
- xfs_trans_ail_cursor_done(&cur);
- spin_unlock(&ailp->ail_lock);
}
/*
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 2d4444d61e98..b88cb2e98227 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -696,12 +696,9 @@ xlog_recover_cui_commit_pass2(
cuip = xfs_cui_init(mp, cui_formatp->cui_nextents);
xfs_cui_copy_format(&cuip->cui_format, cui_formatp);
atomic_set(&cuip->cui_next_extent, cui_formatp->cui_nextents);
- /*
- * Insert the intent into the AIL directly and drop one reference so
- * that finishing or canceling the work will drop the other.
- */
- xfs_trans_ail_insert(log->l_ailp, &cuip->cui_item, lsn);
- xfs_cui_release(cuip);
+
+ xlog_recover_intent_item(log, &cuip->cui_item, lsn,
+ XFS_DEFER_OPS_TYPE_REFCOUNT);
return 0;
}
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 0e0e747028da..c30d4a4a14b2 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -702,12 +702,9 @@ xlog_recover_rui_commit_pass2(
ruip = xfs_rui_init(mp, rui_formatp->rui_nextents);
xfs_rui_copy_format(&ruip->rui_format, rui_formatp);
atomic_set(&ruip->rui_next_extent, rui_formatp->rui_nextents);
- /*
- * Insert the intent into the AIL directly and drop one reference so
- * that finishing or canceling the work will drop the other.
- */
- xfs_trans_ail_insert(log->l_ailp, &ruip->rui_item, lsn);
- xfs_rui_release(ruip);
+
+ xlog_recover_intent_item(log, &ruip->rui_item, lsn,
+ XFS_DEFER_OPS_TYPE_RMAP);
return 0;
}
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/8] xfs: pass the xfs_defer_pending object to iop_recover
2023-12-03 19:00 [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
2023-12-03 19:00 ` [PATCH 1/8] xfs: don't leak recovered attri intent items Darrick J. Wong
2023-12-03 19:01 ` [PATCH 2/8] xfs: use xfs_defer_pending objects to recover " Darrick J. Wong
@ 2023-12-03 19:01 ` Darrick J. Wong
2023-12-04 5:04 ` Christoph Hellwig
2023-12-03 19:01 ` [PATCH 4/8] xfs: transfer recovered intent item ownership in ->iop_recover Darrick J. Wong
` (4 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:01 UTC (permalink / raw)
To: djwong, chandanbabu, hch, leo.lilong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Now that log intent item recovery recreates the xfs_defer_pending state,
we should pass that into the ->iop_recover routines so that the intent
item can finish the recreation work.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_attr_item.c | 3 ++-
fs/xfs/xfs_bmap_item.c | 3 ++-
fs/xfs/xfs_extfree_item.c | 3 ++-
fs/xfs/xfs_log_recover.c | 2 +-
fs/xfs/xfs_refcount_item.c | 3 ++-
fs/xfs/xfs_rmap_item.c | 3 ++-
fs/xfs/xfs_trans.h | 4 +++-
7 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index a32716b8cbbd..6119a7a480a0 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -545,9 +545,10 @@ xfs_attri_validate(
*/
STATIC int
xfs_attri_item_recover(
- struct xfs_log_item *lip,
+ struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
+ struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_attri_log_item *attrip = ATTRI_ITEM(lip);
struct xfs_attr_intent *attr;
struct xfs_mount *mp = lip->li_log->l_mp;
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 6cbae4fdf43f..3ef55de370b5 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -486,11 +486,12 @@ xfs_bui_validate(
*/
STATIC int
xfs_bui_item_recover(
- struct xfs_log_item *lip,
+ struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
struct xfs_bmap_intent fake = { };
struct xfs_trans_res resv;
+ struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_bui_log_item *buip = BUI_ITEM(lip);
struct xfs_trans *tp;
struct xfs_inode *ip = NULL;
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index cf0ddeb70580..a8245c5ffe49 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -657,10 +657,11 @@ xfs_efi_validate_ext(
*/
STATIC int
xfs_efi_item_recover(
- struct xfs_log_item *lip,
+ struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
struct xfs_trans_res resv;
+ struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_efi_log_item *efip = EFI_ITEM(lip);
struct xfs_mount *mp = lip->li_log->l_mp;
struct xfs_efd_log_item *efdp;
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index b9d2152a2bad..ff768217f2c7 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2583,7 +2583,7 @@ xlog_recover_process_intents(
* The recovery function can free the log item, so we must not
* access lip after it returns.
*/
- error = ops->iop_recover(lip, &capture_list);
+ error = ops->iop_recover(dfp, &capture_list);
if (error) {
trace_xlog_intent_recovery_failed(log->l_mp, error,
ops->iop_recover);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index b88cb2e98227..3456201aa3e6 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -474,10 +474,11 @@ xfs_cui_validate_phys(
*/
STATIC int
xfs_cui_item_recover(
- struct xfs_log_item *lip,
+ struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
struct xfs_trans_res resv;
+ struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_cui_log_item *cuip = CUI_ITEM(lip);
struct xfs_cud_log_item *cudp;
struct xfs_trans *tp;
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index c30d4a4a14b2..dfd5a3e4b1fb 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -504,10 +504,11 @@ xfs_rui_validate_map(
*/
STATIC int
xfs_rui_item_recover(
- struct xfs_log_item *lip,
+ struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
struct xfs_trans_res resv;
+ struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_rui_log_item *ruip = RUI_ITEM(lip);
struct xfs_rud_log_item *rudp;
struct xfs_trans *tp;
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 6e3646d524ce..4e38357237c3 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -66,6 +66,8 @@ struct xfs_log_item {
{ (1u << XFS_LI_DIRTY), "DIRTY" }, \
{ (1u << XFS_LI_WHITEOUT), "WHITEOUT" }
+struct xfs_defer_pending;
+
struct xfs_item_ops {
unsigned flags;
void (*iop_size)(struct xfs_log_item *, int *, int *);
@@ -78,7 +80,7 @@ struct xfs_item_ops {
xfs_lsn_t (*iop_committed)(struct xfs_log_item *, xfs_lsn_t);
uint (*iop_push)(struct xfs_log_item *, struct list_head *);
void (*iop_release)(struct xfs_log_item *);
- int (*iop_recover)(struct xfs_log_item *lip,
+ int (*iop_recover)(struct xfs_defer_pending *dfp,
struct list_head *capture_list);
bool (*iop_match)(struct xfs_log_item *item, uint64_t id);
struct xfs_log_item *(*iop_relog)(struct xfs_log_item *intent,
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/8] xfs: transfer recovered intent item ownership in ->iop_recover
2023-12-03 19:00 [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
` (2 preceding siblings ...)
2023-12-03 19:01 ` [PATCH 3/8] xfs: pass the xfs_defer_pending object to iop_recover Darrick J. Wong
@ 2023-12-03 19:01 ` Darrick J. Wong
2023-12-03 19:01 ` [PATCH 5/8] xfs: recreate work items when recovering intent items Darrick J. Wong
` (3 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:01 UTC (permalink / raw)
To: djwong, chandanbabu, hch, leo.lilong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Now that we pass the xfs_defer_pending object into the intent item
recovery functions, we know exactly when ownership of the sole refcount
passes from the recovery context to the intent done item. At that
point, we need to null out dfp_intent so that the recovery mechanism
won't release it. This should fix the UAF problem reported by Long Li.
Note that we still want to recreate the full deferred work state. That
will be addressed in the next patches.
Fixes: 2e76f188fd90 ("xfs: cancel intents immediately if process_intents fails")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_log_recover.h | 2 ++
fs/xfs/xfs_attr_item.c | 1 +
fs/xfs/xfs_bmap_item.c | 2 ++
fs/xfs/xfs_extfree_item.c | 2 ++
fs/xfs/xfs_log_recover.c | 19 ++++++++++++-------
fs/xfs/xfs_refcount_item.c | 1 +
fs/xfs/xfs_rmap_item.c | 2 ++
7 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index 271a4ce7375c..13583df9f239 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -155,5 +155,7 @@ xlog_recover_resv(const struct xfs_trans_res *r)
void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
xfs_lsn_t lsn, unsigned int dfp_type);
+void xlog_recover_transfer_intent(struct xfs_trans *tp,
+ struct xfs_defer_pending *dfp);
#endif /* __XFS_LOG_RECOVER_H__ */
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 6119a7a480a0..82775e9537df 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -632,6 +632,7 @@ xfs_attri_item_recover(
args->trans = tp;
done_item = xfs_trans_get_attrd(tp, attrip);
+ xlog_recover_transfer_intent(tp, dfp);
xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 3ef55de370b5..b6d63b8bdad5 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -524,6 +524,8 @@ xfs_bui_item_recover(
goto err_rele;
budp = xfs_trans_get_bud(tp, buip);
+ xlog_recover_transfer_intent(tp, dfp);
+
xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index a8245c5ffe49..c9908fb33765 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -689,7 +689,9 @@ xfs_efi_item_recover(
error = xfs_trans_alloc(mp, &resv, 0, 0, 0, &tp);
if (error)
return error;
+
efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
+ xlog_recover_transfer_intent(tp, dfp);
for (i = 0; i < efip->efi_format.efi_nextents; i++) {
struct xfs_extent_free_item fake = {
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index ff768217f2c7..cc14cd1c2282 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2590,13 +2590,6 @@ xlog_recover_process_intents(
break;
}
- /*
- * XXX: @lip could have been freed, so detach the log item from
- * the pending item before freeing the pending item. This does
- * not fix the existing UAF bug that occurs if ->iop_recover
- * fails after creating the intent done item.
- */
- dfp->dfp_intent = NULL;
xfs_defer_cancel_recovery(log->l_mp, dfp);
}
if (error)
@@ -2630,6 +2623,18 @@ xlog_recover_cancel_intents(
}
}
+/*
+ * Transfer ownership of the recovered log intent item to the recovery
+ * transaction.
+ */
+void
+xlog_recover_transfer_intent(
+ struct xfs_trans *tp,
+ struct xfs_defer_pending *dfp)
+{
+ dfp->dfp_intent = NULL;
+}
+
/*
* This routine performs a transaction to null out a bad inode pointer
* in an agi unlinked inode hash bucket.
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 3456201aa3e6..f1b259223802 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -523,6 +523,7 @@ xfs_cui_item_recover(
return error;
cudp = xfs_trans_get_cud(tp, cuip);
+ xlog_recover_transfer_intent(tp, dfp);
for (i = 0; i < cuip->cui_format.cui_nextents; i++) {
struct xfs_refcount_intent fake = { };
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index dfd5a3e4b1fb..5e8a02d2b045 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -537,7 +537,9 @@ xfs_rui_item_recover(
XFS_TRANS_RESERVE, &tp);
if (error)
return error;
+
rudp = xfs_trans_get_rud(tp, ruip);
+ xlog_recover_transfer_intent(tp, dfp);
for (i = 0; i < ruip->rui_format.rui_nextents; i++) {
struct xfs_rmap_intent fake = { };
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 5/8] xfs: recreate work items when recovering intent items
2023-12-03 19:00 [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
` (3 preceding siblings ...)
2023-12-03 19:01 ` [PATCH 4/8] xfs: transfer recovered intent item ownership in ->iop_recover Darrick J. Wong
@ 2023-12-03 19:01 ` Darrick J. Wong
2023-12-04 5:05 ` Christoph Hellwig
2023-12-03 19:02 ` [PATCH 6/8] xfs: dump the recovered xattri log item if corruption happens Darrick J. Wong
` (2 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:01 UTC (permalink / raw)
To: djwong, chandanbabu, hch, leo.lilong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Recreate work items for each xfs_defer_pending object when we are
recovering intent items.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/libxfs/xfs_defer.c | 3 -
fs/xfs/libxfs/xfs_defer.h | 9 ++++
fs/xfs/xfs_attr_item.c | 90 ++++++++++++++++++++---------------
fs/xfs/xfs_bmap_item.c | 55 ++++++++++++++--------
fs/xfs/xfs_extfree_item.c | 49 ++++++++++++-------
fs/xfs/xfs_refcount_item.c | 60 +++++++++++-------------
fs/xfs/xfs_rmap_item.c | 112 ++++++++++++++++++++++++--------------------
7 files changed, 215 insertions(+), 163 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 363da37a8e7f..8fb523e4f669 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -676,9 +676,8 @@ xfs_defer_add(
list_add_tail(&dfp->dfp_list, &tp->t_dfops);
}
- list_add_tail(li, &dfp->dfp_work);
+ xfs_defer_add_item(dfp, li);
trace_xfs_defer_add_item(tp->t_mountp, dfp, li);
- dfp->dfp_count++;
}
/*
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 5dce938ba3d5..bef5823f61fb 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -130,6 +130,15 @@ void xfs_defer_start_recovery(struct xfs_log_item *lip,
void xfs_defer_cancel_recovery(struct xfs_mount *mp,
struct xfs_defer_pending *dfp);
+static inline void
+xfs_defer_add_item(
+ struct xfs_defer_pending *dfp,
+ struct list_head *work)
+{
+ list_add_tail(work, &dfp->dfp_work);
+ dfp->dfp_count++;
+}
+
int __init xfs_defer_init_item_caches(void);
void xfs_defer_destroy_item_caches(void);
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 82775e9537df..c4441eacf51c 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -539,42 +539,17 @@ xfs_attri_validate(
return xfs_verify_ino(mp, attrp->alfi_ino);
}
-/*
- * Process an attr intent item that was recovered from the log. We need to
- * delete the attr that it describes.
- */
-STATIC int
-xfs_attri_item_recover(
+static inline struct xfs_attr_intent *
+xfs_attri_recover_work(
+ struct xfs_mount *mp,
struct xfs_defer_pending *dfp,
- struct list_head *capture_list)
+ struct xfs_attri_log_format *attrp,
+ struct xfs_inode *ip,
+ struct xfs_attri_log_nameval *nv)
{
- struct xfs_log_item *lip = dfp->dfp_intent;
- struct xfs_attri_log_item *attrip = ATTRI_ITEM(lip);
struct xfs_attr_intent *attr;
- struct xfs_mount *mp = lip->li_log->l_mp;
- struct xfs_inode *ip;
struct xfs_da_args *args;
- struct xfs_trans *tp;
- struct xfs_trans_res resv;
- struct xfs_attri_log_format *attrp;
- struct xfs_attri_log_nameval *nv = attrip->attri_nameval;
- int error;
- int total;
int local;
- struct xfs_attrd_log_item *done_item = NULL;
-
- /*
- * First check the validity of the attr described by the ATTRI. If any
- * are bad, then assume that all are bad and just toss the ATTRI.
- */
- attrp = &attrip->attri_format;
- if (!xfs_attri_validate(mp, attrp) ||
- !xfs_attr_namecheck(nv->name.i_addr, nv->name.i_len))
- return -EFSCORRUPTED;
-
- error = xlog_recover_iget(mp, attrp->alfi_ino, &ip);
- if (error)
- return error;
attr = kmem_zalloc(sizeof(struct xfs_attr_intent) +
sizeof(struct xfs_da_args), KM_NOFS);
@@ -618,19 +593,58 @@ xfs_attri_item_recover(
case XFS_ATTRI_OP_FLAGS_REMOVE:
attr->xattri_dela_state = xfs_attr_init_remove_state(args);
break;
- default:
- ASSERT(0);
- error = -EFSCORRUPTED;
- goto out;
}
+ xfs_defer_add_item(dfp, &attr->xattri_list);
+ return attr;
+}
+
+/*
+ * Process an attr intent item that was recovered from the log. We need to
+ * delete the attr that it describes.
+ */
+STATIC int
+xfs_attri_item_recover(
+ struct xfs_defer_pending *dfp,
+ struct list_head *capture_list)
+{
+ struct xfs_log_item *lip = dfp->dfp_intent;
+ struct xfs_attri_log_item *attrip = ATTRI_ITEM(lip);
+ struct xfs_attr_intent *attr;
+ struct xfs_mount *mp = lip->li_log->l_mp;
+ struct xfs_inode *ip;
+ struct xfs_da_args *args;
+ struct xfs_trans *tp;
+ struct xfs_trans_res resv;
+ struct xfs_attri_log_format *attrp;
+ struct xfs_attri_log_nameval *nv = attrip->attri_nameval;
+ int error;
+ int total;
+ struct xfs_attrd_log_item *done_item = NULL;
+
+ /*
+ * First check the validity of the attr described by the ATTRI. If any
+ * are bad, then assume that all are bad and just toss the ATTRI.
+ */
+ attrp = &attrip->attri_format;
+ if (!xfs_attri_validate(mp, attrp) ||
+ !xfs_attr_namecheck(nv->name.i_addr, nv->name.i_len))
+ return -EFSCORRUPTED;
+
+ error = xlog_recover_iget(mp, attrp->alfi_ino, &ip);
+ if (error)
+ return error;
+
+ attr = xfs_attri_recover_work(mp, dfp, attrp, ip, nv);
+ args = attr->xattri_da_args;
+
xfs_init_attr_trans(args, &resv, &total);
resv = xlog_recover_resv(&resv);
error = xfs_trans_alloc(mp, &resv, total, 0, XFS_TRANS_RESERVE, &tp);
if (error)
- goto out;
-
+ return error;
args->trans = tp;
+
done_item = xfs_trans_get_attrd(tp, attrip);
xlog_recover_transfer_intent(tp, dfp);
@@ -661,8 +675,6 @@ xfs_attri_item_recover(
out_unlock:
xfs_iunlock(ip, XFS_ILOCK_EXCL);
xfs_irele(ip);
-out:
- xfs_attr_free_item(attr);
return error;
}
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index b6d63b8bdad5..b65999bf0ea3 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -480,6 +480,28 @@ xfs_bui_validate(
return xfs_verify_fsbext(mp, map->me_startblock, map->me_len);
}
+static inline struct xfs_bmap_intent *
+xfs_bui_recover_work(
+ struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp,
+ struct xfs_map_extent *map)
+{
+ struct xfs_bmap_intent *bi;
+
+ bi = kmem_cache_zalloc(xfs_bmap_intent_cache, GFP_NOFS | __GFP_NOFAIL);
+ bi->bi_whichfork = (map->me_flags & XFS_BMAP_EXTENT_ATTR_FORK) ?
+ XFS_ATTR_FORK : XFS_DATA_FORK;
+ bi->bi_type = map->me_flags & XFS_BMAP_EXTENT_TYPE_MASK;
+ bi->bi_bmap.br_startblock = map->me_startblock;
+ bi->bi_bmap.br_startoff = map->me_startoff;
+ bi->bi_bmap.br_blockcount = map->me_len;
+ bi->bi_bmap.br_state = (map->me_flags & XFS_BMAP_EXTENT_UNWRITTEN) ?
+ XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
+
+ xfs_defer_add_item(dfp, &bi->bi_list);
+ return bi;
+}
+
/*
* Process a bmap update intent item that was recovered from the log.
* We need to update some inode's bmbt.
@@ -489,7 +511,6 @@ xfs_bui_item_recover(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
- struct xfs_bmap_intent fake = { };
struct xfs_trans_res resv;
struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_bui_log_item *buip = BUI_ITEM(lip);
@@ -498,6 +519,7 @@ xfs_bui_item_recover(
struct xfs_mount *mp = lip->li_log->l_mp;
struct xfs_map_extent *map;
struct xfs_bud_log_item *budp;
+ struct xfs_bmap_intent *fake;
int iext_delta;
int error = 0;
@@ -508,9 +530,7 @@ xfs_bui_item_recover(
}
map = &buip->bui_format.bui_extents[0];
- fake.bi_whichfork = (map->me_flags & XFS_BMAP_EXTENT_ATTR_FORK) ?
- XFS_ATTR_FORK : XFS_DATA_FORK;
- fake.bi_type = map->me_flags & XFS_BMAP_EXTENT_TYPE_MASK;
+ fake = xfs_bui_recover_work(mp, dfp, map);
error = xlog_recover_iget(mp, map->me_owner, &ip);
if (error)
@@ -529,36 +549,31 @@ xfs_bui_item_recover(
xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
- if (fake.bi_type == XFS_BMAP_MAP)
+ if (fake->bi_type == XFS_BMAP_MAP)
iext_delta = XFS_IEXT_ADD_NOSPLIT_CNT;
else
iext_delta = XFS_IEXT_PUNCH_HOLE_CNT;
- error = xfs_iext_count_may_overflow(ip, fake.bi_whichfork, iext_delta);
+ error = xfs_iext_count_may_overflow(ip, fake->bi_whichfork, iext_delta);
if (error == -EFBIG)
error = xfs_iext_count_upgrade(tp, ip, iext_delta);
if (error)
goto err_cancel;
- fake.bi_owner = ip;
- fake.bi_bmap.br_startblock = map->me_startblock;
- fake.bi_bmap.br_startoff = map->me_startoff;
- fake.bi_bmap.br_blockcount = map->me_len;
- fake.bi_bmap.br_state = (map->me_flags & XFS_BMAP_EXTENT_UNWRITTEN) ?
- XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
+ fake->bi_owner = ip;
- xfs_bmap_update_get_group(mp, &fake);
- error = xfs_trans_log_finish_bmap_update(tp, budp, &fake);
+ xfs_bmap_update_get_group(mp, fake);
+ error = xfs_trans_log_finish_bmap_update(tp, budp, fake);
if (error == -EFSCORRUPTED)
- XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, map,
- sizeof(*map));
- xfs_bmap_update_put_group(&fake);
+ XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+ &buip->bui_format, sizeof(buip->bui_format));
+ xfs_bmap_update_put_group(fake);
if (error)
goto err_cancel;
- if (fake.bi_bmap.br_blockcount > 0) {
- ASSERT(fake.bi_type == XFS_BMAP_UNMAP);
- xfs_bmap_unmap_extent(tp, ip, &fake.bi_bmap);
+ if (fake->bi_bmap.br_blockcount > 0) {
+ ASSERT(fake->bi_type == XFS_BMAP_UNMAP);
+ xfs_bmap_unmap_extent(tp, ip, &fake->bi_bmap);
}
/*
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index c9908fb33765..41108a0b60c9 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -651,6 +651,24 @@ xfs_efi_validate_ext(
return xfs_verify_fsbext(mp, extp->ext_start, extp->ext_len);
}
+static inline void
+xfs_efi_recover_work(
+ struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp,
+ struct xfs_extent *extp)
+{
+ struct xfs_extent_free_item *xefi;
+
+ xefi = kmem_cache_zalloc(xfs_extfree_item_cache,
+ GFP_KERNEL | __GFP_NOFAIL);
+ xefi->xefi_startblock = extp->ext_start;
+ xefi->xefi_blockcount = extp->ext_len;
+ xefi->xefi_agresv = XFS_AG_RESV_NONE;
+ xefi->xefi_owner = XFS_RMAP_OWN_UNKNOWN;
+
+ xfs_defer_add_item(dfp, &xefi->xefi_list);
+}
+
/*
* Process an extent free intent item that was recovered from
* the log. We need to free the extents that it describes.
@@ -666,6 +684,7 @@ xfs_efi_item_recover(
struct xfs_mount *mp = lip->li_log->l_mp;
struct xfs_efd_log_item *efdp;
struct xfs_trans *tp;
+ struct xfs_extent_free_item *fake;
int i;
int error = 0;
bool requeue_only = false;
@@ -683,6 +702,8 @@ xfs_efi_item_recover(
sizeof(efip->efi_format));
return -EFSCORRUPTED;
}
+
+ xfs_efi_recover_work(mp, dfp, &efip->efi_format.efi_extents[i]);
}
resv = xlog_recover_resv(&M_RES(mp)->tr_itruncate);
@@ -693,22 +714,11 @@ xfs_efi_item_recover(
efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
xlog_recover_transfer_intent(tp, dfp);
- for (i = 0; i < efip->efi_format.efi_nextents; i++) {
- struct xfs_extent_free_item fake = {
- .xefi_owner = XFS_RMAP_OWN_UNKNOWN,
- .xefi_agresv = XFS_AG_RESV_NONE,
- };
- struct xfs_extent *extp;
-
- extp = &efip->efi_format.efi_extents[i];
-
- fake.xefi_startblock = extp->ext_start;
- fake.xefi_blockcount = extp->ext_len;
-
+ list_for_each_entry(fake, &dfp->dfp_work, xefi_list) {
if (!requeue_only) {
- xfs_extent_free_get_group(mp, &fake);
- error = xfs_trans_free_extent(tp, efdp, &fake);
- xfs_extent_free_put_group(&fake);
+ xfs_extent_free_get_group(mp, fake);
+ error = xfs_trans_free_extent(tp, efdp, fake);
+ xfs_extent_free_put_group(fake);
}
/*
@@ -717,10 +727,10 @@ xfs_efi_item_recover(
* run again later with a new transaction context.
*/
if (error == -EAGAIN || requeue_only) {
- error = xfs_free_extent_later(tp, fake.xefi_startblock,
- fake.xefi_blockcount,
+ error = xfs_free_extent_later(tp, fake->xefi_startblock,
+ fake->xefi_blockcount,
&XFS_RMAP_OINFO_ANY_OWNER,
- fake.xefi_agresv);
+ fake->xefi_agresv);
if (!error) {
requeue_only = true;
continue;
@@ -729,7 +739,8 @@ xfs_efi_item_recover(
if (error == -EFSCORRUPTED)
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- extp, sizeof(*extp));
+ &efip->efi_format,
+ sizeof(efip->efi_format));
if (error)
goto abort_error;
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index f1b259223802..4ffc34e6f0a0 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -468,6 +468,23 @@ xfs_cui_validate_phys(
return xfs_verify_fsbext(mp, pmap->pe_startblock, pmap->pe_len);
}
+static inline void
+xfs_cui_recover_work(
+ struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp,
+ struct xfs_phys_extent *pmap)
+{
+ struct xfs_refcount_intent *ri;
+
+ ri = kmem_cache_alloc(xfs_refcount_intent_cache,
+ GFP_NOFS | __GFP_NOFAIL);
+ ri->ri_type = pmap->pe_flags & XFS_REFCOUNT_EXTENT_TYPE_MASK;
+ ri->ri_startblock = pmap->pe_startblock;
+ ri->ri_blockcount = pmap->pe_len;
+
+ xfs_defer_add_item(dfp, &ri->ri_list);
+}
+
/*
* Process a refcount update intent item that was recovered from the log.
* We need to update the refcountbt.
@@ -484,7 +501,7 @@ xfs_cui_item_recover(
struct xfs_trans *tp;
struct xfs_btree_cur *rcur = NULL;
struct xfs_mount *mp = lip->li_log->l_mp;
- unsigned int refc_type;
+ struct xfs_refcount_intent *fake;
bool requeue_only = false;
int i;
int error = 0;
@@ -502,6 +519,8 @@ xfs_cui_item_recover(
sizeof(cuip->cui_format));
return -EFSCORRUPTED;
}
+
+ xfs_cui_recover_work(mp, dfp, &cuip->cui_format.cui_extents[i]);
}
/*
@@ -525,35 +544,12 @@ xfs_cui_item_recover(
cudp = xfs_trans_get_cud(tp, cuip);
xlog_recover_transfer_intent(tp, dfp);
- for (i = 0; i < cuip->cui_format.cui_nextents; i++) {
- struct xfs_refcount_intent fake = { };
- struct xfs_phys_extent *pmap;
-
- pmap = &cuip->cui_format.cui_extents[i];
- refc_type = pmap->pe_flags & XFS_REFCOUNT_EXTENT_TYPE_MASK;
- switch (refc_type) {
- case XFS_REFCOUNT_INCREASE:
- case XFS_REFCOUNT_DECREASE:
- case XFS_REFCOUNT_ALLOC_COW:
- case XFS_REFCOUNT_FREE_COW:
- fake.ri_type = refc_type;
- break;
- default:
- XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- &cuip->cui_format,
- sizeof(cuip->cui_format));
- error = -EFSCORRUPTED;
- goto abort_error;
- }
-
- fake.ri_startblock = pmap->pe_startblock;
- fake.ri_blockcount = pmap->pe_len;
-
+ list_for_each_entry(fake, &dfp->dfp_work, ri_list) {
if (!requeue_only) {
- xfs_refcount_update_get_group(mp, &fake);
+ xfs_refcount_update_get_group(mp, fake);
error = xfs_trans_log_finish_refcount_update(tp, cudp,
- &fake, &rcur);
- xfs_refcount_update_put_group(&fake);
+ fake, &rcur);
+ xfs_refcount_update_put_group(fake);
}
if (error == -EFSCORRUPTED)
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
@@ -563,13 +559,13 @@ xfs_cui_item_recover(
goto abort_error;
/* Requeue what we didn't finish. */
- if (fake.ri_blockcount > 0) {
+ if (fake->ri_blockcount > 0) {
struct xfs_bmbt_irec irec = {
- .br_startblock = fake.ri_startblock,
- .br_blockcount = fake.ri_blockcount,
+ .br_startblock = fake->ri_startblock,
+ .br_blockcount = fake->ri_blockcount,
};
- switch (fake.ri_type) {
+ switch (fake->ri_type) {
case XFS_REFCOUNT_INCREASE:
xfs_refcount_increase_extent(tp, &irec);
break;
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 5e8a02d2b045..9fb3ae4bfd59 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -498,6 +498,58 @@ xfs_rui_validate_map(
return xfs_verify_fsbext(mp, map->me_startblock, map->me_len);
}
+static inline void
+xfs_rui_recover_work(
+ struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp,
+ const struct xfs_map_extent *map)
+{
+ struct xfs_rmap_intent *ri;
+
+ ri = kmem_cache_alloc(xfs_rmap_intent_cache, GFP_NOFS | __GFP_NOFAIL);
+
+ switch (map->me_flags & XFS_RMAP_EXTENT_TYPE_MASK) {
+ case XFS_RMAP_EXTENT_MAP:
+ ri->ri_type = XFS_RMAP_MAP;
+ break;
+ case XFS_RMAP_EXTENT_MAP_SHARED:
+ ri->ri_type = XFS_RMAP_MAP_SHARED;
+ break;
+ case XFS_RMAP_EXTENT_UNMAP:
+ ri->ri_type = XFS_RMAP_UNMAP;
+ break;
+ case XFS_RMAP_EXTENT_UNMAP_SHARED:
+ ri->ri_type = XFS_RMAP_UNMAP_SHARED;
+ break;
+ case XFS_RMAP_EXTENT_CONVERT:
+ ri->ri_type = XFS_RMAP_CONVERT;
+ break;
+ case XFS_RMAP_EXTENT_CONVERT_SHARED:
+ ri->ri_type = XFS_RMAP_CONVERT_SHARED;
+ break;
+ case XFS_RMAP_EXTENT_ALLOC:
+ ri->ri_type = XFS_RMAP_ALLOC;
+ break;
+ case XFS_RMAP_EXTENT_FREE:
+ ri->ri_type = XFS_RMAP_FREE;
+ break;
+ default:
+ ASSERT(0);
+ return;
+ }
+
+ ri->ri_owner = map->me_owner;
+ ri->ri_whichfork = (map->me_flags & XFS_RMAP_EXTENT_ATTR_FORK) ?
+ XFS_ATTR_FORK : XFS_DATA_FORK;
+ ri->ri_bmap.br_startblock = map->me_startblock;
+ ri->ri_bmap.br_startoff = map->me_startoff;
+ ri->ri_bmap.br_blockcount = map->me_len;
+ ri->ri_bmap.br_state = (map->me_flags & XFS_RMAP_EXTENT_UNWRITTEN) ?
+ XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
+
+ xfs_defer_add_item(dfp, &ri->ri_list);
+}
+
/*
* Process an rmap update intent item that was recovered from the log.
* We need to update the rmapbt.
@@ -514,6 +566,7 @@ xfs_rui_item_recover(
struct xfs_trans *tp;
struct xfs_btree_cur *rcur = NULL;
struct xfs_mount *mp = lip->li_log->l_mp;
+ struct xfs_rmap_intent *fake;
int i;
int error = 0;
@@ -530,6 +583,8 @@ xfs_rui_item_recover(
sizeof(ruip->rui_format));
return -EFSCORRUPTED;
}
+
+ xfs_rui_recover_work(mp, dfp, &ruip->rui_format.rui_extents[i]);
}
resv = xlog_recover_resv(&M_RES(mp)->tr_itruncate);
@@ -541,60 +596,15 @@ xfs_rui_item_recover(
rudp = xfs_trans_get_rud(tp, ruip);
xlog_recover_transfer_intent(tp, dfp);
- for (i = 0; i < ruip->rui_format.rui_nextents; i++) {
- struct xfs_rmap_intent fake = { };
- struct xfs_map_extent *map;
-
- map = &ruip->rui_format.rui_extents[i];
- switch (map->me_flags & XFS_RMAP_EXTENT_TYPE_MASK) {
- case XFS_RMAP_EXTENT_MAP:
- fake.ri_type = XFS_RMAP_MAP;
- break;
- case XFS_RMAP_EXTENT_MAP_SHARED:
- fake.ri_type = XFS_RMAP_MAP_SHARED;
- break;
- case XFS_RMAP_EXTENT_UNMAP:
- fake.ri_type = XFS_RMAP_UNMAP;
- break;
- case XFS_RMAP_EXTENT_UNMAP_SHARED:
- fake.ri_type = XFS_RMAP_UNMAP_SHARED;
- break;
- case XFS_RMAP_EXTENT_CONVERT:
- fake.ri_type = XFS_RMAP_CONVERT;
- break;
- case XFS_RMAP_EXTENT_CONVERT_SHARED:
- fake.ri_type = XFS_RMAP_CONVERT_SHARED;
- break;
- case XFS_RMAP_EXTENT_ALLOC:
- fake.ri_type = XFS_RMAP_ALLOC;
- break;
- case XFS_RMAP_EXTENT_FREE:
- fake.ri_type = XFS_RMAP_FREE;
- break;
- default:
- XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- &ruip->rui_format,
- sizeof(ruip->rui_format));
- error = -EFSCORRUPTED;
- goto abort_error;
- }
-
- fake.ri_owner = map->me_owner;
- fake.ri_whichfork = (map->me_flags & XFS_RMAP_EXTENT_ATTR_FORK) ?
- XFS_ATTR_FORK : XFS_DATA_FORK;
- fake.ri_bmap.br_startblock = map->me_startblock;
- fake.ri_bmap.br_startoff = map->me_startoff;
- fake.ri_bmap.br_blockcount = map->me_len;
- fake.ri_bmap.br_state = (map->me_flags & XFS_RMAP_EXTENT_UNWRITTEN) ?
- XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
-
- xfs_rmap_update_get_group(mp, &fake);
- error = xfs_trans_log_finish_rmap_update(tp, rudp, &fake,
+ list_for_each_entry(fake, &dfp->dfp_work, ri_list) {
+ xfs_rmap_update_get_group(mp, fake);
+ error = xfs_trans_log_finish_rmap_update(tp, rudp, fake,
&rcur);
if (error == -EFSCORRUPTED)
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- map, sizeof(*map));
- xfs_rmap_update_put_group(&fake);
+ &ruip->rui_format,
+ sizeof(ruip->rui_format));
+ xfs_rmap_update_put_group(fake);
if (error)
goto abort_error;
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 6/8] xfs: dump the recovered xattri log item if corruption happens
2023-12-03 19:00 [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
` (4 preceding siblings ...)
2023-12-03 19:01 ` [PATCH 5/8] xfs: recreate work items when recovering intent items Darrick J. Wong
@ 2023-12-03 19:02 ` Darrick J. Wong
2023-12-04 5:05 ` Christoph Hellwig
2023-12-03 19:02 ` [PATCH 7/8] xfs: use xfs_defer_finish_one to finish recovered work items Darrick J. Wong
2023-12-03 19:02 ` [PATCH 8/8] xfs: move ->iop_recover to xfs_defer_op_type Darrick J. Wong
7 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:02 UTC (permalink / raw)
To: djwong, chandanbabu, hch, leo.lilong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
If xfs_attri_item_recover receives a corruption error when it tries to
finish a recovered log intent item, it should dump the log item for
debugging, just like all the other log intent items.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/xfs_attr_item.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index c4441eacf51c..c7c308d2f897 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -666,6 +666,10 @@ xfs_attri_item_recover(
xfs_irele(ip);
return 0;
}
+ if (error == -EFSCORRUPTED)
+ XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+ &attrip->attri_format,
+ sizeof(attrip->attri_format));
if (error) {
xfs_trans_cancel(tp);
goto out_unlock;
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 7/8] xfs: use xfs_defer_finish_one to finish recovered work items
2023-12-03 19:00 [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
` (5 preceding siblings ...)
2023-12-03 19:02 ` [PATCH 6/8] xfs: dump the recovered xattri log item if corruption happens Darrick J. Wong
@ 2023-12-03 19:02 ` Darrick J. Wong
2023-12-04 5:06 ` Christoph Hellwig
2023-12-03 19:02 ` [PATCH 8/8] xfs: move ->iop_recover to xfs_defer_op_type Darrick J. Wong
7 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:02 UTC (permalink / raw)
To: djwong, chandanbabu, hch, leo.lilong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Get rid of the open-coded calls to xfs_defer_finish_one. This also
means that the recovery transaction takes care of cleaning up the dfp,
and we have solved (I hope) all the ownership issues in recovery.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/libxfs/xfs_defer.c | 2 +
fs/xfs/libxfs/xfs_defer.h | 1 +
fs/xfs/libxfs/xfs_log_recover.h | 2 +
fs/xfs/xfs_attr_item.c | 20 +------------
fs/xfs/xfs_bmap_item.c | 24 ++++-----------
fs/xfs/xfs_extfree_item.c | 45 +++++------------------------
fs/xfs/xfs_log_recover.c | 22 +++++++++-----
fs/xfs/xfs_refcount_item.c | 61 +++++----------------------------------
fs/xfs/xfs_rmap_item.c | 29 +++++--------------
9 files changed, 49 insertions(+), 157 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 8fb523e4f669..eb262ea06122 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -484,7 +484,7 @@ xfs_defer_relog(
* Log an intent-done item for the first pending intent, and finish the work
* items.
*/
-static int
+int
xfs_defer_finish_one(
struct xfs_trans *tp,
struct xfs_defer_pending *dfp)
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index bef5823f61fb..c1a648e99174 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -41,6 +41,7 @@ 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);
int xfs_defer_finish(struct xfs_trans **tp);
+int xfs_defer_finish_one(struct xfs_trans *tp, struct xfs_defer_pending *dfp);
void xfs_defer_cancel(struct xfs_trans *);
void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index 13583df9f239..52162a17fc5e 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -155,7 +155,7 @@ xlog_recover_resv(const struct xfs_trans_res *r)
void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
xfs_lsn_t lsn, unsigned int dfp_type);
-void xlog_recover_transfer_intent(struct xfs_trans *tp,
+int xlog_recover_finish_intent(struct xfs_trans *tp,
struct xfs_defer_pending *dfp);
#endif /* __XFS_LOG_RECOVER_H__ */
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index c7c308d2f897..eaf8a877c2cc 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -620,7 +620,6 @@ xfs_attri_item_recover(
struct xfs_attri_log_nameval *nv = attrip->attri_nameval;
int error;
int total;
- struct xfs_attrd_log_item *done_item = NULL;
/*
* First check the validity of the attr described by the ATTRI. If any
@@ -645,27 +644,10 @@ xfs_attri_item_recover(
return error;
args->trans = tp;
- done_item = xfs_trans_get_attrd(tp, attrip);
- xlog_recover_transfer_intent(tp, dfp);
-
xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
- error = xfs_xattri_finish_update(attr, done_item);
- if (error == -EAGAIN) {
- /*
- * There's more work to do, so add the intent item to this
- * transaction so that we can continue it later.
- */
- xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_ATTR, &attr->xattri_list);
- error = xfs_defer_ops_capture_and_commit(tp, capture_list);
- if (error)
- goto out_unlock;
-
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
- xfs_irele(ip);
- return 0;
- }
+ error = xlog_recover_finish_intent(tp, dfp);
if (error == -EFSCORRUPTED)
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
&attrip->attri_format,
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index b65999bf0ea3..89f2d9e89607 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -497,6 +497,7 @@ xfs_bui_recover_work(
bi->bi_bmap.br_blockcount = map->me_len;
bi->bi_bmap.br_state = (map->me_flags & XFS_BMAP_EXTENT_UNWRITTEN) ?
XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
+ xfs_bmap_update_get_group(mp, bi);
xfs_defer_add_item(dfp, &bi->bi_list);
return bi;
@@ -518,8 +519,7 @@ xfs_bui_item_recover(
struct xfs_inode *ip = NULL;
struct xfs_mount *mp = lip->li_log->l_mp;
struct xfs_map_extent *map;
- struct xfs_bud_log_item *budp;
- struct xfs_bmap_intent *fake;
+ struct xfs_bmap_intent *work;
int iext_delta;
int error = 0;
@@ -530,7 +530,7 @@ xfs_bui_item_recover(
}
map = &buip->bui_format.bui_extents[0];
- fake = xfs_bui_recover_work(mp, dfp, map);
+ work = xfs_bui_recover_work(mp, dfp, map);
error = xlog_recover_iget(mp, map->me_owner, &ip);
if (error)
@@ -543,39 +543,29 @@ xfs_bui_item_recover(
if (error)
goto err_rele;
- budp = xfs_trans_get_bud(tp, buip);
- xlog_recover_transfer_intent(tp, dfp);
-
xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
- if (fake->bi_type == XFS_BMAP_MAP)
+ if (work->bi_type == XFS_BMAP_MAP)
iext_delta = XFS_IEXT_ADD_NOSPLIT_CNT;
else
iext_delta = XFS_IEXT_PUNCH_HOLE_CNT;
- error = xfs_iext_count_may_overflow(ip, fake->bi_whichfork, iext_delta);
+ error = xfs_iext_count_may_overflow(ip, work->bi_whichfork, iext_delta);
if (error == -EFBIG)
error = xfs_iext_count_upgrade(tp, ip, iext_delta);
if (error)
goto err_cancel;
- fake->bi_owner = ip;
+ work->bi_owner = ip;
- xfs_bmap_update_get_group(mp, fake);
- error = xfs_trans_log_finish_bmap_update(tp, budp, fake);
+ error = xlog_recover_finish_intent(tp, dfp);
if (error == -EFSCORRUPTED)
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
&buip->bui_format, sizeof(buip->bui_format));
- xfs_bmap_update_put_group(fake);
if (error)
goto err_cancel;
- if (fake->bi_bmap.br_blockcount > 0) {
- ASSERT(fake->bi_type == XFS_BMAP_UNMAP);
- xfs_bmap_unmap_extent(tp, ip, &fake->bi_bmap);
- }
-
/*
* Commit transaction, which frees the transaction and saves the inode
* for later replay activities.
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 41108a0b60c9..6a434ade486c 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -665,6 +665,7 @@ xfs_efi_recover_work(
xefi->xefi_blockcount = extp->ext_len;
xefi->xefi_agresv = XFS_AG_RESV_NONE;
xefi->xefi_owner = XFS_RMAP_OWN_UNKNOWN;
+ xfs_extent_free_get_group(mp, xefi);
xfs_defer_add_item(dfp, &xefi->xefi_list);
}
@@ -682,12 +683,9 @@ xfs_efi_item_recover(
struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_efi_log_item *efip = EFI_ITEM(lip);
struct xfs_mount *mp = lip->li_log->l_mp;
- struct xfs_efd_log_item *efdp;
struct xfs_trans *tp;
- struct xfs_extent_free_item *fake;
int i;
int error = 0;
- bool requeue_only = false;
/*
* First check the validity of the extents described by the
@@ -711,40 +709,13 @@ xfs_efi_item_recover(
if (error)
return error;
- efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
- xlog_recover_transfer_intent(tp, dfp);
-
- list_for_each_entry(fake, &dfp->dfp_work, xefi_list) {
- if (!requeue_only) {
- xfs_extent_free_get_group(mp, fake);
- error = xfs_trans_free_extent(tp, efdp, fake);
- xfs_extent_free_put_group(fake);
- }
-
- /*
- * If we can't free the extent without potentially deadlocking,
- * requeue the rest of the extents to a new so that they get
- * run again later with a new transaction context.
- */
- if (error == -EAGAIN || requeue_only) {
- error = xfs_free_extent_later(tp, fake->xefi_startblock,
- fake->xefi_blockcount,
- &XFS_RMAP_OINFO_ANY_OWNER,
- fake->xefi_agresv);
- if (!error) {
- requeue_only = true;
- continue;
- }
- }
-
- if (error == -EFSCORRUPTED)
- XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- &efip->efi_format,
- sizeof(efip->efi_format));
- if (error)
- goto abort_error;
-
- }
+ error = xlog_recover_finish_intent(tp, dfp);
+ if (error == -EFSCORRUPTED)
+ XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+ &efip->efi_format,
+ sizeof(efip->efi_format));
+ if (error)
+ goto abort_error;
return xfs_defer_ops_capture_and_commit(tp, capture_list);
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index cc14cd1c2282..6fab490959d4 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2581,7 +2581,8 @@ xlog_recover_process_intents(
* replayed in the wrong order!
*
* The recovery function can free the log item, so we must not
- * access lip after it returns.
+ * access lip after it returns. It must dispose of @dfp if it
+ * returns 0.
*/
error = ops->iop_recover(dfp, &capture_list);
if (error) {
@@ -2589,8 +2590,6 @@ xlog_recover_process_intents(
ops->iop_recover);
break;
}
-
- xfs_defer_cancel_recovery(log->l_mp, dfp);
}
if (error)
goto err;
@@ -2624,15 +2623,22 @@ xlog_recover_cancel_intents(
}
/*
- * Transfer ownership of the recovered log intent item to the recovery
- * transaction.
+ * Transfer ownership of the recovered pending work to the recovery transaction
+ * and try to finish the work. If there is more work to be done, the dfp will
+ * remain attached to the transaction. If not, the dfp is freed.
*/
-void
-xlog_recover_transfer_intent(
+int
+xlog_recover_finish_intent(
struct xfs_trans *tp,
struct xfs_defer_pending *dfp)
{
- dfp->dfp_intent = NULL;
+ int error;
+
+ list_move(&dfp->dfp_list, &tp->t_dfops);
+ error = xfs_defer_finish_one(tp, dfp);
+ if (error == -EAGAIN)
+ return 0;
+ return error;
}
/*
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 4ffc34e6f0a0..f561ca73c784 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -481,6 +481,7 @@ xfs_cui_recover_work(
ri->ri_type = pmap->pe_flags & XFS_REFCOUNT_EXTENT_TYPE_MASK;
ri->ri_startblock = pmap->pe_startblock;
ri->ri_blockcount = pmap->pe_len;
+ xfs_refcount_update_get_group(mp, ri);
xfs_defer_add_item(dfp, &ri->ri_list);
}
@@ -497,12 +498,8 @@ xfs_cui_item_recover(
struct xfs_trans_res resv;
struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_cui_log_item *cuip = CUI_ITEM(lip);
- struct xfs_cud_log_item *cudp;
struct xfs_trans *tp;
- struct xfs_btree_cur *rcur = NULL;
struct xfs_mount *mp = lip->li_log->l_mp;
- struct xfs_refcount_intent *fake;
- bool requeue_only = false;
int i;
int error = 0;
@@ -541,59 +538,17 @@ xfs_cui_item_recover(
if (error)
return error;
- cudp = xfs_trans_get_cud(tp, cuip);
- xlog_recover_transfer_intent(tp, dfp);
+ error = xlog_recover_finish_intent(tp, dfp);
+ if (error == -EFSCORRUPTED)
+ XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+ &cuip->cui_format,
+ sizeof(cuip->cui_format));
+ if (error)
+ goto abort_error;
- list_for_each_entry(fake, &dfp->dfp_work, ri_list) {
- if (!requeue_only) {
- xfs_refcount_update_get_group(mp, fake);
- error = xfs_trans_log_finish_refcount_update(tp, cudp,
- fake, &rcur);
- xfs_refcount_update_put_group(fake);
- }
- if (error == -EFSCORRUPTED)
- XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- &cuip->cui_format,
- sizeof(cuip->cui_format));
- if (error)
- goto abort_error;
-
- /* Requeue what we didn't finish. */
- if (fake->ri_blockcount > 0) {
- struct xfs_bmbt_irec irec = {
- .br_startblock = fake->ri_startblock,
- .br_blockcount = fake->ri_blockcount,
- };
-
- switch (fake->ri_type) {
- case XFS_REFCOUNT_INCREASE:
- xfs_refcount_increase_extent(tp, &irec);
- break;
- case XFS_REFCOUNT_DECREASE:
- xfs_refcount_decrease_extent(tp, &irec);
- break;
- case XFS_REFCOUNT_ALLOC_COW:
- xfs_refcount_alloc_cow_extent(tp,
- irec.br_startblock,
- irec.br_blockcount);
- break;
- case XFS_REFCOUNT_FREE_COW:
- xfs_refcount_free_cow_extent(tp,
- irec.br_startblock,
- irec.br_blockcount);
- break;
- default:
- ASSERT(0);
- }
- requeue_only = true;
- }
- }
-
- xfs_refcount_finish_one_cleanup(tp, rcur, error);
return xfs_defer_ops_capture_and_commit(tp, capture_list);
abort_error:
- xfs_refcount_finish_one_cleanup(tp, rcur, error);
xfs_trans_cancel(tp);
return error;
}
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 9fb3ae4bfd59..23e736179894 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -546,6 +546,7 @@ xfs_rui_recover_work(
ri->ri_bmap.br_blockcount = map->me_len;
ri->ri_bmap.br_state = (map->me_flags & XFS_RMAP_EXTENT_UNWRITTEN) ?
XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
+ xfs_rmap_update_get_group(mp, ri);
xfs_defer_add_item(dfp, &ri->ri_list);
}
@@ -562,11 +563,8 @@ xfs_rui_item_recover(
struct xfs_trans_res resv;
struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_rui_log_item *ruip = RUI_ITEM(lip);
- struct xfs_rud_log_item *rudp;
struct xfs_trans *tp;
- struct xfs_btree_cur *rcur = NULL;
struct xfs_mount *mp = lip->li_log->l_mp;
- struct xfs_rmap_intent *fake;
int i;
int error = 0;
@@ -593,28 +591,17 @@ xfs_rui_item_recover(
if (error)
return error;
- rudp = xfs_trans_get_rud(tp, ruip);
- xlog_recover_transfer_intent(tp, dfp);
+ error = xlog_recover_finish_intent(tp, dfp);
+ if (error == -EFSCORRUPTED)
+ XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+ &ruip->rui_format,
+ sizeof(ruip->rui_format));
+ if (error)
+ goto abort_error;
- list_for_each_entry(fake, &dfp->dfp_work, ri_list) {
- xfs_rmap_update_get_group(mp, fake);
- error = xfs_trans_log_finish_rmap_update(tp, rudp, fake,
- &rcur);
- if (error == -EFSCORRUPTED)
- XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- &ruip->rui_format,
- sizeof(ruip->rui_format));
- xfs_rmap_update_put_group(fake);
- if (error)
- goto abort_error;
-
- }
-
- xfs_rmap_finish_one_cleanup(tp, rcur, error);
return xfs_defer_ops_capture_and_commit(tp, capture_list);
abort_error:
- xfs_rmap_finish_one_cleanup(tp, rcur, error);
xfs_trans_cancel(tp);
return error;
}
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 8/8] xfs: move ->iop_recover to xfs_defer_op_type
2023-12-03 19:00 [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
` (6 preceding siblings ...)
2023-12-03 19:02 ` [PATCH 7/8] xfs: use xfs_defer_finish_one to finish recovered work items Darrick J. Wong
@ 2023-12-03 19:02 ` Darrick J. Wong
2023-12-04 19:05 ` [PATCH v2.1 " Darrick J. Wong
7 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:02 UTC (permalink / raw)
To: djwong, chandanbabu, hch, leo.lilong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Finish off the series by moving the intent item recovery function
pointer to the xfs_defer_op_type struct, since this is really a deferred
work function now.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_defer.c | 17 +++++++++++++++
fs/xfs/libxfs/xfs_defer.h | 4 ++++
fs/xfs/libxfs/xfs_log_recover.h | 2 ++
fs/xfs/xfs_attr_item.c | 4 ++--
fs/xfs/xfs_bmap_item.c | 22 ++++++++++----------
fs/xfs/xfs_extfree_item.c | 43 ++++++++++++++++++++-------------------
fs/xfs/xfs_log_recover.c | 9 +++-----
fs/xfs/xfs_refcount_item.c | 24 +++++++++++-----------
fs/xfs/xfs_rmap_item.c | 24 +++++++++++-----------
fs/xfs/xfs_trans.h | 4 ----
10 files changed, 85 insertions(+), 68 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index eb262ea06122..dd565e4e3daf 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -713,6 +713,23 @@ xfs_defer_cancel_recovery(
xfs_defer_pending_cancel_work(mp, dfp);
}
+/* Replay the deferred work item created from a recovered log intent item. */
+int
+xfs_defer_finish_recovery(
+ struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp,
+ struct list_head *capture_list)
+{
+ const struct xfs_defer_op_type *ops = defer_op_types[dfp->dfp_type];
+ int error;
+
+ error = ops->recover_work(dfp, capture_list);
+ if (error)
+ trace_xlog_intent_recovery_failed(mp, error,
+ ops->recover_work);
+ return error;
+}
+
/*
* Move deferred ops from one transaction to another and reset the source to
* initial state. This is primarily used to carry state forward across
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index c1a648e99174..ef86a7f9b059 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -57,6 +57,8 @@ struct xfs_defer_op_type {
void (*finish_cleanup)(struct xfs_trans *tp,
struct xfs_btree_cur *state, int error);
void (*cancel_item)(struct list_head *item);
+ int (*recover_work)(struct xfs_defer_pending *dfp,
+ struct list_head *capture_list);
unsigned int max_items;
};
@@ -130,6 +132,8 @@ void xfs_defer_start_recovery(struct xfs_log_item *lip,
enum xfs_defer_ops_type dfp_type, struct list_head *r_dfops);
void xfs_defer_cancel_recovery(struct xfs_mount *mp,
struct xfs_defer_pending *dfp);
+int xfs_defer_finish_recovery(struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp, struct list_head *capture_list);
static inline void
xfs_defer_add_item(
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index 52162a17fc5e..c8e5d912895b 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -153,6 +153,8 @@ xlog_recover_resv(const struct xfs_trans_res *r)
return ret;
}
+struct xfs_defer_pending;
+
void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
xfs_lsn_t lsn, unsigned int dfp_type);
int xlog_recover_finish_intent(struct xfs_trans *tp,
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index eaf8a877c2cc..5f7d8e8d87dc 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -604,7 +604,7 @@ xfs_attri_recover_work(
* delete the attr that it describes.
*/
STATIC int
-xfs_attri_item_recover(
+xfs_attr_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -820,6 +820,7 @@ const struct xfs_defer_op_type xfs_attr_defer_type = {
.create_done = xfs_attr_create_done,
.finish_item = xfs_attr_finish_item,
.cancel_item = xfs_attr_cancel_item,
+ .recover_work = xfs_attr_recover_work,
};
/*
@@ -856,7 +857,6 @@ static const struct xfs_item_ops xfs_attri_item_ops = {
.iop_format = xfs_attri_item_format,
.iop_unpin = xfs_attri_item_unpin,
.iop_release = xfs_attri_item_release,
- .iop_recover = xfs_attri_item_recover,
.iop_match = xfs_attri_item_match,
.iop_relog = xfs_attri_item_relog,
};
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 89f2d9e89607..fa305d242f24 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -437,15 +437,6 @@ xfs_bmap_update_cancel_item(
kmem_cache_free(xfs_bmap_intent_cache, bi);
}
-const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
- .max_items = XFS_BUI_MAX_FAST_EXTENTS,
- .create_intent = xfs_bmap_update_create_intent,
- .abort_intent = xfs_bmap_update_abort_intent,
- .create_done = xfs_bmap_update_create_done,
- .finish_item = xfs_bmap_update_finish_item,
- .cancel_item = xfs_bmap_update_cancel_item,
-};
-
/* Is this recovered BUI ok? */
static inline bool
xfs_bui_validate(
@@ -508,7 +499,7 @@ xfs_bui_recover_work(
* We need to update some inode's bmbt.
*/
STATIC int
-xfs_bui_item_recover(
+xfs_bmap_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -587,6 +578,16 @@ xfs_bui_item_recover(
return error;
}
+const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
+ .max_items = XFS_BUI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_bmap_update_create_intent,
+ .abort_intent = xfs_bmap_update_abort_intent,
+ .create_done = xfs_bmap_update_create_done,
+ .finish_item = xfs_bmap_update_finish_item,
+ .cancel_item = xfs_bmap_update_cancel_item,
+ .recover_work = xfs_bmap_recover_work,
+};
+
STATIC bool
xfs_bui_item_match(
struct xfs_log_item *lip,
@@ -627,7 +628,6 @@ static const struct xfs_item_ops xfs_bui_item_ops = {
.iop_format = xfs_bui_item_format,
.iop_unpin = xfs_bui_item_unpin,
.iop_release = xfs_bui_item_release,
- .iop_recover = xfs_bui_item_recover,
.iop_match = xfs_bui_item_match,
.iop_relog = xfs_bui_item_relog,
};
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6a434ade486c..49e96ffd64e0 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -567,15 +567,6 @@ xfs_extent_free_cancel_item(
kmem_cache_free(xfs_extfree_item_cache, xefi);
}
-const struct xfs_defer_op_type xfs_extent_free_defer_type = {
- .max_items = XFS_EFI_MAX_FAST_EXTENTS,
- .create_intent = xfs_extent_free_create_intent,
- .abort_intent = xfs_extent_free_abort_intent,
- .create_done = xfs_extent_free_create_done,
- .finish_item = xfs_extent_free_finish_item,
- .cancel_item = xfs_extent_free_cancel_item,
-};
-
/*
* AGFL blocks are accounted differently in the reserve pools and are not
* inserted into the busy extent list.
@@ -632,16 +623,6 @@ xfs_agfl_free_finish_item(
return error;
}
-/* sub-type with special handling for AGFL deferred frees */
-const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
- .max_items = XFS_EFI_MAX_FAST_EXTENTS,
- .create_intent = xfs_extent_free_create_intent,
- .abort_intent = xfs_extent_free_abort_intent,
- .create_done = xfs_extent_free_create_done,
- .finish_item = xfs_agfl_free_finish_item,
- .cancel_item = xfs_extent_free_cancel_item,
-};
-
/* Is this recovered EFI ok? */
static inline bool
xfs_efi_validate_ext(
@@ -675,7 +656,7 @@ xfs_efi_recover_work(
* the log. We need to free the extents that it describes.
*/
STATIC int
-xfs_efi_item_recover(
+xfs_extent_free_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -724,6 +705,27 @@ xfs_efi_item_recover(
return error;
}
+const struct xfs_defer_op_type xfs_extent_free_defer_type = {
+ .max_items = XFS_EFI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_extent_free_create_intent,
+ .abort_intent = xfs_extent_free_abort_intent,
+ .create_done = xfs_extent_free_create_done,
+ .finish_item = xfs_extent_free_finish_item,
+ .cancel_item = xfs_extent_free_cancel_item,
+ .recover_work = xfs_extent_free_recover_work,
+};
+
+/* sub-type with special handling for AGFL deferred frees */
+const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+ .max_items = XFS_EFI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_extent_free_create_intent,
+ .abort_intent = xfs_extent_free_abort_intent,
+ .create_done = xfs_extent_free_create_done,
+ .finish_item = xfs_agfl_free_finish_item,
+ .cancel_item = xfs_extent_free_cancel_item,
+ .recover_work = xfs_extent_free_recover_work,
+};
+
STATIC bool
xfs_efi_item_match(
struct xfs_log_item *lip,
@@ -766,7 +768,6 @@ static const struct xfs_item_ops xfs_efi_item_ops = {
.iop_format = xfs_efi_item_format,
.iop_unpin = xfs_efi_item_unpin,
.iop_release = xfs_efi_item_release,
- .iop_recover = xfs_efi_item_recover,
.iop_match = xfs_efi_item_match,
.iop_relog = xfs_efi_item_relog,
};
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 6fab490959d4..1984f741802f 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2563,7 +2563,6 @@ xlog_recover_process_intents(
list_for_each_entry_safe(dfp, n, &log->r_dfops, dfp_list) {
struct xfs_log_item *lip = dfp->dfp_intent;
- const struct xfs_item_ops *ops = lip->li_ops;
ASSERT(xlog_item_is_intent(lip));
@@ -2584,12 +2583,10 @@ xlog_recover_process_intents(
* access lip after it returns. It must dispose of @dfp if it
* returns 0.
*/
- error = ops->iop_recover(dfp, &capture_list);
- if (error) {
- trace_xlog_intent_recovery_failed(log->l_mp, error,
- ops->iop_recover);
+ error = xfs_defer_finish_recovery(log->l_mp, dfp,
+ &capture_list);
+ if (error)
break;
- }
}
if (error)
goto err;
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index f561ca73c784..48f1a38b272e 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -433,16 +433,6 @@ xfs_refcount_update_cancel_item(
kmem_cache_free(xfs_refcount_intent_cache, ri);
}
-const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
- .max_items = XFS_CUI_MAX_FAST_EXTENTS,
- .create_intent = xfs_refcount_update_create_intent,
- .abort_intent = xfs_refcount_update_abort_intent,
- .create_done = xfs_refcount_update_create_done,
- .finish_item = xfs_refcount_update_finish_item,
- .finish_cleanup = xfs_refcount_finish_one_cleanup,
- .cancel_item = xfs_refcount_update_cancel_item,
-};
-
/* Is this recovered CUI ok? */
static inline bool
xfs_cui_validate_phys(
@@ -491,7 +481,7 @@ xfs_cui_recover_work(
* We need to update the refcountbt.
*/
STATIC int
-xfs_cui_item_recover(
+xfs_refcount_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -553,6 +543,17 @@ xfs_cui_item_recover(
return error;
}
+const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
+ .max_items = XFS_CUI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_refcount_update_create_intent,
+ .abort_intent = xfs_refcount_update_abort_intent,
+ .create_done = xfs_refcount_update_create_done,
+ .finish_item = xfs_refcount_update_finish_item,
+ .finish_cleanup = xfs_refcount_finish_one_cleanup,
+ .cancel_item = xfs_refcount_update_cancel_item,
+ .recover_work = xfs_refcount_recover_work,
+};
+
STATIC bool
xfs_cui_item_match(
struct xfs_log_item *lip,
@@ -593,7 +594,6 @@ static const struct xfs_item_ops xfs_cui_item_ops = {
.iop_format = xfs_cui_item_format,
.iop_unpin = xfs_cui_item_unpin,
.iop_release = xfs_cui_item_release,
- .iop_recover = xfs_cui_item_recover,
.iop_match = xfs_cui_item_match,
.iop_relog = xfs_cui_item_relog,
};
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 23e736179894..23684bc2ab85 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -452,16 +452,6 @@ xfs_rmap_update_cancel_item(
kmem_cache_free(xfs_rmap_intent_cache, ri);
}
-const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
- .max_items = XFS_RUI_MAX_FAST_EXTENTS,
- .create_intent = xfs_rmap_update_create_intent,
- .abort_intent = xfs_rmap_update_abort_intent,
- .create_done = xfs_rmap_update_create_done,
- .finish_item = xfs_rmap_update_finish_item,
- .finish_cleanup = xfs_rmap_finish_one_cleanup,
- .cancel_item = xfs_rmap_update_cancel_item,
-};
-
/* Is this recovered RUI ok? */
static inline bool
xfs_rui_validate_map(
@@ -556,7 +546,7 @@ xfs_rui_recover_work(
* We need to update the rmapbt.
*/
STATIC int
-xfs_rui_item_recover(
+xfs_rmap_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -606,6 +596,17 @@ xfs_rui_item_recover(
return error;
}
+const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
+ .max_items = XFS_RUI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_rmap_update_create_intent,
+ .abort_intent = xfs_rmap_update_abort_intent,
+ .create_done = xfs_rmap_update_create_done,
+ .finish_item = xfs_rmap_update_finish_item,
+ .finish_cleanup = xfs_rmap_finish_one_cleanup,
+ .cancel_item = xfs_rmap_update_cancel_item,
+ .recover_work = xfs_rmap_recover_work,
+};
+
STATIC bool
xfs_rui_item_match(
struct xfs_log_item *lip,
@@ -646,7 +647,6 @@ static const struct xfs_item_ops xfs_rui_item_ops = {
.iop_format = xfs_rui_item_format,
.iop_unpin = xfs_rui_item_unpin,
.iop_release = xfs_rui_item_release,
- .iop_recover = xfs_rui_item_recover,
.iop_match = xfs_rui_item_match,
.iop_relog = xfs_rui_item_relog,
};
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 4e38357237c3..5fb018ad9fc0 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -66,8 +66,6 @@ struct xfs_log_item {
{ (1u << XFS_LI_DIRTY), "DIRTY" }, \
{ (1u << XFS_LI_WHITEOUT), "WHITEOUT" }
-struct xfs_defer_pending;
-
struct xfs_item_ops {
unsigned flags;
void (*iop_size)(struct xfs_log_item *, int *, int *);
@@ -80,8 +78,6 @@ struct xfs_item_ops {
xfs_lsn_t (*iop_committed)(struct xfs_log_item *, xfs_lsn_t);
uint (*iop_push)(struct xfs_log_item *, struct list_head *);
void (*iop_release)(struct xfs_log_item *);
- int (*iop_recover)(struct xfs_defer_pending *dfp,
- struct list_head *capture_list);
bool (*iop_match)(struct xfs_log_item *item, uint64_t id);
struct xfs_log_item *(*iop_relog)(struct xfs_log_item *intent,
struct xfs_trans *tp);
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/8] xfs: use xfs_defer_pending objects to recover intent items
2023-12-03 19:01 ` [PATCH 2/8] xfs: use xfs_defer_pending objects to recover " Darrick J. Wong
@ 2023-12-04 5:03 ` Christoph Hellwig
0 siblings, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2023-12-04 5:03 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: chandanbabu, hch, leo.lilong, linux-xfs
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/8] xfs: pass the xfs_defer_pending object to iop_recover
2023-12-03 19:01 ` [PATCH 3/8] xfs: pass the xfs_defer_pending object to iop_recover Darrick J. Wong
@ 2023-12-04 5:04 ` Christoph Hellwig
2023-12-04 18:31 ` Darrick J. Wong
0 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2023-12-04 5:04 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: chandanbabu, hch, leo.lilong, linux-xfs
Not sure if I missed it in my first round of review, or if this is new
in this version, but this now generats a warning when asserts are
disabled, given that the lip variable in xlog_recover_process_intents
is only used in asserts. We'll need to remove it and just open code
the dereference in the two asserts that use it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/8] xfs: recreate work items when recovering intent items
2023-12-03 19:01 ` [PATCH 5/8] xfs: recreate work items when recovering intent items Darrick J. Wong
@ 2023-12-04 5:05 ` Christoph Hellwig
0 siblings, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2023-12-04 5:05 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: chandanbabu, hch, leo.lilong, linux-xfs
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/8] xfs: dump the recovered xattri log item if corruption happens
2023-12-03 19:02 ` [PATCH 6/8] xfs: dump the recovered xattri log item if corruption happens Darrick J. Wong
@ 2023-12-04 5:05 ` Christoph Hellwig
0 siblings, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2023-12-04 5:05 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: chandanbabu, hch, leo.lilong, linux-xfs
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/8] xfs: use xfs_defer_finish_one to finish recovered work items
2023-12-03 19:02 ` [PATCH 7/8] xfs: use xfs_defer_finish_one to finish recovered work items Darrick J. Wong
@ 2023-12-04 5:06 ` Christoph Hellwig
0 siblings, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2023-12-04 5:06 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: chandanbabu, hch, leo.lilong, linux-xfs
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/8] xfs: pass the xfs_defer_pending object to iop_recover
2023-12-04 5:04 ` Christoph Hellwig
@ 2023-12-04 18:31 ` Darrick J. Wong
2023-12-04 19:36 ` Christoph Hellwig
0 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-04 18:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: chandanbabu, leo.lilong, linux-xfs
On Mon, Dec 04, 2023 at 06:04:26AM +0100, Christoph Hellwig wrote:
> Not sure if I missed it in my first round of review, or if this is new
> in this version, but this now generats a warning when asserts are
> disabled, given that the lip variable in xlog_recover_process_intents
> is only used in asserts. We'll need to remove it and just open code
> the dereference in the two asserts that use it.
Yeah, that's the last patch, and I forgot to address that warning
because the kbuild robot only sends email to my Outlook now. Clearly
I'm not doing CONFIG_XFS_DEBUG=n builds on my development box...
--D
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2.1 8/8] xfs: move ->iop_recover to xfs_defer_op_type
2023-12-03 19:02 ` [PATCH 8/8] xfs: move ->iop_recover to xfs_defer_op_type Darrick J. Wong
@ 2023-12-04 19:05 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-04 19:05 UTC (permalink / raw)
To: chandanbabu, hch, leo.lilong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Finish off the series by moving the intent item recovery function
pointer to the xfs_defer_op_type struct, since this is really a deferred
work function now.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
v2.1: fix warnings
---
fs/xfs/libxfs/xfs_defer.c | 17 +++++++++++++++
fs/xfs/libxfs/xfs_defer.h | 4 ++++
fs/xfs/libxfs/xfs_log_recover.h | 2 ++
fs/xfs/xfs_attr_item.c | 4 ++--
fs/xfs/xfs_bmap_item.c | 22 ++++++++++----------
fs/xfs/xfs_extfree_item.c | 43 ++++++++++++++++++++-------------------
fs/xfs/xfs_log_recover.c | 19 ++++++-----------
fs/xfs/xfs_refcount_item.c | 24 +++++++++++-----------
fs/xfs/xfs_rmap_item.c | 24 +++++++++++-----------
fs/xfs/xfs_trans.h | 4 ----
10 files changed, 89 insertions(+), 74 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index eb262ea06122..dd565e4e3daf 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -713,6 +713,23 @@ xfs_defer_cancel_recovery(
xfs_defer_pending_cancel_work(mp, dfp);
}
+/* Replay the deferred work item created from a recovered log intent item. */
+int
+xfs_defer_finish_recovery(
+ struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp,
+ struct list_head *capture_list)
+{
+ const struct xfs_defer_op_type *ops = defer_op_types[dfp->dfp_type];
+ int error;
+
+ error = ops->recover_work(dfp, capture_list);
+ if (error)
+ trace_xlog_intent_recovery_failed(mp, error,
+ ops->recover_work);
+ return error;
+}
+
/*
* Move deferred ops from one transaction to another and reset the source to
* initial state. This is primarily used to carry state forward across
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index c1a648e99174..ef86a7f9b059 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -57,6 +57,8 @@ struct xfs_defer_op_type {
void (*finish_cleanup)(struct xfs_trans *tp,
struct xfs_btree_cur *state, int error);
void (*cancel_item)(struct list_head *item);
+ int (*recover_work)(struct xfs_defer_pending *dfp,
+ struct list_head *capture_list);
unsigned int max_items;
};
@@ -130,6 +132,8 @@ void xfs_defer_start_recovery(struct xfs_log_item *lip,
enum xfs_defer_ops_type dfp_type, struct list_head *r_dfops);
void xfs_defer_cancel_recovery(struct xfs_mount *mp,
struct xfs_defer_pending *dfp);
+int xfs_defer_finish_recovery(struct xfs_mount *mp,
+ struct xfs_defer_pending *dfp, struct list_head *capture_list);
static inline void
xfs_defer_add_item(
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index 52162a17fc5e..c8e5d912895b 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -153,6 +153,8 @@ xlog_recover_resv(const struct xfs_trans_res *r)
return ret;
}
+struct xfs_defer_pending;
+
void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
xfs_lsn_t lsn, unsigned int dfp_type);
int xlog_recover_finish_intent(struct xfs_trans *tp,
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index eaf8a877c2cc..5f7d8e8d87dc 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -604,7 +604,7 @@ xfs_attri_recover_work(
* delete the attr that it describes.
*/
STATIC int
-xfs_attri_item_recover(
+xfs_attr_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -820,6 +820,7 @@ const struct xfs_defer_op_type xfs_attr_defer_type = {
.create_done = xfs_attr_create_done,
.finish_item = xfs_attr_finish_item,
.cancel_item = xfs_attr_cancel_item,
+ .recover_work = xfs_attr_recover_work,
};
/*
@@ -856,7 +857,6 @@ static const struct xfs_item_ops xfs_attri_item_ops = {
.iop_format = xfs_attri_item_format,
.iop_unpin = xfs_attri_item_unpin,
.iop_release = xfs_attri_item_release,
- .iop_recover = xfs_attri_item_recover,
.iop_match = xfs_attri_item_match,
.iop_relog = xfs_attri_item_relog,
};
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 89f2d9e89607..fa305d242f24 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -437,15 +437,6 @@ xfs_bmap_update_cancel_item(
kmem_cache_free(xfs_bmap_intent_cache, bi);
}
-const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
- .max_items = XFS_BUI_MAX_FAST_EXTENTS,
- .create_intent = xfs_bmap_update_create_intent,
- .abort_intent = xfs_bmap_update_abort_intent,
- .create_done = xfs_bmap_update_create_done,
- .finish_item = xfs_bmap_update_finish_item,
- .cancel_item = xfs_bmap_update_cancel_item,
-};
-
/* Is this recovered BUI ok? */
static inline bool
xfs_bui_validate(
@@ -508,7 +499,7 @@ xfs_bui_recover_work(
* We need to update some inode's bmbt.
*/
STATIC int
-xfs_bui_item_recover(
+xfs_bmap_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -587,6 +578,16 @@ xfs_bui_item_recover(
return error;
}
+const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
+ .max_items = XFS_BUI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_bmap_update_create_intent,
+ .abort_intent = xfs_bmap_update_abort_intent,
+ .create_done = xfs_bmap_update_create_done,
+ .finish_item = xfs_bmap_update_finish_item,
+ .cancel_item = xfs_bmap_update_cancel_item,
+ .recover_work = xfs_bmap_recover_work,
+};
+
STATIC bool
xfs_bui_item_match(
struct xfs_log_item *lip,
@@ -627,7 +628,6 @@ static const struct xfs_item_ops xfs_bui_item_ops = {
.iop_format = xfs_bui_item_format,
.iop_unpin = xfs_bui_item_unpin,
.iop_release = xfs_bui_item_release,
- .iop_recover = xfs_bui_item_recover,
.iop_match = xfs_bui_item_match,
.iop_relog = xfs_bui_item_relog,
};
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6a434ade486c..49e96ffd64e0 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -567,15 +567,6 @@ xfs_extent_free_cancel_item(
kmem_cache_free(xfs_extfree_item_cache, xefi);
}
-const struct xfs_defer_op_type xfs_extent_free_defer_type = {
- .max_items = XFS_EFI_MAX_FAST_EXTENTS,
- .create_intent = xfs_extent_free_create_intent,
- .abort_intent = xfs_extent_free_abort_intent,
- .create_done = xfs_extent_free_create_done,
- .finish_item = xfs_extent_free_finish_item,
- .cancel_item = xfs_extent_free_cancel_item,
-};
-
/*
* AGFL blocks are accounted differently in the reserve pools and are not
* inserted into the busy extent list.
@@ -632,16 +623,6 @@ xfs_agfl_free_finish_item(
return error;
}
-/* sub-type with special handling for AGFL deferred frees */
-const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
- .max_items = XFS_EFI_MAX_FAST_EXTENTS,
- .create_intent = xfs_extent_free_create_intent,
- .abort_intent = xfs_extent_free_abort_intent,
- .create_done = xfs_extent_free_create_done,
- .finish_item = xfs_agfl_free_finish_item,
- .cancel_item = xfs_extent_free_cancel_item,
-};
-
/* Is this recovered EFI ok? */
static inline bool
xfs_efi_validate_ext(
@@ -675,7 +656,7 @@ xfs_efi_recover_work(
* the log. We need to free the extents that it describes.
*/
STATIC int
-xfs_efi_item_recover(
+xfs_extent_free_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -724,6 +705,27 @@ xfs_efi_item_recover(
return error;
}
+const struct xfs_defer_op_type xfs_extent_free_defer_type = {
+ .max_items = XFS_EFI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_extent_free_create_intent,
+ .abort_intent = xfs_extent_free_abort_intent,
+ .create_done = xfs_extent_free_create_done,
+ .finish_item = xfs_extent_free_finish_item,
+ .cancel_item = xfs_extent_free_cancel_item,
+ .recover_work = xfs_extent_free_recover_work,
+};
+
+/* sub-type with special handling for AGFL deferred frees */
+const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+ .max_items = XFS_EFI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_extent_free_create_intent,
+ .abort_intent = xfs_extent_free_abort_intent,
+ .create_done = xfs_extent_free_create_done,
+ .finish_item = xfs_agfl_free_finish_item,
+ .cancel_item = xfs_extent_free_cancel_item,
+ .recover_work = xfs_extent_free_recover_work,
+};
+
STATIC bool
xfs_efi_item_match(
struct xfs_log_item *lip,
@@ -766,7 +768,6 @@ static const struct xfs_item_ops xfs_efi_item_ops = {
.iop_format = xfs_efi_item_format,
.iop_unpin = xfs_efi_item_unpin,
.iop_release = xfs_efi_item_release,
- .iop_recover = xfs_efi_item_recover,
.iop_match = xfs_efi_item_match,
.iop_relog = xfs_efi_item_relog,
};
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 6fab490959d4..9d411678254f 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2562,17 +2562,14 @@ xlog_recover_process_intents(
#endif
list_for_each_entry_safe(dfp, n, &log->r_dfops, dfp_list) {
- struct xfs_log_item *lip = dfp->dfp_intent;
- const struct xfs_item_ops *ops = lip->li_ops;
-
- ASSERT(xlog_item_is_intent(lip));
+ ASSERT(xlog_item_is_intent(dfp->dfp_intent));
/*
* We should never see a redo item with a LSN higher than
* the last transaction we found in the log at the start
* of recovery.
*/
- ASSERT(XFS_LSN_CMP(last_lsn, lip->li_lsn) >= 0);
+ ASSERT(XFS_LSN_CMP(last_lsn, dfp->dfp_intent->li_lsn) >= 0);
/*
* NOTE: If your intent processing routine can create more
@@ -2581,15 +2578,13 @@ xlog_recover_process_intents(
* replayed in the wrong order!
*
* The recovery function can free the log item, so we must not
- * access lip after it returns. It must dispose of @dfp if it
- * returns 0.
+ * access dfp->dfp_intent after it returns. It must dispose of
+ * @dfp if it returns 0.
*/
- error = ops->iop_recover(dfp, &capture_list);
- if (error) {
- trace_xlog_intent_recovery_failed(log->l_mp, error,
- ops->iop_recover);
+ error = xfs_defer_finish_recovery(log->l_mp, dfp,
+ &capture_list);
+ if (error)
break;
- }
}
if (error)
goto err;
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index f561ca73c784..48f1a38b272e 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -433,16 +433,6 @@ xfs_refcount_update_cancel_item(
kmem_cache_free(xfs_refcount_intent_cache, ri);
}
-const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
- .max_items = XFS_CUI_MAX_FAST_EXTENTS,
- .create_intent = xfs_refcount_update_create_intent,
- .abort_intent = xfs_refcount_update_abort_intent,
- .create_done = xfs_refcount_update_create_done,
- .finish_item = xfs_refcount_update_finish_item,
- .finish_cleanup = xfs_refcount_finish_one_cleanup,
- .cancel_item = xfs_refcount_update_cancel_item,
-};
-
/* Is this recovered CUI ok? */
static inline bool
xfs_cui_validate_phys(
@@ -491,7 +481,7 @@ xfs_cui_recover_work(
* We need to update the refcountbt.
*/
STATIC int
-xfs_cui_item_recover(
+xfs_refcount_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -553,6 +543,17 @@ xfs_cui_item_recover(
return error;
}
+const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
+ .max_items = XFS_CUI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_refcount_update_create_intent,
+ .abort_intent = xfs_refcount_update_abort_intent,
+ .create_done = xfs_refcount_update_create_done,
+ .finish_item = xfs_refcount_update_finish_item,
+ .finish_cleanup = xfs_refcount_finish_one_cleanup,
+ .cancel_item = xfs_refcount_update_cancel_item,
+ .recover_work = xfs_refcount_recover_work,
+};
+
STATIC bool
xfs_cui_item_match(
struct xfs_log_item *lip,
@@ -593,7 +594,6 @@ static const struct xfs_item_ops xfs_cui_item_ops = {
.iop_format = xfs_cui_item_format,
.iop_unpin = xfs_cui_item_unpin,
.iop_release = xfs_cui_item_release,
- .iop_recover = xfs_cui_item_recover,
.iop_match = xfs_cui_item_match,
.iop_relog = xfs_cui_item_relog,
};
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 23e736179894..23684bc2ab85 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -452,16 +452,6 @@ xfs_rmap_update_cancel_item(
kmem_cache_free(xfs_rmap_intent_cache, ri);
}
-const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
- .max_items = XFS_RUI_MAX_FAST_EXTENTS,
- .create_intent = xfs_rmap_update_create_intent,
- .abort_intent = xfs_rmap_update_abort_intent,
- .create_done = xfs_rmap_update_create_done,
- .finish_item = xfs_rmap_update_finish_item,
- .finish_cleanup = xfs_rmap_finish_one_cleanup,
- .cancel_item = xfs_rmap_update_cancel_item,
-};
-
/* Is this recovered RUI ok? */
static inline bool
xfs_rui_validate_map(
@@ -556,7 +546,7 @@ xfs_rui_recover_work(
* We need to update the rmapbt.
*/
STATIC int
-xfs_rui_item_recover(
+xfs_rmap_recover_work(
struct xfs_defer_pending *dfp,
struct list_head *capture_list)
{
@@ -606,6 +596,17 @@ xfs_rui_item_recover(
return error;
}
+const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
+ .max_items = XFS_RUI_MAX_FAST_EXTENTS,
+ .create_intent = xfs_rmap_update_create_intent,
+ .abort_intent = xfs_rmap_update_abort_intent,
+ .create_done = xfs_rmap_update_create_done,
+ .finish_item = xfs_rmap_update_finish_item,
+ .finish_cleanup = xfs_rmap_finish_one_cleanup,
+ .cancel_item = xfs_rmap_update_cancel_item,
+ .recover_work = xfs_rmap_recover_work,
+};
+
STATIC bool
xfs_rui_item_match(
struct xfs_log_item *lip,
@@ -646,7 +647,6 @@ static const struct xfs_item_ops xfs_rui_item_ops = {
.iop_format = xfs_rui_item_format,
.iop_unpin = xfs_rui_item_unpin,
.iop_release = xfs_rui_item_release,
- .iop_recover = xfs_rui_item_recover,
.iop_match = xfs_rui_item_match,
.iop_relog = xfs_rui_item_relog,
};
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 4e38357237c3..5fb018ad9fc0 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -66,8 +66,6 @@ struct xfs_log_item {
{ (1u << XFS_LI_DIRTY), "DIRTY" }, \
{ (1u << XFS_LI_WHITEOUT), "WHITEOUT" }
-struct xfs_defer_pending;
-
struct xfs_item_ops {
unsigned flags;
void (*iop_size)(struct xfs_log_item *, int *, int *);
@@ -80,8 +78,6 @@ struct xfs_item_ops {
xfs_lsn_t (*iop_committed)(struct xfs_log_item *, xfs_lsn_t);
uint (*iop_push)(struct xfs_log_item *, struct list_head *);
void (*iop_release)(struct xfs_log_item *);
- int (*iop_recover)(struct xfs_defer_pending *dfp,
- struct list_head *capture_list);
bool (*iop_match)(struct xfs_log_item *item, uint64_t id);
struct xfs_log_item *(*iop_relog)(struct xfs_log_item *intent,
struct xfs_trans *tp);
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/8] xfs: pass the xfs_defer_pending object to iop_recover
2023-12-04 18:31 ` Darrick J. Wong
@ 2023-12-04 19:36 ` Christoph Hellwig
0 siblings, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2023-12-04 19:36 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, chandanbabu, leo.lilong, linux-xfs
On Mon, Dec 04, 2023 at 10:31:53AM -0800, Darrick J. Wong wrote:
> On Mon, Dec 04, 2023 at 06:04:26AM +0100, Christoph Hellwig wrote:
> > Not sure if I missed it in my first round of review, or if this is new
> > in this version, but this now generats a warning when asserts are
> > disabled, given that the lip variable in xlog_recover_process_intents
> > is only used in asserts. We'll need to remove it and just open code
> > the dereference in the two asserts that use it.
>
> Yeah, that's the last patch, and I forgot to address that warning
> because the kbuild robot only sends email to my Outlook now. Clearly
> I'm not doing CONFIG_XFS_DEBUG=n builds on my development box...
You're getting email quickly from the bot then, for me these kinds of
reports usually show up days later..
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/8] xfs: don't leak recovered attri intent items
2023-12-03 19:00 ` [PATCH 1/8] xfs: don't leak recovered attri intent items Darrick J. Wong
@ 2023-12-05 4:31 ` Christoph Hellwig
2023-12-05 5:19 ` Darrick J. Wong
0 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2023-12-05 4:31 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: chandanbabu, hch, leo.lilong, linux-xfs
Based on my now existent but still vague understanding of the logged
attr code:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/8] xfs: don't leak recovered attri intent items
2023-12-05 4:31 ` Christoph Hellwig
@ 2023-12-05 5:19 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-05 5:19 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: chandanbabu, leo.lilong, linux-xfs
On Tue, Dec 05, 2023 at 05:31:43AM +0100, Christoph Hellwig wrote:
> Based on my now existent but still vague understanding of the logged
> attr code:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Yay! :)
--D
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 7/8] xfs: use xfs_defer_finish_one to finish recovered work items
2023-12-07 2:22 [PATCHSET v3 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
@ 2023-12-07 2:24 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2023-12-07 2:24 UTC (permalink / raw)
To: chandanbabu, leo.lilong, hch, djwong; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Get rid of the open-coded calls to xfs_defer_finish_one. This also
means that the recovery transaction takes care of cleaning up the dfp,
and we have solved (I hope) all the ownership issues in recovery.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_defer.c | 2 +
fs/xfs/libxfs/xfs_defer.h | 1 +
fs/xfs/libxfs/xfs_log_recover.h | 2 +
fs/xfs/xfs_attr_item.c | 20 +------------
fs/xfs/xfs_bmap_item.c | 24 ++++-----------
fs/xfs/xfs_extfree_item.c | 45 +++++------------------------
fs/xfs/xfs_log_recover.c | 22 +++++++++-----
fs/xfs/xfs_refcount_item.c | 61 +++++----------------------------------
fs/xfs/xfs_rmap_item.c | 29 +++++--------------
9 files changed, 49 insertions(+), 157 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 8fb523e4f669..eb262ea06122 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -484,7 +484,7 @@ xfs_defer_relog(
* Log an intent-done item for the first pending intent, and finish the work
* items.
*/
-static int
+int
xfs_defer_finish_one(
struct xfs_trans *tp,
struct xfs_defer_pending *dfp)
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index bef5823f61fb..c1a648e99174 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -41,6 +41,7 @@ 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);
int xfs_defer_finish(struct xfs_trans **tp);
+int xfs_defer_finish_one(struct xfs_trans *tp, struct xfs_defer_pending *dfp);
void xfs_defer_cancel(struct xfs_trans *);
void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index 13583df9f239..52162a17fc5e 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -155,7 +155,7 @@ xlog_recover_resv(const struct xfs_trans_res *r)
void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
xfs_lsn_t lsn, unsigned int dfp_type);
-void xlog_recover_transfer_intent(struct xfs_trans *tp,
+int xlog_recover_finish_intent(struct xfs_trans *tp,
struct xfs_defer_pending *dfp);
#endif /* __XFS_LOG_RECOVER_H__ */
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index c7c308d2f897..eaf8a877c2cc 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -620,7 +620,6 @@ xfs_attri_item_recover(
struct xfs_attri_log_nameval *nv = attrip->attri_nameval;
int error;
int total;
- struct xfs_attrd_log_item *done_item = NULL;
/*
* First check the validity of the attr described by the ATTRI. If any
@@ -645,27 +644,10 @@ xfs_attri_item_recover(
return error;
args->trans = tp;
- done_item = xfs_trans_get_attrd(tp, attrip);
- xlog_recover_transfer_intent(tp, dfp);
-
xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
- error = xfs_xattri_finish_update(attr, done_item);
- if (error == -EAGAIN) {
- /*
- * There's more work to do, so add the intent item to this
- * transaction so that we can continue it later.
- */
- xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_ATTR, &attr->xattri_list);
- error = xfs_defer_ops_capture_and_commit(tp, capture_list);
- if (error)
- goto out_unlock;
-
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
- xfs_irele(ip);
- return 0;
- }
+ error = xlog_recover_finish_intent(tp, dfp);
if (error == -EFSCORRUPTED)
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
&attrip->attri_format,
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index b65999bf0ea3..89f2d9e89607 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -497,6 +497,7 @@ xfs_bui_recover_work(
bi->bi_bmap.br_blockcount = map->me_len;
bi->bi_bmap.br_state = (map->me_flags & XFS_BMAP_EXTENT_UNWRITTEN) ?
XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
+ xfs_bmap_update_get_group(mp, bi);
xfs_defer_add_item(dfp, &bi->bi_list);
return bi;
@@ -518,8 +519,7 @@ xfs_bui_item_recover(
struct xfs_inode *ip = NULL;
struct xfs_mount *mp = lip->li_log->l_mp;
struct xfs_map_extent *map;
- struct xfs_bud_log_item *budp;
- struct xfs_bmap_intent *fake;
+ struct xfs_bmap_intent *work;
int iext_delta;
int error = 0;
@@ -530,7 +530,7 @@ xfs_bui_item_recover(
}
map = &buip->bui_format.bui_extents[0];
- fake = xfs_bui_recover_work(mp, dfp, map);
+ work = xfs_bui_recover_work(mp, dfp, map);
error = xlog_recover_iget(mp, map->me_owner, &ip);
if (error)
@@ -543,39 +543,29 @@ xfs_bui_item_recover(
if (error)
goto err_rele;
- budp = xfs_trans_get_bud(tp, buip);
- xlog_recover_transfer_intent(tp, dfp);
-
xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
- if (fake->bi_type == XFS_BMAP_MAP)
+ if (work->bi_type == XFS_BMAP_MAP)
iext_delta = XFS_IEXT_ADD_NOSPLIT_CNT;
else
iext_delta = XFS_IEXT_PUNCH_HOLE_CNT;
- error = xfs_iext_count_may_overflow(ip, fake->bi_whichfork, iext_delta);
+ error = xfs_iext_count_may_overflow(ip, work->bi_whichfork, iext_delta);
if (error == -EFBIG)
error = xfs_iext_count_upgrade(tp, ip, iext_delta);
if (error)
goto err_cancel;
- fake->bi_owner = ip;
+ work->bi_owner = ip;
- xfs_bmap_update_get_group(mp, fake);
- error = xfs_trans_log_finish_bmap_update(tp, budp, fake);
+ error = xlog_recover_finish_intent(tp, dfp);
if (error == -EFSCORRUPTED)
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
&buip->bui_format, sizeof(buip->bui_format));
- xfs_bmap_update_put_group(fake);
if (error)
goto err_cancel;
- if (fake->bi_bmap.br_blockcount > 0) {
- ASSERT(fake->bi_type == XFS_BMAP_UNMAP);
- xfs_bmap_unmap_extent(tp, ip, &fake->bi_bmap);
- }
-
/*
* Commit transaction, which frees the transaction and saves the inode
* for later replay activities.
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 41108a0b60c9..6a434ade486c 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -665,6 +665,7 @@ xfs_efi_recover_work(
xefi->xefi_blockcount = extp->ext_len;
xefi->xefi_agresv = XFS_AG_RESV_NONE;
xefi->xefi_owner = XFS_RMAP_OWN_UNKNOWN;
+ xfs_extent_free_get_group(mp, xefi);
xfs_defer_add_item(dfp, &xefi->xefi_list);
}
@@ -682,12 +683,9 @@ xfs_efi_item_recover(
struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_efi_log_item *efip = EFI_ITEM(lip);
struct xfs_mount *mp = lip->li_log->l_mp;
- struct xfs_efd_log_item *efdp;
struct xfs_trans *tp;
- struct xfs_extent_free_item *fake;
int i;
int error = 0;
- bool requeue_only = false;
/*
* First check the validity of the extents described by the
@@ -711,40 +709,13 @@ xfs_efi_item_recover(
if (error)
return error;
- efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
- xlog_recover_transfer_intent(tp, dfp);
-
- list_for_each_entry(fake, &dfp->dfp_work, xefi_list) {
- if (!requeue_only) {
- xfs_extent_free_get_group(mp, fake);
- error = xfs_trans_free_extent(tp, efdp, fake);
- xfs_extent_free_put_group(fake);
- }
-
- /*
- * If we can't free the extent without potentially deadlocking,
- * requeue the rest of the extents to a new so that they get
- * run again later with a new transaction context.
- */
- if (error == -EAGAIN || requeue_only) {
- error = xfs_free_extent_later(tp, fake->xefi_startblock,
- fake->xefi_blockcount,
- &XFS_RMAP_OINFO_ANY_OWNER,
- fake->xefi_agresv);
- if (!error) {
- requeue_only = true;
- continue;
- }
- }
-
- if (error == -EFSCORRUPTED)
- XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- &efip->efi_format,
- sizeof(efip->efi_format));
- if (error)
- goto abort_error;
-
- }
+ error = xlog_recover_finish_intent(tp, dfp);
+ if (error == -EFSCORRUPTED)
+ XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+ &efip->efi_format,
+ sizeof(efip->efi_format));
+ if (error)
+ goto abort_error;
return xfs_defer_ops_capture_and_commit(tp, capture_list);
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index cc14cd1c2282..6fab490959d4 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2581,7 +2581,8 @@ xlog_recover_process_intents(
* replayed in the wrong order!
*
* The recovery function can free the log item, so we must not
- * access lip after it returns.
+ * access lip after it returns. It must dispose of @dfp if it
+ * returns 0.
*/
error = ops->iop_recover(dfp, &capture_list);
if (error) {
@@ -2589,8 +2590,6 @@ xlog_recover_process_intents(
ops->iop_recover);
break;
}
-
- xfs_defer_cancel_recovery(log->l_mp, dfp);
}
if (error)
goto err;
@@ -2624,15 +2623,22 @@ xlog_recover_cancel_intents(
}
/*
- * Transfer ownership of the recovered log intent item to the recovery
- * transaction.
+ * Transfer ownership of the recovered pending work to the recovery transaction
+ * and try to finish the work. If there is more work to be done, the dfp will
+ * remain attached to the transaction. If not, the dfp is freed.
*/
-void
-xlog_recover_transfer_intent(
+int
+xlog_recover_finish_intent(
struct xfs_trans *tp,
struct xfs_defer_pending *dfp)
{
- dfp->dfp_intent = NULL;
+ int error;
+
+ list_move(&dfp->dfp_list, &tp->t_dfops);
+ error = xfs_defer_finish_one(tp, dfp);
+ if (error == -EAGAIN)
+ return 0;
+ return error;
}
/*
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 4ffc34e6f0a0..f561ca73c784 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -481,6 +481,7 @@ xfs_cui_recover_work(
ri->ri_type = pmap->pe_flags & XFS_REFCOUNT_EXTENT_TYPE_MASK;
ri->ri_startblock = pmap->pe_startblock;
ri->ri_blockcount = pmap->pe_len;
+ xfs_refcount_update_get_group(mp, ri);
xfs_defer_add_item(dfp, &ri->ri_list);
}
@@ -497,12 +498,8 @@ xfs_cui_item_recover(
struct xfs_trans_res resv;
struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_cui_log_item *cuip = CUI_ITEM(lip);
- struct xfs_cud_log_item *cudp;
struct xfs_trans *tp;
- struct xfs_btree_cur *rcur = NULL;
struct xfs_mount *mp = lip->li_log->l_mp;
- struct xfs_refcount_intent *fake;
- bool requeue_only = false;
int i;
int error = 0;
@@ -541,59 +538,17 @@ xfs_cui_item_recover(
if (error)
return error;
- cudp = xfs_trans_get_cud(tp, cuip);
- xlog_recover_transfer_intent(tp, dfp);
+ error = xlog_recover_finish_intent(tp, dfp);
+ if (error == -EFSCORRUPTED)
+ XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+ &cuip->cui_format,
+ sizeof(cuip->cui_format));
+ if (error)
+ goto abort_error;
- list_for_each_entry(fake, &dfp->dfp_work, ri_list) {
- if (!requeue_only) {
- xfs_refcount_update_get_group(mp, fake);
- error = xfs_trans_log_finish_refcount_update(tp, cudp,
- fake, &rcur);
- xfs_refcount_update_put_group(fake);
- }
- if (error == -EFSCORRUPTED)
- XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- &cuip->cui_format,
- sizeof(cuip->cui_format));
- if (error)
- goto abort_error;
-
- /* Requeue what we didn't finish. */
- if (fake->ri_blockcount > 0) {
- struct xfs_bmbt_irec irec = {
- .br_startblock = fake->ri_startblock,
- .br_blockcount = fake->ri_blockcount,
- };
-
- switch (fake->ri_type) {
- case XFS_REFCOUNT_INCREASE:
- xfs_refcount_increase_extent(tp, &irec);
- break;
- case XFS_REFCOUNT_DECREASE:
- xfs_refcount_decrease_extent(tp, &irec);
- break;
- case XFS_REFCOUNT_ALLOC_COW:
- xfs_refcount_alloc_cow_extent(tp,
- irec.br_startblock,
- irec.br_blockcount);
- break;
- case XFS_REFCOUNT_FREE_COW:
- xfs_refcount_free_cow_extent(tp,
- irec.br_startblock,
- irec.br_blockcount);
- break;
- default:
- ASSERT(0);
- }
- requeue_only = true;
- }
- }
-
- xfs_refcount_finish_one_cleanup(tp, rcur, error);
return xfs_defer_ops_capture_and_commit(tp, capture_list);
abort_error:
- xfs_refcount_finish_one_cleanup(tp, rcur, error);
xfs_trans_cancel(tp);
return error;
}
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 9fb3ae4bfd59..23e736179894 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -546,6 +546,7 @@ xfs_rui_recover_work(
ri->ri_bmap.br_blockcount = map->me_len;
ri->ri_bmap.br_state = (map->me_flags & XFS_RMAP_EXTENT_UNWRITTEN) ?
XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
+ xfs_rmap_update_get_group(mp, ri);
xfs_defer_add_item(dfp, &ri->ri_list);
}
@@ -562,11 +563,8 @@ xfs_rui_item_recover(
struct xfs_trans_res resv;
struct xfs_log_item *lip = dfp->dfp_intent;
struct xfs_rui_log_item *ruip = RUI_ITEM(lip);
- struct xfs_rud_log_item *rudp;
struct xfs_trans *tp;
- struct xfs_btree_cur *rcur = NULL;
struct xfs_mount *mp = lip->li_log->l_mp;
- struct xfs_rmap_intent *fake;
int i;
int error = 0;
@@ -593,28 +591,17 @@ xfs_rui_item_recover(
if (error)
return error;
- rudp = xfs_trans_get_rud(tp, ruip);
- xlog_recover_transfer_intent(tp, dfp);
+ error = xlog_recover_finish_intent(tp, dfp);
+ if (error == -EFSCORRUPTED)
+ XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+ &ruip->rui_format,
+ sizeof(ruip->rui_format));
+ if (error)
+ goto abort_error;
- list_for_each_entry(fake, &dfp->dfp_work, ri_list) {
- xfs_rmap_update_get_group(mp, fake);
- error = xfs_trans_log_finish_rmap_update(tp, rudp, fake,
- &rcur);
- if (error == -EFSCORRUPTED)
- XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
- &ruip->rui_format,
- sizeof(ruip->rui_format));
- xfs_rmap_update_put_group(fake);
- if (error)
- goto abort_error;
-
- }
-
- xfs_rmap_finish_one_cleanup(tp, rcur, error);
return xfs_defer_ops_capture_and_commit(tp, capture_list);
abort_error:
- xfs_rmap_finish_one_cleanup(tp, rcur, error);
xfs_trans_cancel(tp);
return error;
}
^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-12-07 2:24 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-03 19:00 [PATCHSET v2 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
2023-12-03 19:00 ` [PATCH 1/8] xfs: don't leak recovered attri intent items Darrick J. Wong
2023-12-05 4:31 ` Christoph Hellwig
2023-12-05 5:19 ` Darrick J. Wong
2023-12-03 19:01 ` [PATCH 2/8] xfs: use xfs_defer_pending objects to recover " Darrick J. Wong
2023-12-04 5:03 ` Christoph Hellwig
2023-12-03 19:01 ` [PATCH 3/8] xfs: pass the xfs_defer_pending object to iop_recover Darrick J. Wong
2023-12-04 5:04 ` Christoph Hellwig
2023-12-04 18:31 ` Darrick J. Wong
2023-12-04 19:36 ` Christoph Hellwig
2023-12-03 19:01 ` [PATCH 4/8] xfs: transfer recovered intent item ownership in ->iop_recover Darrick J. Wong
2023-12-03 19:01 ` [PATCH 5/8] xfs: recreate work items when recovering intent items Darrick J. Wong
2023-12-04 5:05 ` Christoph Hellwig
2023-12-03 19:02 ` [PATCH 6/8] xfs: dump the recovered xattri log item if corruption happens Darrick J. Wong
2023-12-04 5:05 ` Christoph Hellwig
2023-12-03 19:02 ` [PATCH 7/8] xfs: use xfs_defer_finish_one to finish recovered work items Darrick J. Wong
2023-12-04 5:06 ` Christoph Hellwig
2023-12-03 19:02 ` [PATCH 8/8] xfs: move ->iop_recover to xfs_defer_op_type Darrick J. Wong
2023-12-04 19:05 ` [PATCH v2.1 " Darrick J. Wong
-- strict thread matches above, loose matches on Subject: below --
2023-12-07 2:22 [PATCHSET v3 0/8] xfs: log intent item recovery should reconstruct defer work state Darrick J. Wong
2023-12-07 2:24 ` [PATCH 7/8] xfs: use xfs_defer_finish_one to finish recovered work items 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