On Fri, Aug 23, 2019 at 12:56:53PM +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 an ABBA deadlock between the AGI and AGF here.
>
...
>
> In this patch we move the xfs_iunlink_remove() call to
> before acquiring the AGF lock to preserve correct AGI/AGF locking
> order.
>
> Signed-off-by: kaixuxia <kaixuxia@xxxxxxxxxxx>
> ---
> fs/xfs/xfs_inode.c | 85 +++++++++++++++++++++++++++---------------------------
> 1 file changed, 43 insertions(+), 42 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 6467d5e..584b9d1 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3282,9 +3282,10 @@ struct xfs_iunlink {
> spaceres);
>
> /*
> - * Set up the target.
> + * Check for expected errors before we dirty the transaction
> + * so we can return an error without a transaction abort.
> */
> - if (target_ip == NULL) {
> + if (!target_ip) {
Not sure there's really a point to this change now.
> /*
> * If there's no space reservation, check the entry will
> * fit before actually inserting it.
> @@ -3294,6 +3295,46 @@ struct xfs_iunlink {
> if (error)
> goto out_trans_cancel;
> }
> + } else {
> + /*
> + * If target exists and it's a directory, check that whether
> + * it can be destroyed.
> + */
> + if (S_ISDIR(VFS_I(target_ip)->i_mode) &&
> + (!(xfs_dir_isempty(target_ip)) ||
> + (VFS_I(target_ip)->i_nlink > 2))) {
^ This line needs one more space of indent because it's encapsulated by
the opening brace one line up. The braces around xfs_dir_isempty() also
look spurious, FWIW. With those nits fixed, the rest looks good to me:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
Thanks for the patch.
Brian
> + error = -EEXIST;
> + goto out_trans_cancel;
> + }
> + }
> +
> + /*
> + * Directory entry creation below may acquire the AGF. Remove
> + * the whiteout from the unlinked list first to preserve correct
> + * AGI/AGF locking order. This dirties the transaction so failures
> + * after this point will abort and log recovery will clean up the
> + * mess.
> + *
> + * For whiteouts, we need to bump the link count on the whiteout
> + * inode. After this point, we have a real link, clear the tmpfile
> + * state flag from the inode so it doesn't accidentally get misused
> + * in future.
> + */
> + if (wip) {
> + ASSERT(VFS_I(wip)->i_nlink == 0);
> + error = xfs_iunlink_remove(tp, wip);
> + if (error)
> + goto out_trans_cancel;
> +
> + xfs_bumplink(tp, wip);
> + xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
> + VFS_I(wip)->i_state &= ~I_LINKABLE;
> + }
> +
> + /*
> + * Set up the target.
> + */
> + if (target_ip == NULL) {
> /*
> * If target does not exist and the rename crosses
> * directories, adjust the target directory link count
> @@ -3312,22 +3353,6 @@ struct xfs_iunlink {
> }
> } else { /* target_ip != NULL */
> /*
> - * If target exists and it's a directory, check that both
> - * target and source are directories and that target can be
> - * destroyed, or that neither is a directory.
> - */
> - if (S_ISDIR(VFS_I(target_ip)->i_mode)) {
> - /*
> - * Make sure target dir is empty.
> - */
> - if (!(xfs_dir_isempty(target_ip)) ||
> - (VFS_I(target_ip)->i_nlink > 2)) {
> - error = -EEXIST;
> - goto out_trans_cancel;
> - }
> - }
> -
> - /*
> * Link the source inode under the target name.
> * If the source inode is a directory and we are moving
> * it across directories, its ".." entry will be
> @@ -3417,30 +3442,6 @@ struct xfs_iunlink {
> if (error)
> goto out_trans_cancel;
>
> - /*
> - * For whiteouts, we need to bump the link count on the whiteout inode.
> - * This means that failures all the way up to this point leave the inode
> - * on the unlinked list and so cleanup is a simple matter of dropping
> - * the remaining reference to it. If we fail here after bumping the link
> - * count, we're shutting down the filesystem so we'll never see the
> - * intermediate state on disk.
> - */
> - 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;
> - xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
> -
> - /*
> - * Now we have a real link, clear the "I'm a tmpfile" state
> - * flag from the inode so it doesn't accidentally get misused in
> - * future.
> - */
> - VFS_I(wip)->i_state &= ~I_LINKABLE;
> - }
> -
> xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
> if (new_parent)
> --
> 1.8.3.1
>
> --
> kaixuxia
