Skip to contentSkip to conversation

btrfs: More error handling patches

PATCH Superseded Jeff Mahoney 3 participants 15 messages Aug 10, 2011 – Aug 11, 2011 Btrfs file system

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jeff Mahoney

author
#0 lore raw

cover btrfs: More error handling patches

Hi all -

The following 8 patches add more error handling to the btrfs code:

- Add btrfs_panic
- Catch locking failures in {set,clear}_extent_bit
- Push up set_extent_bit errors to callers
- Push up lock_extent errors to callers
- Push up clear_extent_bit errors to callers
- Push up unlock_extent errors to callers
- Make pin_down_extent return void
- Push up btrfs_pin_extent errors to callers
- Push up non-looped btrfs_transaction_start errors to callers

-Jeff

author
#1 lore raw

PATCH 1/9 btrfs: Add btrfs_panic()

As part of the effort to eliminate BUG_ON as an error handling technique, we need to determine which errors are actual logic errors, which are on-disk corruption, and which are normal runtime errors e.g. -ENOMEM.

Annotating these error cases is helpful to understand and report them.

This patch adds a btrfs_panic() routine that will either panic or BUG depending on the new -ofatal_errors={panic,bug} mount option. Since there are still so many BUG_ONs, it defaults to BUG for now but I expect that to change once the error handling effort has made significant progress.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/ctree.h |   11 +++++++++++
 fs/btrfs/super.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletion(-)
fs/btrfs/ctree.h +11 -0
@@ -1363,6 +1363,7 @@ struct btrfs_ioctl_defrag_range_args {
 #define BTRFS_MOUNT_ENOSPC_DEBUG	 (1 << 15)
 #define BTRFS_MOUNT_AUTO_DEFRAG		(1 << 16)
 #define BTRFS_MOUNT_INODE_MAP_CACHE	(1 << 17)
+#define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR	(1 << 18)
 
 #define btrfs_clear_opt(o, opt)		((o) &= ~BTRFS_MOUNT_##opt)
 #define btrfs_set_opt(o, opt)		((o) |= BTRFS_MOUNT_##opt)

@@ -2650,6 +2651,16 @@ do {								\
 		__btrfs_std_error((fs_info), __func__, __LINE__, (errno));\
 } while (0)
 
+void __btrfs_panic(struct btrfs_fs_info *fs_info, const char *function,
+		   unsigned int line, int errno, const char *fmt, ...);
+
+#define btrfs_panic(fs_info, errno, fmt, args...)			\
+do {									\
+	struct btrfs_fs_info *_i = (fs_info);				\
+	__btrfs_panic(_i, __func__, __LINE__, errno, fmt, ##args);	\
+	BUG_ON(!(_i->mount_opt & BTRFS_MOUNT_PANIC_ON_FATAL_ERROR));	\
+} while (0)
+
 /* acl.c */
 #ifdef CONFIG_BTRFS_FS_POSIX_ACL
 struct posix_acl *btrfs_get_acl(struct inode *inode, int type);
fs/btrfs/super.c +46 -1

83 lines not on this page · raw message

author
#2 lore raw

PATCH 2/9 btrfs: Catch locking failures in {set,clear}_extent_bit

In reply to Jeff Mahoney

The *_state functions can only return 0 or -EEXIST. This patch addresses the cases where those functions return -EEXIST, representing a locking failure. It handles them by panicking with an appropriate error message.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/extent_io.c |   42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)
fs/btrfs/extent_io.c +31 -11
@@ -455,6 +455,7 @@ int clear_extent_bit(struct extent_io_tr
 		     struct extent_state **cached_state,
 		     gfp_t mask)
 {
+	struct btrfs_fs_info *fs_info;
 	struct extent_state *state;
 	struct extent_state *cached;
 	struct extent_state *prealloc = NULL;

@@ -465,6 +466,8 @@ int clear_extent_bit(struct extent_io_tr
 	int set = 0;
 	int clear = 0;
 
+	fs_info = btrfs_sb(tree->mapping->host->i_sb)->fs_info;
+
 	if (delete)
 		bits |= ~EXTENT_CTLBITS;
 	bits |= EXTENT_FIRST_DELALLOC;

@@ -531,7 +534,10 @@ hit_next:
 		prealloc = alloc_extent_state_atomic(prealloc);
 		BUG_ON(!prealloc);
 		err = split_state(tree, state, prealloc, start);
-		BUG_ON(err == -EEXIST);
+		if (err)
+			btrfs_panic(fs_info, err, "Locking error: "
+				    "Extent tree was modified by another "
+				    "thread while locked.");
 		prealloc = NULL;
 		if (err)
 			goto out;

@@ -553,7 +559,10 @@ hit_next:
 		prealloc = alloc_extent_state_atomic(prealloc);
 		BUG_ON(!prealloc);
 		err = split_state(tree, state, prealloc, end + 1);
-		BUG_ON(err == -EEXIST);
+		if (err)
+			btrfs_panic(fs_info, err, "Locking error: "
+				    "Extent tree was modified by another "
+				    "thread while locked.");
 		if (wake)
 			wake_up(&state->wq);
 

@@ -704,6 +713,7 @@ int set_extent_bit(struct extent_io_tree
 		   int bits, int exclusive_bits, u64 *failed_start,
 		   struct extent_state **cached_state, gfp_t mask)
 {
+	struct btrfs_fs_info *fs_info;
 	struct extent_state *state;
 	struct extent_state *prealloc = NULL;
 	struct rb_node *node;

@@ -711,6 +721,8 @@ int set_extent_bit(struct extent_io_tree
 	u64 last_start;
 	u64 last_end;
 
+	fs_info = btrfs_sb(tree->mapping->host->i_sb)->fs_info;
+
 	bits |= EXTENT_FIRST_DELALLOC;
 again:
 	if (!prealloc && (mask & __GFP_WAIT)) {

@@ -736,8 +748,11 @@ again:
 		prealloc = alloc_extent_state_atomic(prealloc);
 		BUG_ON(!prealloc);
 		err = insert_state(tree, prealloc, start, end, &bits);
+		if (err)
+			btrfs_panic(fs_info, err, "Locking error: "
+				    "Extent tree was modified by another "
+				    "thread while locked.");
 		prealloc = NULL;
-		BUG_ON(err == -EEXIST);
 		goto out;
 	}
 	state = rb_entry(node, struct extent_state, rb_node);

@@ -803,7 +818,10 @@ hit_next:
 		prealloc = alloc_extent_state_atomic(prealloc);
 		BUG_ON(!prealloc);
 		err = split_state(tree, state, prealloc, start);
-		BUG_ON(err == -EEXIST);
+		if (err)
+			btrfs_panic(fs_info, err, "Locking error: "
+				    "Extent tree was modified by another "
+				    "thread while locked.");
 		prealloc = NULL;
 		if (err)
 			goto out;

@@ -840,12 +858,11 @@ hit_next:
 		 */
 		err = insert_state(tree, prealloc, start, this_end,
 				   &bits);
-		BUG_ON(err == -EEXIST);
-		if (err) {
-			free_extent_state(prealloc);
-			prealloc = NULL;
-			goto out;
-		}
+		if (err)
+			btrfs_panic(fs_info, err, "Locking error: "
+				    "Extent tree was modified by another "
+				    "thread while locked.");
+
 		cache_state(prealloc, cached_state);
 		prealloc = NULL;
 		start = this_end + 1;

@@ -867,7 +884,10 @@ hit_next:
 		prealloc = alloc_extent_state_atomic(prealloc);
 		BUG_ON(!prealloc);
 		err = split_state(tree, state, prealloc, end + 1);
-		BUG_ON(err == -EEXIST);
+		if (err)
+			btrfs_panic(fs_info, err, "Locking error: "
+				    "Extent tree was modified by another "
+				    "thread while locked.");
 
 		set_state_bits(tree, prealloc, &bits);
 		cache_state(prealloc, cached_state);
author
#3 lore raw

PATCH 3/9 btrfs: Push up set_extent_bit errors to callers

In reply to Jeff Mahoney

In the locking case, set_extent_bit can return -EEXIST but callers already handle that.

In the non-locking case, it can't fail. Memory allocation failures are handled by BUG_ON.

This patch pushes up the BUG_ONs from set_extent_bit to callers, except where -ENOMEM can't occur (e.g. __GFP_WAIT && __GFP_NOFAIL).

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/extent-tree.c |   37 +++++++++++++++++++++++++------------
 fs/btrfs/extent_io.c   |   46 +++++++++++++++++++++++++++++++++++-----------
 fs/btrfs/file-item.c   |    3 ++-
 fs/btrfs/inode.c       |   25 +++++++++++++++++--------
 fs/btrfs/ioctl.c       |    5 +++--
 fs/btrfs/relocation.c  |   21 +++++++++++++--------
 6 files changed, 95 insertions(+), 42 deletions(-)
fs/btrfs/extent-tree.c +25 -12
@@ -192,11 +192,14 @@ block_group_cache_tree_search(struct btr
 static int add_excluded_extent(struct btrfs_root *root,
 			       u64 start, u64 num_bytes)
 {
+	int ret;
 	u64 end = start + num_bytes - 1;
-	set_extent_bits(&root->fs_info->freed_extents[0],
-			start, end, EXTENT_UPTODATE, GFP_NOFS);
-	set_extent_bits(&root->fs_info->freed_extents[1],
-			start, end, EXTENT_UPTODATE, GFP_NOFS);
+	ret = set_extent_bits(&root->fs_info->freed_extents[0],
+			      start, end, EXTENT_UPTODATE, GFP_NOFS);
+	BUG_ON(ret);
+	ret = set_extent_bits(&root->fs_info->freed_extents[1],
+			      start, end, EXTENT_UPTODATE, GFP_NOFS);
+	BUG_ON(ret);
 	return 0;
 }
 

@@ -4142,8 +4145,9 @@ static int update_block_group(struct btr
 			spin_unlock(&cache->lock);
 			spin_unlock(&cache->space_info->lock);
 
-			set_extent_dirty(info->pinned_extents,
-					 bytenr, bytenr + num_bytes - 1,
+			/* Can't return -ENOMEM */
+			set_extent_dirty(info->pinned_extents, bytenr,
+					 bytenr + num_bytes - 1,
 					 GFP_NOFS | __GFP_NOFAIL);
 		}
 		btrfs_put_block_group(cache);

@@ -4184,6 +4188,7 @@ static int pin_down_extent(struct btrfs_
 	spin_unlock(&cache->lock);
 	spin_unlock(&cache->space_info->lock);
 
+	/* __GFP_NOFAIL means it can't return -ENOMEM */
 	set_extent_dirty(root->fs_info->pinned_extents, bytenr,
 			 bytenr + num_bytes - 1, GFP_NOFS | __GFP_NOFAIL);
 	return 0;

@@ -5636,6 +5641,7 @@ struct extent_buffer *btrfs_init_new_buf
 					    int level)
 {
 	struct extent_buffer *buf;
+	int ret;
 
 	buf = btrfs_find_create_tree_block(root, bytenr, blocksize);
 	if (!buf)

@@ -5654,14 +5660,21 @@ struct extent_buffer *btrfs_init_new_buf
 		 * EXENT bit to differentiate dirty pages.
 		 */
 		if (root->log_transid % 2 == 0)
-			set_extent_dirty(&root->dirty_log_pages, buf->start,
-					buf->start + buf->len - 1, GFP_NOFS);
+			ret = set_extent_dirty(&root->dirty_log_pages,
+					       buf->start,
+					       buf->start + buf->len - 1,
+					       GFP_NOFS);
 		else
-			set_extent_new(&root->dirty_log_pages, buf->start,
-					buf->start + buf->len - 1, GFP_NOFS);
+			ret = set_extent_new(&root->dirty_log_pages,
+					     buf->start,
+					     buf->start + buf->len - 1,
+					     GFP_NOFS);
+		BUG_ON(ret);
 	} else {
-		set_extent_dirty(&trans->transaction->dirty_pages, buf->start,
-			 buf->start + buf->len - 1, GFP_NOFS);
+		ret = set_extent_dirty(&trans->transaction->dirty_pages,
+				       buf->start, buf->start + buf->len - 1,
+				       GFP_NOFS);
+		BUG_ON(ret);
 	}
 	trans->blocks_used++;
 	/* this returns a buffer locked for blocking */
fs/btrfs/extent_io.c +35 -11

112 lines not on this page · raw message

fs/btrfs/file-item.c +2 -1
@@ -212,9 +212,10 @@ static int __btrfs_lookup_bio_sums(struc
 				sum = 0;
 				if (BTRFS_I(inode)->root->root_key.objectid ==
 				    BTRFS_DATA_RELOC_TREE_OBJECTID) {
-					set_extent_bits(io_tree, offset,
+					ret = set_extent_bits(io_tree, offset,
 						offset + bvec->bv_len - 1,
 						EXTENT_NODATASUM, GFP_NOFS);
+					BUG_ON(ret);
 				} else {
 					printk(KERN_INFO "btrfs no csum found "
 					       "for inode %llu start %llu\n",
fs/btrfs/inode.c +17 -8
@@ -1548,6 +1548,7 @@ static void btrfs_writepage_fixup_worker
 	struct inode *inode;
 	u64 page_start;
 	u64 page_end;
+	int ret;
 
 	fixup = container_of(work, struct btrfs_writepage_fixup, work);
 	page = fixup->page;

@@ -1579,7 +1580,9 @@ again:
 	}
 
 	BUG();
-	btrfs_set_extent_delalloc(inode, page_start, page_end, &cached_state);
+	ret = btrfs_set_extent_delalloc(inode, page_start, page_end,
+					&cached_state);
+	BUG_ON(ret);
 	ClearPageChecked(page);
 out:
 	unlock_extent_cached(&BTRFS_I(inode)->io_tree, page_start, page_end,

@@ -1883,8 +1886,11 @@ static int btrfs_io_failed_hook(struct b
 		}
 		failrec->logical = logical;
 		free_extent_map(em);
-		set_extent_bits(failure_tree, start, end, EXTENT_LOCKED |
-				EXTENT_DIRTY, GFP_NOFS);
+
+		/* Doesn't this ignore locking failures? */
+		ret = set_extent_bits(failure_tree, start, end, EXTENT_LOCKED |
+				      EXTENT_DIRTY, GFP_NOFS);
+		BUG_ON(ret);
 		set_state_private(failure_tree, start,
 				 (u64)(unsigned long)failrec);
 	} else {

@@ -5167,8 +5173,10 @@ again:
 			kunmap(page);
 			btrfs_mark_buffer_dirty(leaf);
 		}
-		set_extent_uptodate(io_tree, em->start,
-				    extent_map_end(em) - 1, NULL, GFP_NOFS);
+		ret = set_extent_uptodate(io_tree, em->start,
+					  extent_map_end(em) - 1,
+					  NULL, GFP_NOFS);
+		BUG_ON(ret);
 		goto insert;
 	} else {
 		printk(KERN_ERR "btrfs unknown found_type %d\n", found_type);

@@ -6230,9 +6238,10 @@ static ssize_t btrfs_direct_IO(int rw, s
 	 */
 	if (writing) {
 		write_bits = EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING;
-		ret = set_extent_bit(&BTRFS_I(inode)->io_tree, lockstart, lockend,
-				     EXTENT_DELALLOC, 0, NULL, &cached_state,
-				     GFP_NOFS);
+		ret = set_extent_bit(&BTRFS_I(inode)->io_tree, lockstart,
+				     lockend, EXTENT_DELALLOC, 0, NULL,
+				     &cached_state, GFP_NOFS);
+		BUG_ON(ret);
 		if (ret) {
 			clear_extent_bit(&BTRFS_I(inode)->io_tree, lockstart,
 					 lockend, EXTENT_LOCKED | write_bits,
fs/btrfs/ioctl.c +3 -2
@@ -938,8 +938,9 @@ again:
 	}
 
 
-	btrfs_set_extent_delalloc(inode, page_start, page_end - 1,
-				  &cached_state);
+	ret = btrfs_set_extent_delalloc(inode, page_start, page_end - 1,
+					&cached_state);
+	BUG_ON(ret);
 
 	unlock_extent_cached(&BTRFS_I(inode)->io_tree,
 			     page_start, page_end - 1, &cached_state,
fs/btrfs/relocation.c +13 -8
@@ -2623,11 +2623,11 @@ static int finish_pending_nodes(struct b
 	return err;
 }
 
-static void mark_block_processed(struct reloc_control *rc,
+static int mark_block_processed(struct reloc_control *rc,
 				 u64 bytenr, u32 blocksize)
 {
-	set_extent_bits(&rc->processed_blocks, bytenr, bytenr + blocksize - 1,
-			EXTENT_DIRTY, GFP_NOFS);
+	return set_extent_bits(&rc->processed_blocks, bytenr,
+			       bytenr + blocksize - 1, EXTENT_DIRTY, GFP_NOFS);
 }
 
 static void __mark_block_processed(struct reloc_control *rc,

@@ -2636,8 +2636,10 @@ static void __mark_block_processed(struc
 	u32 blocksize;
 	if (node->level == 0 ||
 	    in_block_group(node->bytenr, rc->block_group)) {
+		int ret;
 		blocksize = btrfs_level_size(rc->extent_root, node->level);
-		mark_block_processed(rc, node->bytenr, blocksize);
+		ret = mark_block_processed(rc, node->bytenr, blocksize);
+		BUG_ON(ret);
 	}
 	node->processed = 1;
 }

@@ -2994,13 +2996,16 @@ static int relocate_file_extent_cluster(
 
 		if (nr < cluster->nr &&
 		    page_start + offset == cluster->boundary[nr]) {
-			set_extent_bits(&BTRFS_I(inode)->io_tree,
-					page_start, page_end,
-					EXTENT_BOUNDARY, GFP_NOFS);
+			ret = set_extent_bits(&BTRFS_I(inode)->io_tree,
+					      page_start, page_end,
+					      EXTENT_BOUNDARY, GFP_NOFS);
+			BUG_ON(ret);
 			nr++;
 		}
 
-		btrfs_set_extent_delalloc(inode, page_start, page_end, NULL);
+		ret = btrfs_set_extent_delalloc(inode, page_start,
+						page_end, NULL);
+		BUG_ON(ret);
 		set_page_dirty(page);
 
 		unlock_extent(&BTRFS_I(inode)->io_tree,
#4 lore raw

Re: [patch 3/9] btrfs: Push up set_extent_bit errors to callers

fs/btrfs/extent_io.c

Quoted 2 lines · Jeff Mahoney
fs/btrfs/extent_io.c +0 -0

[...]

Oops. This is missing one more case. I'll re-send this one as part of a general post-review re-send.

@@ -3292,8 +3316,10 @@ int set_extent_buffer_uptodate(struct ex
num_pages = num_extent_pages(eb->start, eb->len);

if (eb_straddles_pages(eb)) {
- set_extent_uptodate(tree, eb->start, eb->start + eb->len - 1,
- NULL, GFP_NOFS);
+ int ret = set_extent_uptodate(tree, eb->start,
+ eb->start + eb->len - 1,
+ NULL, GFP_NOFS);
+ BUG_ON(ret < 0);
}
for (i = 0; i < num_pages; i++) {
page = extent_buffer_page(eb, i);

-Jeff

Signature · 2 lines
Jeff Mahoney
SUSE Labs
author
#5 lore raw

PATCH 4/9 btrfs: Push up lock_extent errors to callers

In reply to Jeff Mahoney

lock_extent, try_lock_extent, and lock_extent_bits can't currently fail because errors are caught via BUG_ON.

This patch pushes the error handling up to callers, which currently only handle them via BUG_ON themselves.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/compression.c      |    3 +-
 fs/btrfs/disk-io.c          |    5 ++-
 fs/btrfs/extent_io.c        |   20 ++++++++-----
 fs/btrfs/file.c             |   17 ++++++-----
 fs/btrfs/free-space-cache.c |    6 ++--
 fs/btrfs/inode.c            |   66 ++++++++++++++++++++++++++------------------
 fs/btrfs/ioctl.c            |   16 ++++++----
 fs/btrfs/relocation.c       |   18 ++++++++----
 8 files changed, 94 insertions(+), 57 deletions(-)
fs/btrfs/compression.c +2 -1
@@ -496,7 +496,8 @@ static noinline int add_ra_bio_pages(str
 		 * sure they map to this compressed extent on disk.
 		 */
 		set_page_extent_mapped(page);
-		lock_extent(tree, last_offset, end, GFP_NOFS);
+		ret = lock_extent(tree, last_offset, end, GFP_NOFS);
+		BUG_ON(ret < 0);
 		read_lock(&em_tree->lock);
 		em = lookup_extent_mapping(em_tree, last_offset,
 					   PAGE_CACHE_SIZE);
fs/btrfs/disk-io.c +3 -2
@@ -331,8 +331,9 @@ static int verify_parent_transid(struct
 	if (!parent_transid || btrfs_header_generation(eb) == parent_transid)
 		return 0;
 
-	lock_extent_bits(io_tree, eb->start, eb->start + eb->len - 1,
-			 0, &cached_state, GFP_NOFS);
+	ret = lock_extent_bits(io_tree, eb->start, eb->start + eb->len - 1,
+			       0, &cached_state, GFP_NOFS);
+	BUG_ON(ret < 0);
 	if (extent_buffer_uptodate(io_tree, eb, cached_state) &&
 	    btrfs_header_generation(eb) == parent_transid) {
 		ret = 0;
fs/btrfs/extent_io.c +12 -8
@@ -1013,7 +1013,6 @@ int lock_extent_bits(struct extent_io_tr
 		}
 		WARN_ON(start > end);
 	}
-	BUG_ON(err);
 	return err;
 }
 

@@ -1035,8 +1034,8 @@ int try_lock_extent(struct extent_io_tre
 			clear_extent_bit(tree, start, failed_start - 1,
 					 EXTENT_LOCKED, 1, 0, NULL, mask);
 		return 0;
-	}
-	BUG_ON(err);
+	} else if (err < 0)
+		return err;
 	return 1;
 }
 

@@ -1347,8 +1346,9 @@ again:
 	BUG_ON(ret);
 
 	/* step three, lock the state bits for the whole range */
-	lock_extent_bits(tree, delalloc_start, delalloc_end,
-			 0, &cached_state, GFP_NOFS);
+	ret = lock_extent_bits(tree, delalloc_start, delalloc_end,
+			       0, &cached_state, GFP_NOFS);
+	BUG_ON(ret < 0);
 
 	/* then test to make sure it is all still delalloc */
 	ret = test_range_bit(tree, delalloc_start, delalloc_end,

@@ -1977,7 +1977,8 @@ static int __extent_read_full_page(struc
 
 	end = page_end;
 	while (1) {
-		lock_extent(tree, start, end, GFP_NOFS);
+		ret = lock_extent(tree, start, end, GFP_NOFS);
+		BUG_ON(ret < 0);
 		ordered = btrfs_lookup_ordered_extent(inode, start);
 		if (!ordered)
 			break;

@@ -2663,12 +2664,14 @@ int extent_invalidatepage(struct extent_
 	u64 start = ((u64)page->index << PAGE_CACHE_SHIFT);
 	u64 end = start + PAGE_CACHE_SIZE - 1;
 	size_t blocksize = page->mapping->host->i_sb->s_blocksize;
+	int ret;
 
 	start += (offset + blocksize - 1) & ~(blocksize - 1);
 	if (start > end)
 		return 0;
 
-	lock_extent_bits(tree, start, end, 0, &cached_state, GFP_NOFS);
+	ret = lock_extent_bits(tree, start, end, 0, &cached_state, GFP_NOFS);
+	BUG_ON(ret < 0);
 	wait_on_page_writeback(page);
 	clear_extent_bit(tree, start, end,
 			 EXTENT_LOCKED | EXTENT_DIRTY | EXTENT_DELALLOC |

@@ -2878,8 +2881,9 @@ int extent_fiemap(struct inode *inode, s
 		last_for_get_extent = isize;
 	}
 
-	lock_extent_bits(&BTRFS_I(inode)->io_tree, start, start + len, 0,
+	ret = lock_extent_bits(&BTRFS_I(inode)->io_tree, start, start + len, 0,
 			 &cached_state, GFP_NOFS);
+	BUG_ON(ret < 0);
 
 	em = get_extent_skip_holes(inode, off, last_for_get_extent,
 				   get_extent);
fs/btrfs/file.c +10 -7
@@ -1104,9 +1104,10 @@ again:
 	err = 0;
 	if (start_pos < inode->i_size) {
 		struct btrfs_ordered_extent *ordered;
-		lock_extent_bits(&BTRFS_I(inode)->io_tree,
-				 start_pos, last_pos - 1, 0, &cached_state,
-				 GFP_NOFS);
+		err = lock_extent_bits(&BTRFS_I(inode)->io_tree,
+				       start_pos, last_pos - 1, 0,
+				       &cached_state, GFP_NOFS);
+		BUG_ON(err < 0);
 		ordered = btrfs_lookup_first_ordered_extent(inode,
 							    last_pos - 1);
 		if (ordered &&

@@ -1612,8 +1613,9 @@ static long btrfs_fallocate(struct file
 		/* the extent lock is ordered inside the running
 		 * transaction
 		 */
-		lock_extent_bits(&BTRFS_I(inode)->io_tree, alloc_start,
-				 locked_end, 0, &cached_state, GFP_NOFS);
+		ret = lock_extent_bits(&BTRFS_I(inode)->io_tree, alloc_start,
+				       locked_end, 0, &cached_state, GFP_NOFS);
+		BUG_ON(ret < 0);
 		ordered = btrfs_lookup_first_ordered_extent(inode,
 							    alloc_end - 1);
 		if (ordered &&

@@ -1696,8 +1698,9 @@ static int find_desired_extent(struct in
 	if (inode->i_size == 0)
 		return -ENXIO;
 
-	lock_extent_bits(&BTRFS_I(inode)->io_tree, lockstart, lockend, 0,
-			 &cached_state, GFP_NOFS);
+	ret = lock_extent_bits(&BTRFS_I(inode)->io_tree, lockstart, lockend, 0,
+			       &cached_state, GFP_NOFS);
+	BUG_ON(ret < 0);
 
 	/*
 	 * Delalloc is such a pain.  If we have a hole and we have pending
fs/btrfs/free-space-cache.c +4 -2
@@ -609,8 +609,10 @@ int __btrfs_write_out_cache(struct btrfs
 	}
 
 	index = 0;
-	lock_extent_bits(&BTRFS_I(inode)->io_tree, 0, i_size_read(inode) - 1,
-			 0, &cached_state, GFP_NOFS);
+	ret = lock_extent_bits(&BTRFS_I(inode)->io_tree, 0,
+			       i_size_read(inode) - 1, 0, &cached_state,
+			       GFP_NOFS);
+	BUG_ON(ret < 0);
 
 	/*
 	 * When searching for pinned extents, we need to start at our start
fs/btrfs/inode.c +40 -26

138 lines not on this page · raw message

fs/btrfs/ioctl.c +10 -6
@@ -757,7 +757,7 @@ static int should_defrag_range(struct in
 	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
 	struct extent_map *em = NULL;
 	struct extent_map_tree *em_tree = &BTRFS_I(inode)->extent_tree;
-	int ret = 1;
+	int ret = 1, err;
 
 	/*
 	 * make sure that once we start defragging and extent, we keep on

@@ -778,7 +778,8 @@ static int should_defrag_range(struct in
 
 	if (!em) {
 		/* get the big lock and read metadata off disk */
-		lock_extent(io_tree, start, start + len - 1, GFP_NOFS);
+		err = lock_extent(io_tree, start, start + len - 1, GFP_NOFS);
+		BUG_ON(err < 0);
 		em = btrfs_get_extent(inode, NULL, 0, start, len, 0);
 		unlock_extent(io_tree, start, start + len - 1, GFP_NOFS);
 

@@ -902,9 +903,10 @@ again:
 	page_start = page_offset(pages[0]);
 	page_end = page_offset(pages[i_done - 1]) + PAGE_CACHE_SIZE;
 
-	lock_extent_bits(&BTRFS_I(inode)->io_tree,
-			 page_start, page_end - 1, 0, &cached_state,
-			 GFP_NOFS);
+	ret = lock_extent_bits(&BTRFS_I(inode)->io_tree,
+			       page_start, page_end - 1, 0, &cached_state,
+			       GFP_NOFS);
+	BUG_ON(ret < 0);
 	ordered = btrfs_lookup_first_ordered_extent(inode, page_end - 1);
 	if (ordered &&
 	    ordered->file_offset + ordered->len > page_start &&

@@ -2225,7 +2227,9 @@ static noinline long btrfs_ioctl_clone(s
 	   another, and lock file content */
 	while (1) {
 		struct btrfs_ordered_extent *ordered;
-		lock_extent(&BTRFS_I(src)->io_tree, off, off+len, GFP_NOFS);
+		ret = lock_extent(&BTRFS_I(src)->io_tree, off, off+len,
+				  GFP_NOFS);
+		BUG_ON(ret < 0);
 		ordered = btrfs_lookup_first_ordered_extent(src, off+len);
 		if (!ordered &&
 		    !test_range_bit(&BTRFS_I(src)->io_tree, off, off+len,
fs/btrfs/relocation.c +13 -5
@@ -1577,6 +1577,7 @@ int replace_file_extents(struct btrfs_tr
 				ret = try_lock_extent(&BTRFS_I(inode)->io_tree,
 						      key.offset, end,
 						      GFP_NOFS);
+				BUG_ON(ret < 0);
 				if (!ret)
 					continue;
 

@@ -1899,6 +1900,7 @@ static int invalidate_extent_cache(struc
 	u64 objectid;
 	u64 start, end;
 	u64 ino;
+	int ret;
 
 	objectid = min_key->objectid;
 	while (1) {

@@ -1952,7 +1954,9 @@ static int invalidate_extent_cache(struc
 		}
 
 		/* the lock_extent waits for readpage to complete */
-		lock_extent(&BTRFS_I(inode)->io_tree, start, end, GFP_NOFS);
+		ret = lock_extent(&BTRFS_I(inode)->io_tree, start, end,
+				  GFP_NOFS);
+		BUG_ON(ret < 0);
 		btrfs_drop_extent_cache(inode, start, end, 1);
 		unlock_extent(&BTRFS_I(inode)->io_tree, start, end, GFP_NOFS);
 	}

@@ -2862,7 +2866,9 @@ int prealloc_file_extent_cluster(struct
 		else
 			end = cluster->end - offset;
 
-		lock_extent(&BTRFS_I(inode)->io_tree, start, end, GFP_NOFS);
+		ret = lock_extent(&BTRFS_I(inode)->io_tree, start,
+				  end, GFP_NOFS);
+		BUG_ON(ret < 0);
 		num_bytes = end + 1 - start;
 		ret = btrfs_prealloc_file_range(inode, 0, start,
 						num_bytes, num_bytes,

@@ -2899,7 +2905,8 @@ int setup_extent_mapping(struct inode *i
 	em->bdev = root->fs_info->fs_devices->latest_bdev;
 	set_bit(EXTENT_FLAG_PINNED, &em->flags);
 
-	lock_extent(&BTRFS_I(inode)->io_tree, start, end, GFP_NOFS);
+	ret = lock_extent(&BTRFS_I(inode)->io_tree, start, end, GFP_NOFS);
+	BUG_ON(ret < 0);
 	while (1) {
 		write_lock(&em_tree->lock);
 		ret = add_extent_mapping(em_tree, em);

@@ -2989,8 +2996,9 @@ static int relocate_file_extent_cluster(
 		page_start = (u64)page->index << PAGE_CACHE_SHIFT;
 		page_end = page_start + PAGE_CACHE_SIZE - 1;
 
-		lock_extent(&BTRFS_I(inode)->io_tree,
-			    page_start, page_end, GFP_NOFS);
+		ret = lock_extent(&BTRFS_I(inode)->io_tree,
+				  page_start, page_end, GFP_NOFS);
+		BUG_ON(ret < 0);
 
 		set_page_extent_mapped(page);
author
#6 lore raw

PATCH 5/9 btrfs: Push up clear_extent_bit errors to callers

In reply to Jeff Mahoney

clear_extent_bit can fail with -ENOMEM for a specific case but will BUG on other memory allocation failures.

This patch returns -ENOMEM for memory allocation failures and handles them with BUG_ON in callers which don't handle it already.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/disk-io.c          |    7 ++-
 fs/btrfs/extent-tree.c      |   14 ++++--
 fs/btrfs/extent_io.c        |   56 ++++++++++++++++++--------
 fs/btrfs/file.c             |   10 ++--
 fs/btrfs/free-space-cache.c |   20 +++++----
 fs/btrfs/inode.c            |   92 +++++++++++++++++++++++++++-----------------
 fs/btrfs/ioctl.c            |    9 ++--
 fs/btrfs/relocation.c       |    5 +-
 fs/btrfs/transaction.c      |    4 +
 fs/btrfs/tree-log.c         |    5 +-
 10 files changed, 142 insertions(+), 80 deletions(-)
fs/btrfs/disk-io.c +5 -2
@@ -2981,7 +2981,9 @@ static int btrfs_destroy_marked_extents(
 		if (ret)
 			break;
 
-		clear_extent_bits(dirty_pages, start, end, mark, GFP_NOFS);
+		ret = clear_extent_bits(dirty_pages, start, end, mark,
+					GFP_NOFS);
+		BUG_ON(ret < 0);
 		while (start <= end) {
 			index = start >> PAGE_CACHE_SHIFT;
 			start = (u64)(index + 1) << PAGE_CACHE_SHIFT;

@@ -3042,7 +3044,8 @@ static int btrfs_destroy_pinned_extent(s
 							 end + 1 - start,
 							 NULL);
 
-		clear_extent_dirty(unpin, start, end, GFP_NOFS);
+		ret = clear_extent_dirty(unpin, start, end, GFP_NOFS);
+		BUG_ON(ret < 0);
 		btrfs_error_unpin_extent_range(root, start, end);
 		cond_resched();
 	}
fs/btrfs/extent-tree.c +9 -5
@@ -207,14 +207,17 @@ static void free_excluded_extents(struct
 				  struct btrfs_block_group_cache *cache)
 {
 	u64 start, end;
+	int ret;
 
 	start = cache->key.objectid;
 	end = start + cache->key.offset - 1;
 
-	clear_extent_bits(&root->fs_info->freed_extents[0],
-			  start, end, EXTENT_UPTODATE, GFP_NOFS);
-	clear_extent_bits(&root->fs_info->freed_extents[1],
-			  start, end, EXTENT_UPTODATE, GFP_NOFS);
+	ret = clear_extent_bits(&root->fs_info->freed_extents[0],
+				start, end, EXTENT_UPTODATE, GFP_NOFS);
+	BUG_ON(ret < 0);
+	ret = clear_extent_bits(&root->fs_info->freed_extents[1],
+				start, end, EXTENT_UPTODATE, GFP_NOFS);
+	BUG_ON(ret < 0);
 }
 
 static int exclude_super_stripes(struct btrfs_root *root,

@@ -4359,7 +4362,8 @@ int btrfs_finish_extent_commit(struct bt
 			ret = btrfs_discard_extent(root, start,
 						   end + 1 - start, NULL);
 
-		clear_extent_dirty(unpin, start, end, GFP_NOFS);
+		ret = clear_extent_dirty(unpin, start, end, GFP_NOFS);
+		BUG_ON(ret < 0);
 		unpin_extent_range(root, start, end);
 		cond_resched();
 	}
fs/btrfs/extent_io.c +39 -17

108 lines not on this page · raw message

fs/btrfs/file.c +6 -4
@@ -1128,10 +1128,12 @@ again:
 		if (ordered)
 			btrfs_put_ordered_extent(ordered);
 
-		clear_extent_bit(&BTRFS_I(inode)->io_tree, start_pos,
-				  last_pos - 1, EXTENT_DIRTY | EXTENT_DELALLOC |
-				  EXTENT_DO_ACCOUNTING, 0, 0, &cached_state,
-				  GFP_NOFS);
+		err = clear_extent_bit(&BTRFS_I(inode)->io_tree, start_pos,
+				      last_pos - 1,
+				      EXTENT_DIRTY | EXTENT_DELALLOC |
+				      EXTENT_DO_ACCOUNTING, 0, 0,
+				      &cached_state, GFP_NOFS);
+		BUG_ON(err < 0);
 		unlock_extent_cached(&BTRFS_I(inode)->io_tree,
 				     start_pos, last_pos - 1, &cached_state,
 				     GFP_NOFS);
fs/btrfs/free-space-cache.c +12 -8
@@ -800,10 +800,12 @@ int __btrfs_write_out_cache(struct btrfs
 
 	ret = btrfs_search_slot(trans, root, &key, path, 1, 1);
 	if (ret < 0) {
+		ret = clear_extent_bit(&BTRFS_I(inode)->io_tree, 0, bytes - 1,
+				       EXTENT_DIRTY | EXTENT_DELALLOC |
+				       EXTENT_DO_ACCOUNTING, 0, 0, NULL,
+				       GFP_NOFS);
+		BUG_ON(ret < 0);
 		ret = -1;
-		clear_extent_bit(&BTRFS_I(inode)->io_tree, 0, bytes - 1,
-				 EXTENT_DIRTY | EXTENT_DELALLOC |
-				 EXTENT_DO_ACCOUNTING, 0, 0, NULL, GFP_NOFS);
 		goto out;
 	}
 	leaf = path->nodes[0];

@@ -814,12 +816,14 @@ int __btrfs_write_out_cache(struct btrfs
 		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
 		if (found_key.objectid != BTRFS_FREE_SPACE_OBJECTID ||
 		    found_key.offset != offset) {
-			ret = -1;
-			clear_extent_bit(&BTRFS_I(inode)->io_tree, 0, bytes - 1,
-					 EXTENT_DIRTY | EXTENT_DELALLOC |
-					 EXTENT_DO_ACCOUNTING, 0, 0, NULL,
-					 GFP_NOFS);
+			ret = clear_extent_bit(&BTRFS_I(inode)->io_tree, 0,
+					       bytes - 1,
+					       EXTENT_DIRTY | EXTENT_DELALLOC |
+					       EXTENT_DO_ACCOUNTING, 0, 0,
+					       NULL, GFP_NOFS);
+			BUG_ON(ret < 0);
 			btrfs_release_path(path);
+			ret = -1;
 			goto out;
 		}
 	}
fs/btrfs/inode.c +57 -35

174 lines not on this page · raw message

fs/btrfs/ioctl.c +5 -4
@@ -926,10 +926,11 @@ again:
 	if (ordered)
 		btrfs_put_ordered_extent(ordered);
 
-	clear_extent_bit(&BTRFS_I(inode)->io_tree, page_start,
-			  page_end - 1, EXTENT_DIRTY | EXTENT_DELALLOC |
-			  EXTENT_DO_ACCOUNTING, 0, 0, &cached_state,
-			  GFP_NOFS);
+	ret = clear_extent_bit(&BTRFS_I(inode)->io_tree, page_start,
+			       page_end - 1, EXTENT_DIRTY | EXTENT_DELALLOC |
+			       EXTENT_DO_ACCOUNTING, 0, 0, &cached_state,
+			       GFP_NOFS);
+	BUG_ON(ret < 0);
 
 	if (i_done != num_pages) {
 		spin_lock(&BTRFS_I(inode)->lock);
fs/btrfs/relocation.c +3 -2
@@ -3834,8 +3834,9 @@ restart:
 	}
 
 	btrfs_release_path(path);
-	clear_extent_bits(&rc->processed_blocks, 0, (u64)-1, EXTENT_DIRTY,
-			  GFP_NOFS);
+	ret = clear_extent_bits(&rc->processed_blocks, 0, (u64)-1,
+				EXTENT_DIRTY, GFP_NOFS);
+	BUG_ON(ret < 0);
 
 	if (trans) {
 		nr = trans->blocks_used;
fs/btrfs/transaction.c +3 -1
@@ -636,7 +636,9 @@ int btrfs_wait_marked_extents(struct btr
 		if (ret)
 			break;
 
-		clear_extent_bits(dirty_pages, start, end, mark, GFP_NOFS);
+		ret = clear_extent_bits(dirty_pages, start, end,
+					mark, GFP_NOFS);
+		BUG_ON(ret < 0);
 		while (start <= end) {
 			index = start >> PAGE_CACHE_SHIFT;
 			start = (u64)(index + 1) << PAGE_CACHE_SHIFT;
fs/btrfs/tree-log.c +3 -2
@@ -2158,8 +2158,9 @@ static void free_log_tree(struct btrfs_t
 		if (ret)
 			break;
 
-		clear_extent_bits(&log->dirty_log_pages, start, end,
-				  EXTENT_DIRTY | EXTENT_NEW, GFP_NOFS);
+		ret = clear_extent_bits(&log->dirty_log_pages, start, end,
+					EXTENT_DIRTY | EXTENT_NEW, GFP_NOFS);
+		BUG_ON(ret < 0);
 	}
 
 	free_extent_buffer(log->node);
author
#7 lore raw

PATCH 6/9 btrfs: Push up unlock_extent errors to callers

In reply to Jeff Mahoney

The previous patch pushed the clear_extent_bit error handling up a level, which included unlock_extent and unlock_extent_cache.

This patch pushes the BUG_ON up into the callers of those functions.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/compression.c      |    6 +-
 fs/btrfs/disk-io.c          |    7 +--
 fs/btrfs/extent_io.c        |   52 +++++++++++++----------
 fs/btrfs/file.c             |   35 ++++++++-------
 fs/btrfs/free-space-cache.c |   15 ++++--
 fs/btrfs/inode.c            |   98 ++++++++++++++++++++++++++------------------
 fs/btrfs/ioctl.c            |   26 +++++++----
 fs/btrfs/relocation.c       |   24 +++++++---
 8 files changed, 159 insertions(+), 104 deletions(-)
fs/btrfs/compression.c +4 -2
@@ -507,7 +507,8 @@ static noinline int add_ra_bio_pages(str
 		    (last_offset + PAGE_CACHE_SIZE > extent_map_end(em)) ||
 		    (em->block_start >> 9) != cb->orig_bio->bi_sector) {
 			free_extent_map(em);
-			unlock_extent(tree, last_offset, end, GFP_NOFS);
+			ret = unlock_extent(tree, last_offset, end, GFP_NOFS);
+			BUG_ON(ret < 0);
 			unlock_page(page);
 			page_cache_release(page);
 			break;

@@ -535,7 +536,8 @@ static noinline int add_ra_bio_pages(str
 			nr_pages++;
 			page_cache_release(page);
 		} else {
-			unlock_extent(tree, last_offset, end, GFP_NOFS);
+			ret = unlock_extent(tree, last_offset, end, GFP_NOFS);
+			BUG_ON(ret < 0);
 			unlock_page(page);
 			page_cache_release(page);
 			break;
fs/btrfs/disk-io.c +4 -3
@@ -326,7 +326,7 @@ static int verify_parent_transid(struct
 				 struct extent_buffer *eb, u64 parent_transid)
 {
 	struct extent_state *cached_state = NULL;
-	int ret;
+	int ret, err;
 
 	if (!parent_transid || btrfs_header_generation(eb) == parent_transid)
 		return 0;

@@ -347,8 +347,9 @@ static int verify_parent_transid(struct
 	ret = 1;
 	clear_extent_buffer_uptodate(io_tree, eb, &cached_state);
 out:
-	unlock_extent_cached(io_tree, eb->start, eb->start + eb->len - 1,
-			     &cached_state, GFP_NOFS);
+	err = unlock_extent_cached(io_tree, eb->start, eb->start + eb->len - 1,
+				   &cached_state, GFP_NOFS);
+	BUG_ON(err < 0);
 	return ret;
 }
fs/btrfs/extent_io.c +30 -22

122 lines not on this page · raw message

fs/btrfs/file.c +20 -15
@@ -1114,9 +1114,10 @@ again:
 		    ordered->file_offset + ordered->len > start_pos &&
 		    ordered->file_offset < last_pos) {
 			btrfs_put_ordered_extent(ordered);
-			unlock_extent_cached(&BTRFS_I(inode)->io_tree,
-					     start_pos, last_pos - 1,
-					     &cached_state, GFP_NOFS);
+			err = unlock_extent_cached(&BTRFS_I(inode)->io_tree,
+						   start_pos, last_pos - 1,
+						   &cached_state, GFP_NOFS);
+			BUG_ON(err < 0);
 			for (i = 0; i < num_pages; i++) {
 				unlock_page(pages[i]);
 				page_cache_release(pages[i]);

@@ -1134,9 +1135,10 @@ again:
 				      EXTENT_DO_ACCOUNTING, 0, 0,
 				      &cached_state, GFP_NOFS);
 		BUG_ON(err < 0);
-		unlock_extent_cached(&BTRFS_I(inode)->io_tree,
-				     start_pos, last_pos - 1, &cached_state,
-				     GFP_NOFS);
+		err = unlock_extent_cached(&BTRFS_I(inode)->io_tree,
+					   start_pos, last_pos - 1,
+					   &cached_state, GFP_NOFS);
+		BUG_ON(err < 0);
 	}
 	for (i = 0; i < num_pages; i++) {
 		clear_page_dirty_for_io(pages[i]);

@@ -1577,7 +1579,7 @@ static long btrfs_fallocate(struct file
 	u64 locked_end;
 	u64 mask = BTRFS_I(inode)->root->sectorsize - 1;
 	struct extent_map *em;
-	int ret;
+	int ret, err;
 
 	alloc_start = offset & ~mask;
 	alloc_end =  (offset + len + mask) & ~mask;

@@ -1624,9 +1626,10 @@ static long btrfs_fallocate(struct file
 		    ordered->file_offset + ordered->len > alloc_start &&
 		    ordered->file_offset < alloc_end) {
 			btrfs_put_ordered_extent(ordered);
-			unlock_extent_cached(&BTRFS_I(inode)->io_tree,
-					     alloc_start, locked_end,
-					     &cached_state, GFP_NOFS);
+			ret = unlock_extent_cached(&BTRFS_I(inode)->io_tree,
+						   alloc_start, locked_end,
+						   &cached_state, GFP_NOFS);
+			BUG_ON(ret < 0);
 			/*
 			 * we can't wait on the range with the transaction
 			 * running or with the extent lock held

@@ -1668,8 +1671,9 @@ static long btrfs_fallocate(struct file
 			break;
 		}
 	}
-	unlock_extent_cached(&BTRFS_I(inode)->io_tree, alloc_start, locked_end,
-			     &cached_state, GFP_NOFS);
+	err = unlock_extent_cached(&BTRFS_I(inode)->io_tree, alloc_start,
+				   locked_end, &cached_state, GFP_NOFS);
+	BUG_ON(err < 0);
 
 	btrfs_free_reserved_data_space(inode, alloc_end - alloc_start);
 out:

@@ -1688,7 +1692,7 @@ static int find_desired_extent(struct in
 	u64 orig_start = *offset;
 	u64 len = i_size_read(inode);
 	u64 last_end = 0;
-	int ret = 0;
+	int ret = 0, err;
 
 	lockend = max_t(u64, root->sectorsize, lockend);
 	if (lockend <= lockstart)

@@ -1784,8 +1788,9 @@ static int find_desired_extent(struct in
 	if (!ret)
 		*offset = min(*offset, inode->i_size);
 out:
-	unlock_extent_cached(&BTRFS_I(inode)->io_tree, lockstart, lockend,
-			     &cached_state, GFP_NOFS);
+	err = unlock_extent_cached(&BTRFS_I(inode)->io_tree, lockstart, lockend,
+				   &cached_state, GFP_NOFS);
+	BUG_ON(err < 0);
 	return ret;
 }
fs/btrfs/free-space-cache.c +9 -6
@@ -549,7 +549,7 @@ int __btrfs_write_out_cache(struct btrfs
 	int index = 0, num_pages = 0;
 	int entries = 0;
 	int bitmaps = 0;
-	int ret = -1;
+	int ret = -1, err;
 	bool next_page = false;
 	bool out_of_space = false;
 

@@ -760,9 +760,10 @@ int __btrfs_write_out_cache(struct btrfs
 
 	if (out_of_space) {
 		btrfs_drop_pages(pages, num_pages);
-		unlock_extent_cached(&BTRFS_I(inode)->io_tree, 0,
-				     i_size_read(inode) - 1, &cached_state,
-				     GFP_NOFS);
+		err = unlock_extent_cached(&BTRFS_I(inode)->io_tree, 0,
+					   i_size_read(inode) - 1,
+					   &cached_state, GFP_NOFS);
+		BUG_ON(err < 0);
 		ret = 0;
 		goto out;
 	}

@@ -782,8 +783,10 @@ int __btrfs_write_out_cache(struct btrfs
 	ret = btrfs_dirty_pages(root, inode, pages, num_pages, 0,
 					    bytes, &cached_state);
 	btrfs_drop_pages(pages, num_pages);
-	unlock_extent_cached(&BTRFS_I(inode)->io_tree, 0,
-			     i_size_read(inode) - 1, &cached_state, GFP_NOFS);
+	err = unlock_extent_cached(&BTRFS_I(inode)->io_tree, 0,
+				   i_size_read(inode) - 1,
+				   &cached_state, GFP_NOFS);
+	BUG_ON(err < 0);
 
 	if (ret) {
 		ret = 0;
fs/btrfs/inode.c +60 -38

224 lines not on this page · raw message

fs/btrfs/ioctl.c +16 -10
@@ -781,7 +781,8 @@ static int should_defrag_range(struct in
 		err = lock_extent(io_tree, start, start + len - 1, GFP_NOFS);
 		BUG_ON(err < 0);
 		em = btrfs_get_extent(inode, NULL, 0, start, len, 0);
-		unlock_extent(io_tree, start, start + len - 1, GFP_NOFS);
+		err = unlock_extent(io_tree, start, start + len - 1, GFP_NOFS);
+		BUG_ON(err < 0);
 
 		if (IS_ERR(em))
 			return 0;

@@ -912,9 +913,10 @@ again:
 	    ordered->file_offset + ordered->len > page_start &&
 	    ordered->file_offset < page_end) {
 		btrfs_put_ordered_extent(ordered);
-		unlock_extent_cached(&BTRFS_I(inode)->io_tree,
-				     page_start, page_end - 1,
-				     &cached_state, GFP_NOFS);
+		ret = unlock_extent_cached(&BTRFS_I(inode)->io_tree,
+					   page_start, page_end - 1,
+					   &cached_state, GFP_NOFS);
+		BUG_ON(ret < 0);
 		for (i = 0; i < i_done; i++) {
 			unlock_page(pages[i]);
 			page_cache_release(pages[i]);

@@ -945,9 +947,10 @@ again:
 					&cached_state);
 	BUG_ON(ret);
 
-	unlock_extent_cached(&BTRFS_I(inode)->io_tree,
-			     page_start, page_end - 1, &cached_state,
-			     GFP_NOFS);
+	ret = unlock_extent_cached(&BTRFS_I(inode)->io_tree,
+				   page_start, page_end - 1, &cached_state,
+				   GFP_NOFS);
+	BUG_ON(ret < 0);
 
 	for (i = 0; i < i_done; i++) {
 		clear_page_dirty_for_io(pages[i]);

@@ -2139,7 +2142,7 @@ static noinline long btrfs_ioctl_clone(s
 	struct btrfs_key key;
 	u32 nritems;
 	int slot;
-	int ret;
+	int ret, err;
 	u64 len = olen;
 	u64 bs = root->fs_info->sb->s_blocksize;
 	u64 hint_byte;

@@ -2236,7 +2239,9 @@ static noinline long btrfs_ioctl_clone(s
 		    !test_range_bit(&BTRFS_I(src)->io_tree, off, off+len,
 				   EXTENT_DELALLOC, 0, NULL))
 			break;
-		unlock_extent(&BTRFS_I(src)->io_tree, off, off+len, GFP_NOFS);
+		ret = unlock_extent(&BTRFS_I(src)->io_tree, off,
+				    off+len, GFP_NOFS);
+		BUG_ON(ret < 0);
 		if (ordered)
 			btrfs_put_ordered_extent(ordered);
 		btrfs_wait_ordered_range(src, off, len);

@@ -2443,7 +2448,8 @@ next:
 	ret = 0;
 out:
 	btrfs_release_path(path);
-	unlock_extent(&BTRFS_I(src)->io_tree, off, off+len, GFP_NOFS);
+	err = unlock_extent(&BTRFS_I(src)->io_tree, off, off+len, GFP_NOFS);
+	BUG_ON(err < 0);
 out_unlock:
 	mutex_unlock(&src->i_mutex);
 	mutex_unlock(&inode->i_mutex);
fs/btrfs/relocation.c +16 -8
@@ -1583,8 +1583,9 @@ int replace_file_extents(struct btrfs_tr
 
 				btrfs_drop_extent_cache(inode, key.offset, end,
 							1);
-				unlock_extent(&BTRFS_I(inode)->io_tree,
-					      key.offset, end, GFP_NOFS);
+				ret = unlock_extent(&BTRFS_I(inode)->io_tree,
+						    key.offset, end, GFP_NOFS);
+				BUG_ON(ret < 0);
 			}
 		}
 

@@ -1958,7 +1959,9 @@ static int invalidate_extent_cache(struc
 				  GFP_NOFS);
 		BUG_ON(ret < 0);
 		btrfs_drop_extent_cache(inode, start, end, 1);
-		unlock_extent(&BTRFS_I(inode)->io_tree, start, end, GFP_NOFS);
+		ret = unlock_extent(&BTRFS_I(inode)->io_tree, start, end,
+				    GFP_NOFS);
+		BUG_ON(ret < 0);
 	}
 	return 0;
 }

@@ -2860,6 +2863,7 @@ int prealloc_file_extent_cluster(struct
 		goto out;
 
 	while (nr < cluster->nr) {
+		int err;
 		start = cluster->boundary[nr] - offset;
 		if (nr + 1 < cluster->nr)
 			end = cluster->boundary[nr + 1] - 1 - offset;

@@ -2873,7 +2877,9 @@ int prealloc_file_extent_cluster(struct
 		ret = btrfs_prealloc_file_range(inode, 0, start,
 						num_bytes, num_bytes,
 						end + 1, &alloc_hint);
-		unlock_extent(&BTRFS_I(inode)->io_tree, start, end, GFP_NOFS);
+		err = unlock_extent(&BTRFS_I(inode)->io_tree, start, end,
+				    GFP_NOFS);
+		BUG_ON(err < 0);
 		if (ret)
 			break;
 		nr++;

@@ -2892,7 +2898,7 @@ int setup_extent_mapping(struct inode *i
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	struct extent_map_tree *em_tree = &BTRFS_I(inode)->extent_tree;
 	struct extent_map *em;
-	int ret = 0;
+	int ret = 0, err;
 
 	em = alloc_extent_map();
 	if (!em)

@@ -2917,7 +2923,8 @@ int setup_extent_mapping(struct inode *i
 		}
 		btrfs_drop_extent_cache(inode, start, end, 0);
 	}
-	unlock_extent(&BTRFS_I(inode)->io_tree, start, end, GFP_NOFS);
+	err = unlock_extent(&BTRFS_I(inode)->io_tree, start, end, GFP_NOFS);
+	BUG_ON(err < 0);
 	return ret;
 }
 

@@ -3016,8 +3023,9 @@ static int relocate_file_extent_cluster(
 		BUG_ON(ret);
 		set_page_dirty(page);
 
-		unlock_extent(&BTRFS_I(inode)->io_tree,
-			      page_start, page_end, GFP_NOFS);
+		ret = unlock_extent(&BTRFS_I(inode)->io_tree,
+				    page_start, page_end, GFP_NOFS);
+		BUG_ON(ret < 0);
 		unlock_page(page);
 		page_cache_release(page);
author
#8 lore raw

PATCH 7/9 btrfs: Make pin_down_extent return void

In reply to Jeff Mahoney

pin_down_extent performs some operations which can't fail and then calls set_extent_dirty, which has two failure cases via set_extent_bit:
1) Return -EEXIST if exclusive bits are set
- Since it doesn't use any exclusive bits, this failure case can't occur.
2) Return -ENOMEM if memory can't be allocated
- Since it's called with gfp_flags & __GFP_NOFAIL, this failure case can't occur.

With no failure cases, it can return void.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/extent-tree.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
fs/btrfs/extent-tree.c +3 -4
@@ -4175,9 +4175,9 @@ static u64 first_logical_byte(struct btr
 	return bytenr;
 }
 
-static int pin_down_extent(struct btrfs_root *root,
-			   struct btrfs_block_group_cache *cache,
-			   u64 bytenr, u64 num_bytes, int reserved)
+static void pin_down_extent(struct btrfs_root *root,
+			    struct btrfs_block_group_cache *cache,
+			    u64 bytenr, u64 num_bytes, int reserved)
 {
 	spin_lock(&cache->space_info->lock);
 	spin_lock(&cache->lock);

@@ -4194,7 +4194,6 @@ static int pin_down_extent(struct btrfs_
 	/* __GFP_NOFAIL means it can't return -ENOMEM */
 	set_extent_dirty(root->fs_info->pinned_extents, bytenr,
 			 bytenr + num_bytes - 1, GFP_NOFS | __GFP_NOFAIL);
-	return 0;
 }
 
 /*
author
#9 lore raw

PATCH 8/9 btrfs: Push up btrfs_pin_extent failures

In reply to Jeff Mahoney

btrfs_pin_extent looks up a block group and then calls pin_down_extent with it. If the lookup fails, it should return -ENOENT to allow callers to handle the error condition. For the three existing callers, it is a logic error if the lookup fails and a panic will occur.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/extent-tree.c |   20 +++++++++++++++-----
 fs/btrfs/tree-log.c    |   10 +++++++---
 2 files changed, 22 insertions(+), 8 deletions(-)
fs/btrfs/extent-tree.c +15 -5
@@ -2075,8 +2075,14 @@ static int run_one_delayed_ref(struct bt
 		BUG_ON(extent_op);
 		head = btrfs_delayed_node_to_head(node);
 		if (insert_reserved) {
-			btrfs_pin_extent(root, node->bytenr,
-					 node->num_bytes, 1);
+			ret = btrfs_pin_extent(root, node->bytenr,
+					       node->num_bytes, 1);
+			if (ret)
+				btrfs_panic(root->fs_info, ret,
+					    "Cannot pin extent in range "
+					    "%llu(%llu)\n",
+					    node->bytenr, node->num_bytes);
+
 			if (head->is_data) {
 				ret = btrfs_del_csums(trans, root,
 						      node->bytenr,

@@ -4205,7 +4211,8 @@ int btrfs_pin_extent(struct btrfs_root *
 	struct btrfs_block_group_cache *cache;
 
 	cache = btrfs_lookup_block_group(root->fs_info, bytenr);
-	BUG_ON(!cache);
+	if (cache == NULL)
+		return -ENOENT;
 
 	pin_down_extent(root, cache, bytenr, num_bytes, reserved);
 

@@ -4765,8 +4772,11 @@ int btrfs_free_extent(struct btrfs_trans
 	if (root_objectid == BTRFS_TREE_LOG_OBJECTID) {
 		WARN_ON(owner >= BTRFS_FIRST_FREE_OBJECTID);
 		/* unlocks the pinned mutex */
-		btrfs_pin_extent(root, bytenr, num_bytes, 1);
-		ret = 0;
+		ret = btrfs_pin_extent(root, bytenr, num_bytes, 1);
+		if (ret)
+			btrfs_panic(root->fs_info, ret, "Cannot pin "
+				    "extent in range %llu(%llu)\n",
+				    bytenr, num_bytes);
 	} else if (owner < BTRFS_FIRST_FREE_OBJECTID) {
 		ret = btrfs_add_delayed_tree_ref(trans, bytenr, num_bytes,
 					parent, root_objectid, (int)owner,
fs/btrfs/tree-log.c +7 -3
@@ -275,9 +275,13 @@ static int process_one_buffer(struct btr
 			      struct extent_buffer *eb,
 			      struct walk_control *wc, u64 gen)
 {
-	if (wc->pin)
-		btrfs_pin_extent(log->fs_info->extent_root,
-				 eb->start, eb->len, 0);
+	if (wc->pin) {
+		int ret = btrfs_pin_extent(log->fs_info->extent_root,
+					   eb->start, eb->len, 0);
+		if (ret)
+			btrfs_panic(log->fs_info, ret, "Cannot pin extent in "
+				    "range %llu(%llu)\n", eb->start, eb->len);
+	}
 
 	if (btrfs_buffer_uptodate(eb, gen)) {
 		if (wc->write)
author
#10 lore raw

PATCH 9/9 btrfs: Push up non-looped btrfs_start_transaction failures

In reply to Jeff Mahoney

This patch handles btrfs_start_transaction failures that don't occur in a loop and are obvious to simply push up. In all cases except the mark_garbage_root case, the error is already handled by BUG_ON in the caller.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/extent-tree.c |    6 +++++-
 fs/btrfs/relocation.c  |    6 ++++--
 fs/btrfs/tree-log.c    |    5 ++++-
 fs/btrfs/volumes.c     |    3 ++-
 4 files changed, 15 insertions(+), 5 deletions(-)
fs/btrfs/extent-tree.c +5 -1
@@ -6319,7 +6319,11 @@ int btrfs_drop_snapshot(struct btrfs_roo
 	}
 
 	trans = btrfs_start_transaction(tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		kfree(wc);
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}
 
 	if (block_rsv)
 		trans->block_rsv = block_rsv;
fs/btrfs/relocation.c +4 -2
@@ -4096,7 +4096,8 @@ static noinline_for_stack int mark_garba
 	int ret;
 
 	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
 	memset(&root->root_item.drop_progress, 0,
 		sizeof(root->root_item.drop_progress));

@@ -4176,7 +4177,8 @@ int btrfs_recover_relocation(struct btrf
 					err = ret;
 					goto out;
 				}
-				mark_garbage_root(reloc_root);
+				ret = mark_garbage_root(reloc_root);
+				BUG_ON(ret);
 			}
 		}
fs/btrfs/tree-log.c +4 -1
@@ -3151,7 +3151,10 @@ int btrfs_recover_log_trees(struct btrfs
 	fs_info->log_root_recovering = 1;
 
 	trans = btrfs_start_transaction(fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}
 
 	wc.trans = trans;
 	wc.pin = 1;
fs/btrfs/volumes.c +2 -1
@@ -1876,7 +1876,8 @@ static int btrfs_relocate_chunk(struct b
 		return ret;
 
 	trans = btrfs_start_transaction(root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
 	lock_chunks(root);
#11 lore raw

Re: [patch 9/9] btrfs: Push up non-looped btrfs_start_transaction failures

Hi, Jeff,

Quoted 16 lines · Jeff Mahoney

This patch handles btrfs_start_transaction failures that don't occur in a loop and are obvious to simply push up. In all cases except the mark_garbage_root case, the error is already handled by BUG_ON in the caller.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/extent-tree.c |    6 +++++-
 fs/btrfs/relocation.c  |    6 ++++--
 fs/btrfs/tree-log.c    |    5 ++++-
 fs/btrfs/volumes.c     |    3 ++-
 4 files changed, 15 insertions(+), 5 deletions(-)

fs/btrfs/extent-tree.c int btrfs_drop_snapshot(struct btrfs_roo · line 6326

Quoted 9 lines
fs/btrfs/extent-tree.c +5 -1
@@ -6319,7 +6319,11 @@ int btrfs_drop_snapshot(struct btrfs_roo
 	}
 
 	trans = btrfs_start_transaction(tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		kfree(wc);
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}

The caller of btrfs_drop_snapshot() ignore the error. So, I don't think that it is significant even if the error is returned to the caller.

I think that it should make the filesystem readonly when becoming an error in btrfs_start_transaction().

Thanks,
Tsutomu

Quoted 51 lines
fs/btrfs/extent-tree.c +0 -0
 	if (block_rsv)
 		trans->block_rsv = block_rsv;
fs/btrfs/relocation.c +4 -2
@@ -4096,7 +4096,8 @@ static noinline_for_stack int mark_garba
 	int ret;
 
 	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
 	memset(&root->root_item.drop_progress, 0,
 		sizeof(root->root_item.drop_progress));

@@ -4176,7 +4177,8 @@ int btrfs_recover_relocation(struct btrf
 					err = ret;
 					goto out;
 				}
-				mark_garbage_root(reloc_root);
+				ret = mark_garbage_root(reloc_root);
+				BUG_ON(ret);
 			}
 		}
fs/btrfs/tree-log.c +4 -1
@@ -3151,7 +3151,10 @@ int btrfs_recover_log_trees(struct btrfs
 	fs_info->log_root_recovering = 1;
 
 	trans = btrfs_start_transaction(fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}
 
 	wc.trans = trans;
 	wc.pin = 1;
fs/btrfs/volumes.c +2 -1
@@ -1876,7 +1876,8 @@ static int btrfs_relocate_chunk(struct b
 		return ret;
 
 	trans = btrfs_start_transaction(root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
 	lock_chunks(root);
author
#12 lore raw

Re: [patch 9/9] btrfs: Push up non-looped btrfs_start_transaction failures

Quoted 21 lines · Tsutomu Itoh

Hi, Jeff,

Quoted 18 lines · Jeff Mahoney

This patch handles btrfs_start_transaction failures that don't occur in a loop and are obvious to simply push up. In all cases except the mark_garbage_root case, the error is already handled by BUG_ON in the caller.

Signed-off-by Jeff Mahoney<[email protected]>
---
  fs/btrfs/extent-tree.c |    6 +++++-
  fs/btrfs/relocation.c  |    6 ++++--
  fs/btrfs/tree-log.c    |    5 ++++-
  fs/btrfs/volumes.c     |    3 ++-
  4 files changed, 15 insertions(+), 5 deletions(-)
fs/btrfs/extent-tree.c +0 -0
@@ -6319,7 +6319,11 @@ int btrfs_drop_snapshot(struct btrfs_roo
  	}
Quoted 10 lines

fs/btrfs/extent-tree.c int btrfs_drop_snapshot(struct btrfs_roo · line 6326

Quoted 7 lines
fs/btrfs/extent-tree.c +5 -1
  	trans = btrfs_start_transaction(tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		kfree(wc);
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}

The caller of btrfs_drop_snapshot() ignore the error. So, I don't think that it is significant even if the error is returned to the caller.

I'd actually consider that a separate issue since btrfs_drop_snapshot
also returns -ENOMEM. The errors should be properly caught or BUG_ON'd in the caller. My patchset usually catches cases like this but since btrfs_drop_snapshot already returned an error, I mistakenly assumed it was handled by the caller.

Quoted 2 lines

I think that it should make the filesystem readonly when becoming an error in btrfs_start_transaction().

For -ENOMEM, I don't think that's the way to handle it. Some transaction start failures can be caught and handled (e.g. just creating a file) easily by returning errors to the user. Other cases, deep in the code, may be too complex to unwind and recover from and then a ROFS is the next-best answer. The callers should be responsible for determining the correct course of action.

-Jeff

Quoted 55 lines

Thanks,
Tsutomu

Quoted 51 lines
fs/btrfs/extent-tree.c +0 -0
  	if (block_rsv)
  		trans->block_rsv = block_rsv;
fs/btrfs/relocation.c +4 -2
@@ -4096,7 +4096,8 @@ static noinline_for_stack int mark_garba
  	int ret;
 
  	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
  	memset(&root->root_item.drop_progress, 0,
  		sizeof(root->root_item.drop_progress));

@@ -4176,7 +4177,8 @@ int btrfs_recover_relocation(struct btrf
  					err = ret;
  					goto out;
  				}
-				mark_garbage_root(reloc_root);
+				ret = mark_garbage_root(reloc_root);
+				BUG_ON(ret);
  			}
  		}
fs/btrfs/tree-log.c +4 -1
@@ -3151,7 +3151,10 @@ int btrfs_recover_log_trees(struct btrfs
  	fs_info->log_root_recovering = 1;
 
  	trans = btrfs_start_transaction(fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}
 
  	wc.trans = trans;
  	wc.pin = 1;
fs/btrfs/volumes.c +2 -1
@@ -1876,7 +1876,8 @@ static int btrfs_relocate_chunk(struct b
  		return ret;
 
  	trans = btrfs_start_transaction(root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
  	lock_chunks(root);
Signature · 2 lines
Jeff Mahoney
SuSE Labs
#13 lore raw

Re: [patch 9/9] btrfs: Push up non-looped btrfs_start_transaction failures

Quoted 42 lines · Jeff Mahoney
Quoted 21 lines · Tsutomu Itoh

Hi, Jeff,

Quoted 18 lines · Jeff Mahoney

This patch handles btrfs_start_transaction failures that don't occur in a loop and are obvious to simply push up. In all cases except the mark_garbage_root case, the error is already handled by BUG_ON in the caller.

Signed-off-by Jeff Mahoney<[email protected]>
---
  fs/btrfs/extent-tree.c |    6 +++++-
  fs/btrfs/relocation.c  |    6 ++++--
  fs/btrfs/tree-log.c    |    5 ++++-
  fs/btrfs/volumes.c     |    3 ++-
  4 files changed, 15 insertions(+), 5 deletions(-)
fs/btrfs/extent-tree.c +0 -0
@@ -6319,7 +6319,11 @@ int btrfs_drop_snapshot(struct btrfs_roo
  	}
Quoted 10 lines

fs/btrfs/extent-tree.c int btrfs_drop_snapshot(struct btrfs_roo · line 6326

Quoted 7 lines
fs/btrfs/extent-tree.c +5 -1
  	trans = btrfs_start_transaction(tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		kfree(wc);
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}

The caller of btrfs_drop_snapshot() ignore the error. So, I don't think that it is significant even if the error is returned to the caller.

I'd actually consider that a separate issue since btrfs_drop_snapshot
also returns -ENOMEM. The errors should be properly caught or BUG_ON'd in the caller. My patchset usually catches cases like this but since btrfs_drop_snapshot already returned an error, I mistakenly assumed it was handled by the caller.

Quoted 2 lines

I think that it should make the filesystem readonly when becoming an error in btrfs_start_transaction().

Quoted 6 lines

For -ENOMEM, I don't think that's the way to handle it. Some transaction start failures can be caught and handled (e.g. just creating a file) easily by returning errors to the user. Other cases, deep in the code, may be too complex to unwind and recover from and then a ROFS is the next-best answer. The callers should be responsible for determining the correct course of action.

OK.
Could you please append BUG_ON() in the caller or correctly handle the error of btrfs_start_transaction()?

-Tsutomu

Quoted 58 lines

-Jeff

Quoted 55 lines

Thanks,
Tsutomu

Quoted 51 lines
fs/btrfs/extent-tree.c +0 -0
  	if (block_rsv)
  		trans->block_rsv = block_rsv;
fs/btrfs/relocation.c +4 -2
@@ -4096,7 +4096,8 @@ static noinline_for_stack int mark_garba
  	int ret;
 
  	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
  	memset(&root->root_item.drop_progress, 0,
  		sizeof(root->root_item.drop_progress));

@@ -4176,7 +4177,8 @@ int btrfs_recover_relocation(struct btrf
  					err = ret;
  					goto out;
  				}
-				mark_garbage_root(reloc_root);
+				ret = mark_garbage_root(reloc_root);
+				BUG_ON(ret);
  			}
  		}
fs/btrfs/tree-log.c +4 -1
@@ -3151,7 +3151,10 @@ int btrfs_recover_log_trees(struct btrfs
  	fs_info->log_root_recovering = 1;
 
  	trans = btrfs_start_transaction(fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}
 
  	wc.trans = trans;
  	wc.pin = 1;
fs/btrfs/volumes.c +2 -1
@@ -1876,7 +1876,8 @@ static int btrfs_relocate_chunk(struct b
  		return ret;
 
  	trans = btrfs_start_transaction(root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
  	lock_chunks(root);
author
#14 lore raw

Re: [patch 9/9] btrfs: Push up non-looped btrfs_start_transaction failures

Quoted 50 lines · Tsutomu Itoh
Quoted 42 lines · Jeff Mahoney
Quoted 21 lines · Tsutomu Itoh

Hi, Jeff,

Quoted 18 lines · Jeff Mahoney

This patch handles btrfs_start_transaction failures that don't occur in a loop and are obvious to simply push up. In all cases except the mark_garbage_root case, the error is already handled by BUG_ON in the caller.

Signed-off-by Jeff Mahoney<[email protected]>
---
   fs/btrfs/extent-tree.c |    6 +++++-
   fs/btrfs/relocation.c  |    6 ++++--
   fs/btrfs/tree-log.c    |    5 ++++-
   fs/btrfs/volumes.c     |    3 ++-
   4 files changed, 15 insertions(+), 5 deletions(-)
fs/btrfs/extent-tree.c +0 -0
@@ -6319,7 +6319,11 @@ int btrfs_drop_snapshot(struct btrfs_roo
   	}
Quoted 10 lines

fs/btrfs/extent-tree.c int btrfs_drop_snapshot(struct btrfs_roo · line 6326

Quoted 7 lines
fs/btrfs/extent-tree.c +5 -1
   	trans = btrfs_start_transaction(tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		kfree(wc);
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}

The caller of btrfs_drop_snapshot() ignore the error. So, I don't think that it is significant even if the error is returned to the caller.

I'd actually consider that a separate issue since btrfs_drop_snapshot
also returns -ENOMEM. The errors should be properly caught or BUG_ON'd in the caller. My patchset usually catches cases like this but since btrfs_drop_snapshot already returned an error, I mistakenly assumed it was handled by the caller.

Quoted 2 lines

I think that it should make the filesystem readonly when becoming an error in btrfs_start_transaction().

Quoted 6 lines

For -ENOMEM, I don't think that's the way to handle it. Some transaction start failures can be caught and handled (e.g. just creating a file) easily by returning errors to the user. Other cases, deep in the code, may be too complex to unwind and recover from and then a ROFS is the next-best answer. The callers should be responsible for determining the correct course of action.

Quoted 3 lines

OK.
Could you please append BUG_ON() in the caller or correctly handle the error of btrfs_start_transaction()?

Yep. I'll add that in. Thanks for the review.

-Jef

Quoted 58 lines
Quoted 57 lines

-Jeff

Quoted 55 lines

Thanks,
Tsutomu

Quoted 51 lines
fs/btrfs/extent-tree.c +0 -0
   	if (block_rsv)
   		trans->block_rsv = block_rsv;
fs/btrfs/relocation.c +4 -2
@@ -4096,7 +4096,8 @@ static noinline_for_stack int mark_garba
   	int ret;
 
   	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
   	memset(&root->root_item.drop_progress, 0,
   		sizeof(root->root_item.drop_progress));

@@ -4176,7 +4177,8 @@ int btrfs_recover_relocation(struct btrf
   					err = ret;
   					goto out;
   				}
-				mark_garbage_root(reloc_root);
+				ret = mark_garbage_root(reloc_root);
+				BUG_ON(ret);
   			}
   		}
fs/btrfs/tree-log.c +4 -1
@@ -3151,7 +3151,10 @@ int btrfs_recover_log_trees(struct btrfs
   	fs_info->log_root_recovering = 1;
 
   	trans = btrfs_start_transaction(fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}
 
   	wc.trans = trans;
   	wc.pin = 1;
fs/btrfs/volumes.c +2 -1
@@ -1876,7 +1876,8 @@ static int btrfs_relocate_chunk(struct b
   		return ret;
 
   	trans = btrfs_start_transaction(root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
   	lock_chunks(root);
Signature · 2 lines
Jeff Mahoney
SuSE Labs