diff mbox series

btrfs: do not BUG_ON() when dropping inode items from log root

Message ID 05225b2c8b1848cfb68125b858998111e18dd5cb.1686566185.git.fdmanana@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: do not BUG_ON() when dropping inode items from log root | expand

Commit Message

Filipe Manana June 12, 2023, 10:40 a.m. UTC 
From: Filipe Manana <fdmanana@suse.com>

When dropping inode items from a log tree at drop_inode_items(), we this
BUG_ON() on the result of btrfs_search_slot() because we don't expect an
exact match since having a key with an offset of (u64)-1 is unexpected.
That is generally true, but for dir index keys for example, we can get a
key with such an offset value, even though it's very unlikely and it would
take ages to increase the sequence counter for a dir index up to (u64)-1.
We can deal with an exact match, we just have to delete the key at that
slot, so there is really no need to BUG_ON(), error out or trigger any
warning. So remove the BUG_ON().

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Johannes Thumshirn June 12, 2023, 11:49 a.m. UTC | #1 
On 12.06.23 12:55, fdmanana@kernel.org wrote:

> -
> -		if (path->slots[0] == 0)
> +		if (ret < 0) {
>  			break;

Style nit, the else after a break isn't needed.

> +		} else if (ret > 0) {

Anyways,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
David Sterba June 12, 2023, 4:06 p.m. UTC | #2 
On Mon, Jun 12, 2023 at 11:40:17AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When dropping inode items from a log tree at drop_inode_items(), we this
> BUG_ON() on the result of btrfs_search_slot() because we don't expect an
> exact match since having a key with an offset of (u64)-1 is unexpected.
> That is generally true, but for dir index keys for example, we can get a
> key with such an offset value, even though it's very unlikely and it would
> take ages to increase the sequence counter for a dir index up to (u64)-1.
> We can deal with an exact match, we just have to delete the key at that
> slot, so there is really no need to BUG_ON(), error out or trigger any
> warning. So remove the BUG_ON().
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next (without change), thanks. If you want to fold the
change suggested by Johannes please let me know.
Filipe Manana June 12, 2023, 4:21 p.m. UTC | #3 
On Mon, Jun 12, 2023 at 12:50 PM Johannes Thumshirn
<Johannes.Thumshirn@wdc.com> wrote:
>
> On 12.06.23 12:55, fdmanana@kernel.org wrote:
>
> > -
> > -             if (path->slots[0] == 0)
> > +             if (ret < 0) {
> >                       break;
>
> Style nit, the else after a break isn't needed.

It isn't, but I usually prefer it like that, to have a single if-else
testing the same variable, rather than 2 (or more) separate if
statements testing the same variable. I find it more clear this way.

Thanks.

>
> > +             } else if (ret > 0) {
>
> Anyways,
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
diff mbox series

Patch

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index f91a6175fd11..365a1cc0a3c3 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4056,14 +4056,14 @@  static int drop_inode_items(struct btrfs_trans_handle *trans,
 
 	while (1) {
 		ret = btrfs_search_slot(trans, log, &key, path, -1, 1);
-		BUG_ON(ret == 0); /* Logic error */
-		if (ret < 0)
-			break;
-
-		if (path->slots[0] == 0)
+		if (ret < 0) {
 			break;
+		} else if (ret > 0) {
+			if (path->slots[0] == 0)
+				break;
+			path->slots[0]--;
+		}
 
-		path->slots[0]--;
 		btrfs_item_key_to_cpu(path->nodes[0], &found_key,
 				      path->slots[0]);