| Message ID | 20181023094147.7906-4-suy.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: fixes of file extent in original and lowmem check | expand |
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@cn.fujitsu.com> > --- > 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. 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: >
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@cn.fujitsu.com> >>> --- >>> 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: >>> >> >> > >
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@cn.fujitsu.com> >> --- >> 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. 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: >> > >
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. * Returns 0 nothing happened */ static int repair_extent_item(struct btrfs_root *root, struct btrfs_path *path, u64 bytenr, u64 num_bytes, u64 parent, u64 root_objectid, - u64 owner, u64 offset, int err) + u64 owner, u64 offset, int *err_ret) { struct btrfs_trans_handle *trans; struct btrfs_root *extent_root = root->fs_info->extent_root; struct btrfs_key old_key; - int freed = 0; int ret; + int err = *err_ret; btrfs_item_key_to_cpu(path->nodes[0], &old_key, path->slots[0]); if ((err & (REFERENCER_MISSING | REFERENCER_MISMATCH)) == 0) - return err; + return 0; ret = avoid_extents_overwrite(root->fs_info); if (ret) - return err; + return 0; trans = btrfs_start_transaction(extent_root, 1); if (IS_ERR(trans)) { ret = PTR_ERR(trans); error("fail to start transaction %s", strerror(-ret)); - /* nothing happened */ + err |= FATAL_ERROR; ret = 0; goto out; } @@ -3824,24 +3824,44 @@ static int repair_extent_item(struct btrfs_root *root, struct btrfs_path *path, ret = btrfs_free_extent(trans, root->fs_info->fs_root, bytenr, num_bytes, parent, root_objectid, owner, offset); if (!ret) { - freed = 1; - err &= ~REFERENCER_MISSING; + err &= ~(REFERENCER_MISSING | REFERENCER_MISMATCH); printf("Delete backref in extent [%llu %llu]\n", bytenr, num_bytes); } else { error("fail to delete backref in extent [%llu %llu]", bytenr, num_bytes); + if (ret < 0) + err |= FATAL_ERROR; } btrfs_commit_transaction(trans, extent_root); - /* btrfs_free_extent may delete the extent */ btrfs_release_path(path); ret = btrfs_search_slot(NULL, root, &old_key, path, 0, 0); - if (ret) + if (ret > 0) { + /* odd, there must be one block group before at least*/ + if (path->slots[0] == 0) { + ret = -EUCLEAN; + err |= FATAL_ERROR; + goto out; + } + /* + * btrfs_free_extent() has deleted the extent item, + * let path point to last checked item. + */ + if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) + path->slots[0] = btrfs_header_nritems(path->nodes[0]) - 1; + else + path->slots[0]--; + ret = -ENOENT; - else if (freed) - ret = err; + err = 0; + } else if (ret < 0) { + err |= FATAL_ERROR; + } else { + ret = 1; + } out: + *err_ret = err; return ret; } @@ -3858,7 +3878,6 @@ static int check_extent_item(struct btrfs_fs_info *fs_info, struct btrfs_extent_inline_ref *iref; struct btrfs_extent_data_ref *dref; struct extent_buffer *eb = path->nodes[0]; - unsigned long end; unsigned long ptr; int slot = path->slots[0]; int type; @@ -3876,6 +3895,8 @@ static int check_extent_item(struct btrfs_fs_info *fs_info, struct btrfs_key key; int ret; int err = 0; + int tmp_err; + u32 ptr_offset; btrfs_item_key_to_cpu(eb, &key, slot); if (key.type == BTRFS_EXTENT_ITEM_KEY) { @@ -3921,21 +3942,22 @@ static int check_extent_item(struct btrfs_fs_info *fs_info, /* New METADATA_ITEM */ level = key.offset; } - end = (unsigned long)ei + item_size; + ptr_offset = ptr - (unsigned long)ei; next: /* Reached extent item end normally */ - if (ptr == end) + if (ptr_offset == item_size) goto out; /* Beyond extent item end, wrong item size */ - if (ptr > end) { + if (ptr_offset > item_size) { err |= ITEM_SIZE_MISMATCH; error("extent item at bytenr %llu slot %d has wrong size", eb->start, slot); goto out; } + ptr = (unsigned long)ei + ptr_offset; parent = 0; root_objectid = 0; owner = 0; @@ -3948,52 +3970,60 @@ next: case BTRFS_TREE_BLOCK_REF_KEY: root_objectid = offset; owner = level; - ret = check_tree_block_backref(fs_info, offset, key.objectid, - level); - err |= ret; + tmp_err = check_tree_block_backref(fs_info, offset, + key.objectid, level); break; case BTRFS_SHARED_BLOCK_REF_KEY: parent = offset; - ret = check_shared_block_backref(fs_info, offset, key.objectid, - level); - err |= ret; + tmp_err = check_shared_block_backref(fs_info, offset, + key.objectid, level); break; case BTRFS_EXTENT_DATA_REF_KEY: dref = (struct btrfs_extent_data_ref *)(&iref->offset); root_objectid = btrfs_extent_data_ref_root(eb, dref); owner = btrfs_extent_data_ref_objectid(eb, dref); owner_offset = btrfs_extent_data_ref_offset(eb, dref); - ret = check_extent_data_backref(fs_info, root_objectid, owner, - owner_offset, key.objectid, key.offset, - btrfs_extent_data_ref_count(eb, dref)); - err |= ret; + tmp_err = check_extent_data_backref(fs_info, root_objectid, + owner, owner_offset, key.objectid, key.offset, + btrfs_extent_data_ref_count(eb, dref)); break; case BTRFS_SHARED_DATA_REF_KEY: parent = offset; - ret = check_shared_data_backref(fs_info, offset, key.objectid); - err |= ret; + tmp_err = check_shared_data_backref(fs_info, offset, + key.objectid); break; default: error("extent[%llu %d %llu] has unknown ref type: %d", key.objectid, key.type, key.offset, type); - ret = UNKNOWN_TYPE; - err |= ret; + err |= UNKNOWN_TYPE; + goto out; } - 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) + 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:
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@cn.fujitsu.com> --- check/mode-lowmem.c | 106 ++++++++++++++++++++++++++++---------------- 1 file changed, 68 insertions(+), 38 deletions(-)