Re: [PATCH] btrfs: qgroup: don't commit transaction when we already hold the handle

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2020/12/4 下午3:37, Nikolay Borisov wrote:
>
>
> On 4.12.20 г. 3:24 ч., Qu Wenruo wrote:
>> [BUG]
>> When running the following script, btrfs will trigger an ASSERT():
>>
>>   #/bin/bash
>>   mkfs.btrfs -f $dev
>>   mount $dev $mnt
>>   xfs_io -f -c "pwrite 0 1G" $mnt/file
>>   sync
>>   btrfs quota enable $mnt
>>   btrfs quota rescan -w $mnt
>>
>>   # Manually set the limit below current usage
>>   btrfs qgroup limit 512M $mnt $mnt
>>
>>   # Crash happens
>>   touch $mnt/file
>>
>> The dmesg looks like this:
>>
>>   assertion failed: refcount_read(&trans->use_count) == 1, in fs/btrfs/transaction.c:2022
>>   ------------[ cut here ]------------
>>   kernel BUG at fs/btrfs/ctree.h:3230!
>>   invalid opcode: 0000 [#1] SMP PTI
>>   RIP: 0010:assertfail.constprop.0+0x18/0x1a [btrfs]
>>    btrfs_commit_transaction.cold+0x11/0x5d [btrfs]
>>    try_flush_qgroup+0x67/0x100 [btrfs]
>>    __btrfs_qgroup_reserve_meta+0x3a/0x60 [btrfs]
>>    btrfs_delayed_update_inode+0xaa/0x350 [btrfs]
>>    btrfs_update_inode+0x9d/0x110 [btrfs]
>>    btrfs_dirty_inode+0x5d/0xd0 [btrfs]
>>    touch_atime+0xb5/0x100
>>    iterate_dir+0xf1/0x1b0
>>    __x64_sys_getdents64+0x78/0x110
>>    do_syscall_64+0x33/0x80
>>    entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>   RIP: 0033:0x7fb5afe588db
>>
>> [CAUSE]
>> In try_flush_qgroup(), we assume we don't hold a transaction handle at
>> all.  This is true for data reservation and mostly true for metadata.
>> Since data space reservation always happens before we start a
>> transaction, and for most metadata operation we reserve space in
>> start_transaction().
>>
>> But there is an exception, btrfs_delayed_inode_reserve_metadata().
>> It holds a transaction handle, while still trying to reserve extra
>> metadata space.
>>
>> When we hit EDQUOT inside btrfs_delayed_inode_reserve_metadata(), we
>> will join current transaction and commit, while we still have
>> transaction handle from qgroup code.
>>
>> [FIX]
>> Let's check current->journal before we join the transaction.
>>
>> If current->journal is unset or BTRFS_SEND_TRANS_STUB, it means
>> we are not holding a transaction, thus are able to join and then commit
>> transaction.
>>
>> If current->journal is a valid transaction handle, we avoid committing
>> transaction and just end it
>>
>> This is less effective than committing current transaction, as it won't
>> free metadata reserved space, but we may still free some data space
>> before new data writes.
>>
>> Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1178634
>> Fixes: c53e9653605d ("btrfs: qgroup: try to flush qgroup space when we get -EDQUOT")
>> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> Signed-off-by: David Sterba <dsterba@suse.com>
>
> Wasn't this submitted already? Also are you going to turn the example
> script into a fstest?
>
Sorry, I forgot to cleanup my patches directory. (Facepalm

The fstests is already submitted:
https://patchwork.kernel.org/project/linux-btrfs/patch/20201111113152.136729-1-wqu@suse.com/

Thanks,
Qu