On 2019/8/19 23:13, Brian Foster wrote:
On Mon, Aug 19, 2019 at 09:06:39PM +0800, kaixuxia wrote:
When performing rename operation with RENAME_WHITEOUT flag, we will
hold AGF lock to allocate or free extents in manipulating the dirents
firstly, and then doing the xfs_iunlink_remove() call last to hold
AGI lock to modify the tmpfile info, so we the lock order AGI->AGF.
The big problem here is that we have an ordering constraint on AGF
and AGI locking - inode allocation locks the AGI, then can allocate
a new extent for new inodes, locking the AGF after the AGI. Hence
the ordering that is imposed by other parts of the code is AGI before
AGF. So we get the ABBA agi&agf deadlock here.
...
In this patch we move the xfs_iunlink_remove() call to between
xfs_dir_canenter() and xfs_dir_createname(). By doing xfs_iunlink
_remove() firstly, we remove the AGI/AGF lock inversion problem.
Signed-off-by: kaixuxia <kaixuxia@xxxxxxxxxxx>
---
fs/xfs/xfs_inode.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 6467d5e..48691f2 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3294,6 +3294,18 @@ struct xfs_iunlink {
if (error)
goto out_trans_cancel;
}
+
+ /*
+ * Handle the whiteout inode and acquire the AGI lock, so
+ * fix the AGI/AGF lock inversion problem.
+ */
The comment could be a little more specific. For example:
"Directory entry creation may acquire the AGF. Remove the whiteout from
the unlinked list first to preserve correct AGI/AGF locking order."
Right, makes more sense.
+ if (wip) {
+ ASSERT(VFS_I(wip)->i_nlink == 0);
+ error = xfs_iunlink_remove(tp, wip);
+ if (error)
+ goto out_trans_cancel;
+ }
+
/*
* If target does not exist and the rename crosses
* directories, adjust the target directory link count
@@ -3428,9 +3440,11 @@ struct xfs_iunlink {
if (wip) {
ASSERT(VFS_I(wip)->i_nlink == 0);
xfs_bumplink(tp, wip);
- error = xfs_iunlink_remove(tp, wip);
- if (error)
- goto out_trans_cancel;
+ if (target_ip != NULL) {
+ error = xfs_iunlink_remove(tp, wip);
+ if (error)
+ goto out_trans_cancel;
+ }
The comment above this hunk needs to be updated. I'm also not a big fan
of this factoring of doing the removal in the if branch above and then
encoding the else logic down here. It might be cleaner and more
consistent to have a call in each branch of the if/else above.
FWIW, I'm also curious if this could be cleaned up further by pulling
the -ENOSPC/-EEXIST checks out of the earlier branch, following that
with the whiteout removal, and then doing the dir_create/replace. For
example, something like:
/* error checks before we dirty the transaction */
if (!target_ip && !spaceres) {
error = xfs_dir_canenter();
...
} else if (S_ISDIR() && !(empty || nlink > 2))
error = -EEXIST;
...
}
if (wip) {
...
xfs_iunlink_remove();
}
if (!target_ip) {
xfs_dir_create();
...
} else {
xfs_dir_replace();
...
}
... but that may not be any cleaner..? It could also be done as a
followup cleanup patch as well.
Yep, it is cleaner that making the whole check before the transaction
becomes dirty, just return the error code if check failed and
the filesystem is clean.
Dave gave another solution in the other subthread that using
XFS_DIR3_FT_WHT, it's a bit more work for this bug, include
refactoring the xfs_rename() and xfs_lookup(), not sure whether
it's worth the complex changes for this bug.
Brian
xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
/*
--
1.8.3.1
--
kaixuxia
--
kaixuxia
