(2011/04/12 7:46), Yoshinori Sano wrote:
> Thank you for your review.
> I modified the previous patch.
Other points still existed. I'm sorry not to point it out at a time.
>
> Specifically, all the callers that calls the following are modified
> because of the lack of return value check:
> - btrfs_drop_snapshot()
> - btrfs_update_inode()
> - wc->process_func()
>
> However, BUG_ON() code are increased by this modification.
>
> Regards,
>
> Signed-off-by: Yoshinori Sano <yoshinori.s...@gmail.com>
> ---
> fs/btrfs/dir-item.c | 2 +
> fs/btrfs/extent-tree.c | 12 +++++++---
> fs/btrfs/file-item.c | 6 +++-
> fs/btrfs/file.c | 3 +-
> fs/btrfs/free-space-cache.c | 4 ++-
> fs/btrfs/inode.c | 44 ++++++++++++++++++++++++++++--------------
> fs/btrfs/relocation.c | 4 ++-
> fs/btrfs/root-tree.c | 6 +++-
> fs/btrfs/transaction.c | 9 ++++++-
> fs/btrfs/tree-log.c | 24 +++++++++++++++-------
> fs/btrfs/volumes.c | 8 +++++-
> 11 files changed, 84 insertions(+), 38 deletions(-)
>
> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> index c62f02f..e60bf8e 100644
> --- a/fs/btrfs/dir-item.c
> +++ b/fs/btrfs/dir-item.c
> @@ -142,6 +142,8 @@ int btrfs_insert_dir_item(struct
> btrfs_trans_handle *trans, struct btrfs_root
> key.offset = btrfs_name_hash(name, name_len);
>
> path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> path->leave_spinning = 1;
>
> data_size = sizeof(*dir_item) + name_len;
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index f619c3c..b830db8 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -645,7 +645,8 @@ int btrfs_lookup_extent(struct btrfs_root *root,
> u64 start, u64 len)
> struct btrfs_path *path;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
> key.objectid = start;
> key.offset = len;
> btrfs_set_key_type(&key, BTRFS_EXTENT_ITEM_KEY);
> @@ -5531,7 +5532,8 @@ static int alloc_reserved_tree_block(struct
> btrfs_trans_handle *trans,
> u32 size = sizeof(*extent_item) + sizeof(*block_info) + sizeof(*iref);
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
>
> path->leave_spinning = 1;
> ret = btrfs_insert_empty_item(trans, fs_info->extent_root, path,
> @@ -6302,7 +6304,8 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
> int level;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
>
> wc = kzalloc(sizeof(*wc), GFP_NOFS);
> BUG_ON(!wc);
> @@ -8699,7 +8702,8 @@ int btrfs_remove_block_group(struct
> btrfs_trans_handle *trans,
> spin_unlock(&cluster->refill_lock);
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
>
> inode = lookup_free_space_inode(root, block_group, path);
> if (!IS_ERR(inode)) {
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index a6a9d4e..097911e 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -281,7 +281,8 @@ int btrfs_lookup_csums_range(struct btrfs_root
> *root, u64 start, u64 end,
> u16 csum_size = btrfs_super_csum_size(&root->fs_info->super_copy);
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
>
> key.objectid = BTRFS_EXTENT_CSUM_OBJECTID;
> key.offset = start;
> @@ -665,7 +666,8 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle
> *trans,
> btrfs_super_csum_size(&root->fs_info->super_copy);
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
> sector_sum = sums->sums;
> again:
> next_offset = (u64)-1;
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index e621ea5..fe623ea 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -599,7 +599,8 @@ int btrfs_mark_extent_written(struct
> btrfs_trans_handle *trans,
> btrfs_drop_extent_cache(inode, start, end - 1, 0);
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
fs/btrfs/inode.c
5827 ret = btrfs_mark_extent_written(trans, inode,
5828 ordered->file_offset,
5829 ordered->file_offset +
5830 ordered->len);
5831 if (ret) {
5832 err = ret;
5833 goto out_unlock;
5834 }
I think that you should insert WARN_ON() or BUG_ON() before 'goto out_unlock'.
> again:
> recow = 0;
> split = start;
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index f561c95..101b96c 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -522,6 +522,7 @@ int btrfs_write_out_cache(struct btrfs_root *root,
> int num_checksums;
> int entries = 0;
> int bitmaps = 0;
> + int err;
> int ret = 0;
> bool next_page = false;
>
> @@ -853,7 +854,8 @@ out_free:
> BTRFS_I(inode)->generation = 0;
> }
> kfree(checksums);
> - btrfs_update_inode(trans, root, inode);
> + err = btrfs_update_inode(trans, root, inode);
> + BUG_ON(err);
> iput(inode);
> return ret;
> }
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index cc60228..c023053 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -201,9 +201,9 @@ static noinline int insert_inline_extent(struct
> btrfs_trans_handle *trans,
> * could end up racing with unlink.
> */
> BTRFS_I(inode)->disk_i_size = inode->i_size;
> - btrfs_update_inode(trans, root, inode);
> + ret = btrfs_update_inode(trans, root, inode);
>
> - return 0;
> + return ret;
> fail:
> btrfs_free_path(path);
> return err;
> @@ -1007,6 +1007,7 @@ static noinline int csum_exist_in_range(struct
> btrfs_root *root,
>
> ret = btrfs_lookup_csums_range(root->fs_info->csum_root, bytenr,
> bytenr + num_bytes - 1, &list);
> + BUG_ON(ret);
> if (ret == 0 && list_empty(&list))
> return 0;
>
> @@ -1050,7 +1051,8 @@ static noinline int run_delalloc_nocow(struct
> inode *inode,
> bool nolock = false;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
If you change this, you should change following caller.
fs/btrfs/extent_io.c
2240 tree->ops->fill_delalloc(inode, page,
delalloc_start,
2241 delalloc_end,
&page_started,
2242 &nr_written);
> if (root == root->fs_info->tree_root) {
> nolock = true;
> trans = btrfs_join_transaction_nolock(root, 1);
> @@ -1496,13 +1498,15 @@ static noinline int add_pending_csums(struct
> btrfs_trans_handle *trans,
> struct inode *inode, u64 file_offset,
> struct list_head *list)
> {
> + int ret;
> struct btrfs_ordered_sum *sum;
>
> btrfs_set_trans_block_group(trans, inode);
>
> list_for_each_entry(sum, list, list) {
> - btrfs_csum_file_blocks(trans,
> + ret = btrfs_csum_file_blocks(trans,
> BTRFS_I(inode)->root->fs_info->csum_root, sum);
> + BUG_ON(ret);
> }
> return 0;
> }
> @@ -1625,8 +1629,8 @@ static int insert_reserved_file_extent(struct
> btrfs_trans_handle *trans,
> int ret;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> -
> + if (!path)
> + return -ENOMEM;
> path->leave_spinning = 1;
>
> /*
> @@ -2493,7 +2497,7 @@ static void btrfs_read_locked_inode(struct inode *inode)
> int ret;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + BUG_ON(!path); /* FIXME, should not use BUG_ON */
> memcpy(&location, &BTRFS_I(inode)->location, sizeof(location));
>
> ret = btrfs_lookup_inode(NULL, root, path, &location, 0);
> @@ -2631,7 +2635,8 @@ noinline int btrfs_update_inode(struct
> btrfs_trans_handle *trans,
> int ret;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
> path->leave_spinning = 1;
> ret = btrfs_lookup_inode(trans, root, path,
> &BTRFS_I(inode)->location, 1);
> @@ -2735,7 +2740,7 @@ err:
>
> btrfs_i_size_write(dir, dir->i_size - name_len * 2);
> inode->i_ctime = dir->i_mtime = dir->i_ctime = CURRENT_TIME;
> - btrfs_update_inode(trans, root, dir);
> + ret = btrfs_update_inode(trans, root, dir);
> out:
> return ret;
> }
> @@ -3290,7 +3295,8 @@ int btrfs_truncate_inode_items(struct
> btrfs_trans_handle *trans,
> btrfs_drop_extent_cache(inode, new_size & (~mask), (u64)-1, 0);
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
If you return ENOMEM here, I think that following codes lead the problem.
fs/btrfs/inode.c
3782 ret = btrfs_truncate_inode_items(trans, root, inode, 0,
0);
3783 if (ret != -EAGAIN)
3784 break;
...
3791 }
...
3802 end_writeback(inode);
3803 return;
3804 }
> path->reada = -1;
>
> key.objectid = inode->i_ino;
> @@ -3817,7 +3823,8 @@ static int btrfs_inode_by_name(struct inode
> *dir, struct dentry *dentry,
> int ret = 0;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (path)
> + return -ENOMEM;
>
> di = btrfs_lookup_dir_item(NULL, root, path, dir->i_ino, name,
> namelen, 0);
> @@ -4523,7 +4530,8 @@ static struct inode *btrfs_new_inode(struct
> btrfs_trans_handle *trans,
> int owner;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return ERR_PTR(-ENOMEM);
>
> inode = new_inode(root->fs_info->sb);
> if (!inode)
> @@ -4737,7 +4745,8 @@ static int btrfs_mknod(struct inode *dir, struct
> dentry *dentry,
> else {
> inode->i_op = &btrfs_special_inode_operations;
> init_special_inode(inode, inode->i_mode, rdev);
> - btrfs_update_inode(trans, root, inode);
> + err = btrfs_update_inode(trans, root, inode);
> + BUG_ON(err);
> }
> btrfs_update_inode_block_group(trans, inode);
> btrfs_update_inode_block_group(trans, dir);
> @@ -5854,7 +5863,8 @@ again:
>
> add_pending_csums(trans, inode, ordered->file_offset, &ordered->list);
> btrfs_ordered_update_i_size(inode, 0, ordered);
> - btrfs_update_inode(trans, root, inode);
> + ret = btrfs_update_inode(trans, root, inode);
> + BUG_ON(ret);
> out_unlock:
> unlock_extent_cached(&BTRFS_I(inode)->io_tree, ordered->file_offset,
> ordered->file_offset + ordered->len - 1,
> @@ -7235,7 +7245,11 @@ static int btrfs_symlink(struct inode *dir,
> struct dentry *dentry,
> goto out_unlock;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path) {
> + err = -ENOMEM;
> + drop_inode = 1;
> + goto out_unlock;
> + }
> key.objectid = inode->i_ino;
> key.offset = 0;
> btrfs_set_key_type(&key, BTRFS_EXTENT_DATA_KEY);
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 58250e0..d336517 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2228,7 +2228,8 @@ again:
> } else {
> list_del_init(&reloc_root->root_list);
> }
> - btrfs_drop_snapshot(reloc_root, rc->block_rsv, 0);
> + ret = btrfs_drop_snapshot(reloc_root, rc->block_rsv, 0);
> + BUG_ON(ret);
> }
>
> if (found) {
> @@ -4243,6 +4244,7 @@ int btrfs_reloc_clone_csums(struct inode *inode,
> u64 file_pos, u64 len)
> disk_bytenr = file_pos + BTRFS_I(inode)->index_cnt;
> ret = btrfs_lookup_csums_range(root->fs_info->csum_root, disk_bytenr,
> disk_bytenr + len - 1, &list);
> + BUG_ON(ret);
>
> while (!list_empty(&list)) {
> sums = list_entry(list.next, struct btrfs_ordered_sum, list);
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 6928bff..c330cad 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -40,7 +40,8 @@ int btrfs_search_root(struct btrfs_root *root, u64
> search_start,
> search_key.offset = (u64)-1;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
> again:
> ret = btrfs_search_slot(NULL, root, &search_key, path, 0, 0);
> if (ret < 0)
> @@ -141,7 +142,8 @@ int btrfs_update_root(struct btrfs_trans_handle
> *trans, struct btrfs_root
> unsigned long ptr;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
> ret = btrfs_search_slot(trans, root, key, path, 0, 1);
> if (ret < 0)
> goto out;
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 5b158da..c53469c 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1417,6 +1417,8 @@ int btrfs_commit_transaction(struct
> btrfs_trans_handle *trans,
> */
> int btrfs_clean_old_snapshots(struct btrfs_root *root)
> {
> + int err;
> + int update_ref;
> LIST_HEAD(list);
> struct btrfs_fs_info *fs_info = root->fs_info;
>
> @@ -1430,9 +1432,12 @@ int btrfs_clean_old_snapshots(struct btrfs_root *root)
>
> if (btrfs_header_backref_rev(root->node) <
> BTRFS_MIXED_BACKREF_REV)
> - btrfs_drop_snapshot(root, NULL, 0);
> + update_ref = 0;
> else
> - btrfs_drop_snapshot(root, NULL, 1);
> + update_ref = 1;
> +
> + err = btrfs_drop_snapshot(root, NULL, update_ref);
> + BUG_ON(err);
> }
> return 0;
> }
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index c50271a..eff407c 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -638,7 +638,8 @@ static noinline int replay_one_extent(struct
> btrfs_trans_handle *trans,
> }
>
> inode_set_bytes(inode, saved_nbytes);
> - btrfs_update_inode(trans, root, inode);
> + ret = btrfs_update_inode(trans, root, inode);
> + BUG_ON(ret);
> out:
> if (inode)
> iput(inode);
> @@ -909,7 +910,8 @@ insert:
> btrfs_inode_ref_index(eb, ref));
> BUG_ON(ret);
>
> - btrfs_update_inode(trans, root, inode);
> + ret = btrfs_update_inode(trans, root, inode);
> + BUG_ON(ret);
>
> out:
> ref_ptr = (unsigned long)(ref + 1) + namelen;
> @@ -1004,7 +1006,8 @@ static noinline int
> fixup_inode_link_count(struct btrfs_trans_handle *trans,
> btrfs_release_path(root, path);
> if (nlink != inode->i_nlink) {
> inode->i_nlink = nlink;
> - btrfs_update_inode(trans, root, inode);
> + ret = btrfs_update_inode(trans, root, inode);
> + BUG_ON(ret);
> }
> BTRFS_I(inode)->index_cnt = (u64)-1;
>
> @@ -1099,7 +1102,8 @@ static noinline int link_to_fixup_dir(struct
> btrfs_trans_handle *trans,
> btrfs_release_path(root, path);
> if (ret == 0) {
> btrfs_inc_nlink(inode);
> - btrfs_update_inode(trans, root, inode);
> + ret = btrfs_update_inode(trans, root, inode);
> + BUG_ON(ret);
> } else if (ret == -EEXIST) {
> ret = 0;
> } else {
> @@ -1601,7 +1605,8 @@ static int replay_one_buffer(struct btrfs_root
> *log, struct extent_buffer *eb,
> return 0;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
>
> nritems = btrfs_header_nritems(eb);
> for (i = 0; i < nritems; i++) {
> @@ -1707,7 +1712,8 @@ static noinline int walk_down_log_tree(struct
> btrfs_trans_handle *trans,
> return -ENOMEM;
>
> if (*level == 1) {
> - wc->process_func(root, next, wc, ptr_gen);
> + ret = wc->process_func(root, next, wc, ptr_gen);
> + BUG_ON(ret);
>
> path->slots[*level]++;
> if (wc->free) {
> @@ -1772,8 +1778,9 @@ static noinline int walk_up_log_tree(struct
> btrfs_trans_handle *trans,
> parent = path->nodes[*level + 1];
>
> root_owner = btrfs_header_owner(parent);
> - wc->process_func(root, path->nodes[*level], wc,
> + ret = wc->process_func(root, path->nodes[*level], wc,
> btrfs_header_generation(path->nodes[*level]));
> + BUG_ON(ret);
> if (wc->free) {
> struct extent_buffer *next;
>
> @@ -1840,8 +1847,9 @@ static int walk_log_tree(struct btrfs_trans_handle
> *trans,
>
> /* was the root node processed? if not, catch it here */
> if (path->nodes[orig_level]) {
> - wc->process_func(log, path->nodes[orig_level], wc,
> + ret = wc->process_func(log, path->nodes[orig_level], wc,
> btrfs_header_generation(path->nodes[orig_level]));
> + BUG_ON(ret);
> if (wc->free) {
> struct extent_buffer *next;
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 8b9fb8c..fa84172 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1058,7 +1058,8 @@ static noinline int find_next_chunk(struct
> btrfs_root *root,
> struct btrfs_key found_key;
>
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path)
> + return -ENOMEM;
I think that you should review the callers(do_chunk_alloc(),
btrfs_check_data_free_space(), btrfs_reserve_extent(), and so on).
Thanks,
Tsutomu
>
> key.objectid = objectid;
> key.offset = (u64)-1;
> @@ -2067,7 +2068,10 @@ int btrfs_balance(struct btrfs_root *dev_root)
>
> /* step two, relocate all the chunks */
> path = btrfs_alloc_path();
> - BUG_ON(!path);
> + if (!path) {
> + mutex_unlock(&dev_root->fs_info->volume_mutex);
> + return -ENOMEM;
> + }
>
> key.objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
> key.offset = (u64)-1;
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
