On 2018年04月24日 14:43, Su Yue wrote:
>
>
> On 04/24/2018 02:17 PM, Qu Wenruo wrote:
>>
>>
>> On 2018年04月24日 13:52, Su Yue wrote:
>>> For an extent item which contains many tree block backrefs, like
>>> ===============================================================
>>> In 020-extent-ref-cases/keyed_block_ref.img
>>>
>>> item 10 key (29470720 METADATA_ITEM 0) itemoff 3450 itemsize 222
>>> refs 23 gen 10 flags TREE_BLOCK
>>> tree block skinny level 0
>>> tree block backref root 278
>>> tree block backref root 277
>>> tree block backref root 276
>>> tree block backref root 275
>>> tree block backref root 274
>>> tree block backref root 273
>>> tree block backref root 272
>>> tree block backref root 271
>>> tree block backref root 270
>>> tree block backref root 269
>>> tree block backref root 268
>>> tree block backref root 267
>>> tree block backref root 266
>>> tree block backref root 265
>>> tree block backref root 264
>>> tree block backref root 263
>>> tree block backref root 262
>>> tree block backref root 261
>>> tree block backref root 260
>>> tree block backref root 259
>>> tree block backref root 258
>>> tree block backref root 257
>>> ===============================================================
>>> In find_parent_nodes(), these refs's parents are 0, then __merge_refs
>>> will merge refs to one ref. It causes only one root to be returned.
>>
>> This is a pretty big problem, and it would cause qgroup verification
>> code to cause false alerts.
>>
>> So a new test case would do great help here.
>>
> Let me think how to construct a test case a while.
> If I remember correctly, the function is called rarely only
> in check/lowmem check.
> Check calls it in a very corner case.
> As you know, extent check in lowmme is buggy which I'm try to fix.
I mean, if the problem is find_parent_nodes() can only return one root,
it would definitely cause problem for qgroup_verify_all() function, then
it should be pretty easy to craft a test image.
Thanks,
Qu
>
>>>
>>> So, if both parents are 0, do not merge refs.
>>>
>>> Lowmem check calls find_parent_nodes frequently to decide whether.
>>> check an extent buffer or not. These bug influences bytes accounting.
>>
>> Although it looks good so far, and would fix the problem you found, but
>
> Fixing another bug in lowmem, found it by accident.
>
>> I strongly recommend to port kernel backref code to btrfs-progs here, as
>> kernel backref code is more tested than btrfs-progs backref code.
>>
> It's a fact. Seen backref.c in btrfs-progs, there are many useless codes
> and confusing logicals.
> I agree that port kernel backref code to btrfs-progs.
>
>> I'm pretty sure we have more bugs in btrfs-progs backref, and I'm not a
>> big fan of playing whac-a-mole here.
>>
>>>
>>> Signed-off-by: Su Yue <suy.fnst@xxxxxxxxxxxxxx>
>>> ---
>>> backref.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/backref.c b/backref.c
>>> index 51553c702187..daadb6299c79 100644
>>> --- a/backref.c
>>> +++ b/backref.c
>>> @@ -507,6 +507,8 @@ static void __merge_refs(struct pref_state
>>> *prefstate, int mode)
>>> } else {
>>> if (ref1->parent !=ef2->parent)
>>> continue;
>>> + if (ref1->parent =0)
>>> + continue;
>>
>> It's better to put it above (ref1->parent !=ef2->parent).
>> As parent =0 means we haven't resolve the parent bytenr yet, so can't
>> be merged nor compared.
>
> Ok, will do it in V2.
>
> Thanks,
> Su
>>
>> Thanks,
>> Qu
>>
>>> }
>>> eie =ef1->inode_list;
>>>
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: OpenPGP digital signature
