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 10:53:05 +0800, Qu Wenruo wrote:
> Add CC to Miao Xie <miaoxie@huawei.com>
> 
> -------- Original Message --------
> Subject: Re: [PATCH] btrfs: Don't call btrfs_start_transaction() on frozen fs to
> avoid deadlock.
> From: Qu Wenruo <quwenruo@cn.fujitsu.com>
> To: dsterba@suse.cz, linux-btrfs@vger.kernel.org, Miao Xie <miaox@cn.fujitsu.com>
> Date: 2015年01月20日 10:51
>>
>> -------- Original Message --------
>> Subject: Re: [PATCH] btrfs: Don't call btrfs_start_transaction() on frozen fs
>> to avoid deadlock.
>> From: David Sterba <dsterba@suse.cz>
>> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> Date: 2015年01月19日 22:06
>>> On Mon, Jan 19, 2015 at 03:42:41PM +0800, Qu Wenruo wrote:
>>>> The fix is to check if the fs is frozen, if the fs is frozen, just
>>>> return and waiting for the next transaction.
>>>>
>>>> --- 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.

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);
>>
> 
> .
>