* [PATCH] Return best entry, if it is the first one @ 2016-11-10 15:01 Goldwyn Rodrigues 2016-11-10 15:01 ` [PATCH] btrfs-progs: Fix extents after finding all errors Goldwyn Rodrigues 2016-11-11 12:34 ` [PATCH] Return best entry, if it is the first one David Sterba 0 siblings, 2 replies; 4+ messages in thread From: Goldwyn Rodrigues @ 2016-11-10 15:01 UTC (permalink / raw) To: linux-btrfs; +Cc: Goldwyn Rodrigues The find_most_right_entry() tends to miss on the best entry if it is the first one on the list and there are only two entries in the list. So, we assign both prev and best to entry. To do this, the selection process (rather the rejection) has to be performed earlier to skip on broken==count. Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> --- cmds-check.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cmds-check.c b/cmds-check.c index 368c1c5..779870a 100644 --- a/cmds-check.c +++ b/cmds-check.c @@ -8184,11 +8184,6 @@ static struct extent_entry *find_most_right_entry(struct list_head *entries) struct extent_entry *entry, *best = NULL, *prev = NULL; list_for_each_entry(entry, entries, list) { - if (!prev) { - prev = entry; - continue; - } - /* * If there are as many broken entries as entries then we know * not to trust this particular entry. @@ -8196,6 +8191,12 @@ static struct extent_entry *find_most_right_entry(struct list_head *entries) if (entry->broken == entry->count) continue; + if (!prev) { + best = entry; + prev = entry; + continue; + } + /* * If our current entry == best then we can't be sure our best * is really the best, so we need to keep searching. -- 2.10.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] btrfs-progs: Fix extents after finding all errors 2016-11-10 15:01 [PATCH] Return best entry, if it is the first one Goldwyn Rodrigues @ 2016-11-10 15:01 ` Goldwyn Rodrigues 2016-11-30 15:32 ` David Sterba 2016-11-11 12:34 ` [PATCH] Return best entry, if it is the first one David Sterba 1 sibling, 1 reply; 4+ messages in thread From: Goldwyn Rodrigues @ 2016-11-10 15:01 UTC (permalink / raw) To: linux-btrfs; +Cc: Goldwyn Rodrigues Simplifying the logic of fixing. Calling fixup_extent_ref() after encountering every error causes more error messages after the extent is fixed. In case of multiple errors, this is confusing because the error message is displayed after the fix message and it works on stale data. It is best to show all errors and then fix the extents. Set a variable and call fixup_extent_ref() if it is set. err is not used, so cleared it. Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> --- cmds-check.c | 75 +++++++++++++++++++----------------------------------------- 1 file changed, 24 insertions(+), 51 deletions(-) diff --git a/cmds-check.c b/cmds-check.c index 779870a..8fa0b38 100644 --- a/cmds-check.c +++ b/cmds-check.c @@ -8994,6 +8994,9 @@ out: ret = err; } + if (!ret) + fprintf(stderr, "Repaired extent references for %llu\n", (unsigned long long)rec->start); + btrfs_release_path(&path); return ret; } @@ -9051,7 +9054,11 @@ static int fixup_extent_flags(struct btrfs_fs_info *fs_info, btrfs_set_extent_flags(path.nodes[0], ei, flags); btrfs_mark_buffer_dirty(path.nodes[0]); btrfs_release_path(&path); - return btrfs_commit_transaction(trans, root); + ret = btrfs_commit_transaction(trans, root); + if (!ret) + fprintf(stderr, "Repaired extent flags for %llu\n", (unsigned long long)rec->start); + + return ret; } /* right now we only prune from the extent allocation tree */ @@ -9178,11 +9185,8 @@ static int check_extent_refs(struct btrfs_root *root, { struct extent_record *rec; struct cache_extent *cache; - int err = 0; int ret = 0; - int fixed = 0; int had_dups = 0; - int recorded = 0; if (repair) { /* @@ -9251,9 +9255,8 @@ static int check_extent_refs(struct btrfs_root *root, while(1) { int cur_err = 0; + int fix = 0; - fixed = 0; - recorded = 0; cache = search_cache_extent(extent_cache, 0); if (!cache) break; @@ -9261,7 +9264,6 @@ static int check_extent_refs(struct btrfs_root *root, if (rec->num_duplicates) { fprintf(stderr, "extent item %llu has multiple extent " "items\n", (unsigned long long)rec->start); - err = 1; cur_err = 1; } @@ -9272,57 +9274,33 @@ static int check_extent_refs(struct btrfs_root *root, fprintf(stderr, "extent item %llu, found %llu\n", (unsigned long long)rec->extent_item_refs, (unsigned long long)rec->refs); - ret = record_orphan_data_extents(root->fs_info, rec); - if (ret < 0) + fix = record_orphan_data_extents(root->fs_info, rec); + if (fix < 0) goto repair_abort; - if (ret == 0) { - recorded = 1; - } else { - /* - * we can't use the extent to repair file - * extent, let the fallback method handle it. - */ - if (!fixed && repair) { - ret = fixup_extent_refs( - root->fs_info, - extent_cache, rec); - if (ret) - goto repair_abort; - fixed = 1; - } - } - err = 1; cur_err = 1; } if (all_backpointers_checked(rec, 1)) { fprintf(stderr, "backpointer mismatch on [%llu %llu]\n", (unsigned long long)rec->start, (unsigned long long)rec->nr); - - if (!fixed && !recorded && repair) { - ret = fixup_extent_refs(root->fs_info, - extent_cache, rec); - if (ret) - goto repair_abort; - fixed = 1; - } + fix = 1; cur_err = 1; - err = 1; } if (!rec->owner_ref_checked) { fprintf(stderr, "owner ref check failed [%llu %llu]\n", (unsigned long long)rec->start, (unsigned long long)rec->nr); - if (!fixed && !recorded && repair) { - ret = fixup_extent_refs(root->fs_info, - extent_cache, rec); - if (ret) - goto repair_abort; - fixed = 1; - } - err = 1; + fix = 1; cur_err = 1; } + + if (repair && fix) { + ret = fixup_extent_refs(root->fs_info, extent_cache, rec); + if (ret) + goto repair_abort; + } + + if (rec->bad_full_backref) { fprintf(stderr, "bad full backref, on [%llu]\n", (unsigned long long)rec->start); @@ -9330,9 +9308,8 @@ static int check_extent_refs(struct btrfs_root *root, ret = fixup_extent_flags(root->fs_info, rec); if (ret) goto repair_abort; - fixed = 1; + fix = 1; } - err = 1; cur_err = 1; } /* @@ -9344,7 +9321,6 @@ static int check_extent_refs(struct btrfs_root *root, fprintf(stderr, "bad metadata [%llu, %llu) crossing stripe boundary\n", rec->start, rec->start + rec->max_size); - err = 1; cur_err = 1; } @@ -9352,13 +9328,12 @@ static int check_extent_refs(struct btrfs_root *root, fprintf(stderr, "bad extent [%llu, %llu), type mismatch with chunk\n", rec->start, rec->start + rec->max_size); - err = 1; cur_err = 1; } remove_cache_extent(extent_cache, cache); free_all_extent_backrefs(rec); - if (!init_extent_tree && repair && (!cur_err || fixed)) + if (!init_extent_tree && repair && (!cur_err || fix)) clear_extent_dirty(root->fs_info->excluded_extents, rec->start, rec->start + rec->max_size - 1, @@ -9385,11 +9360,9 @@ repair_abort: if (ret) goto repair_abort; } - if (err) - fprintf(stderr, "repaired damaged extent references\n"); return ret; } - return err; + return 0; } u64 calc_stripe_length(u64 type, u64 length, int num_stripes) -- 2.10.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs-progs: Fix extents after finding all errors 2016-11-10 15:01 ` [PATCH] btrfs-progs: Fix extents after finding all errors Goldwyn Rodrigues @ 2016-11-30 15:32 ` David Sterba 0 siblings, 0 replies; 4+ messages in thread From: David Sterba @ 2016-11-30 15:32 UTC (permalink / raw) To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues On Thu, Nov 10, 2016 at 09:01:47AM -0600, Goldwyn Rodrigues wrote: > Simplifying the logic of fixing. > > Calling fixup_extent_ref() after encountering every error causes > more error messages after the extent is fixed. In case of multiple errors, > this is confusing because the error message is displayed after the fix > message and it works on stale data. It is best to show all errors and > then fix the extents. > > Set a variable and call fixup_extent_ref() if it is set. err is not used, > so cleared it. Sounds ok, more comments below. > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > --- > cmds-check.c | 75 +++++++++++++++++++----------------------------------------- > 1 file changed, 24 insertions(+), 51 deletions(-) > > diff --git a/cmds-check.c b/cmds-check.c > index 779870a..8fa0b38 100644 > --- a/cmds-check.c > +++ b/cmds-check.c > @@ -8994,6 +8994,9 @@ out: > ret = err; > } > > + if (!ret) > + fprintf(stderr, "Repaired extent references for %llu\n", (unsigned long long)rec->start); Line too long, please stick to ~80 chars, here it's easy to break line after string. > + > btrfs_release_path(&path); > return ret; > } > @@ -9051,7 +9054,11 @@ static int fixup_extent_flags(struct btrfs_fs_info *fs_info, > btrfs_set_extent_flags(path.nodes[0], ei, flags); > btrfs_mark_buffer_dirty(path.nodes[0]); > btrfs_release_path(&path); > - return btrfs_commit_transaction(trans, root); > + ret = btrfs_commit_transaction(trans, root); > + if (!ret) > + fprintf(stderr, "Repaired extent flags for %llu\n", (unsigned long long)rec->start); > + > + return ret; > } > > /* right now we only prune from the extent allocation tree */ > @@ -9178,11 +9185,8 @@ static int check_extent_refs(struct btrfs_root *root, > { > struct extent_record *rec; > struct cache_extent *cache; > - int err = 0; > int ret = 0; > - int fixed = 0; > int had_dups = 0; > - int recorded = 0; > > if (repair) { > /* > @@ -9251,9 +9255,8 @@ static int check_extent_refs(struct btrfs_root *root, > > while(1) { > int cur_err = 0; > + int fix = 0; > > - fixed = 0; > - recorded = 0; > cache = search_cache_extent(extent_cache, 0); > if (!cache) > break; > @@ -9261,7 +9264,6 @@ static int check_extent_refs(struct btrfs_root *root, > if (rec->num_duplicates) { > fprintf(stderr, "extent item %llu has multiple extent " > "items\n", (unsigned long long)rec->start); > - err = 1; > cur_err = 1; > } > > @@ -9272,57 +9274,33 @@ static int check_extent_refs(struct btrfs_root *root, > fprintf(stderr, "extent item %llu, found %llu\n", > (unsigned long long)rec->extent_item_refs, > (unsigned long long)rec->refs); > - ret = record_orphan_data_extents(root->fs_info, rec); > - if (ret < 0) > + fix = record_orphan_data_extents(root->fs_info, rec); > + if (fix < 0) > goto repair_abort; I think ret has to be set to fix here as well (in some way, eg. not using fix for a return value), otherwise the repair_abort label will not thake the same code path as before. > - if (ret == 0) { > - recorded = 1; > - } else { > - /* > - * we can't use the extent to repair file > - * extent, let the fallback method handle it. > - */ > - if (!fixed && repair) { > - ret = fixup_extent_refs( > - root->fs_info, > - extent_cache, rec); > - if (ret) > - goto repair_abort; > - fixed = 1; > - } > - } > - err = 1; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Return best entry, if it is the first one 2016-11-10 15:01 [PATCH] Return best entry, if it is the first one Goldwyn Rodrigues 2016-11-10 15:01 ` [PATCH] btrfs-progs: Fix extents after finding all errors Goldwyn Rodrigues @ 2016-11-11 12:34 ` David Sterba 1 sibling, 0 replies; 4+ messages in thread From: David Sterba @ 2016-11-11 12:34 UTC (permalink / raw) To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues On Thu, Nov 10, 2016 at 09:01:46AM -0600, Goldwyn Rodrigues wrote: > The find_most_right_entry() tends to miss on the best entry if it > is the first one on the list and there are only two entries in the list. > So, we assign both prev and best to entry. > > To do this, the selection process (rather the rejection) has to be > performed earlier to skip on broken==count. > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> Applied, and I've put part of the changelog as comment to the code. Minor nit, please don't forget to add the prefix to the subject. I fix that, no problem, but the patches could be delayed if it's not clear that's for progs. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-30 15:32 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-10 15:01 [PATCH] Return best entry, if it is the first one Goldwyn Rodrigues 2016-11-10 15:01 ` [PATCH] btrfs-progs: Fix extents after finding all errors Goldwyn Rodrigues 2016-11-30 15:32 ` David Sterba 2016-11-11 12:34 ` [PATCH] Return best entry, if it is the first one David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).