On 2018年01月27日 10:26, Qu Wenruo wrote: > > > On 2018年01月26日 22:13, Nikolay Borisov wrote: >> Running generic/019 with qgroups on the scratch device enabled is >> almost guaranteed to trigger the BUG_ON in btrfs_free_tree_block. It's >> supposed to trigger only on -ENOMEM, in reality, however, it's possible >> to get -EIO from btrfs_qgroup_trace_extent_post. This function just >> finds the roots of the extent being tracked and sets the qrecord->old_roots >> list. If this operation fails nothing critical happens except the >> quota accounting can be considered wrong. In such case just set the >> INCONSISTENT flag for the quota and be done with it. >> >> Signed-off-by: Nikolay Borisov <nbori...@suse.com> >> --- >> >> Qu, >> >> This fixes the crash for me, however I'm not entirely sure it's really the >> best fix since it's leaking the usage of INCONSISTENT out of qgroup.c. >> Ideally >> I'd like to avoid this. Since you have more experience with qgroups and also >> you >> introduced the extent tracking code in add_delayed_tree_ref how does >> look to you?
And for leaking INCONSISTENT flag out of qgroup scope, it could be
integrated into btrfs_qgroup_trace_extent_post() so we automatically
mark qgroup inconsistent without the help from callers.
Thanks,
Qu
>>
>>
>> fs/btrfs/delayed-ref.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>> index a1a40cf382e3..1e9aa18cc0d8 100644
>> --- a/fs/btrfs/delayed-ref.c
>> +++ b/fs/btrfs/delayed-ref.c
>> @@ -820,8 +820,13 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info
>> *fs_info,
>> num_bytes, parent, ref_root, level, action);
>> spin_unlock(&delayed_refs->lock);
>>
>> - if (qrecord_inserted)
>> - return btrfs_qgroup_trace_extent_post(fs_info, record);
>> + if (qrecord_inserted) {
>> + int ret = btrfs_qgroup_trace_extent_post(fs_info, record);
>
> Since generic/019 is using make_fail_request, it can cause some tree
> reads fail.
>
> And since btrfs_qgroup_trace_extent_post() cause btrfs_find_all_roots()
> to fill @old_roots for new extents, it will definitely read tree blocks,
> and when its request failed, it returns -EIO as expected.
>
> So yes, the qgroup code break the old assumption that
> btrfs_add_delayed_tree|data_ref() could only return -ENOMEM.
>
>
> But compared to delayed-ref, qgroup is not a critical payload (if
> delayed-ref is screwed up, extent tree and possible data extent will be
> corrupted, while qgroup failure only affects qgroup), so it's OK to keep
> the old assumption.
>
>> + if (ret != -ENOMEM)
> Here we don't really need to handle ENOMEM specially.
> Just check ( ret < 0 ) should be enough.
>
>> + fs_info->qgroup_flags |=
>> BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
>
> And extra btrfs_warn() would help.
>
>> + else
>> + return -ENOMEM;
>
> Since btrfs is not a critical payload compared to delayed ref, returning
> 0 should not be a problem as we have already marked qgroup inconsistent.
> (With comment explaining why it's OK to return 0)
>
> Thanks,
> Qu
>
>> + }
>> return 0;
>>
>> free_head_ref:
>>
>
signature.asc
Description: OpenPGP digital signature
