On 2019/8/22 14:06, Dave Chinner wrote:
> On Thu, Aug 22, 2019 at 01:45:48PM +0800, kaixuxia wrote:
>> On 2019/8/22 13:01, Dave Chinner wrote:
>>> On Thu, Aug 22, 2019 at 12:33:23PM +0800, kaixuxia wrote:
>>>
>>>> @@ -3419,25 +3431,15 @@ struct xfs_iunlink {
>>>>
>>>> /*
>>>> * For whiteouts, we need to bump the link count on the whiteout inode.
>>>
>>> Shouldn't this line be removed as well?
>>
>> Because the xfs_bumplink() call below will do this.
>
> Oh, yeah, I just assumed that from the "we have a real link" part of
> the new comment :P
>
>>>> - * 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.
>>>> + * The whiteout inode has been removed from the unlinked list and log
>>>> + * recovery will clean up the mess for the failures up to this point.
>>>> + * 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);
>>>> 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;
>>>> }
>>>
>>> Why not move all this up into the same branch that removes the
>>> whiteout from the unlinked list? Why separate this logic as none of
>>> what is left here could cause a failure even if it is run earlier?
>>
>> Yep, it could not cause a failure if we move all this into the same
>> branch that xfs_iunlink_remove() call. We move the xfs_iunlink_remove()
>> first to preserve correct AGI/AGF locking order, and maybe it is better
>> we bump the link count after using the whiteout inode really, such as
>> xfs_dir_replace(...,wip,...) ...
>
> It makes no difference where we bump the link count as long as we do
> it after the xfs_iunlink_remove() call. At that point, any failure
> will result in a shutdown and so it doesn't matter that we've
> already bumped the link count because the shutdown with prevent
> it from reaching the disk...
Yeah, so it can be like this:
/*
* For whiteouts, we need to bump the link count on the whiteout inode.
* The whiteout inode is removed from the unlinked list and log recovery
* will clean up the mess for the failures after this point. 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)
...
xfs_bumplink(tp, wip);
xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
VFS_I(wip)->i_state &= ~I_LINKABLE;
}
Right?
>
> Cheers,
>
> Dave.
>
--
kaixuxia
