diff mbox

[9/9] btrfs: Push up non-looped btrfs_start_transaction failures

Message ID 20110810232123.600894199@suse.com
State New, archived
Headers show

Commit Message

Jeff Mahoney Aug. 10, 2011, 11:20 p.m. UTC
This patch handles btrfs_start_transaction failures that don't occur
 in a loop and are obvious to simply push up. In all cases except the
 mark_garbage_root case, the error is already handled by BUG_ON in the
 caller.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/btrfs/extent-tree.c |    6 +++++-
 fs/btrfs/relocation.c  |    6 ++++--
 fs/btrfs/tree-log.c    |    5 ++++-
 fs/btrfs/volumes.c     |    3 ++-
 4 files changed, 15 insertions(+), 5 deletions(-)



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

Comments

Tsutomu Itoh Aug. 11, 2011, 1:27 a.m. UTC | #1
Hi, Jeff,

(2011/08/11 8:20), Jeff Mahoney wrote:
>  This patch handles btrfs_start_transaction failures that don't occur
>  in a loop and are obvious to simply push up. In all cases except the
>  mark_garbage_root case, the error is already handled by BUG_ON in the
>  caller.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  fs/btrfs/extent-tree.c |    6 +++++-
>  fs/btrfs/relocation.c  |    6 ++++--
>  fs/btrfs/tree-log.c    |    5 ++++-
>  fs/btrfs/volumes.c     |    3 ++-
>  4 files changed, 15 insertions(+), 5 deletions(-)
> 
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6319,7 +6319,11 @@ int btrfs_drop_snapshot(struct btrfs_roo
>  	}
>  
>  	trans = btrfs_start_transaction(tree_root, 0);
> -	BUG_ON(IS_ERR(trans));
> +	if (IS_ERR(trans)) {
> +		kfree(wc);
> +		btrfs_free_path(path);
> +		return PTR_ERR(trans);
> +	}

The caller of btrfs_drop_snapshot() ignore the error. So, I don't think
that it is significant even if the error is returned to the caller.

I think that it should make the filesystem readonly when becoming an error
in btrfs_start_transaction().

Thanks,
Tsutomu
 
>  
>  	if (block_rsv)
>  		trans->block_rsv = block_rsv;
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4096,7 +4096,8 @@ static noinline_for_stack int mark_garba
>  	int ret;
>  
>  	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
> -	BUG_ON(IS_ERR(trans));
> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
>  
>  	memset(&root->root_item.drop_progress, 0,
>  		sizeof(root->root_item.drop_progress));
> @@ -4176,7 +4177,8 @@ int btrfs_recover_relocation(struct btrf
>  					err = ret;
>  					goto out;
>  				}
> -				mark_garbage_root(reloc_root);
> +				ret = mark_garbage_root(reloc_root);
> +				BUG_ON(ret);
>  			}
>  		}
>  
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3151,7 +3151,10 @@ int btrfs_recover_log_trees(struct btrfs
>  	fs_info->log_root_recovering = 1;
>  
>  	trans = btrfs_start_transaction(fs_info->tree_root, 0);
> -	BUG_ON(IS_ERR(trans));
> +	if (IS_ERR(trans)) {
> +		btrfs_free_path(path);
> +		return PTR_ERR(trans);
> +	}
>  
>  	wc.trans = trans;
>  	wc.pin = 1;
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1876,7 +1876,8 @@ static int btrfs_relocate_chunk(struct b
>  		return ret;
>  
>  	trans = btrfs_start_transaction(root, 0);
> -	BUG_ON(IS_ERR(trans));
> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
>  
>  	lock_chunks(root);
>  

--
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
Jeff Mahoney Aug. 11, 2011, 1:58 a.m. UTC | #2
On 08/10/2011 09:27 PM, Tsutomu Itoh wrote:
> Hi, Jeff,
> 
> (2011/08/11 8:20), Jeff Mahoney wrote:
>>   This patch handles btrfs_start_transaction failures that don't occur
>>   in a loop and are obvious to simply push up. In all cases except the
>>   mark_garbage_root case, the error is already handled by BUG_ON in the
>>   caller.
>>
>> Signed-off-by: Jeff Mahoney<jeffm@suse.com>
>> ---
>>   fs/btrfs/extent-tree.c |    6 +++++-
>>   fs/btrfs/relocation.c  |    6 ++++--
>>   fs/btrfs/tree-log.c    |    5 ++++-
>>   fs/btrfs/volumes.c     |    3 ++-
>>   4 files changed, 15 insertions(+), 5 deletions(-)
>>
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -6319,7 +6319,11 @@ int btrfs_drop_snapshot(struct btrfs_roo
>>   	}
>>
>>   	trans = btrfs_start_transaction(tree_root, 0);
>> -	BUG_ON(IS_ERR(trans));
>> +	if (IS_ERR(trans)) {
>> +		kfree(wc);
>> +		btrfs_free_path(path);
>> +		return PTR_ERR(trans);
>> +	}
> 
> The caller of btrfs_drop_snapshot() ignore the error. So, I don't think
> that it is significant even if the error is returned to the caller.

 I'd actually consider that a separate issue since btrfs_drop_snapshot
also returns -ENOMEM. The errors should be properly caught or BUG_ON'd
in the caller. My patchset usually catches cases like this but since
btrfs_drop_snapshot already returned an error, I mistakenly assumed it
was handled by the caller.

> I think that it should make the filesystem readonly when becoming an error
> in btrfs_start_transaction().

For -ENOMEM, I don't think that's the way to handle it. Some transaction
start failures can be caught and handled (e.g. just creating a file)
easily by returning errors to the user. Other cases, deep in the code,
may be too complex to unwind and recover from and then a ROFS is the
next-best answer. The callers should be responsible for determining the
correct course of action.

-Jeff

> Thanks,
> Tsutomu
> 
>>
>>   	if (block_rsv)
>>   		trans->block_rsv = block_rsv;
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -4096,7 +4096,8 @@ static noinline_for_stack int mark_garba
>>   	int ret;
>>
>>   	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
>> -	BUG_ON(IS_ERR(trans));
>> +	if (IS_ERR(trans))
>> +		return PTR_ERR(trans);
>>
>>   	memset(&root->root_item.drop_progress, 0,
>>   		sizeof(root->root_item.drop_progress));
>> @@ -4176,7 +4177,8 @@ int btrfs_recover_relocation(struct btrf
>>   					err = ret;
>>   					goto out;
>>   				}
>> -				mark_garbage_root(reloc_root);
>> +				ret = mark_garbage_root(reloc_root);
>> +				BUG_ON(ret);
>>   			}
>>   		}
>>
>> --- a/fs/btrfs/tree-log.c
>> +++ b/fs/btrfs/tree-log.c
>> @@ -3151,7 +3151,10 @@ int btrfs_recover_log_trees(struct btrfs
>>   	fs_info->log_root_recovering = 1;
>>
>>   	trans = btrfs_start_transaction(fs_info->tree_root, 0);
>> -	BUG_ON(IS_ERR(trans));
>> +	if (IS_ERR(trans)) {
>> +		btrfs_free_path(path);
>> +		return PTR_ERR(trans);
>> +	}
>>
>>   	wc.trans = trans;
>>   	wc.pin = 1;
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1876,7 +1876,8 @@ static int btrfs_relocate_chunk(struct b
>>   		return ret;
>>
>>   	trans = btrfs_start_transaction(root, 0);
>> -	BUG_ON(IS_ERR(trans));
>> +	if (IS_ERR(trans))
>> +		return PTR_ERR(trans);
>>
>>   	lock_chunks(root);
>>
>
Tsutomu Itoh Aug. 11, 2011, 2:18 a.m. UTC | #3
(2011/08/11 10:58), Jeff Mahoney wrote:
> On 08/10/2011 09:27 PM, Tsutomu Itoh wrote:
>> Hi, Jeff,
>>
>> (2011/08/11 8:20), Jeff Mahoney wrote:
>>>   This patch handles btrfs_start_transaction failures that don't occur
>>>   in a loop and are obvious to simply push up. In all cases except the
>>>   mark_garbage_root case, the error is already handled by BUG_ON in the
>>>   caller.
>>>
>>> Signed-off-by: Jeff Mahoney<jeffm@suse.com>
>>> ---
>>>   fs/btrfs/extent-tree.c |    6 +++++-
>>>   fs/btrfs/relocation.c  |    6 ++++--
>>>   fs/btrfs/tree-log.c    |    5 ++++-
>>>   fs/btrfs/volumes.c     |    3 ++-
>>>   4 files changed, 15 insertions(+), 5 deletions(-)
>>>
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -6319,7 +6319,11 @@ int btrfs_drop_snapshot(struct btrfs_roo
>>>   	}
>>>
>>>   	trans = btrfs_start_transaction(tree_root, 0);
>>> -	BUG_ON(IS_ERR(trans));
>>> +	if (IS_ERR(trans)) {
>>> +		kfree(wc);
>>> +		btrfs_free_path(path);
>>> +		return PTR_ERR(trans);
>>> +	}
>>
>> The caller of btrfs_drop_snapshot() ignore the error. So, I don't think
>> that it is significant even if the error is returned to the caller.
> 
>  I'd actually consider that a separate issue since btrfs_drop_snapshot
> also returns -ENOMEM. The errors should be properly caught or BUG_ON'd
> in the caller. My patchset usually catches cases like this but since
> btrfs_drop_snapshot already returned an error, I mistakenly assumed it
> was handled by the caller.
> 
>> I think that it should make the filesystem readonly when becoming an error
>> in btrfs_start_transaction().
> 
> For -ENOMEM, I don't think that's the way to handle it. Some transaction
> start failures can be caught and handled (e.g. just creating a file)
> easily by returning errors to the user. Other cases, deep in the code,
> may be too complex to unwind and recover from and then a ROFS is the
> next-best answer. The callers should be responsible for determining the
> correct course of action.

OK.
Could you please append BUG_ON() in the caller or correctly handle the error
of btrfs_start_transaction()?

 -Tsutomu

> 
> -Jeff
> 
>> Thanks,
>> Tsutomu
>>
>>>
>>>   	if (block_rsv)
>>>   		trans->block_rsv = block_rsv;
>>> --- a/fs/btrfs/relocation.c
>>> +++ b/fs/btrfs/relocation.c
>>> @@ -4096,7 +4096,8 @@ static noinline_for_stack int mark_garba
>>>   	int ret;
>>>
>>>   	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
>>> -	BUG_ON(IS_ERR(trans));
>>> +	if (IS_ERR(trans))
>>> +		return PTR_ERR(trans);
>>>
>>>   	memset(&root->root_item.drop_progress, 0,
>>>   		sizeof(root->root_item.drop_progress));
>>> @@ -4176,7 +4177,8 @@ int btrfs_recover_relocation(struct btrf
>>>   					err = ret;
>>>   					goto out;
>>>   				}
>>> -				mark_garbage_root(reloc_root);
>>> +				ret = mark_garbage_root(reloc_root);
>>> +				BUG_ON(ret);
>>>   			}
>>>   		}
>>>
>>> --- a/fs/btrfs/tree-log.c
>>> +++ b/fs/btrfs/tree-log.c
>>> @@ -3151,7 +3151,10 @@ int btrfs_recover_log_trees(struct btrfs
>>>   	fs_info->log_root_recovering = 1;
>>>
>>>   	trans = btrfs_start_transaction(fs_info->tree_root, 0);
>>> -	BUG_ON(IS_ERR(trans));
>>> +	if (IS_ERR(trans)) {
>>> +		btrfs_free_path(path);
>>> +		return PTR_ERR(trans);
>>> +	}
>>>
>>>   	wc.trans = trans;
>>>   	wc.pin = 1;
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -1876,7 +1876,8 @@ static int btrfs_relocate_chunk(struct b
>>>   		return ret;
>>>
>>>   	trans = btrfs_start_transaction(root, 0);
>>> -	BUG_ON(IS_ERR(trans));
>>> +	if (IS_ERR(trans))
>>> +		return PTR_ERR(trans);
>>>
>>>   	lock_chunks(root);
>>>
>>

--
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
Jeff Mahoney Aug. 11, 2011, 2:32 a.m. UTC | #4
On 08/10/2011 10:18 PM, Tsutomu Itoh wrote:
> (2011/08/11 10:58), Jeff Mahoney wrote:
>> On 08/10/2011 09:27 PM, Tsutomu Itoh wrote:
>>> Hi, Jeff,
>>>
>>> (2011/08/11 8:20), Jeff Mahoney wrote:
>>>>    This patch handles btrfs_start_transaction failures that don't occur
>>>>    in a loop and are obvious to simply push up. In all cases except the
>>>>    mark_garbage_root case, the error is already handled by BUG_ON in the
>>>>    caller.
>>>>
>>>> Signed-off-by: Jeff Mahoney<jeffm@suse.com>
>>>> ---
>>>>    fs/btrfs/extent-tree.c |    6 +++++-
>>>>    fs/btrfs/relocation.c  |    6 ++++--
>>>>    fs/btrfs/tree-log.c    |    5 ++++-
>>>>    fs/btrfs/volumes.c     |    3 ++-
>>>>    4 files changed, 15 insertions(+), 5 deletions(-)
>>>>
>>>> --- a/fs/btrfs/extent-tree.c
>>>> +++ b/fs/btrfs/extent-tree.c
>>>> @@ -6319,7 +6319,11 @@ int btrfs_drop_snapshot(struct btrfs_roo
>>>>    	}
>>>>
>>>>    	trans = btrfs_start_transaction(tree_root, 0);
>>>> -	BUG_ON(IS_ERR(trans));
>>>> +	if (IS_ERR(trans)) {
>>>> +		kfree(wc);
>>>> +		btrfs_free_path(path);
>>>> +		return PTR_ERR(trans);
>>>> +	}
>>>
>>> The caller of btrfs_drop_snapshot() ignore the error. So, I don't think
>>> that it is significant even if the error is returned to the caller.
>>
>>   I'd actually consider that a separate issue since btrfs_drop_snapshot
>> also returns -ENOMEM. The errors should be properly caught or BUG_ON'd
>> in the caller. My patchset usually catches cases like this but since
>> btrfs_drop_snapshot already returned an error, I mistakenly assumed it
>> was handled by the caller.
>>
>>> I think that it should make the filesystem readonly when becoming an error
>>> in btrfs_start_transaction().
>>
>> For -ENOMEM, I don't think that's the way to handle it. Some transaction
>> start failures can be caught and handled (e.g. just creating a file)
>> easily by returning errors to the user. Other cases, deep in the code,
>> may be too complex to unwind and recover from and then a ROFS is the
>> next-best answer. The callers should be responsible for determining the
>> correct course of action.
> 
> OK.
> Could you please append BUG_ON() in the caller or correctly handle the error
> of btrfs_start_transaction()?

Yep. I'll add that in. Thanks for the review.

-Jef

>> -Jeff
>>
>>> Thanks,
>>> Tsutomu
>>>
>>>>
>>>>    	if (block_rsv)
>>>>    		trans->block_rsv = block_rsv;
>>>> --- a/fs/btrfs/relocation.c
>>>> +++ b/fs/btrfs/relocation.c
>>>> @@ -4096,7 +4096,8 @@ static noinline_for_stack int mark_garba
>>>>    	int ret;
>>>>
>>>>    	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
>>>> -	BUG_ON(IS_ERR(trans));
>>>> +	if (IS_ERR(trans))
>>>> +		return PTR_ERR(trans);
>>>>
>>>>    	memset(&root->root_item.drop_progress, 0,
>>>>    		sizeof(root->root_item.drop_progress));
>>>> @@ -4176,7 +4177,8 @@ int btrfs_recover_relocation(struct btrf
>>>>    					err = ret;
>>>>    					goto out;
>>>>    				}
>>>> -				mark_garbage_root(reloc_root);
>>>> +				ret = mark_garbage_root(reloc_root);
>>>> +				BUG_ON(ret);
>>>>    			}
>>>>    		}
>>>>
>>>> --- a/fs/btrfs/tree-log.c
>>>> +++ b/fs/btrfs/tree-log.c
>>>> @@ -3151,7 +3151,10 @@ int btrfs_recover_log_trees(struct btrfs
>>>>    	fs_info->log_root_recovering = 1;
>>>>
>>>>    	trans = btrfs_start_transaction(fs_info->tree_root, 0);
>>>> -	BUG_ON(IS_ERR(trans));
>>>> +	if (IS_ERR(trans)) {
>>>> +		btrfs_free_path(path);
>>>> +		return PTR_ERR(trans);
>>>> +	}
>>>>
>>>>    	wc.trans = trans;
>>>>    	wc.pin = 1;
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -1876,7 +1876,8 @@ static int btrfs_relocate_chunk(struct b
>>>>    		return ret;
>>>>
>>>>    	trans = btrfs_start_transaction(root, 0);
>>>> -	BUG_ON(IS_ERR(trans));
>>>> +	if (IS_ERR(trans))
>>>> +		return PTR_ERR(trans);
>>>>
>>>>    	lock_chunks(root);
>>>>
>>>
>
diff mbox

Patch

--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6319,7 +6319,11 @@  int btrfs_drop_snapshot(struct btrfs_roo
 	}
 
 	trans = btrfs_start_transaction(tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		kfree(wc);
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}
 
 	if (block_rsv)
 		trans->block_rsv = block_rsv;
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4096,7 +4096,8 @@  static noinline_for_stack int mark_garba
 	int ret;
 
 	trans = btrfs_start_transaction(root->fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
 	memset(&root->root_item.drop_progress, 0,
 		sizeof(root->root_item.drop_progress));
@@ -4176,7 +4177,8 @@  int btrfs_recover_relocation(struct btrf
 					err = ret;
 					goto out;
 				}
-				mark_garbage_root(reloc_root);
+				ret = mark_garbage_root(reloc_root);
+				BUG_ON(ret);
 			}
 		}
 
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3151,7 +3151,10 @@  int btrfs_recover_log_trees(struct btrfs
 	fs_info->log_root_recovering = 1;
 
 	trans = btrfs_start_transaction(fs_info->tree_root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		btrfs_free_path(path);
+		return PTR_ERR(trans);
+	}
 
 	wc.trans = trans;
 	wc.pin = 1;
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1876,7 +1876,8 @@  static int btrfs_relocate_chunk(struct b
 		return ret;
 
 	trans = btrfs_start_transaction(root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
 	lock_chunks(root);