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: 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.

For example, if we change the features/label through sysfs, and then 
umount the fs,
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