[PATCH] btrfs: fix return value check of btrfs_start_transaction()

[PATCH] btrfs: fix return value check of btrfs_start_transaction()

[Tsutomu Itoh]

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;

Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()

[Josef Bacik]

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

Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()

[Tsutomu Itoh]

(2011/01/21 1:09), Josef Bacik wrote:
> I'd rather we go through and have these things return an error than d=
o a
> BUG_ON().  We're moving towards a more stable BTRFS, not one that pan=
ics 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=
=2E=20
 - The panic is not done by the NULL pointer reference etc.

2. long term
=E3=80=80- BUG_ON() is decreased by using the forced-readonly framework=
(already posted by Liu Bo),
    etc.=20

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

Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()

[Tsutomu Itoh]

(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);
 

Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()

[Chris Mason]

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

Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()

[Tsutomu Itoh]

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

Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()

[Tsutomu Itoh]

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

Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()

[Chris Mason]

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

Thanks, I have this one now as well.

-chris

Re: [PATCH] btrfs: fix return value check of btrfs_start_transaction()

[liubo]

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
>