From: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
To: Yoshinori Sano <yoshinori.sano@gmail.com>
Cc: linux-btrfs@vger.kernel.org, Chris Mason <chris.mason@oracle.com>
Subject: Re: [PATCH] Btrfs: cleanup btrfs_alloc_path()'s caller code
Date: Tue, 12 Apr 2011 14:25:24 +0900 [thread overview]
Message-ID: <4DA3E244.9020008@jp.fujitsu.com> (raw)
In-Reply-To: <BANLkTik5YWVX1rs0HhOd8p4n6SY1mh-UmA@mail.gmail.com>
(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.sano@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;
prev parent reply other threads:[~2011-04-12 5:25 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-09 2:23 [PATCH] Btrfs: cleanup btrfs_alloc_path()'s caller code Yoshinori Sano
2011-04-11 3:20 ` Tsutomu Itoh
2011-04-11 22:46 ` Yoshinori Sano
2011-04-12 5:25 ` Tsutomu Itoh [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DA3E244.9020008@jp.fujitsu.com \
--to=t-itoh@jp.fujitsu.com \
--cc=chris.mason@oracle.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=yoshinori.sano@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.