| Message ID | 20190403115919.17049-2-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves | expand |
On Wed, Apr 03, 2019 at 07:59:19PM +0800, Qu Wenruo wrote: > Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has > zero item") introduced comprehensive root owner checker. That's 4.7, so we might want to do stable backport to 4.9+. > > However it's pretty expensive tree search to locate the owner root, > especially when it get reused by mandatory read and write time > tree-checker. > > This patch will remove that check, and completely rely on owner based > empty leaf check, which is much faster and still works fine for most > case. > > And since we skip the old root owner check, now write time tree check > can be merged with btrfs_check_leaf_full(). And for that reason the change should be minimal, this patch depends on current development queue so it's not suitable for the backport. I'm thinking what to do here, I want to avoid patches that effectively revert changes that are still in the development branch and haven't been released. This means to rework "btrfs: Do mandatory tree block check before submitting bio", though it's deep in the branch, the scope of changes is only 2 files so this should not cause any conflicts.
On 3.04.19 г. 14:59 ч., Qu Wenruo wrote: > Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has > zero item") introduced comprehensive root owner checker. > > However it's pretty expensive tree search to locate the owner root, > especially when it get reused by mandatory read and write time > tree-checker. > > This patch will remove that check, and completely rely on owner based > empty leaf check, which is much faster and still works fine for most > case. > > And since we skip the old root owner check, now write time tree check > can be merged with btrfs_check_leaf_full(). > > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
On 2019/4/3 下午10:21, David Sterba wrote: > On Wed, Apr 03, 2019 at 07:59:19PM +0800, Qu Wenruo wrote: >> Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has >> zero item") introduced comprehensive root owner checker. > > That's 4.7, so we might want to do stable backport to 4.9+. It's just a performance optimization. I'm not sure if we should backport such things even if we can solve the conflicts. >> >> However it's pretty expensive tree search to locate the owner root, >> especially when it get reused by mandatory read and write time >> tree-checker. >> >> This patch will remove that check, and completely rely on owner based >> empty leaf check, which is much faster and still works fine for most >> case. >> >> And since we skip the old root owner check, now write time tree check >> can be merged with btrfs_check_leaf_full(). > > And for that reason the change should be minimal, this patch depends on > current development queue so it's not suitable for the backport. > > I'm thinking what to do here, I want to avoid patches that effectively > revert changes that are still in the development branch and haven't been > released. OK, I'll resend write time tree checker patchset. > > This means to rework "btrfs: Do mandatory tree block check before > submitting bio", though it's deep in the branch, the scope of changes is > only 2 files so this should not cause any conflicts. BTW, I'm a little interesting in the workflow on your side. In this case, what's the correct way to handle them? Just remove those two related patches from misc-next branch and re-apply the newer version? Or remove all those write time tree checker and later enhancement patches and reapply them all? Either way, I hope to experience the same workflow so I could reduce your workload. Thanks, Qu
On Wed, Apr 03, 2019 at 10:26:05PM +0800, Qu Wenruo wrote: > On 2019/4/3 下午10:21, David Sterba wrote: > > On Wed, Apr 03, 2019 at 07:59:19PM +0800, Qu Wenruo wrote: > >> Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has > >> zero item") introduced comprehensive root owner checker. > > > > That's 4.7, so we might want to do stable backport to 4.9+. > > It's just a performance optimization. > > I'm not sure if we should backport such things even if we can solve the > conflicts. That's for the consideration, if the performance improvement is significant, but due to other changes in the codebases it might not be safe. > >> However it's pretty expensive tree search to locate the owner root, > >> especially when it get reused by mandatory read and write time > >> tree-checker. > >> > >> This patch will remove that check, and completely rely on owner based > >> empty leaf check, which is much faster and still works fine for most > >> case. > >> > >> And since we skip the old root owner check, now write time tree check > >> can be merged with btrfs_check_leaf_full(). > > > > And for that reason the change should be minimal, this patch depends on > > current development queue so it's not suitable for the backport. > > > > I'm thinking what to do here, I want to avoid patches that effectively > > revert changes that are still in the development branch and haven't been > > released. > > OK, I'll resend write time tree checker patchset. Wait, it's just one patch that needs update, "btrfs: Do mandatory tree block check before submitting bio". If the above patch is dropped, then the expensive checks in check_leaf can be removed as you do in this patch, without the updates to parameters. > > This means to rework "btrfs: Do mandatory tree block check before > > submitting bio", though it's deep in the branch, the scope of changes is > > only 2 files so this should not cause any conflicts. > > BTW, I'm a little interesting in the workflow on your side. > > In this case, what's the correct way to handle them? > Just remove those two related patches from misc-next branch and re-apply > the newer version? If you mean these two btrfs: disk-io: Show the timing of corrupted tree block explicitly btrfs: Do mandatory tree block check before submitting bio we don't need to remove both, just the second one. > Or remove all those write time tree checker and later enhancement > patches and reapply them all? No, why removing them? That's independent enhancement of the tree checker. What needs to be removed is the call at write time. > Either way, I hope to experience the same workflow so I could reduce > your workload. This is an infrequent case when a patch deep in the branch needs update so the solution depends on the impact on the patches above it. Here it's only minor conflict with a cleanup patch removing redundant fs_info from check_leaf. So that's trivial and we don't need to update the patches in place. Further updates can be applied on top of misc-next (with the patch removed). The ideal workflow is: 1) patches are developed, tested by the developer 2) posted to mailinglist 3) after review added to a topic branch in for-next for wider testing - patchset revisions are easy to update as it's in a topic branch 4) after some time the patches get merged to misc-next and no updates are expected, beyond minor updates like coding style or changelog 5) patch goes to release branch and is effectively frozen, all fixes go as separate patches So if you want to reduce my workload, spend more time on 1-3 so we don't have to be creative when fixing problems we discover at 4-5. I know this cannot be followed 100% times, but so far we've been able to resolve all the situations and the number of times is overall low. So it's bearable.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index c3b89584dad6..fddcf7d7c30c 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -539,7 +539,7 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page) if (btrfs_header_level(eb)) err = btrfs_check_node(fs_info, eb); else - err = btrfs_check_leaf_write(fs_info, eb); + err = btrfs_check_leaf_full(fs_info, eb); if (err < 0) { btrfs_err(fs_info, diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 204fe53c90aa..e325d4df5c23 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -812,8 +812,7 @@ static int check_leaf_item(struct extent_buffer *leaf, return ret; } -static int check_leaf(struct extent_buffer *leaf, bool check_item_data, - bool check_empty_leaf) +static int check_leaf(struct extent_buffer *leaf, bool check_item_data) { struct btrfs_fs_info *fs_info = leaf->fs_info; /* No valid key type is 0, so all key should be larger than this key */ @@ -839,7 +838,6 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data, */ if (nritems == 0 && !btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_RELOC)) { u64 owner = btrfs_header_owner(leaf); - struct btrfs_root *check_root; /* These trees must never be empty */ if (owner == BTRFS_ROOT_TREE_OBJECTID || @@ -853,38 +851,6 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data, owner); return -EUCLEAN; } - - /* - * Don't check empty leaves at write time for trees where we - * can't use @owner to indicate the right owner. This happens - * for reloc trees or for a new commit root. - */ - if (!check_empty_leaf) - return 0; - - key.objectid = owner; - key.type = BTRFS_ROOT_ITEM_KEY; - key.offset = (u64)-1; - - check_root = btrfs_get_fs_root(fs_info, &key, false); - /* - * The only reason we also check NULL here is that during - * open_ctree() some roots has not yet been set up. - */ - if (!IS_ERR_OR_NULL(check_root)) { - struct extent_buffer *eb; - - eb = btrfs_root_node(check_root); - /* if leaf is the root, then it's fine */ - if (leaf != eb) { - generic_err(leaf, 0, - "invalid nritems, have %u should not be 0 for non-root leaf", - nritems); - free_extent_buffer(eb); - return -EUCLEAN; - } - free_extent_buffer(eb); - } return 0; } @@ -982,24 +948,13 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data, int btrfs_check_leaf_full(struct btrfs_fs_info *fs_info, struct extent_buffer *leaf) { - return check_leaf(leaf, true, true); + return check_leaf(leaf, true); } int btrfs_check_leaf_relaxed(struct btrfs_fs_info *fs_info, struct extent_buffer *leaf) { - return check_leaf(leaf, false, true); -} - -/* - * Write time specific leaf checker. - * Don't check if the empty leaf belongs to a tree root. Mostly for balance - * and new tree created in current transaction. - */ -int btrfs_check_leaf_write(struct btrfs_fs_info *fs_info, - struct extent_buffer *leaf) -{ - return check_leaf(leaf, true, false); + return check_leaf(leaf, false); } int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node) diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h index 281eb7136bc1..4df45e8a6659 100644 --- a/fs/btrfs/tree-checker.h +++ b/fs/btrfs/tree-checker.h @@ -23,8 +23,6 @@ int btrfs_check_leaf_full(struct btrfs_fs_info *fs_info, */ int btrfs_check_leaf_relaxed(struct btrfs_fs_info *fs_info, struct extent_buffer *leaf); -int btrfs_check_leaf_write(struct btrfs_fs_info *fs_info, - struct extent_buffer *leaf); int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node); int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has zero item") introduced comprehensive root owner checker. However it's pretty expensive tree search to locate the owner root, especially when it get reused by mandatory read and write time tree-checker. This patch will remove that check, and completely rely on owner based empty leaf check, which is much faster and still works fine for most case. And since we skip the old root owner check, now write time tree check can be merged with btrfs_check_leaf_full(). Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/disk-io.c | 2 +- fs/btrfs/tree-checker.c | 51 +++-------------------------------------- fs/btrfs/tree-checker.h | 2 -- 3 files changed, 4 insertions(+), 51 deletions(-)