On 2019/8/23 22:07, Brian Foster wrote:
> 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! I will send the new patch to address them with your
Reviewed-by.
>
> 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
--
kaixuxia
