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:53:34 +0800, Qu Wenruo wrote:
>>>>>>>>>> +            /*
>>>>>>>>>> +             * 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.
> Pending changes are *not* only mount options. Feature change and label change
> are also pending changes if using sysfs.

My miss, I don't notice feature and label change by sysfs.

But the implementation of feature and label change by sysfs is wrong, we can
not change them without write permission.

> Normal ioctl label changing is not affected.
> 
> For freeze, it's not the same problem since the fs will be unfreeze sooner or
> later and transaction will be initiated.

You can not assume the operations of the users, they might freeze the fs and
then shutdown the machine.

>>
>>> For example, if we change the features/label through sysfs, and then umount
>>> the fs,
>> It is different from pending change.
> No, now features/label changing using sysfs both use pending changes to do the
> commit.
> See BTRFS_PENDING_COMMIT bit.
> So freeze -> change features/label -> sync will still cause the deadlock in the
> same way,
> and you can try it yourself.

As I said above, the implementation of sysfs feature and label change is wrong,
it is better to separate them from the pending mount option change, make the
sysfs feature and label change be done in the context of transaction after
getting the write permission. If so, we needn't do anything special when sync
the fs.

In short, changing the sysfs feature and label change implementation and
removing the unnecessary btrfs_start_transaction in sync_fs can fix the
deadlock.

Thanks
Miao

> 
> Thanks,
> Qu
> 
>> 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
>>> .
>>>
> 
> .
>