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.f...@cn.fujitsu.com>
>>> ---
>>> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
signature.asc
Description: OpenPGP digital signature
