On 2019/8/29 11:04, Eric Sandeen wrote:
> On 8/28/19 8:27 PM, kaixuxia wrote:
>> ping...
>> Because there isn't this patch in the latest xfs-for-next branch
>> update...
>
> 1) V3 appears to have no changes from V2. Why was V3 sent?
>
The V3 subject has been changed to distinguish from another droplink
deadlock patch that will be sent soon. This V3 patch aim to fix the
rename whiteout deadlock.
> 2) Neither version has a reviewed-by yet, Darrick has questions outstanding
> AFAICT, they may need to be answered prior to review and merge.
Maybe it is mainly about the reproducer that will be put into xfstests,
I am trying to reduce the reproduce time and will send it when done.
>
> -Eric
>
>>
>> On 2019/8/27 10:54, 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.
>>>
>>> Process A:
>>> Call trace:
>>> ? __schedule+0x2bd/0x620
>>> schedule+0x33/0x90
>>> schedule_timeout+0x17d/0x290
>>> __down_common+0xef/0x125
>>> ? xfs_buf_find+0x215/0x6c0 [xfs]
>>> down+0x3b/0x50
>>> xfs_buf_lock+0x34/0xf0 [xfs]
>>> xfs_buf_find+0x215/0x6c0 [xfs]
>>> xfs_buf_get_map+0x37/0x230 [xfs]
>>> xfs_buf_read_map+0x29/0x190 [xfs]
>>> xfs_trans_read_buf_map+0x13d/0x520 [xfs]
>>> xfs_read_agf+0xa6/0x180 [xfs]
>>> ? schedule_timeout+0x17d/0x290
>>> xfs_alloc_read_agf+0x52/0x1f0 [xfs]
>>> xfs_alloc_fix_freelist+0x432/0x590 [xfs]
>>> ? down+0x3b/0x50
>>> ? xfs_buf_lock+0x34/0xf0 [xfs]
>>> ? xfs_buf_find+0x215/0x6c0 [xfs]
>>> xfs_alloc_vextent+0x301/0x6c0 [xfs]
>>> xfs_ialloc_ag_alloc+0x182/0x700 [xfs]
>>> ? _xfs_trans_bjoin+0x72/0xf0 [xfs]
>>> xfs_dialloc+0x116/0x290 [xfs]
>>> xfs_ialloc+0x6d/0x5e0 [xfs]
>>> ? xfs_log_reserve+0x165/0x280 [xfs]
>>> xfs_dir_ialloc+0x8c/0x240 [xfs]
>>> xfs_create+0x35a/0x610 [xfs]
>>> xfs_generic_create+0x1f1/0x2f0 [xfs]
>>> ...
>>>
>>> Process B:
>>> Call trace:
>>> ? __schedule+0x2bd/0x620
>>> ? xfs_bmapi_allocate+0x245/0x380 [xfs]
>>> schedule+0x33/0x90
>>> schedule_timeout+0x17d/0x290
>>> ? xfs_buf_find+0x1fd/0x6c0 [xfs]
>>> __down_common+0xef/0x125
>>> ? xfs_buf_get_map+0x37/0x230 [xfs]
>>> ? xfs_buf_find+0x215/0x6c0 [xfs]
>>> down+0x3b/0x50
>>> xfs_buf_lock+0x34/0xf0 [xfs]
>>> xfs_buf_find+0x215/0x6c0 [xfs]
>>> xfs_buf_get_map+0x37/0x230 [xfs]
>>> xfs_buf_read_map+0x29/0x190 [xfs]
>>> xfs_trans_read_buf_map+0x13d/0x520 [xfs]
>>> xfs_read_agi+0xa8/0x160 [xfs]
>>> xfs_iunlink_remove+0x6f/0x2a0 [xfs]
>>> ? current_time+0x46/0x80
>>> ? xfs_trans_ichgtime+0x39/0xb0 [xfs]
>>> xfs_rename+0x57a/0xae0 [xfs]
>>> xfs_vn_rename+0xe4/0x150 [xfs]
>>> ...
>>>
>>> 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>
>>> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
>>> ---
>>> fs/xfs/xfs_inode.c | 83 +++++++++++++++++++++++++++---------------------------
>>> 1 file changed, 42 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>>> index 6467d5e..8ffd44f 100644
>>> --- a/fs/xfs/xfs_inode.c
>>> +++ b/fs/xfs/xfs_inode.c
>>> @@ -3282,7 +3282,8 @@ 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) {
>>> /*
>>> @@ -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))) {
>>> + 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)
>>>
>>
--
kaixuxia
