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

[Miao Xie <miaoxie@huawei.com>]

On Wed, 21 Jan 2015 11:15:41 +0800, Qu Wenruo wrote:
> 
> -------- 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: Chris Mason <clm@fb.com>, Qu Wenruo <quwenruo@cn.fujitsu.com>
> Date: 2015年01月21日 11:10
>> On Tue, 20 Jan 2015 20:10:56 -0500, Chris Mason wrote:
>>> On Tue, Jan 20, 2015 at 8:09 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>>>> -------- Original Message --------
>>>> Subject: Re: [PATCH] btrfs: Don't call btrfs_start_transaction() on frozen fs
>>>> to avoid deadlock.
>>>> From: Chris Mason <clm@fb.com>
>>>> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>> Date: 2015年01月21日 09:05
>>>>>
>>>>> On Tue, Jan 20, 2015 at 7:58 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>>>>>> -------- 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月21日 01:13
>>>>>>> On Mon, Jan 19, 2015 at 03:42:41PM +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;
>>>>> But what if someone freezes the FS after __sb_end_write() and before
>>>>> btrfs_start_transaction()?   I don't see what keeps new freezers from
>>>>> coming in.
>>>>>
>>>>> -chris
>>>> Either VFS::freeze_super() and VFS::syncfs() will hold the s_umount mutex, so
>>>> freeze will not happen
>>>> during sync.
>>> You're right.  I was worried about the sync ioctl, but the mutex won't be held
>>> there to deadlock against.  We'll be fine.
>> There is another problem which is introduced by pending change. That is we will
>> start and commmit a transaction by changing pending mount option after we set
>> the fs to be R/O.
> Oh, I missed this problem.
>>
>> I think it is better that we don't start a new transaction for pending changes
>> which are set after the transaction is committed, just make them be handled by
>> the next transaction,
> This will cause another problem, nobody can ensure there will be next
> transaction and the change may
> never to written into disk.

First, the pending changes is mount option, that is in-memory data.
Second, the same problem would happen after you freeze fs.

> 
> For example, if we change the features/label through sysfs, and then umount the fs,

It is different from pending change.
If you want to change features/label,  you should get write permission and make
sure the fs is not be freezed because those are on-disk data. So the problem
doesn't exist, or there is a bug.

Thanks
Miao

> since there is no write, there is no running transaction and if we don't start a
> new transaction,
> it won't be flushed to disk.
> 
> Thanks,
> Qu
>> the reason is:
>> - Make the behavior of the fs be consistent(both freezed fs and unfreezed fs)
>> - Data on the disk is right and integrated
>>
>>
>> Thanks
>> Miao
> 
> .
>