Re: [PATCH V2] xfs: Fix agi&agf ABBA deadlock when performing rename with RENAME_WHITEOUT flag

kaixuxia
2019-08-20T08:53:30Z

On 2019/8/20 16:07, Dave Chinner wrote:
> On Tue, Aug 20, 2019 at 02:45:36PM +0800, kaixuxia wrote:
>> On 2019/8/19 23:13, Brian Foster wrote:
>>> /* 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.
>
> *nod*
>
>> 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.
>
> It's not necessary to fix the bug, but it's somethign we should
> be looking to do because it makes whiteout handling a lot more
> efficient - it's just dirent modifications at that point, no inodes
> are necessary.
>
> This is how I always intended to handle whiteouts - it's just
> another thing on the "we need to fix" list....

Right, it is more efficient because there is no need to store it on disk,
and it will improve performance just like the async deferred operations.
Maybe it is on the roadmap, so I'm not sure whether I should send the V3
patch to address Brian's comments. Maybe we can choose the V3 patch first,
and then the whiteout improvement could be done as the followup patch
in future...

>
> Cheers,
>
> Dave.
>

--
kaixuxia