On 2018/10/24 上午9:27, Su Yue wrote:
>
>
> On 10/23/18 6:30 PM, Qu Wenruo wrote:
>>
>>
>> On 2018/10/23 下午5:41, Su Yue wrote:
>>> Previously, @err are assigned immediately after check but before
>>> repair.
>>> repair_extent_item()'s return value also confuses the caller. If
>>> error has been repaired and returns 0, check_extent_item() will try
>>> to continue check the nonexistent and cause flase alerts.
>>>
>>> Here make repair_extent_item()'s return codes only represents status
>>> of the extent item, error bits are passed by pointer.
>>> Move the change of @err after repair.
>>>
>>> Signed-off-by: Su Yue <suy.fnst@xxxxxxxxxxxxxx>
>>> ---
>>> check/mode-lowmem.c | 106 ++++++++++++++++++++++++++++----------------
>>> 1 file changed, 68 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
>>> index 76e7be81ceb1..769b3141de8b 100644
>>> --- a/check/mode-lowmem.c
>>> +++ b/check/mode-lowmem.c
>>> @@ -3788,35 +3788,35 @@ out:
>>> /*
>>> * Only delete backref if REFERENCER_MISSING now
>>> *
>>> - * Returns <0 the extent was deleted
>>> - * Returns >0 the backref was deleted but extent still exists,
>>> returned value
>>> - * means error after repair
>>> + * Returns <0 the whole extent item was deleted
>>> + * Returns >0 the backref was deleted but extent still exists,
>>> @err_ret
>>> + * will be set to be error bits after repair.
>>
>> Well, I have to admit the parameter list is really human friendly from
>> the initial design.
>>
>
> As my previous design, callers of repair_xxx functions just take
> care of successes, failures and pathes. If exposing the logic
> to the callers is acceptable, will do it as your suggestions.
Sorry, I miss the "not" word, so the correct line should be:
> Well, I have to admit the parameter list is really mot human friendly
> from the initial design.
Thanks,
Qu
>
> Thanks,
> Su
>
>> In fact, the only real user of @err is just that "if ((err &
>> (REFERENCER_MISSING | REFERENCER_MISMATCH)) == 0)" line.
>>
>> We can in fact get rid of that @err parameter and do that check at the
>> only caller of repair_extent_item().
>>
>>
>> And then redefine the return value.
>>
>> Like "Return < 0 if we failed to repair backref for the extent.
>> Return 0 if we repaired the backref for the extent".
>>
>> [snip]
>>> - if (err && repair) {
>>> + if (tmp_err && repair) {
>>> ret = repair_extent_item(fs_info->extent_root, path,
>>> key.objectid, num_bytes, parent, root_objectid,
>>> - owner, owner_offset, ret);
>>> - if (ret < 0)
>>
>> If follow my above method, here the code can be a little easier:
>> if (tmp_err && repair) {
>> if (!(tmp_err & (REFERENCER_MISSING |
>> REFENCER_MISMATCH)) {
>> err |= tmp_err;
>> goto next;
>> }
>> ret = repair_extent_item(...);
>> if (ret < 0)
>> err |= tmp_err;
>> } else if (tmp_err) {
>> err |= tmp_err;
>> }
>>
>
>> Without the need to passing @err into repair_extent_item() nor modifying
>> @err in that function.
>>
>> Thanks,
>> Qu
>>
>>> + owner, owner_offset, &tmp_err);
>>> + err |= tmp_err;
>>> +
>>> + if (tmp_err & FATAL_ERROR || ret < 0)
>>> goto out;
>>> - if (ret) {
>>> + /*
>>> + * the error has been repaired which means the extent item
>>> + * is still existed with other backrefs, go to check next.
>>> + */> + if (ret > 0) {
>>> + eb = path->nodes[0];
>>> + slot = path->slots[0];
>>> + ei = btrfs_item_ptr(eb, slot, struct btrfs_extent_item);
>>> + item_size = btrfs_item_size_nr(eb, slot);
>>> goto next;
>>> - err = ret;
>>> }
>>> + } else {
>>> + err |= tmp_err;
>>> }
>>> - ptr += btrfs_extent_inline_ref_size(type);
>>> + ptr_offset += btrfs_extent_inline_ref_size(type);
>>> goto next;
>>> out:
>>>
>>
>>
>
>
