On Wed, Aug 07, 2019 at 11:18:20AM +0300, Nikolay Borisov wrote:
> >> @@ -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 ?
I agree with Filipe and the common error handling style is to coalesce
the common code at the end of the function. In this function there are
several places that decrement the nocow writers and then jump to error.
So this asks for a cleanup that does something like that (untested):
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1314,6 +1314,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
bool check_prev = true;
const bool freespace_inode = btrfs_is_free_space_inode(BTRFS_I(inode));
u64 ino = btrfs_ino(BTRFS_I(inode));
+ bool nocow = false;
+ u64 disk_bytenr = 0;
path = btrfs_alloc_path();
if (!path) {
@@ -1333,12 +1335,12 @@ static noinline int run_delalloc_nocow(struct inode *inode,
struct extent_buffer *leaf;
u64 extent_end;
u64 extent_offset;
- u64 disk_bytenr = 0;
u64 num_bytes = 0;
u64 disk_num_bytes;
u64 ram_bytes;
int extent_type;
- bool nocow = false;
+
+ nocow = false;
ret = btrfs_lookup_file_extent(NULL, root, path, ino,
cur_offset, 0);
@@ -1541,12 +1543,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
ret = cow_file_range(inode, locked_page,
cow_start, found_key.offset - 1,
page_started, nr_written, 1);
- if (ret) {
- if (nocow)
- btrfs_dec_nocow_writers(fs_info,
- disk_bytenr);
+ if (ret)
goto error;
- }
cow_start = (u64)-1;
}
@@ -1562,9 +1560,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
ram_bytes, BTRFS_COMPRESS_NONE,
BTRFS_ORDERED_PREALLOC);
if (IS_ERR(em)) {
- if (nocow)
- btrfs_dec_nocow_writers(fs_info,
- disk_bytenr);
ret = PTR_ERR(em);
goto error;
}
@@ -1583,6 +1578,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
if (nocow)
btrfs_dec_nocow_writers(fs_info, disk_bytenr);
BUG_ON(ret); /* -ENOMEM */
+ nocow = false;
if (root->root_key.objectid ==
BTRFS_DATA_RELOC_TREE_OBJECTID)
@@ -1627,6 +1623,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
}
error:
+ if (nocow)
+ btrfs_dec_nocow_writers(fs_info, disk_bytenr);
if (ret && cur_offset < end)
extent_clear_unlock_delalloc(inode, cur_offset, end,
locked_page, EXTENT_LOCKED |
