| Message ID | 50484569.8080709@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 06, 2012 at 02:40:41PM +0800, Wang Sheng-Hui wrote: > The memory allocation failure is BUG_ON in add_excluded_extent (following > the code path) and btrfs_rmap_block. No need to BUG_ON -ENOMEM inside > exclude_super_stripes itself. No please. > Its return value is always 0, and useless for its callers. Set it as void > instead 0-returned. btrfs_rmap_block itself contains a BUG_ON: 3980 int btrfs_rmap_block(struct btrfs_mapping_tree *map_tree, 3981 u64 chunk_start, u64 physical, u64 devid, 3982 u64 **logical, int *naddrs, int *stripe_len) 3983 { 3984 struct extent_map_tree *em_tree = &map_tree->map_tree; 3985 struct extent_map *em; 3986 struct map_lookup *map; 3987 u64 *buf; 3988 u64 bytenr; 3989 u64 length; 3990 u64 stripe_nr; 3991 int i, j, nr = 0; 3992 3993 read_lock(&em_tree->lock); 3994 em = lookup_extent_mapping(em_tree, chunk_start, 1); 3995 read_unlock(&em_tree->lock); 3996 3997 BUG_ON(!em || em->start != chunk_start); And this should be turned into an 'return error', thus giving a non-zero return code that should be handled in the callers. Eg. this patch attempts to do that http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg15470.html but has not been merged due to incorrect fix inside exclude_super_stripes (introduced in the patch). The same objection for return code cleanups will hold for any function that returns 0 but is full of BUG_ONs. 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
On 2012?09?06? 18:09, David Sterba wrote: > On Thu, Sep 06, 2012 at 02:40:41PM +0800, Wang Sheng-Hui wrote: >> The memory allocation failure is BUG_ON in add_excluded_extent (following >> the code path) and btrfs_rmap_block. No need to BUG_ON -ENOMEM inside >> exclude_super_stripes itself. > > No please. > >> Its return value is always 0, and useless for its callers. Set it as void >> instead 0-returned. > > btrfs_rmap_block itself contains a BUG_ON: > > 3980 int btrfs_rmap_block(struct btrfs_mapping_tree *map_tree, > 3981 u64 chunk_start, u64 physical, u64 devid, > 3982 u64 **logical, int *naddrs, int *stripe_len) > 3983 { > 3984 struct extent_map_tree *em_tree = &map_tree->map_tree; > 3985 struct extent_map *em; > 3986 struct map_lookup *map; > 3987 u64 *buf; > 3988 u64 bytenr; > 3989 u64 length; > 3990 u64 stripe_nr; > 3991 int i, j, nr = 0; > 3992 > 3993 read_lock(&em_tree->lock); > 3994 em = lookup_extent_mapping(em_tree, chunk_start, 1); > 3995 read_unlock(&em_tree->lock); > 3996 > 3997 BUG_ON(!em || em->start != chunk_start); > > And this should be turned into an 'return error', thus giving a non-zero return > code that should be handled in the callers. > > Eg. this patch attempts to do that > http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg15470.html > > but has not been merged due to incorrect fix inside exclude_super_stripes > (introduced in the patch). > > The same objection for return code cleanups will hold for any function that > returns 0 but is full of BUG_ONs. > > > david Got it. Thanks, David! Regards, Sheng-Hui -- 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 --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index ba58024..95492cc 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -234,39 +234,33 @@ static void free_excluded_extents(struct btrfs_root *root, start, end, EXTENT_UPTODATE, GFP_NOFS); } -static int exclude_super_stripes(struct btrfs_root *root, +static void exclude_super_stripes(struct btrfs_root *root, struct btrfs_block_group_cache *cache) { u64 bytenr; u64 *logical; int stripe_len; - int i, nr, ret; + int i, nr; if (cache->key.objectid < BTRFS_SUPER_INFO_OFFSET) { stripe_len = BTRFS_SUPER_INFO_OFFSET - cache->key.objectid; cache->bytes_super += stripe_len; - ret = add_excluded_extent(root, cache->key.objectid, - stripe_len); - BUG_ON(ret); /* -ENOMEM */ + add_excluded_extent(root, cache->key.objectid, stripe_len); } for (i = 0; i < BTRFS_SUPER_MIRROR_MAX; i++) { bytenr = btrfs_sb_offset(i); - ret = btrfs_rmap_block(&root->fs_info->mapping_tree, - cache->key.objectid, bytenr, - 0, &logical, &nr, &stripe_len); - BUG_ON(ret); /* -ENOMEM */ + btrfs_rmap_block(&root->fs_info->mapping_tree, + cache->key.objectid, bytenr, + 0, &logical, &nr, &stripe_len); while (nr--) { cache->bytes_super += stripe_len; - ret = add_excluded_extent(root, logical[nr], - stripe_len); - BUG_ON(ret); /* -ENOMEM */ + add_excluded_extent(root, logical[nr], stripe_len); } kfree(logical); } - return 0; } static struct btrfs_caching_control *
The memory allocation failure is BUG_ON in add_excluded_extent (following the code path) and btrfs_rmap_block. No need to BUG_ON -ENOMEM inside exclude_super_stripes itself. Its return value is always 0, and useless for its callers. Set it as void instead 0-returned. Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com> --- fs/btrfs/extent-tree.c | 20 +++++++------------- 1 files changed, 7 insertions(+), 13 deletions(-)