Re: [PATCH 6/6] btrfs: Remove BUG_ON from run_delalloc_nocow

[Nikolay Borisov]

On 6.08.19 г. 13:34 ч., Filipe Manana wrote:
> On Mon, Aug 5, 2019 at 3:47 PM Nikolay Borisov <nbori...@suse.com> wrote:
>>
>> Correctly handle failure cases when adding an ordered extents in case
>> of REGULAR or PREALLOC extents.
>>
>> Signed-off-by: Nikolay Borisov <nbori...@suse.com>
> 
> Reviewed-by: Filipe Manana <fdman...@suse.com>
> 
> It's correct, but:
> 
>> ---
>>  fs/btrfs/inode.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 6c3f9f3a7ed1..b935c301ca72 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -1569,16 +1569,26 @@ static noinline int run_delalloc_nocow(struct inode 
>> *inode,
>>                                                        disk_bytenr, 
>> num_bytes,
>>                                                        num_bytes,
>>                                                        
>> BTRFS_ORDERED_PREALLOC);
>> +                       if (nocow)
>> +                               btrfs_dec_nocow_writers(fs_info, 
>> disk_bytenr);
>> +                       if (ret) {
>> +                               btrfs_drop_extent_cache(BTRFS_I(inode),
>> +                                                       cur_offset,
>> +                                                       cur_offset + 
>> num_bytes - 1,
>> +                                                       0);
>> +                               goto error;
>> +                       }
>>                 } else {
>>                         ret = btrfs_add_ordered_extent(inode, cur_offset,
>>                                                        disk_bytenr, 
>> num_bytes,
>>                                                        num_bytes,
>>                                                        BTRFS_ORDERED_NOCOW);
>> +                       if (nocow)
>> +                               btrfs_dec_nocow_writers(fs_info, 
>> disk_bytenr);
>> +                       if (ret)
>> +                               goto error;
> 
> We are now duplicating some error handling. Could be done like before,
> outside the if branches.
> 

Dependson the POV. IMO it's better to have all error handling for the
respective branch in one place. That way when someone is reading the
function and gets to that branch they see that in case one of the
functions fail what is the error handling. Otherwise as they are
scanning the code they'd have to look up and see if something tricky is
going on.

David, what's your take on that ?

>