| Message ID | 8e323961434327423faeea50a3c6a09ff82a364b.1682505780.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: improve sibling keys check failure report | expand |
On 2023/4/26 18:51, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When checking siblings keys, before moving keys from one node/leaf to a > sibling node/leaf, it's very unexpected to have the last key of the left > sibling greater than or equals to the first key of the right sibling, as > that means we have a (serious) corruption that breaks the key ordering > properties of a b+tree. Since this is unexpected, surround the comparison > with the unlikely macro, which helps the compiler generate better code > for the most expected case (no existing b+tree corruption). This is also > what we do for other unexpected cases of invalid key ordering (like at > btrfs_set_item_key_safe()). > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/ctree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index a061d7092369..c315fb793b30 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -2707,7 +2707,7 @@ static bool check_sibling_keys(struct extent_buffer *left, > btrfs_item_key_to_cpu(right, &right_first, 0); > } > > - if (btrfs_comp_cpu_keys(&left_last, &right_first) >= 0) { > + if (unlikely(btrfs_comp_cpu_keys(&left_last, &right_first) >= 0)) { > btrfs_crit(left->fs_info, "left extent buffer:"); > btrfs_print_tree(left, false); > btrfs_crit(left->fs_info, "right extent buffer:");
On Wed, Apr 26, 2023 at 11:51:37AM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When checking siblings keys, before moving keys from one node/leaf to a > sibling node/leaf, it's very unexpected to have the last key of the left > sibling greater than or equals to the first key of the right sibling, as > that means we have a (serious) corruption that breaks the key ordering > properties of a b+tree. Since this is unexpected, surround the comparison > with the unlikely macro, which helps the compiler generate better code > for the most expected case (no existing b+tree corruption). This is also > what we do for other unexpected cases of invalid key ordering (like at > btrfs_set_item_key_safe()). > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/ctree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index a061d7092369..c315fb793b30 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -2707,7 +2707,7 @@ static bool check_sibling_keys(struct extent_buffer *left, > btrfs_item_key_to_cpu(right, &right_first, 0); > } > > - if (btrfs_comp_cpu_keys(&left_last, &right_first) >= 0) { > + if (unlikely(btrfs_comp_cpu_keys(&left_last, &right_first) >= 0)) { > btrfs_crit(left->fs_info, "left extent buffer:"); > btrfs_print_tree(left, false); > btrfs_crit(left->fs_info, "right extent buffer:"); Yeah, this is the pattern for unlikely(), compiler unfortunately does not recognize the cold functions like printk in the statement block so the manual annotations are needed. There's a minor effect on assembly, some of the instructions are reordered.
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index a061d7092369..c315fb793b30 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -2707,7 +2707,7 @@ static bool check_sibling_keys(struct extent_buffer *left, btrfs_item_key_to_cpu(right, &right_first, 0); } - if (btrfs_comp_cpu_keys(&left_last, &right_first) >= 0) { + if (unlikely(btrfs_comp_cpu_keys(&left_last, &right_first) >= 0)) { btrfs_crit(left->fs_info, "left extent buffer:"); btrfs_print_tree(left, false); btrfs_crit(left->fs_info, "right extent buffer:");