| Message ID | 5891832d9130694cc60cffac74b95db92521728c.1686307630.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: replace BUG_ON() at split_item() with proper error handling | expand |
On 2023/6/9 18:49, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > There's no need to BUG_ON() at split_item() if the leaf does not have > enough free space for the new item, we can just return -ENOSPC since > the caller can deal with errors from split_item(). Also, as this is a > very unlikely condition to happen, because the caller has invoked > setup_leaf_for_split() before calling split_item(), surround the > condition with a WARN_ON() which makes it easier to notice this > unexpected condition and tags the if branch with 'unlikely' as well. > > I've actually once hit this BUG_ON() with some incorrect code changes > I had, which was very inconvenient as it required rebooting the VM. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/ctree.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index 29c5fa252eb1..e6009da34835 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -4000,7 +4000,12 @@ static noinline int split_item(struct btrfs_path *path, > struct btrfs_disk_key disk_key; > > leaf = path->nodes[0]; > - BUG_ON(btrfs_leaf_free_space(leaf) < sizeof(struct btrfs_item)); > + /* > + * Shouldn't happen because the caller must have previously called > + * setup_leaf_for_split() to make room for the new item in the leaf. > + */ > + if (WARN_ON(btrfs_leaf_free_space(leaf) < sizeof(struct btrfs_item))) > + return -ENOSPC; > > orig_slot = path->slots[0]; > orig_offset = btrfs_item_offset(leaf, path->slots[0]);
On Fri, Jun 09, 2023 at 11:49:07AM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > There's no need to BUG_ON() at split_item() if the leaf does not have > enough free space for the new item, we can just return -ENOSPC since > the caller can deal with errors from split_item(). Also, as this is a > very unlikely condition to happen, because the caller has invoked > setup_leaf_for_split() before calling split_item(), surround the > condition with a WARN_ON() which makes it easier to notice this > unexpected condition and tags the if branch with 'unlikely' as well. > > I've actually once hit this BUG_ON() with some incorrect code changes > I had, which was very inconvenient as it required rebooting the VM. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 29c5fa252eb1..e6009da34835 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -4000,7 +4000,12 @@ static noinline int split_item(struct btrfs_path *path, struct btrfs_disk_key disk_key; leaf = path->nodes[0]; - BUG_ON(btrfs_leaf_free_space(leaf) < sizeof(struct btrfs_item)); + /* + * Shouldn't happen because the caller must have previously called + * setup_leaf_for_split() to make room for the new item in the leaf. + */ + if (WARN_ON(btrfs_leaf_free_space(leaf) < sizeof(struct btrfs_item))) + return -ENOSPC; orig_slot = path->slots[0]; orig_offset = btrfs_item_offset(leaf, path->slots[0]);