Re: [PATCH] btrfs: Don't call btrfs_start_transaction() on frozen fs to avoid deadlock.

[Miao Xie <miaoxie@huawei.com>]

On Tue, 20 Jan 2015 11:17:07 +0800, Qu Wenruo wrote:
>>>>>> --- a/fs/btrfs/super.c
>>>>>> +++ b/fs/btrfs/super.c
>>>>>> @@ -1000,6 +1000,14 @@ int btrfs_sync_fs(struct super_block *sb, int wait)
>>>>>>                 */
>>>>>>                if (fs_info->pending_changes == 0)
>>>>>>                    return 0;
>>>>>> +            /*
>>>>>> +             * Test if the fs is frozen, or start_trasaction
>>>>>> +             * will deadlock on itself.
>>>>>> +             */
>>>>>> +            if (__sb_start_write(sb, SB_FREEZE_FS, false))
>>>>>> +                __sb_end_write(sb, SB_FREEZE_FS);
>>>>>> +            else
>>>>>> +                return 0;
>>>>> I'm not sure this is the right fix. We should use either
>>>>> mnt_want_write_file or sb_start_write around the start/commit functions.
>>>>> The fs may be frozen already, but we also have to catch transition to
>>>>> that state, or RO remount.
>>>> But the deadlock between s_umount and frozen level is a larger problem...
>>>>
>>>> Even Miao mentioned that we can start a transaction in btrfs_freeze(), but
>>>> there is still possibility that
>>>> we try to change the feature of the frozen btrfs and do sync, again the
>>>> deadlock will happen.
>>>> Although handling in btrfs_freeze() is also needed, but can't resolve all the
>>>> problem.
>>>>
>>>> IMHO the fix is still needed, or at least as a workaround until we find a real
>>>> root solution for it
>>>> (If nobody want to revert the patchset)
>>>>
>>>> BTW, what about put the pending changes to a workqueue? If we don't start
>>>> transaction under
>>>> s_umount context like sync_fs()
>> I don't like this fix.
>> I think we should deal with those pending changes when we freeze a filesystem.
>> or we break the rule of fs freeze.
> I am afraid handling it in btrfs_freeze() won't help.
> Case like freeze() -> change_feature -> sync() -> unfreeze() will still deadlock
> in sync().

We should not change feature after the fs is freezed.

> Even cleared the pending changes in freeze(), it can still be set through sysfs
> interface even the fs is frozen.
> 
> And in fact, if we put the things like attach/create a transaction into a
> workqueue, we will not break
> the freeze rule.
> Since if the fs is frozen, there is no running transaction and we need to create
> a new one,
> that will call sb_start_intwrite(), which will sleep until the fs is unfreeze.

I read the pending change code just now, and I found the pending change is just
used for changing the mount option now, so I think as a work-around fix we
needn't start a new transaction to handle the pending flags which are set after
the current transaction is committed, because the data on the disk is
integrated.

Thanks
Miao


> 
> Thanks,
> Qu
>>
>> Thanks
>> Miao
>>
>>>> Thanks,
>>>> Qu
>>>>> Also, returning 0 is not right, the ioctl actually skipped the expected
>>>>> work.
>>>>>
>>>>>>                trans = btrfs_start_transaction(root, 0);
>>>>>>            } else {
>>>>>>                return PTR_ERR(trans);
>>> .
>>>
> 
> .
>