Tsutomu Itoh
2011-Jan-20 06:19 UTC
[PATCH] btrfs: fix return value check of btrfs_start_transaction()
The error check of btrfs_start_transaction() is added, and the mistake of the error check on several places is corrected. Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> --- fs/btrfs/extent-tree.c | 7 +++++-- fs/btrfs/inode.c | 1 + fs/btrfs/ioctl.c | 10 ++++++++-- fs/btrfs/relocation.c | 3 +++ fs/btrfs/super.c | 2 ++ fs/btrfs/tree-log.c | 1 + fs/btrfs/volumes.c | 19 +++++++++++++++++-- 7 files changed, 37 insertions(+), 6 deletions(-) diff -urNp linux-2.6.38-rc1/fs/btrfs/extent-tree.c linux-2.6.38-rc1.new/fs/btrfs/extent-tree.c --- linux-2.6.38-rc1/fs/btrfs/extent-tree.c 2011-01-19 08:14:02.000000000 +0900 +++ linux-2.6.38-rc1.new/fs/btrfs/extent-tree.c 2011-01-20 11:35:49.000000000 +0900 @@ -6221,6 +6221,8 @@ int btrfs_drop_snapshot(struct btrfs_roo BUG_ON(!wc); trans = btrfs_start_transaction(tree_root, 0); + BUG_ON(IS_ERR(trans)); + if (block_rsv) trans->block_rsv = block_rsv; @@ -6318,6 +6320,7 @@ int btrfs_drop_snapshot(struct btrfs_roo btrfs_end_transaction_throttle(trans, tree_root); trans = btrfs_start_transaction(tree_root, 0); + BUG_ON(IS_ERR(trans)); if (block_rsv) trans->block_rsv = block_rsv; } @@ -7535,7 +7538,7 @@ int btrfs_cleanup_reloc_trees(struct btr if (found) { trans = btrfs_start_transaction(root, 1); - BUG_ON(!trans); + BUG_ON(IS_ERR(trans)); ret = btrfs_commit_transaction(trans, root); BUG_ON(ret); } @@ -7779,7 +7782,7 @@ static noinline int relocate_one_extent( trans = btrfs_start_transaction(extent_root, 1); - BUG_ON(!trans); + BUG_ON(IS_ERR(trans)); if (extent_key->objectid == 0) { ret = del_extent_zero(trans, extent_root, path, extent_key); diff -urNp linux-2.6.38-rc1/fs/btrfs/inode.c linux-2.6.38-rc1.new/fs/btrfs/inode.c --- linux-2.6.38-rc1/fs/btrfs/inode.c 2011-01-19 08:14:02.000000000 +0900 +++ linux-2.6.38-rc1.new/fs/btrfs/inode.c 2011-01-20 11:35:49.000000000 +0900 @@ -2354,6 +2354,7 @@ void btrfs_orphan_cleanup(struct btrfs_r */ if (is_bad_inode(inode)) { trans = btrfs_start_transaction(root, 0); + BUG_ON(IS_ERR(trans)); btrfs_orphan_del(trans, inode); btrfs_end_transaction(trans, root); iput(inode); diff -urNp linux-2.6.38-rc1/fs/btrfs/ioctl.c linux-2.6.38-rc1.new/fs/btrfs/ioctl.c --- linux-2.6.38-rc1/fs/btrfs/ioctl.c 2011-01-19 08:14:02.000000000 +0900 +++ linux-2.6.38-rc1.new/fs/btrfs/ioctl.c 2011-01-20 11:35:49.000000000 +0900 @@ -907,6 +907,10 @@ static noinline int btrfs_ioctl_resize(s if (new_size > old_size) { trans = btrfs_start_transaction(root, 0); + if (IS_ERR(trans)) { + ret = PTR_ERR(trans); + goto out_unlock; + } ret = btrfs_grow_device(trans, device, new_size); btrfs_commit_transaction(trans, root); } else { @@ -2138,9 +2142,9 @@ static long btrfs_ioctl_default_subvol(s path->leave_spinning = 1; trans = btrfs_start_transaction(root, 1); - if (!trans) { + if (IS_ERR(trans)) { btrfs_free_path(path); - return -ENOMEM; + return PTR_ERR(trans); } dir_id = btrfs_super_root_dir(&root->fs_info->super_copy); @@ -2334,6 +2338,8 @@ static noinline long btrfs_ioctl_start_s u64 transid; trans = btrfs_start_transaction(root, 0); + if (IS_ERR(trans)) + return PTR_ERR(trans); transid = trans->transid; btrfs_commit_transaction_async(trans, root, 0); diff -urNp linux-2.6.38-rc1/fs/btrfs/relocation.c linux-2.6.38-rc1.new/fs/btrfs/relocation.c --- linux-2.6.38-rc1/fs/btrfs/relocation.c 2011-01-19 08:14:02.000000000 +0900 +++ linux-2.6.38-rc1.new/fs/btrfs/relocation.c 2011-01-20 11:35:49.000000000 +0900 @@ -2028,6 +2028,7 @@ static noinline_for_stack int merge_relo while (1) { trans = btrfs_start_transaction(root, 0); + BUG_ON(IS_ERR(trans)); trans->block_rsv = rc->block_rsv; ret = btrfs_block_rsv_check(trans, root, rc->block_rsv, @@ -3657,6 +3658,7 @@ static noinline_for_stack int relocate_b while (1) { trans = btrfs_start_transaction(rc->extent_root, 0); + BUG_ON(IS_ERR(trans)); if (update_backref_cache(trans, &rc->backref_cache)) { btrfs_end_transaction(trans, rc->extent_root); @@ -4022,6 +4024,7 @@ static noinline_for_stack int mark_garba int ret; trans = btrfs_start_transaction(root->fs_info->tree_root, 0); + BUG_ON(IS_ERR(trans)); memset(&root->root_item.drop_progress, 0, sizeof(root->root_item.drop_progress)); diff -urNp linux-2.6.38-rc1/fs/btrfs/super.c linux-2.6.38-rc1.new/fs/btrfs/super.c --- linux-2.6.38-rc1/fs/btrfs/super.c 2011-01-19 08:14:02.000000000 +0900 +++ linux-2.6.38-rc1.new/fs/btrfs/super.c 2011-01-20 11:35:49.000000000 +0900 @@ -623,6 +623,8 @@ int btrfs_sync_fs(struct super_block *sb btrfs_wait_ordered_extents(root, 0, 0); trans = btrfs_start_transaction(root, 0); + if (IS_ERR(trans)) + return PTR_ERR(trans); ret = btrfs_commit_transaction(trans, root); return ret; } diff -urNp linux-2.6.38-rc1/fs/btrfs/tree-log.c linux-2.6.38-rc1.new/fs/btrfs/tree-log.c --- linux-2.6.38-rc1/fs/btrfs/tree-log.c 2011-01-19 08:14:02.000000000 +0900 +++ linux-2.6.38-rc1.new/fs/btrfs/tree-log.c 2011-01-20 11:35:49.000000000 +0900 @@ -3080,6 +3080,7 @@ int btrfs_recover_log_trees(struct btrfs BUG_ON(!path); trans = btrfs_start_transaction(fs_info->tree_root, 0); + BUG_ON(IS_ERR(trans)); wc.trans = trans; wc.pin = 1; diff -urNp linux-2.6.38-rc1/fs/btrfs/volumes.c linux-2.6.38-rc1.new/fs/btrfs/volumes.c --- linux-2.6.38-rc1/fs/btrfs/volumes.c 2011-01-19 08:14:02.000000000 +0900 +++ linux-2.6.38-rc1.new/fs/btrfs/volumes.c 2011-01-20 11:35:49.000000000 +0900 @@ -1213,6 +1213,10 @@ static int btrfs_rm_dev_item(struct btrf return -ENOMEM; trans = btrfs_start_transaction(root, 0); + if (IS_ERR(trans)) { + btrfs_free_path(path); + return PTR_ERR(trans); + } key.objectid = BTRFS_DEV_ITEMS_OBJECTID; key.type = BTRFS_DEV_ITEM_KEY; key.offset = device->devid; @@ -1606,6 +1610,12 @@ int btrfs_init_new_device(struct btrfs_r } trans = btrfs_start_transaction(root, 0); + if (IS_ERR(trans)) { + kfree(device); + ret = PTR_ERR(trans); + goto error; + } + lock_chunks(root); device->writeable = 1; @@ -1873,7 +1883,7 @@ static int btrfs_relocate_chunk(struct b return ret; trans = btrfs_start_transaction(root, 0); - BUG_ON(!trans); + BUG_ON(IS_ERR(trans)); lock_chunks(root); @@ -2047,7 +2057,7 @@ int btrfs_balance(struct btrfs_root *dev BUG_ON(ret); trans = btrfs_start_transaction(dev_root, 0); - BUG_ON(!trans); + BUG_ON(IS_ERR(trans)); ret = btrfs_grow_device(trans, device, old_size); BUG_ON(ret); @@ -2213,6 +2223,11 @@ again: /* Shrinking succeeded, else we would be at "done". */ trans = btrfs_start_transaction(root, 0); + if (IS_ERR(trans)) { + ret = PTR_ERR(trans); + goto done; + } + lock_chunks(root); device->disk_total_bytes = new_size; -- 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
Josef Bacik
2011-Jan-20 16:09 UTC
Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()
On Thu, Jan 20, 2011 at 03:19:37PM +0900, Tsutomu Itoh wrote:> The error check of btrfs_start_transaction() is added, and the mistake > of the error check on several places is corrected. >I''d rather we go through and have these things return an error than do a BUG_ON(). We''re moving towards a more stable BTRFS, not one that panics more often :). Thanks, Josef -- 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
Tsutomu Itoh
2011-Jan-20 23:47 UTC
Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()
(2011/01/21 1:09), Josef Bacik wrote:> I''d rather we go through and have these things return an error than do a > BUG_ON(). We''re moving towards a more stable BTRFS, not one that panics more > often :).Yes, I also think so. This patch is my first step. My modification policy is as follows: 1. short term - To more stable BTRFS, the part that should be corrected is clarified. - The panic is not done by the NULL pointer reference etc. 2. long term - BUG_ON() is decreased by using the forced-readonly framework(already posted by Liu Bo), etc. Thanks, Itoh -- 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
liubo
2011-Jan-21 01:59 UTC
Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()
On 01/21/2011 12:09 AM, Josef Bacik wrote:> On Thu, Jan 20, 2011 at 03:19:37PM +0900, Tsutomu Itoh wrote: >> The error check of btrfs_start_transaction() is added, and the mistake >> of the error check on several places is corrected. >> > > I''d rather we go through and have these things return an error than do a > BUG_ON(). We''re moving towards a more stable BTRFS, not one that panics more > often :). Thanks,Great, seems that we all feel it is the time to focus on this. :)> > Josef > -- > 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 >-- 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
Tsutomu Itoh
2011-Jan-21 06:06 UTC
Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()
(2011/01/21 8:47), Tsutomu Itoh wrote:> (2011/01/21 1:09), Josef Bacik wrote: >> I''d rather we go through and have these things return an error than do a >> BUG_ON(). We''re moving towards a more stable BTRFS, not one that panics more >> often :). > > Yes, I also think so. > This patch is my first step. > > My modification policy is as follows: > > 1. short term > - To more stable BTRFS, the part that should be corrected is clarified. > - The panic is not done by the NULL pointer reference etc.This means, even if temporary increase BUG_ON()... In addition, I think that an important memory allocation should retry several times. So, I propose the following patches as this sample.> > 2. long term > - BUG_ON() is decreased by using the forced-readonly framework(already posted by Liu Bo), > etc.This patch retries kmem_cache_alloc() in start_transaction() several times. Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> --- fs/btrfs/transaction.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff -urNp linux-2.6.38-rc1/fs/btrfs/transaction.c linux-2.6.38-rc1.new/fs/btrfs/transaction.c --- linux-2.6.38-rc1/fs/btrfs/transaction.c 2011-01-19 08:14:02.000000000 +0900 +++ linux-2.6.38-rc1.new/fs/btrfs/transaction.c 2011-01-21 14:08:02.000000000 +0900 @@ -22,6 +22,7 @@ #include <linux/writeback.h> #include <linux/pagemap.h> #include <linux/blkdev.h> +#include <linux/ratelimit.h> #include "ctree.h" #include "disk-io.h" #include "transaction.h" @@ -175,6 +176,25 @@ static int may_wait_transaction(struct b return 0; } +#define MAX_ITERATIONS 10 + +static struct btrfs_trans_handle *alloc_trans_handle(void) +{ + struct btrfs_trans_handle *ret; + int i = 0; + + ret = kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS); + if (!ret) { + pr_notice_ratelimited("ENOMEM in %s, retrying.\n", __func__); + do { + yield(); + ret = kmem_cache_alloc(btrfs_trans_handle_cachep, + GFP_NOFS); + } while (!ret && i++ < MAX_ITERATIONS); + } + return ret; +} + static struct btrfs_trans_handle *start_transaction(struct btrfs_root *root, u64 num_items, int type) { @@ -185,7 +205,7 @@ static struct btrfs_trans_handle *start_ if (root->fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) return ERR_PTR(-EROFS); again: - h = kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS); + h = alloc_trans_handle(); if (!h) return ERR_PTR(-ENOMEM); -- 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
Chris Mason
2011-Jan-28 21:53 UTC
Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()
Excerpts from Tsutomu Itoh''s message of 2011-01-21 01:06:29 -0500:> (2011/01/21 8:47), Tsutomu Itoh wrote: > > (2011/01/21 1:09), Josef Bacik wrote: > >> I''d rather we go through and have these things return an error than do a > >> BUG_ON(). We''re moving towards a more stable BTRFS, not one that panics more > >> often :). > > > > Yes, I also think so. > > This patch is my first step. > > > > My modification policy is as follows: > > > > 1. short term > > - To more stable BTRFS, the part that should be corrected is clarified. > > - The panic is not done by the NULL pointer reference etc. > This means, even if temporary increase BUG_ON()... > > In addition, I think that an important memory allocation should retry several times. > So, I propose the following patches as this sample. > > > > > 2. long term > > - BUG_ON() is decreased by using the forced-readonly framework(already posted by Liu Bo), > > etc. > > > This patch retries kmem_cache_alloc() in start_transaction() several times.Thanks for working on these, it''s really good to see these error checks going on. We don''t want to loop on kmem_cache_alloc(), for allocations less than 4KB, the allocator only returns NULL if the box has gone into OOM anyway. By the time we get these, things have gone horribly wrong. If we really can''t fail, we can use GFP_NOFAIL, which loops for us. -chris> > Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> > --- > fs/btrfs/transaction.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff -urNp linux-2.6.38-rc1/fs/btrfs/transaction.c linux-2.6.38-rc1.new/fs/btrfs/transaction.c > --- linux-2.6.38-rc1/fs/btrfs/transaction.c 2011-01-19 08:14:02.000000000 +0900 > +++ linux-2.6.38-rc1.new/fs/btrfs/transaction.c 2011-01-21 14:08:02.000000000 +0900 > @@ -22,6 +22,7 @@ > #include <linux/writeback.h> > #include <linux/pagemap.h> > #include <linux/blkdev.h> > +#include <linux/ratelimit.h> > #include "ctree.h" > #include "disk-io.h" > #include "transaction.h" > @@ -175,6 +176,25 @@ static int may_wait_transaction(struct b > return 0; > } > > +#define MAX_ITERATIONS 10 > + > +static struct btrfs_trans_handle *alloc_trans_handle(void) > +{ > + struct btrfs_trans_handle *ret; > + int i = 0; > + > + ret = kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS); > + if (!ret) { > + pr_notice_ratelimited("ENOMEM in %s, retrying.\n", __func__); > + do { > + yield(); > + ret = kmem_cache_alloc(btrfs_trans_handle_cachep, > + GFP_NOFS); > + } while (!ret && i++ < MAX_ITERATIONS); > + } > + return ret; > +} > + > static struct btrfs_trans_handle *start_transaction(struct btrfs_root *root, > u64 num_items, int type) > { > @@ -185,7 +205,7 @@ static struct btrfs_trans_handle *start_ > if (root->fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) > return ERR_PTR(-EROFS); > again: > - h = kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS); > + h = alloc_trans_handle(); > if (!h) > return ERR_PTR(-ENOMEM); >-- 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
Tsutomu Itoh
2011-Jan-31 00:03 UTC
Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()
Hi, Chris, (2011/01/29 6:53), Chris Mason wrote:> Excerpts from Tsutomu Itoh''s message of 2011-01-21 01:06:29 -0500: >> (2011/01/21 8:47), Tsutomu Itoh wrote: >>> (2011/01/21 1:09), Josef Bacik wrote: >>>> I''d rather we go through and have these things return an error than do a >>>> BUG_ON(). We''re moving towards a more stable BTRFS, not one that panics more >>>> often :). >>> >>> Yes, I also think so. >>> This patch is my first step. >>> >>> My modification policy is as follows: >>> >>> 1. short term >>> - To more stable BTRFS, the part that should be corrected is clarified. >>> - The panic is not done by the NULL pointer reference etc. >> This means, even if temporary increase BUG_ON()... >> >> In addition, I think that an important memory allocation should retry several times. >> So, I propose the following patches as this sample. >> >>> >>> 2. long term >>> - BUG_ON() is decreased by using the forced-readonly framework(already posted by Liu Bo), >>> etc. >> >> >> This patch retries kmem_cache_alloc() in start_transaction() several times. > > Thanks for working on these, it''s really good to see these error checks > going on. >> We don''t want to loop on kmem_cache_alloc(), for allocations less than > 4KB, the allocator only returns NULL if the box has gone into OOM > anyway. By the time we get these, things have gone horribly wrong. > > If we really can''t fail, we can use GFP_NOFAIL, which loops for us.OK, I understand. Thanks, Itoh> > -chris > >> >> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> >> --- >> fs/btrfs/transaction.c | 22 +++++++++++++++++++++- >> 1 file changed, 21 insertions(+), 1 deletion(-) >> >> diff -urNp linux-2.6.38-rc1/fs/btrfs/transaction.c linux-2.6.38-rc1.new/fs/btrfs/transaction.c >> --- linux-2.6.38-rc1/fs/btrfs/transaction.c 2011-01-19 08:14:02.000000000 +0900 >> +++ linux-2.6.38-rc1.new/fs/btrfs/transaction.c 2011-01-21 14:08:02.000000000 +0900 >> @@ -22,6 +22,7 @@ >> #include <linux/writeback.h> >> #include <linux/pagemap.h> >> #include <linux/blkdev.h> >> +#include <linux/ratelimit.h> >> #include "ctree.h" >> #include "disk-io.h" >> #include "transaction.h" >> @@ -175,6 +176,25 @@ static int may_wait_transaction(struct b >> return 0; >> } >> >> +#define MAX_ITERATIONS 10 >> + >> +static struct btrfs_trans_handle *alloc_trans_handle(void) >> +{ >> + struct btrfs_trans_handle *ret; >> + int i = 0; >> + >> + ret = kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS); >> + if (!ret) { >> + pr_notice_ratelimited("ENOMEM in %s, retrying.\n", __func__); >> + do { >> + yield(); >> + ret = kmem_cache_alloc(btrfs_trans_handle_cachep, >> + GFP_NOFS); >> + } while (!ret && i++ < MAX_ITERATIONS); >> + } >> + return ret; >> +} >> + >> static struct btrfs_trans_handle *start_transaction(struct btrfs_root *root, >> u64 num_items, int type) >> { >> @@ -185,7 +205,7 @@ static struct btrfs_trans_handle *start_ >> if (root->fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) >> return ERR_PTR(-EROFS); >> again: >> - h = kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS); >> + h = alloc_trans_handle(); >> if (!h) >> return ERR_PTR(-ENOMEM); >>-- 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
Tsutomu Itoh
2011-Feb-01 02:15 UTC
Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()
Hi Chris, (2011/01/29 6:53), Chris Mason wrote:> Excerpts from Tsutomu Itoh''s message of 2011-01-21 01:06:29 -0500: >> (2011/01/21 8:47), Tsutomu Itoh wrote: >>> (2011/01/21 1:09), Josef Bacik wrote: >>>> I''d rather we go through and have these things return an error than do a >>>> BUG_ON(). We''re moving towards a more stable BTRFS, not one that panics more >>>> often :). >>> >>> Yes, I also think so. >>> This patch is my first step. >>> >>> My modification policy is as follows: >>> >>> 1. short term >>> - To more stable BTRFS, the part that should be corrected is clarified. >>> - The panic is not done by the NULL pointer reference etc. >> This means, even if temporary increase BUG_ON()... >> >> In addition, I think that an important memory allocation should retry several times. >> So, I propose the following patches as this sample. >> >>> >>> 2. long term >>> - BUG_ON() is decreased by using the forced-readonly framework(already posted by Liu Bo), >>> etc. >> >> >> This patch retries kmem_cache_alloc() in start_transaction() several times. > > Thanks for working on these, it''s really good to see these error checks > going on. > > We don''t want to loop on kmem_cache_alloc(), for allocations less than > 4KB, the allocator only returns NULL if the box has gone into OOM > anyway. By the time we get these, things have gone horribly wrong. > > If we really can''t fail, we can use GFP_NOFAIL, which loops for us.I agree to your opinion, and please ignore following patch. But, please apply http://marc.info/?l=linux-btrfs&m=129550441505242&w=2 Thanks, Itoh> > -chris > >> >> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> >> --- >> fs/btrfs/transaction.c | 22 +++++++++++++++++++++- >> 1 file changed, 21 insertions(+), 1 deletion(-) >> >> diff -urNp linux-2.6.38-rc1/fs/btrfs/transaction.c linux-2.6.38-rc1.new/fs/btrfs/transaction.c >> --- linux-2.6.38-rc1/fs/btrfs/transaction.c 2011-01-19 08:14:02.000000000 +0900 >> +++ linux-2.6.38-rc1.new/fs/btrfs/transaction.c 2011-01-21 14:08:02.000000000 +0900 >> @@ -22,6 +22,7 @@ >> #include <linux/writeback.h> >> #include <linux/pagemap.h> >> #include <linux/blkdev.h> >> +#include <linux/ratelimit.h> >> #include "ctree.h" >> #include "disk-io.h" >> #include "transaction.h" >> @@ -175,6 +176,25 @@ static int may_wait_transaction(struct b >> return 0; >> } >> >> +#define MAX_ITERATIONS 10 >> + >> +static struct btrfs_trans_handle *alloc_trans_handle(void) >> +{ >> + struct btrfs_trans_handle *ret; >> + int i = 0; >> + >> + ret = kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS); >> + if (!ret) { >> + pr_notice_ratelimited("ENOMEM in %s, retrying.\n", __func__); >> + do { >> + yield(); >> + ret = kmem_cache_alloc(btrfs_trans_handle_cachep, >> + GFP_NOFS); >> + } while (!ret && i++ < MAX_ITERATIONS); >> + } >> + return ret; >> +} >> + >> static struct btrfs_trans_handle *start_transaction(struct btrfs_root *root, >> u64 num_items, int type) >> { >> @@ -185,7 +205,7 @@ static struct btrfs_trans_handle *start_ >> if (root->fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) >> return ERR_PTR(-EROFS); >> again: >> - h = kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS); >> + h = alloc_trans_handle(); >> if (!h) >> return ERR_PTR(-ENOMEM); >>-- 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
Chris Mason
2011-Feb-01 12:38 UTC
Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()
Excerpts from Tsutomu Itoh''s message of 2011-01-31 21:15:32 -0500:> Hi Chris, > > (2011/01/29 6:53), Chris Mason wrote: > > Excerpts from Tsutomu Itoh''s message of 2011-01-21 01:06:29 -0500: > >> (2011/01/21 8:47), Tsutomu Itoh wrote: > >>> (2011/01/21 1:09), Josef Bacik wrote: > >>>> I''d rather we go through and have these things return an error than do a > >>>> BUG_ON(). We''re moving towards a more stable BTRFS, not one that panics more > >>>> often :). > >>> > >>> Yes, I also think so. > >>> This patch is my first step. > >>> > >>> My modification policy is as follows: > >>> > >>> 1. short term > >>> - To more stable BTRFS, the part that should be corrected is clarified. > >>> - The panic is not done by the NULL pointer reference etc. > >> This means, even if temporary increase BUG_ON()... > >> > >> In addition, I think that an important memory allocation should retry several times. > >> So, I propose the following patches as this sample. > >> > >>> > >>> 2. long term > >>> - BUG_ON() is decreased by using the forced-readonly framework(already posted by Liu Bo), > >>> etc. > >> > >> > >> This patch retries kmem_cache_alloc() in start_transaction() several times. > > > > Thanks for working on these, it''s really good to see these error checks > > going on. > > > > We don''t want to loop on kmem_cache_alloc(), for allocations less than > > 4KB, the allocator only returns NULL if the box has gone into OOM > > anyway. By the time we get these, things have gone horribly wrong. > > > > If we really can''t fail, we can use GFP_NOFAIL, which loops for us. > > I agree to your opinion, and please ignore following patch. > But, please apply http://marc.info/?l=linux-btrfs&m=129550441505242&w=2Thanks, I have this one now as well. -chris -- 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