diff mbox

btrfs-progs: readahead errors are not fatal

Message ID 20140827181603.GF12827@lenny.home.zabbo.net (mailing list archive)
State Accepted
Headers show

Commit Message

Zach Brown Aug. 27, 2014, 6:16 p.m. UTC
On Wed, Aug 27, 2014 at 07:47:45PM +0200, David Sterba wrote:
> Kill the BUG_ON.
> 
> Signed-off-by: David Sterba <dsterba@suse.cz>
> ---
>  disk-io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/disk-io.c b/disk-io.c
> index 8db0335bc81b..7ddd4b90836f 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -152,7 +152,8 @@ int readahead_tree_block(struct btrfs_root *root, u64 bytenr, u32 blocksize,
>  	length = blocksize;
>  	ret = btrfs_map_block(&root->fs_info->mapping_tree, READ,
>  			      bytenr, &length, &multi, 0, NULL);
> -	BUG_ON(ret);
> +	if (ret)
> +		return 0
>  	device = multi->stripes[0].dev;
>  	device->total_ios++;
>  	blocksize = min(blocksize, (u32)(64 * 1024));

Seems like this is already leaking a refcount and only returns 0..  so
maybe remove the bug_on something like this totally untested patch?

There's another "#if 0"-ed out caller in extent-tree.c but that nonsense
should just be removed.

- z

commit 69a668994c70cc1fd86d1c349d272b1cbfde23b1
Author: Zach Brown <zab@zabbo.net>
Date:   Wed Aug 27 11:07:23 2014 -0700

    btrfs-progs: kill BUG_ON in readahead_tree_block()
    
    David sent a quick patch that removed a BUG_ON().  I took a peek and
    found that the function was already leaking an eb ref and only returned
    0.  So this fixes the leak and makes the function void and fixes up the
    callers.
    
    Accidentally-motivated-by: David Sterba <dsterba@suse.cz>
    Signed-off-by: Zach Brown <zab@zabbo.net>

--
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

Comments

David Sterba Aug. 29, 2014, 2:38 p.m. UTC | #1
On Wed, Aug 27, 2014 at 11:16:03AM -0700, Zach Brown wrote:
> Seems like this is already leaking a refcount and only returns 0..  so
> maybe remove the bug_on something like this totally untested patch?

Looks much better than my patch, I'm taking it.

> There's another "#if 0"-ed out caller in extent-tree.c but that nonsense
> should just be removed.

Indeed, looking at the commit that added "#if 0", we're not interested
in the code anymore, feel free to kill it.

Thanks.
--
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
diff mbox

Patch

diff --git a/cmds-check.c b/cmds-check.c
index 268e588..d479361 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -1287,7 +1287,6 @@  static void reada_walk_down(struct btrfs_root *root,
 	u32 nritems;
 	u32 blocksize;
 	int i;
-	int ret;
 	int level;
 
 	level = btrfs_header_level(node);
@@ -1299,9 +1298,7 @@  static void reada_walk_down(struct btrfs_root *root,
 	for (i = slot; i < nritems; i++) {
 		bytenr = btrfs_node_blockptr(node, i);
 		ptr_gen = btrfs_node_ptr_generation(node, i);
-		ret = readahead_tree_block(root, bytenr, blocksize, ptr_gen);
-		if (ret)
-			break;
+		readahead_tree_block(root, bytenr, blocksize, ptr_gen);
 	}
 }
 
diff --git a/disk-io.c b/disk-io.c
index f71f5ca..1f1ce75 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -134,31 +134,26 @@  struct extent_buffer *btrfs_find_create_tree_block(struct btrfs_root *root,
 				   blocksize);
 }
 
-int readahead_tree_block(struct btrfs_root *root, u64 bytenr, u32 blocksize,
-			 u64 parent_transid)
+void readahead_tree_block(struct btrfs_root *root, u64 bytenr, u32 blocksize,
+			  u64 parent_transid)
 {
-	int ret;
 	struct extent_buffer *eb;
 	u64 length;
 	struct btrfs_multi_bio *multi = NULL;
 	struct btrfs_device *device;
 
 	eb = btrfs_find_tree_block(root, bytenr, blocksize);
-	if (eb && btrfs_buffer_uptodate(eb, parent_transid)) {
-		free_extent_buffer(eb);
-		return 0;
+	if (!(eb && btrfs_buffer_uptodate(eb, parent_transid)) &&
+	    !btrfs_map_block(&root->fs_info->mapping_tree, READ,
+			     bytenr, &length, &multi, 0, NULL)) {
+		device = multi->stripes[0].dev;
+		device->total_ios++;
+		blocksize = min(blocksize, (u32)(64 * 1024));
+		readahead(device->fd, multi->stripes[0].physical, blocksize);
 	}
 
-	length = blocksize;
-	ret = btrfs_map_block(&root->fs_info->mapping_tree, READ,
-			      bytenr, &length, &multi, 0, NULL);
-	BUG_ON(ret);
-	device = multi->stripes[0].dev;
-	device->total_ios++;
-	blocksize = min(blocksize, (u32)(64 * 1024));
-	readahead(device->fd, multi->stripes[0].physical, blocksize);
+	free_extent_buffer(eb);
 	kfree(multi);
-	return 0;
 }
 
 static int verify_parent_transid(struct extent_io_tree *io_tree,
diff --git a/disk-io.h b/disk-io.h
index 13d4420..c296055 100644
--- a/disk-io.h
+++ b/disk-io.h
@@ -48,8 +48,8 @@  struct btrfs_device;
 int read_whole_eb(struct btrfs_fs_info *info, struct extent_buffer *eb, int mirror);
 struct extent_buffer *read_tree_block(struct btrfs_root *root, u64 bytenr,
 				      u32 blocksize, u64 parent_transid);
-int readahead_tree_block(struct btrfs_root *root, u64 bytenr, u32 blocksize,
-			 u64 parent_transid);
+void readahead_tree_block(struct btrfs_root *root, u64 bytenr, u32 blocksize,
+			  u64 parent_transid);
 struct extent_buffer *btrfs_find_create_tree_block(struct btrfs_root *root,
 						   u64 bytenr, u32 blocksize);