Re: [PATCH 7/7] btrfs: don't BUG_ON allocation errors in btrfs_drop_snapshot

[Justin Ossevoort <justin@internetionals.nl>]

On 25/07/11 23:10, Mark Fasheh wrote:
> On Fri, Jul 22, 2011 at 09:45:19AM +0900, Tsutomu Itoh wrote:
>> (2011/07/22 4:48), Mark Fasheh wrote:
>>> In addition to properly handling allocation failure from btrfs_alloc_path, I
>>> also fixed up the kzalloc error handling code immediately below it.
>>>
>>> Signed-off-by: Mark Fasheh <mfasheh@suse.com>
>>> ---
>>>  fs/btrfs/extent-tree.c |    8 ++++++--
>>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index ff339b2..4cf5257 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -6271,10 +6271,14 @@ 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);
>>> +	if (!wc) {
>>> +		btrfs_free_path(path);
>>> +		return -ENOMEM;
>>> +	}
>>>  
>>>  	trans = btrfs_start_transaction(tree_root, 0);
>>>  	BUG_ON(IS_ERR(trans));
>>
>> Currently, callers of btrfs_drop_snapshot() ignore the return code.
>> But btrfs_drop_snapshot() detects the error by BUG_ON.
>>
>> The caller still ignore the return code though your modification returns
>> the error code to the caller. 
>> So, we can not detect error. I don't think that it is good.
> 
> IMHO, this is a seperate issue that btrfs_drop_snapshot() has even without
> my patch. You can see in the code that it might return any number of errors,
> all of which get ignored by callers. So my patch is cleaning up some of the
> BUG_ON() usage, but not really solving the 2nd problem of ignored return
> codes. Of course that was on purpose as I like to fix one problem per patch
> if possible and practicle.
> 	--Mark

I Think Tsutomu's point was more that you've changed the behavior from a
BUG() on error to silently ignoring the error.

So you should at least add 'BUG_ON(ERR_PTR(...) == -ENOMEM)' in the
callers to maintain the current behavior while still pushing the check
up the call chain.

Regards,

	justin....