thr3ads.net - Btrfs devel - [PATCH 0/20] btrfs: More error handling fixes [Sep 2011]

If this information is useful, please help other people find it:
Share via:

Mark Fasheh

2011-Sep-15 17:34 UTC

[PATCH 0/20] btrfs: More error handling fixes

Hi,

The following are assorted fixes to error handling from all parts of the
Btrfs code.  I included in this series an uncommited patch from Tsutomu Itoh
which was a better version of a patch I had written.  He should be cc''d
on
that mail.

Some of the patches (the first 8) were previously sent to this list but got
no comments so I''m resending them with this set. Changes from last time
are
that I also started setting the file system readonly on errors which look
like possible corruption.

For the most part, I''m still concentrating on eliminating sites where
we
BUG_ON(ret) instead of bubbling errors up the stack. The patches were
tested using some simple file system commands and a background kernel build.

Please review, all constructive feedback is appreciated :)

Thanks,
	--Mark

--
Mark Fasheh
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 01/20] btrfs: Don''t BUG_ON errors from btrfs_create_subvol_root()

From: Mark Fasheh <mfasheh@suse.com>

This is called from only one place - create_subvol() which passes errors
safely back out to it''s caller, btrfs_mksubvol where they are handled.

Additionally, btrfs_create_subvol_root() itself bug''s needlessly from
error
return of btrfs_update_inode(). Since create_subvol() was fixed to catch
errors we can bubble this one up too.

Signed-off-by: Mark Fasheh <mfasheh@suse.com>
---
 fs/btrfs/inode.c |    3 +--
 fs/btrfs/ioctl.c |    2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 15fceef..7028c0c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6722,10 +6722,9 @@ int btrfs_create_subvol_root(struct btrfs_trans_handle
*trans,
 	btrfs_i_size_write(inode, 0);
 
 	err = btrfs_update_inode(trans, new_root, inode);
-	BUG_ON(err);
 
 	iput(inode);
-	return 0;
+	return err;
 }
 
 struct inode *btrfs_alloc_inode(struct super_block *sb)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7cf0133..b3440f5 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -411,6 +411,8 @@ static noinline int create_subvol(struct btrfs_root *root,
 	btrfs_record_root_in_trans(trans, new_root);
 
 	ret = btrfs_create_subvol_root(trans, new_root, new_dirid);
+	if (ret)
+		goto fail;
 	/*
 	 * insert the directory item
 	 */
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 02/20] btrfs: Don''t BUG_ON() errors in update_ref_for_cow()

From: Mark Fasheh <mfasheh@suse.com>

The only caller of update_ref_for_cow() is __btrfs_cow_block() which was
originally ignoring any return values. update_ref_for_cow() however
doesn''t
look like a candidate to become a void function - there are a few places
where errors can occur.

So instead I changed update_ref_for_cow() to bubble all errors up (instead
of BUG_ON). __btrfs_cow_block() was then updated to catch and BUG_ON() any
errors from update_ref_for_cow(). The end effect is that we have no change
in behavior, but about 8 different places where a BUG_ON(ret) was removed.

Obviously a future patch will have to address the BUG_ON() in
__btrfs_cow_block().

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/ctree.c |   31 +++++++++++++++++++++----------
 1 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 011cab3..5064930 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -331,7 +331,8 @@ static noinline int update_ref_for_cow(struct
btrfs_trans_handle *trans,
 	if (btrfs_block_can_be_shared(root, buf)) {
 		ret = btrfs_lookup_extent_info(trans, root, buf->start,
 					       buf->len, &refs, &flags);
-		BUG_ON(ret);
+		if (ret)
+			return ret;
 		BUG_ON(refs == 0);
 	} else {
 		refs = 1;
@@ -351,14 +352,18 @@ static noinline int update_ref_for_cow(struct
btrfs_trans_handle *trans,
 		     root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) &&
 		    !(flags & BTRFS_BLOCK_FLAG_FULL_BACKREF)) {
 			ret = btrfs_inc_ref(trans, root, buf, 1);
-			BUG_ON(ret);
+			if (ret)
+				return ret;
 
 			if (root->root_key.objectid = 			    BTRFS_TREE_RELOC_OBJECTID) {
 				ret = btrfs_dec_ref(trans, root, buf, 0);
-				BUG_ON(ret);
+				if (ret)
+					return ret;
+
 				ret = btrfs_inc_ref(trans, root, cow, 1);
-				BUG_ON(ret);
+				if (ret)
+					return ret;
 			}
 			new_flags |= BTRFS_BLOCK_FLAG_FULL_BACKREF;
 		} else {
@@ -368,14 +373,16 @@ static noinline int update_ref_for_cow(struct
btrfs_trans_handle *trans,
 				ret = btrfs_inc_ref(trans, root, cow, 1);
 			else
 				ret = btrfs_inc_ref(trans, root, cow, 0);
-			BUG_ON(ret);
+			if (ret)
+				return ret;
 		}
 		if (new_flags != 0) {
 			ret = btrfs_set_disk_extent_flags(trans, root,
 							  buf->start,
 							  buf->len,
 							  new_flags, 0);
-			BUG_ON(ret);
+			if (ret)
+				return ret;
 		}
 	} else {
 		if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
@@ -384,9 +391,12 @@ static noinline int update_ref_for_cow(struct
btrfs_trans_handle *trans,
 				ret = btrfs_inc_ref(trans, root, cow, 1);
 			else
 				ret = btrfs_inc_ref(trans, root, cow, 0);
-			BUG_ON(ret);
+			if (ret)
+				return ret;
+
 			ret = btrfs_dec_ref(trans, root, buf, 1);
-			BUG_ON(ret);
+			if (ret)
+				return ret;
 		}
 		clean_tree_block(trans, root, buf);
 		*last_ref = 1;
@@ -415,7 +425,7 @@ static noinline int __btrfs_cow_block(struct
btrfs_trans_handle *trans,
 {
 	struct btrfs_disk_key disk_key;
 	struct extent_buffer *cow;
-	int level;
+	int level, ret;
 	int last_ref = 0;
 	int unlock_orig = 0;
 	u64 parent_start;
@@ -467,7 +477,8 @@ static noinline int __btrfs_cow_block(struct
btrfs_trans_handle *trans,
 			    (unsigned long)btrfs_header_fsid(cow),
 			    BTRFS_FSID_SIZE);
 
-	update_ref_for_cow(trans, root, buf, cow, &last_ref);
+	ret = update_ref_for_cow(trans, root, buf, cow, &last_ref);
+	BUG_ON(ret);
 
 	if (root->ref_cows)
 		btrfs_reloc_cow_block(trans, root, buf, cow);
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 03/20] btrfs: Don''t BUG_ON kzalloc error in btrfs_lookup_csums_range()

From: Mark Fasheh <mfasheh@suse.com>

Unfortunately it isn''t enough to just exit here - the kzalloc() happens
in a
loop and the allocated items are added to a linked list whose head is passed
in from the caller.

To fix the BUG_ON() and also provide the semantic that the list passed in is
only modified on success, I create function-local temporary list that we add
items too. If no error is met, that list is spliced to the callers at the
end of the function. Otherwise the list will be walked and all items freed
before the error value is returned.

I did a simple test on this patch by forcing an error at the kzalloc() point
and verifying that when this hits (git clone seemed to exercise this), the
function throws the proper error. Unfortunately but predictably, we later
hit a BUG_ON(ret) type line that still hasn''t been fixed up ;)

Signed-off-by: Mark Fasheh <mfasheh@suse.com>
---
 fs/btrfs/file-item.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index b910694..679fbff 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -284,6 +284,7 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64
start, u64 end,
 	struct btrfs_ordered_sum *sums;
 	struct btrfs_sector_sum *sector_sum;
 	struct btrfs_csum_item *item;
+	LIST_HEAD(tmplist);
 	unsigned long offset;
 	int ret;
 	size_t size;
@@ -358,7 +359,10 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64
start, u64 end,
 					MAX_ORDERED_SUM_BYTES(root));
 			sums = kzalloc(btrfs_ordered_sum_size(root, size),
 					GFP_NOFS);
-			BUG_ON(!sums);
+			if (!sums) {
+				ret = -ENOMEM;
+				goto fail;
+			}
 
 			sector_sum = sums->sums;
 			sums->bytenr = start;
@@ -380,12 +384,19 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64
start, u64 end,
 				offset += csum_size;
 				sector_sum++;
 			}
-			list_add_tail(&sums->list, list);
+			list_add_tail(&sums->list, &tmplist);
 		}
 		path->slots[0]++;
 	}
 	ret = 0;
 fail:
+	while (ret < 0 && !list_empty(&tmplist)) {
+		sums = list_entry(&tmplist, struct btrfs_ordered_sum, list);
+		list_del(&sums->list);
+		kfree(sums);
+	}
+	list_splice_tail(&tmplist, list);
+
 	btrfs_free_path(path);
 	return ret;
 }
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 04/20] btrfs: make insert_ptr() void

From: Mark Fasheh <mfasheh@suse.com>

insert_ptr() always returns zero, so all the exta error handling can go
away.  This makes it trivial to also make copy_for_split() a void function
as it''s only return was from insert_ptr(). Finally, this all makes the
BUG_ON(ret) in split_leaf() meaningless so I removed that.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/ctree.c |   59 ++++++++++++++++-------------------------------------
 1 files changed, 18 insertions(+), 41 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 011cab3..41605ac 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2123,12 +2123,10 @@ static noinline int insert_new_root(struct
btrfs_trans_handle *trans,
  *
  * slot and level indicate where you want the key to go, and
  * blocknr is the block the key points to.
- *
- * returns zero on success and < 0 on any error
  */
-static int insert_ptr(struct btrfs_trans_handle *trans, struct btrfs_root
-		      *root, struct btrfs_path *path, struct btrfs_disk_key
-		      *key, u64 bytenr, int slot, int level)
+static void insert_ptr(struct btrfs_trans_handle *trans, struct btrfs_root
+		       *root, struct btrfs_path *path, struct btrfs_disk_key
+		       *key, u64 bytenr, int slot, int level)
 {
 	struct extent_buffer *lower;
 	int nritems;
@@ -2152,7 +2150,6 @@ static int insert_ptr(struct btrfs_trans_handle *trans,
struct btrfs_root
 	btrfs_set_node_ptr_generation(lower, slot, trans->transid);
 	btrfs_set_header_nritems(lower, nritems + 1);
 	btrfs_mark_buffer_dirty(lower);
-	return 0;
 }
 
 /*
@@ -2173,7 +2170,6 @@ static noinline int split_node(struct btrfs_trans_handle
*trans,
 	struct btrfs_disk_key disk_key;
 	int mid;
 	int ret;
-	int wret;
 	u32 c_nritems;
 
 	c = path->nodes[level];
@@ -2230,11 +2226,8 @@ static noinline int split_node(struct btrfs_trans_handle
*trans,
 	btrfs_mark_buffer_dirty(c);
 	btrfs_mark_buffer_dirty(split);
 
-	wret = insert_ptr(trans, root, path, &disk_key, split->start,
-			  path->slots[level + 1] + 1,
-			  level + 1);
-	if (wret)
-		ret = wret;
+	insert_ptr(trans, root, path, &disk_key, split->start,
+		   path->slots[level + 1] + 1, level + 1);
 
 	if (path->slots[level] >= mid) {
 		path->slots[level] -= mid;
@@ -2724,18 +2717,16 @@ out:
  *
  * returns 0 if all went well and < 0 on failure.
  */
-static noinline int copy_for_split(struct btrfs_trans_handle *trans,
-			       struct btrfs_root *root,
-			       struct btrfs_path *path,
-			       struct extent_buffer *l,
-			       struct extent_buffer *right,
-			       int slot, int mid, int nritems)
+static noinline void copy_for_split(struct btrfs_trans_handle *trans,
+				    struct btrfs_root *root,
+				    struct btrfs_path *path,
+				    struct extent_buffer *l,
+				    struct extent_buffer *right,
+				    int slot, int mid, int nritems)
 {
 	int data_copy_size;
 	int rt_data_off;
 	int i;
-	int ret = 0;
-	int wret;
 	struct btrfs_disk_key disk_key;
 
 	nritems = nritems - mid;
@@ -2763,12 +2754,9 @@ static noinline int copy_for_split(struct
btrfs_trans_handle *trans,
 	}
 
 	btrfs_set_header_nritems(l, mid);
-	ret = 0;
 	btrfs_item_key(right, &disk_key, 0);
-	wret = insert_ptr(trans, root, path, &disk_key, right->start,
-			  path->slots[1] + 1, 1);
-	if (wret)
-		ret = wret;
+	insert_ptr(trans, root, path, &disk_key, right->start,
+		   path->slots[1] + 1, 1);
 
 	btrfs_mark_buffer_dirty(right);
 	btrfs_mark_buffer_dirty(l);
@@ -2786,8 +2774,6 @@ static noinline int copy_for_split(struct
btrfs_trans_handle *trans,
 	}
 
 	BUG_ON(path->slots[0] < 0);
-
-	return ret;
 }
 
 /*
@@ -2976,12 +2962,8 @@ again:
 	if (split == 0) {
 		if (mid <= slot) {
 			btrfs_set_header_nritems(right, 0);
-			wret = insert_ptr(trans, root, path,
-					  &disk_key, right->start,
-					  path->slots[1] + 1, 1);
-			if (wret)
-				ret = wret;
-
+			insert_ptr(trans, root, path, &disk_key, right->start,
+				   path->slots[1] + 1, 1);
 			btrfs_tree_unlock(path->nodes[0]);
 			free_extent_buffer(path->nodes[0]);
 			path->nodes[0] = right;
@@ -2989,12 +2971,8 @@ again:
 			path->slots[1] += 1;
 		} else {
 			btrfs_set_header_nritems(right, 0);
-			wret = insert_ptr(trans, root, path,
-					  &disk_key,
-					  right->start,
-					  path->slots[1], 1);
-			if (wret)
-				ret = wret;
+			insert_ptr(trans, root, path, &disk_key, right->start,
+				   path->slots[1], 1);
 			btrfs_tree_unlock(path->nodes[0]);
 			free_extent_buffer(path->nodes[0]);
 			path->nodes[0] = right;
@@ -3010,8 +2988,7 @@ again:
 		return ret;
 	}
 
-	ret = copy_for_split(trans, root, path, l, right, slot, mid, nritems);
-	BUG_ON(ret);
+	copy_for_split(trans, root, path, l, right, slot, mid, nritems);
 
 	if (split == 2) {
 		BUG_ON(num_doubles != 0);
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 05/20] btrfs: Don''t BUG_ON errors in __finish_chunk_alloc()

From: Mark Fasheh <mfasheh@suse.com>

All callers of __finish_chunk_alloc() BUG_ON() return value, so it''s
trivial
for us to always bubble up any errors caught in __finish_chunk_alloc() to be
caught there.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/volumes.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 53875ae..5d166c2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2600,16 +2600,13 @@ static int __finish_chunk_alloc(struct
btrfs_trans_handle *trans,
 	key.offset = chunk_offset;
 
 	ret = btrfs_insert_item(trans, chunk_root, &key, chunk, item_size);
-	BUG_ON(ret);
-
-	if (map->type & BTRFS_BLOCK_GROUP_SYSTEM) {
+	if (ret == 0 && map->type & BTRFS_BLOCK_GROUP_SYSTEM) {
 		ret = btrfs_add_system_chunk(trans, chunk_root, &key, chunk,
 					     item_size);
-		BUG_ON(ret);
 	}
 
 	kfree(chunk);
-	return 0;
+	return ret;
 }
 
 /*
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 06/20] btrfs: fix error check of btrfs_lookup_dentry()

From: Tsutomu Itoh <t-itoh@jp.fujitsu.com>

Clean up btrfs_lookup_dentry() to never return NULL, but PTR_ERR(-ENOENT)
instead. This keeps the return value convention consistent.

Callers who pass to d_instatiate() require a trivial update.

create_snapshot() in particular looks like it can also lose a BUG_ON(!inode)
which is not really needed - there seems less harm in returning ENOENT to
userspace at that point in the stack than there is to crash the machine.

Mark: Fixed conflicts against latest tree, gave the patch a more thorough
description.

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/inode.c |   12 ++++++++++--
 fs/btrfs/ioctl.c |   11 +++++++++--
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 15fceef..5fdb700 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4027,7 +4027,7 @@ struct inode *btrfs_lookup_dentry(struct inode *dir,
struct dentry *dentry)
 		return ERR_PTR(ret);
 
 	if (location.objectid == 0)
-		return NULL;
+		return ERR_PTR(-ENOENT);
 
 	if (location.type == BTRFS_INODE_ITEM_KEY) {
 		inode = btrfs_iget(dir->i_sb, &location, root, NULL);
@@ -4085,7 +4085,15 @@ static void btrfs_dentry_release(struct dentry *dentry)
 static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
 				   struct nameidata *nd)
 {
-	return d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry);
+	struct inode *inode = btrfs_lookup_dentry(dir, dentry);
+	if (IS_ERR(inode)) {
+		if (PTR_ERR(inode) == -ENOENT)
+			inode = NULL;
+		else
+			return ERR_CAST(inode);
+	}
+
+	return d_splice_alias(inode, dentry);
 }
 
 unsigned char btrfs_filetype_table[] = {
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7cf0133..9245d24 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -325,6 +325,7 @@ static noinline int create_subvol(struct btrfs_root *root,
 	struct btrfs_root *new_root;
 	struct dentry *parent = dentry->d_parent;
 	struct inode *dir;
+	struct inode *inode;
 	int ret;
 	int err;
 	u64 objectid;
@@ -433,7 +434,13 @@ static noinline int create_subvol(struct btrfs_root *root,
 
 	BUG_ON(ret);
 
-	d_instantiate(dentry, btrfs_lookup_dentry(dir, dentry));
+	inode = btrfs_lookup_dentry(dir, dentry);
+	if (IS_ERR(inode)) {
+		ret = PTR_ERR(inode);
+		goto fail;
+	}
+
+	d_instantiate(dentry, inode);
 fail:
 	if (async_transid) {
 		*async_transid = trans->transid;
@@ -503,7 +510,7 @@ static int create_snapshot(struct btrfs_root *root, struct
dentry *dentry,
 		ret = PTR_ERR(inode);
 		goto fail;
 	}
-	BUG_ON(!inode);
+
 	d_instantiate(dentry, inode);
 	ret = 0;
 fail:
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 07/20] btrfs: make fixup_low_keys() void

From: Mark Fasheh <mfasheh@suse.com>

This is trivial - fixup_low_keys always returns zero so we can make it void.
As a result, we can then make setup_items_for_insert() void too which lets
us cut out a couple of BUG_ON(ret) lines.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/ctree.c         |   59 ++++++++++++++-------------------------------
 fs/btrfs/ctree.h         |    8 +++---
 fs/btrfs/delayed-inode.c |    6 +---
 3 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 011cab3..b4a0801 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1863,16 +1863,12 @@ done:
  * This is used after shifting pointers to the left, so it stops
  * fixing up pointers when a given leaf/node is not in slot 0 of the
  * higher levels
- *
- * If this fails to write a tree block, it returns -1, but continues
- * fixing up the blocks in ram so the tree is consistent.
  */
-static int fixup_low_keys(struct btrfs_trans_handle *trans,
-			  struct btrfs_root *root, struct btrfs_path *path,
-			  struct btrfs_disk_key *key, int level)
+static void fixup_low_keys(struct btrfs_trans_handle *trans,
+			   struct btrfs_root *root, struct btrfs_path *path,
+			   struct btrfs_disk_key *key, int level)
 {
 	int i;
-	int ret = 0;
 	struct extent_buffer *t;
 
 	for (i = level; i < BTRFS_MAX_LEVEL; i++) {
@@ -1885,7 +1881,6 @@ static int fixup_low_keys(struct btrfs_trans_handle
*trans,
 		if (tslot != 0)
 			break;
 	}
-	return ret;
 }
 
 /*
@@ -2520,7 +2515,6 @@ static noinline int __push_leaf_left(struct
btrfs_trans_handle *trans,
 	u32 old_left_nritems;
 	u32 nr;
 	int ret = 0;
-	int wret;
 	u32 this_item_size;
 	u32 old_left_item_size;
 
@@ -2626,9 +2620,7 @@ static noinline int __push_leaf_left(struct
btrfs_trans_handle *trans,
 		clean_tree_block(trans, root, right);
 
 	btrfs_item_key(right, &disk_key, 0);
-	wret = fixup_low_keys(trans, root, path, &disk_key, 1);
-	if (wret)
-		ret = wret;
+	fixup_low_keys(trans, root, path, &disk_key, 1);
 
 	/* then fixup the leaf pointer in the path */
 	if (path->slots[0] < push_items) {
@@ -2999,12 +2991,8 @@ again:
 			free_extent_buffer(path->nodes[0]);
 			path->nodes[0] = right;
 			path->slots[0] = 0;
-			if (path->slots[1] == 0) {
-				wret = fixup_low_keys(trans, root,
-						path, &disk_key, 1);
-				if (wret)
-					ret = wret;
-			}
+			if (path->slots[1] == 0)
+				fixup_low_keys(trans, root, path, &disk_key, 1);
 		}
 		btrfs_mark_buffer_dirty(right);
 		return ret;
@@ -3221,10 +3209,9 @@ int btrfs_duplicate_item(struct btrfs_trans_handle
*trans,
 		return ret;
 
 	path->slots[0]++;
-	ret = setup_items_for_insert(trans, root, path, new_key, &item_size,
-				     item_size, item_size +
-				     sizeof(struct btrfs_item), 1);
-	BUG_ON(ret);
+	setup_items_for_insert(trans, root, path, new_key, &item_size,
+			       item_size, item_size +
+			       sizeof(struct btrfs_item), 1);
 
 	leaf = path->nodes[0];
 	memcpy_extent_buffer(leaf,
@@ -3527,7 +3514,7 @@ int btrfs_insert_some_items(struct btrfs_trans_handle
*trans,
 	ret = 0;
 	if (slot == 0) {
 		btrfs_cpu_key_to_disk(&disk_key, cpu_key);
-		ret = fixup_low_keys(trans, root, path, &disk_key, 1);
+		fixup_low_keys(trans, root, path, &disk_key, 1);
 	}
 
 	if (btrfs_leaf_free_space(root, leaf) < 0) {
@@ -3545,17 +3532,16 @@ out:
  * to save stack depth by doing the bulk of the work in a function
  * that doesn''t call btrfs_search_slot
  */
-int setup_items_for_insert(struct btrfs_trans_handle *trans,
-			   struct btrfs_root *root, struct btrfs_path *path,
-			   struct btrfs_key *cpu_key, u32 *data_size,
-			   u32 total_data, u32 total_size, int nr)
+void setup_items_for_insert(struct btrfs_trans_handle *trans,
+			    struct btrfs_root *root, struct btrfs_path *path,
+			    struct btrfs_key *cpu_key, u32 *data_size,
+			    u32 total_data, u32 total_size, int nr)
 {
 	struct btrfs_item *item;
 	int i;
 	u32 nritems;
 	unsigned int data_end;
 	struct btrfs_disk_key disk_key;
-	int ret;
 	struct extent_buffer *leaf;
 	int slot;
 
@@ -3616,10 +3602,9 @@ int setup_items_for_insert(struct btrfs_trans_handle
*trans,
 
 	btrfs_set_header_nritems(leaf, nritems + nr);
 
-	ret = 0;
 	if (slot == 0) {
 		btrfs_cpu_key_to_disk(&disk_key, cpu_key);
-		ret = fixup_low_keys(trans, root, path, &disk_key, 1);
+		fixup_low_keys(trans, root, path, &disk_key, 1);
 	}
 	btrfs_unlock_up_safe(path, 1);
 	btrfs_mark_buffer_dirty(leaf);
@@ -3628,7 +3613,6 @@ int setup_items_for_insert(struct btrfs_trans_handle
*trans,
 		btrfs_print_leaf(root, leaf);
 		BUG();
 	}
-	return ret;
 }
 
 /*
@@ -3660,7 +3644,8 @@ int btrfs_insert_empty_items(struct btrfs_trans_handle
*trans,
 	slot = path->slots[0];
 	BUG_ON(slot < 0);
 
-	ret = setup_items_for_insert(trans, root, path, cpu_key, data_size,
+	ret = 0;
+	setup_items_for_insert(trans, root, path, cpu_key, data_size,
 			       total_data, total_size, nr);
 
 out:
@@ -3706,7 +3691,6 @@ static int del_ptr(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
 	struct extent_buffer *parent = path->nodes[level];
 	u32 nritems;
 	int ret = 0;
-	int wret;
 
 	nritems = btrfs_header_nritems(parent);
 	if (slot != nritems - 1) {
@@ -3726,9 +3710,7 @@ static int del_ptr(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
 		struct btrfs_disk_key disk_key;
 
 		btrfs_node_key(parent, &disk_key, 0);
-		wret = fixup_low_keys(trans, root, path, &disk_key, level + 1);
-		if (wret)
-			ret = wret;
+		fixup_low_keys(trans, root, path, &disk_key, level + 1);
 	}
 	btrfs_mark_buffer_dirty(parent);
 	return ret;
@@ -3831,10 +3813,7 @@ int btrfs_del_items(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
 			struct btrfs_disk_key disk_key;
 
 			btrfs_item_key(leaf, &disk_key, 0);
-			wret = fixup_low_keys(trans, root, path,
-					      &disk_key, 1);
-			if (wret)
-				ret = wret;
+			fixup_low_keys(trans, root, path, &disk_key, 1);
 		}
 
 		/* delete the leaf if it is mostly empty */
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0469263..d0b7b58 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2344,10 +2344,10 @@ static inline int btrfs_del_item(struct
btrfs_trans_handle *trans,
 	return btrfs_del_items(trans, root, path, path->slots[0], 1);
 }
 
-int setup_items_for_insert(struct btrfs_trans_handle *trans,
-			   struct btrfs_root *root, struct btrfs_path *path,
-			   struct btrfs_key *cpu_key, u32 *data_size,
-			   u32 total_data, u32 total_size, int nr);
+void setup_items_for_insert(struct btrfs_trans_handle *trans,
+			    struct btrfs_root *root, struct btrfs_path *path,
+			    struct btrfs_key *cpu_key, u32 *data_size,
+			    u32 total_data, u32 total_size, int nr);
 int btrfs_insert_item(struct btrfs_trans_handle *trans, struct btrfs_root
 		      *root, struct btrfs_key *key, void *data, u32 data_size);
 int btrfs_insert_empty_items(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index b52c672..6211997 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -738,10 +738,8 @@ static int btrfs_batch_insert_items(struct
btrfs_trans_handle *trans,
 	btrfs_clear_path_blocking(path, NULL, 0);
 
 	/* insert the keys of the items */
-	ret = setup_items_for_insert(trans, root, path, keys, data_size,
-				     total_data_size, total_size, nitems);
-	if (ret)
-		goto error;
+	setup_items_for_insert(trans, root, path, keys, data_size,
+			       total_data_size, total_size, nitems);
 
 	/* insert the dir index items */
 	slot = path->slots[0];
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 08/20] btrfs: make del_ptr() and btrfs_del_leaf() void

From: Mark Fasheh <mfasheh@suse.com>

Since fixup_low_keys() has been made void, del_ptr() always returns zero. We
can then make it void as well. This allows us in turn to make
btrfs_del_leaf() void as the only return value it was previously catching
was from del_ptr(). This winds up removing a couple of un-needed BUG_ON(ret)
lines.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/ctree.c |   40 +++++++++++++---------------------------
 1 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index b4a0801..2b2f8fd 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -36,8 +36,8 @@ static int balance_node_right(struct btrfs_trans_handle
*trans,
 			      struct btrfs_root *root,
 			      struct extent_buffer *dst_buf,
 			      struct extent_buffer *src_buf);
-static int del_ptr(struct btrfs_trans_handle *trans, struct btrfs_root *root,
-		   struct btrfs_path *path, int level, int slot);
+static void del_ptr(struct btrfs_trans_handle *trans, struct btrfs_root *root,
+		    struct btrfs_path *path, int level, int slot);
 
 struct btrfs_path *btrfs_alloc_path(void)
 {
@@ -994,10 +994,7 @@ static noinline int balance_level(struct btrfs_trans_handle
*trans,
 		if (btrfs_header_nritems(right) == 0) {
 			clean_tree_block(trans, root, right);
 			btrfs_tree_unlock(right);
-			wret = del_ptr(trans, root, path, level + 1, pslot +
-				       1);
-			if (wret)
-				ret = wret;
+			del_ptr(trans, root, path, level + 1, pslot + 1);
 			root_sub_used(root, right->len);
 			btrfs_free_tree_block(trans, root, right, 0, 1);
 			free_extent_buffer(right);
@@ -1035,9 +1032,7 @@ static noinline int balance_level(struct
btrfs_trans_handle *trans,
 	if (btrfs_header_nritems(mid) == 0) {
 		clean_tree_block(trans, root, mid);
 		btrfs_tree_unlock(mid);
-		wret = del_ptr(trans, root, path, level + 1, pslot);
-		if (wret)
-			ret = wret;
+		del_ptr(trans, root, path, level + 1, pslot);
 		root_sub_used(root, mid->len);
 		btrfs_free_tree_block(trans, root, mid, 0, 1);
 		free_extent_buffer(mid);
@@ -3685,12 +3680,11 @@ int btrfs_insert_item(struct btrfs_trans_handle *trans,
struct btrfs_root
  * the tree should have been previously balanced so the deletion does not
  * empty a node.
  */
-static int del_ptr(struct btrfs_trans_handle *trans, struct btrfs_root *root,
-		   struct btrfs_path *path, int level, int slot)
+static void del_ptr(struct btrfs_trans_handle *trans, struct btrfs_root *root,
+		    struct btrfs_path *path, int level, int slot)
 {
 	struct extent_buffer *parent = path->nodes[level];
 	u32 nritems;
-	int ret = 0;
 
 	nritems = btrfs_header_nritems(parent);
 	if (slot != nritems - 1) {
@@ -3713,7 +3707,6 @@ static int del_ptr(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
 		fixup_low_keys(trans, root, path, &disk_key, level + 1);
 	}
 	btrfs_mark_buffer_dirty(parent);
-	return ret;
 }
 
 /*
@@ -3726,17 +3719,13 @@ static int del_ptr(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
  * The path must have already been setup for deleting the leaf, including
  * all the proper balancing.  path->nodes[1] must be locked.
  */
-static noinline int btrfs_del_leaf(struct btrfs_trans_handle *trans,
-				   struct btrfs_root *root,
-				   struct btrfs_path *path,
-				   struct extent_buffer *leaf)
+static noinline void btrfs_del_leaf(struct btrfs_trans_handle *trans,
+				    struct btrfs_root *root,
+				    struct btrfs_path *path,
+				    struct extent_buffer *leaf)
 {
-	int ret;
-
 	WARN_ON(btrfs_header_generation(leaf) != trans->transid);
-	ret = del_ptr(trans, root, path, 1, path->slots[1]);
-	if (ret)
-		return ret;
+	del_ptr(trans, root, path, 1, path->slots[1]);
 
 	/*
 	 * btrfs_free_extent is expensive, we want to make sure we
@@ -3747,7 +3736,6 @@ static noinline int btrfs_del_leaf(struct
btrfs_trans_handle *trans,
 	root_sub_used(root, leaf->len);
 
 	btrfs_free_tree_block(trans, root, leaf, 0, 1);
-	return 0;
 }
 /*
  * delete the item at the leaf level in path.  If that empties
@@ -3804,8 +3792,7 @@ int btrfs_del_items(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
 		} else {
 			btrfs_set_path_blocking(path);
 			clean_tree_block(trans, root, leaf);
-			ret = btrfs_del_leaf(trans, root, path, leaf);
-			BUG_ON(ret);
+			btrfs_del_leaf(trans, root, path, leaf);
 		}
 	} else {
 		int used = leaf_space_used(leaf, 0, nritems);
@@ -3841,8 +3828,7 @@ int btrfs_del_items(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
 
 			if (btrfs_header_nritems(leaf) == 0) {
 				path->slots[1] = slot;
-				ret = btrfs_del_leaf(trans, root, path, leaf);
-				BUG_ON(ret);
+				btrfs_del_leaf(trans, root, path, leaf);
 				free_extent_buffer(leaf);
 			} else {
 				/* if we''re still in the path, make sure
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 09/20] btrfs: Don''t BUG_ON failures in find_and_setup_root()

From: Mark Fasheh <mfasheh@suse.com>

This is very easy - we can just pass an error from btrfs_find_last_root()
back out to the callers - all of them have proper error handling.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/disk-io.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 07b3ac6..e9c7afb 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1131,7 +1131,8 @@ static int find_and_setup_root(struct btrfs_root
*tree_root,
 				   &root->root_item, &root->root_key);
 	if (ret > 0)
 		return -ENOENT;
-	BUG_ON(ret);
+	if (ret < 0)
+		return ret;
 
 	generation = btrfs_root_generation(&root->root_item);
 	blocksize = btrfs_level_size(root, btrfs_root_level(&root->root_item));
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 10/20] btrfs: go readonly on insert error in btrfs_add_root_ref()

From: Mark Fasheh <mfasheh@suse.com>

In btrfs_add_root_ref() we BUG if an error is encountered during
REF/BACKREF insertion. This does not look like a logic error, thus the BUG
is not called for. However, I don''t think there''s a simple way
to recover
from such an error at that point, so we mark the fs readonly instead.

At the same time, we can update the following caller of
btrfs_add_root_ref() which BUG_ON from an error:

- create_subvol: now passes the return code back to userspace

- create_pending_snapshot: goes readonly on any error from
  btrfs_add_root_ref().

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/ioctl.c       |    4 ++--
 fs/btrfs/root-tree.c   |    8 ++++++--
 fs/btrfs/transaction.c |    2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7cf0133..8adb220 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -430,8 +430,8 @@ static noinline int create_subvol(struct btrfs_root *root,
 	ret = btrfs_add_root_ref(trans, root->fs_info->tree_root,
 				 objectid, root->root_key.objectid,
 				 btrfs_ino(dir), index, name, namelen);
-
-	BUG_ON(ret);
+	if (ret)
+		goto fail;
 
 	d_instantiate(dentry, btrfs_lookup_dentry(dir, dentry));
 fail:
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index f409990..02f2bf3 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -407,7 +407,10 @@ int btrfs_add_root_ref(struct btrfs_trans_handle *trans,
 again:
 	ret = btrfs_insert_empty_item(trans, tree_root, path, &key,
 				      sizeof(*ref) + name_len);
-	BUG_ON(ret);
+	if (ret) {
+		btrfs_std_error(tree_root->fs_info, ret);
+		goto out_free;
+	}
 
 	leaf = path->nodes[0];
 	ref = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_root_ref);
@@ -426,8 +429,9 @@ again:
 		goto again;
 	}
 
+out_free:
 	btrfs_free_path(path);
-	return 0;
+	return ret;
 }
 
 /*
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 7dc36fa..7c46ece 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -991,7 +991,7 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
 				 parent_root->root_key.objectid,
 				 btrfs_ino(parent_inode), index,
 				 dentry->d_name.name, dentry->d_name.len);
-	BUG_ON(ret);
+	btrfs_std_error(fs_info, ret);
 	dput(parent);
 
 	key.offset = (u64)-1;
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 11/20] btrfs: Go readonly on bad extent refs in update_ref_for_cow()

From: Mark Fasheh <mfasheh@suse.com>

update_ref_for_cow() will BUG_ON() after it''s call to
btrfs_lookup_extent_info() if no existing references are found.  Since refs
are computed directly from disk, this should be treated as a corruption
instead of a logic error.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/ctree.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 5064930..8d8182f 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -333,7 +333,11 @@ static noinline int update_ref_for_cow(struct
btrfs_trans_handle *trans,
 					       buf->len, &refs, &flags);
 		if (ret)
 			return ret;
-		BUG_ON(refs == 0);
+		if (refs == 0) {
+			ret = -EROFS;
+			btrfs_std_error(root->fs_info, ret);
+			return ret;
+		}
 	} else {
 		refs = 1;
 		if (root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID ||
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 12/20] btrfs: Don''t BUG_ON errors from update_ref_for_cow()

From: Mark Fasheh <mfasheh@suse.com>

__btrfs_cow_block(), the only caller of update_ref_for_cow() will BUG_ON()
any error return.  Instead, we can go read-only fs as update_ref_for_cow()
manipulates disk data in a way which doesn''t look like it''s
easily rolled
back.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/ctree.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 8d8182f..31fdadf 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -482,7 +482,10 @@ static noinline int __btrfs_cow_block(struct
btrfs_trans_handle *trans,
 			    BTRFS_FSID_SIZE);
 
 	ret = update_ref_for_cow(trans, root, buf, cow, &last_ref);
-	BUG_ON(ret);
+	if (ret) {
+		btrfs_std_error(root->fs_info, ret);
+		return ret;
+	}
 
 	if (root->ref_cows)
 		btrfs_reloc_cow_block(trans, root, buf, cow);
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 13/20] btrfs: Go readonly on tree errors in balance_level

From: Mark Fasheh <mfasheh@suse.com>

balace_level() seems to deal with missing tree nodes by BUG_ON(). Instead,
we can easily just set the file system readonly and bubble -EROFS back up
the stack.

Signed-off-by: Mark Fasheh <mfasheh@suse.com>
---
 fs/btrfs/ctree.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 011cab3..dfb061b 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -918,7 +918,12 @@ static noinline int balance_level(struct btrfs_trans_handle
*trans,
 
 		/* promote the child to a root */
 		child = read_node_slot(root, mid, 0);
-		BUG_ON(!child);
+		if (!child) {
+			ret = -EROFS;
+			btrfs_std_error(root->fs_info, ret);
+			goto enospc;
+		}
+
 		btrfs_tree_lock(child);
 		btrfs_set_lock_blocking(child);
 		ret = btrfs_cow_block(trans, root, child, mid, 0, &child);
@@ -1019,7 +1024,11 @@ static noinline int balance_level(struct
btrfs_trans_handle *trans,
 		 * otherwise we would have pulled some pointers from the
 		 * right
 		 */
-		BUG_ON(!left);
+		if (!left) {
+			ret = -EROFS;
+			btrfs_std_error(root->fs_info, ret);
+			goto enospc;
+		}
 		wret = balance_node_right(trans, root, mid, left);
 		if (wret < 0) {
 			ret = wret;
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 14/20] btrfs: Document BUG() in find_lock_delalloc_range()

From: Mark Fasheh <mfasheh@suse.com>

We BUG_ON a nonzero, non -EAGAIN ret from lock_delalloc_range(). As it turns
out there is no other possible return value that makes sense anyway.  The
bare BUG_ON(ret) was a bit confusing and looked like something that needed
fixing. This patch documents the BUG_ON() so we know why it''s there.

Signed-off-by: Mark Fasheh <mfasheh@suse.com>
---
 fs/btrfs/extent_io.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d418164..2eb366d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1303,7 +1303,15 @@ again:
 			goto out_failed;
 		}
 	}
-	BUG_ON(ret);
+	if (ret) {
+		/*
+		 * This should never happen - lock_delalloc_pages only returns
+		 * 0 or -EAGAIN which are handled above.
+		 */
+		printk(KERN_ERR "btrfs: unexpected return %d from "
+		       "lock_delalloc_pages\n", ret);
+		BUG();
+	}
 
 	/* step three, lock the state bits for the whole range */
 	lock_extent_bits(tree, delalloc_start, delalloc_end,
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 15/20] btrfs: Go readonly on missing ref in btrfs_get_parent()

From: Mark Fasheh <mfasheh@suse.com>

btrfs_get_parent() searches the btree for a ref to the current object. From
there it can compute the parent objectid from which it can return a dentry.
if the reference is not found in the tree however, we BUG(). I believe a
more appropriate response would be to go read-only as this seems like a
corruption.

Signed-off-by: Mark Fasheh <mfasheh@suse.com>
---
 fs/btrfs/export.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
index 1b8dc33..a335169 100644
--- a/fs/btrfs/export.c
+++ b/fs/btrfs/export.c
@@ -193,7 +193,13 @@ static struct dentry *btrfs_get_parent(struct dentry
*child)
 	if (ret < 0)
 		goto fail;
 
-	BUG_ON(ret == 0);
+	if (ret == 0) {
+		/* Missing ref, should go readonly. */
+		ret = -ENOENT;
+		btrfs_std_error(root->fs_info, ret);
+		goto fail;
+	}
+
 	if (path->slots[0] == 0) {
 		ret = -ENOENT;
 		goto fail;
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 16/20] btrfs: make add_delayed_ref_head() void

From: Mark Fasheh <mfasheh@suse.com>

This is trivial as the function always returns success. We can remove 3
BUG_ON(ret) lines as a result.

Signed-off-by: Mark Fasheh <mfasheh@suse.com>
---
 fs/btrfs/delayed-ref.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 125cf76..b5c3d7c 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -390,10 +390,10 @@ update_existing_head_ref(struct btrfs_delayed_ref_node
*existing,
  * this does all the dirty work in terms of maintaining the correct
  * overall modification count.
  */
-static noinline int add_delayed_ref_head(struct btrfs_trans_handle *trans,
-					struct btrfs_delayed_ref_node *ref,
-					u64 bytenr, u64 num_bytes,
-					int action, int is_data)
+static noinline void add_delayed_ref_head(struct btrfs_trans_handle *trans,
+					  struct btrfs_delayed_ref_node *ref,
+					  u64 bytenr, u64 num_bytes,
+					  int action, int is_data)
 {
 	struct btrfs_delayed_ref_node *existing;
 	struct btrfs_delayed_ref_head *head_ref = NULL;
@@ -462,7 +462,6 @@ static noinline int add_delayed_ref_head(struct
btrfs_trans_handle *trans,
 		delayed_refs->num_entries++;
 		trans->delayed_ref_updates++;
 	}
-	return 0;
 }
 
 /*
@@ -610,9 +609,8 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle
*trans,
 	 * insert both the head node and the new ref without dropping
 	 * the spin lock
 	 */
-	ret = add_delayed_ref_head(trans, &head_ref->node, bytenr, num_bytes,
-				   action, 0);
-	BUG_ON(ret);
+	add_delayed_ref_head(trans, &head_ref->node, bytenr, num_bytes, action,
+			     0);
 
 	ret = add_delayed_tree_ref(trans, &ref->node, bytenr, num_bytes,
 				   parent, ref_root, level, action);
@@ -655,9 +653,8 @@ int btrfs_add_delayed_data_ref(struct btrfs_trans_handle
*trans,
 	 * insert both the head node and the new ref without dropping
 	 * the spin lock
 	 */
-	ret = add_delayed_ref_head(trans, &head_ref->node, bytenr, num_bytes,
-				   action, 1);
-	BUG_ON(ret);
+	add_delayed_ref_head(trans, &head_ref->node, bytenr, num_bytes,
+			     action, 1);
 
 	ret = add_delayed_data_ref(trans, &ref->node, bytenr, num_bytes,
 				   parent, ref_root, owner, offset, action);
@@ -672,7 +669,6 @@ int btrfs_add_delayed_extent_op(struct btrfs_trans_handle
*trans,
 {
 	struct btrfs_delayed_ref_head *head_ref;
 	struct btrfs_delayed_ref_root *delayed_refs;
-	int ret;
 
 	head_ref = kmalloc(sizeof(*head_ref), GFP_NOFS);
 	if (!head_ref)
@@ -683,10 +679,8 @@ int btrfs_add_delayed_extent_op(struct btrfs_trans_handle
*trans,
 	delayed_refs = &trans->transaction->delayed_refs;
 	spin_lock(&delayed_refs->lock);
 
-	ret = add_delayed_ref_head(trans, &head_ref->node, bytenr,
-				   num_bytes, BTRFS_UPDATE_DELAYED_HEAD,
-				   extent_op->is_data);
-	BUG_ON(ret);
+	add_delayed_ref_head(trans, &head_ref->node, bytenr, num_bytes,
+			     BTRFS_UPDATE_DELAYED_HEAD, extent_op->is_data);
 
 	spin_unlock(&delayed_refs->lock);
 	return 0;
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 17/20] btrfs: make add_delayed_tree_ref() void

From: Mark Fasheh <mfasheh@suse.com>

add_delayed_tree_ref() unconditionally returns 0. This makes it trivial to
turn into a void function. This kills another BUG_ON().

Signed-off-by: Mark Fasheh <mfasheh@suse.com>
---
 fs/btrfs/delayed-ref.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index b5c3d7c..afe1b1e 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -467,10 +467,10 @@ static noinline void add_delayed_ref_head(struct
btrfs_trans_handle *trans,
 /*
  * helper to insert a delayed tree ref into the rbtree.
  */
-static noinline int add_delayed_tree_ref(struct btrfs_trans_handle *trans,
-					 struct btrfs_delayed_ref_node *ref,
-					 u64 bytenr, u64 num_bytes, u64 parent,
-					 u64 ref_root, int level, int action)
+static void add_delayed_tree_ref(struct btrfs_trans_handle *trans,
+				 struct btrfs_delayed_ref_node *ref,
+				 u64 bytenr, u64 num_bytes, u64 parent,
+				 u64 ref_root, int level, int action)
 {
 	struct btrfs_delayed_ref_node *existing;
 	struct btrfs_delayed_tree_ref *full_ref;
@@ -515,7 +515,6 @@ static noinline int add_delayed_tree_ref(struct
btrfs_trans_handle *trans,
 		delayed_refs->num_entries++;
 		trans->delayed_ref_updates++;
 	}
-	return 0;
 }
 
 /*
@@ -587,7 +586,6 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle
*trans,
 	struct btrfs_delayed_tree_ref *ref;
 	struct btrfs_delayed_ref_head *head_ref;
 	struct btrfs_delayed_ref_root *delayed_refs;
-	int ret;
 
 	BUG_ON(extent_op && extent_op->is_data);
 	ref = kmalloc(sizeof(*ref), GFP_NOFS);
@@ -612,9 +610,9 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle
*trans,
 	add_delayed_ref_head(trans, &head_ref->node, bytenr, num_bytes, action,
 			     0);
 
-	ret = add_delayed_tree_ref(trans, &ref->node, bytenr, num_bytes,
-				   parent, ref_root, level, action);
-	BUG_ON(ret);
+	add_delayed_tree_ref(trans, &ref->node, bytenr, num_bytes, parent,
+			     ref_root, level, action);
+
 	spin_unlock(&delayed_refs->lock);
 	return 0;
 }
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 18/20] btrfs: Don''t BUG_ON insert errors in btrfs_alloc_dev_extent()

The only caller of btrfs_alloc_dev_extent() is __btrfs_alloc_chunk() which
already bugs on any error returned. We can remove the BUG_ON''s in
btrfs_alloc_dev_extent() then since __btrfs_alloc_chunk() will "catch"
them
anyway.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/volumes.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 53875ae..1f5c3b1 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1008,7 +1008,8 @@ int btrfs_alloc_dev_extent(struct btrfs_trans_handle
*trans,
 	key.type = BTRFS_DEV_EXTENT_KEY;
 	ret = btrfs_insert_empty_item(trans, root, path, &key,
 				      sizeof(*extent));
-	BUG_ON(ret);
+	if (ret)
+		goto out;
 
 	leaf = path->nodes[0];
 	extent = btrfs_item_ptr(leaf, path->slots[0],
@@ -1023,6 +1024,7 @@ int btrfs_alloc_dev_extent(struct btrfs_trans_handle
*trans,
 
 	btrfs_set_dev_extent_length(leaf, extent, num_bytes);
 	btrfs_mark_buffer_dirty(leaf);
+out:
 	btrfs_free_path(path);
 	return ret;
 }
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 19/20] btrfs: Remove BUG_ON from __btrfs_alloc_chunk()

We BUG_ON() error from add_extent_mapping(), but that error looks pretty
easy to bubble back up - as far as I can tell there have not been any
permanent modifications to fs state at that point.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/volumes.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1f5c3b1..50547f3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2515,8 +2515,9 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle
*trans,
 	write_lock(&em_tree->lock);
 	ret = add_extent_mapping(em_tree, em);
 	write_unlock(&em_tree->lock);
-	BUG_ON(ret);
 	free_extent_map(em);
+	if (ret)
+		goto error;
 
 	ret = btrfs_make_block_group(trans, extent_root, 0, type,
 				     BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Fasheh

2011-Sep-15 17:34 UTC

head link

[PATCH 20/20] btrfs: Remove BUG_ON from __finish_chunk_alloc()

btrfs_alloc_chunk() unconditionally BUGs on any error returned from
__finish_chunk_alloc() so there''s no need for two BUG_ON lines. Remove
the
one from __finish_chunk_alloc().

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/volumes.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 50547f3..804328c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2571,7 +2571,8 @@ static int __finish_chunk_alloc(struct btrfs_trans_handle
*trans,
 		device = map->stripes[index].dev;
 		device->bytes_used += stripe_size;
 		ret = btrfs_update_device(trans, device);
-		BUG_ON(ret);
+		if (ret)
+			goto out_free;
 		index++;
 	}
 
@@ -2611,6 +2612,7 @@ static int __finish_chunk_alloc(struct btrfs_trans_handle
*trans,
 		BUG_ON(ret);
 	}
 
+out_free:
 	kfree(chunk);
 	return 0;
 }
-- 
1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba

2011-Sep-15 18:03 UTC

head link

Re: [PATCH 0/20] btrfs: More error handling fixes

Hi,

On Thu, Sep 15, 2011 at 10:34:39AM -0700, Mark Fasheh
wrote:> Some of the patches (the first 8) were previously sent to this list but got
> no comments so I''m resending them with this set. Changes from last
time are
> that I also started setting the file system readonly on errors which look
> like possible corruption.
I''ve been testing your branch ''all_fixes'' from your
git.k.org repo for
some time, among other patchsets and fixes in my development repository
at http://repo.or.cz/w/linux-2.6/btrfs-unstable.git #integration/btrfs-next
and I have not hit any single problem which would point back to your patches.
I''m testing with xfstests and various hand-made stress tests for
specific
areas (usually from reports on irc).


HTH,
david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Btrfs devel - Sep 2011 - [PATCH 0/20] btrfs: More error handling fixes

[PATCH 0/20] btrfs: More error handling fixes

[PATCH 01/20] btrfs: Don''t BUG_ON errors from btrfs_create_subvol_root()

[PATCH 02/20] btrfs: Don''t BUG_ON() errors in update_ref_for_cow()

[PATCH 03/20] btrfs: Don''t BUG_ON kzalloc error in btrfs_lookup_csums_range()

[PATCH 04/20] btrfs: make insert_ptr() void

[PATCH 05/20] btrfs: Don''t BUG_ON errors in __finish_chunk_alloc()

[PATCH 06/20] btrfs: fix error check of btrfs_lookup_dentry()

[PATCH 07/20] btrfs: make fixup_low_keys() void

[PATCH 08/20] btrfs: make del_ptr() and btrfs_del_leaf() void

[PATCH 09/20] btrfs: Don''t BUG_ON failures in find_and_setup_root()

[PATCH 10/20] btrfs: go readonly on insert error in btrfs_add_root_ref()

[PATCH 11/20] btrfs: Go readonly on bad extent refs in update_ref_for_cow()

[PATCH 12/20] btrfs: Don''t BUG_ON errors from update_ref_for_cow()

[PATCH 13/20] btrfs: Go readonly on tree errors in balance_level

[PATCH 14/20] btrfs: Document BUG() in find_lock_delalloc_range()

[PATCH 15/20] btrfs: Go readonly on missing ref in btrfs_get_parent()

[PATCH 16/20] btrfs: make add_delayed_ref_head() void

[PATCH 17/20] btrfs: make add_delayed_tree_ref() void

[PATCH 18/20] btrfs: Don''t BUG_ON insert errors in btrfs_alloc_dev_extent()

[PATCH 19/20] btrfs: Remove BUG_ON from __btrfs_alloc_chunk()

[PATCH 20/20] btrfs: Remove BUG_ON from __finish_chunk_alloc()

Re: [PATCH 0/20] btrfs: More error handling fixes