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

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

-------- Original Message --------
Subject: Re: [PATCH] btrfs: Don't call btrfs_start_transaction() on 
frozen fs to avoid deadlock.
From: Miao Xie <miaoxie@huawei.com>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>, <dsterba@suse.cz>, 
<linux-btrfs@vger.kernel.org>
Date: 2015年01月20日 11:06
> 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.
I am afraid handling it in btrfs_freeze() won't help.
Case like freeze() -> change_feature -> sync() -> unfreeze() will still 
deadlock in sync().

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.

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