| Message ID | 14bd267ea479d4c4d104966d4dae2d88ff403a99.1710857863.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trivial adjustments for return variable coding style | expand |
On Tue, Mar 19, 2024 at 08:25:20PM +0530, Anand Jain wrote: > Rename the function's local variable werr to ret and err to ret2, then > move ret2 to the local variable of the while loop. Drop the initialization > there since it's immediately assigned below. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/transaction.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index feffff91c6fe..167893457b58 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans) > int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, > struct extent_io_tree *dirty_pages, int mark) > { > - int err = 0; > - int werr = 0; > + int ret = 0; > struct address_space *mapping = fs_info->btree_inode->i_mapping; > struct extent_state *cached_state = NULL; > u64 start = 0; > @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, > > while (find_first_extent_bit(dirty_pages, start, &start, &end, > mark, &cached_state)) { > + int ret2; > bool wait_writeback = false; > > - err = convert_extent_bit(dirty_pages, start, end, > + ret2 = convert_extent_bit(dirty_pages, start, end, > EXTENT_NEED_WAIT, > mark, &cached_state); > /* > @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, > * We cleanup any entries left in the io tree when committing > * the transaction (through extent_io_tree_release()). > */ > - if (err == -ENOMEM) { > - err = 0; > + if (ret2 == -ENOMEM) { > + ret2 = 0; > wait_writeback = true; > } > - if (!err) > - err = filemap_fdatawrite_range(mapping, start, end); > - if (err) > - werr = err; > + if (!ret2) > + ret2 = filemap_fdatawrite_range(mapping, start, end); > + if (ret2) > + ret = ret2; > else if (wait_writeback) > - werr = filemap_fdatawait_range(mapping, start, end); > + ret = filemap_fdatawait_range(mapping, start, end); Ok so this is a correct conversion, but we'll lose "ret" here. Can you follow up with a different series to fix this? I think we just say free_extent_state(cached_state); if (ret) break; otherwise this patch looks fine, you can add Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
在 2024/3/20 01:25, Anand Jain 写道: > Rename the function's local variable werr to ret and err to ret2, then > move ret2 to the local variable of the while loop. Drop the initialization > there since it's immediately assigned below. I love the local return value inside the loop, but I still think we can do further improvement. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/transaction.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index feffff91c6fe..167893457b58 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans) > int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, > struct extent_io_tree *dirty_pages, int mark) > { > - int err = 0; > - int werr = 0; > + int ret = 0; Can we rename it to something like global_ret or whatever can indicate that variable is our final result? > struct address_space *mapping = fs_info->btree_inode->i_mapping; > struct extent_state *cached_state = NULL; > u64 start = 0; > @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, > > while (find_first_extent_bit(dirty_pages, start, &start, &end, > mark, &cached_state)) { > + int ret2; And @ret inside the loop. > bool wait_writeback = false; > > - err = convert_extent_bit(dirty_pages, start, end, > + ret2 = convert_extent_bit(dirty_pages, start, end, > EXTENT_NEED_WAIT, > mark, &cached_state); > /* > @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, > * We cleanup any entries left in the io tree when committing > * the transaction (through extent_io_tree_release()). > */ > - if (err == -ENOMEM) { > - err = 0; > + if (ret2 == -ENOMEM) { > + ret2 = 0; > wait_writeback = true; > } > - if (!err) > - err = filemap_fdatawrite_range(mapping, start, end); > - if (err) > - werr = err; > + if (!ret2) > + ret2 = filemap_fdatawrite_range(mapping, start, end); > + if (ret2) > + ret = ret2; > else if (wait_writeback) > - werr = filemap_fdatawait_range(mapping, start, end); > + ret = filemap_fdatawait_range(mapping, start, end); This does not follow the regular update sequence, we always update the local one @ret2/@err, but here we directly update the @ret/@werr. Just for the sake of consistency, can we only use @ret2 for all the functions calls? > free_extent_state(cached_state); > cached_state = NULL; > cond_resched(); > start = end + 1; Another thing is, we can move most of the writeback code (aka code inside the loop) into a helper function, and make things much simpler at least for the caller. { int global_ret = 0; while (find_first_extent_bit(dirty_pages, start, &start, &end, mark, &cached_state)) { int ret; ret = writeback_range(); if (ret < 0) global_ret = ret; cond_resched(); start = end + 1 } return global_ret; } Thanks, Qu > } > - return werr; > + return ret; > } > > /*
On 3/20/24 01:53, Josef Bacik wrote: > On Tue, Mar 19, 2024 at 08:25:20PM +0530, Anand Jain wrote: >> Rename the function's local variable werr to ret and err to ret2, then >> move ret2 to the local variable of the while loop. Drop the initialization >> there since it's immediately assigned below. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/transaction.c | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c >> index feffff91c6fe..167893457b58 100644 >> --- a/fs/btrfs/transaction.c >> +++ b/fs/btrfs/transaction.c >> @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans) >> int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, >> struct extent_io_tree *dirty_pages, int mark) >> { >> - int err = 0; >> - int werr = 0; >> + int ret = 0; >> struct address_space *mapping = fs_info->btree_inode->i_mapping; >> struct extent_state *cached_state = NULL; >> u64 start = 0; >> @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, >> >> while (find_first_extent_bit(dirty_pages, start, &start, &end, >> mark, &cached_state)) { >> + int ret2; >> bool wait_writeback = false; >> >> - err = convert_extent_bit(dirty_pages, start, end, >> + ret2 = convert_extent_bit(dirty_pages, start, end, >> EXTENT_NEED_WAIT, >> mark, &cached_state); >> /* >> @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, >> * We cleanup any entries left in the io tree when committing >> * the transaction (through extent_io_tree_release()). >> */ >> - if (err == -ENOMEM) { >> - err = 0; >> + if (ret2 == -ENOMEM) { >> + ret2 = 0; >> wait_writeback = true; >> } >> - if (!err) >> - err = filemap_fdatawrite_range(mapping, start, end); >> - if (err) >> - werr = err; >> + if (!ret2) >> + ret2 = filemap_fdatawrite_range(mapping, start, end); >> + if (ret2) >> + ret = ret2; >> else if (wait_writeback) >> - werr = filemap_fdatawait_range(mapping, start, end); >> + ret = filemap_fdatawait_range(mapping, start, end); > > Ok so this is a correct conversion, but we'll lose "ret" here. Can you follow > up with a different series to fix this? I think we just say > > free_extent_state(cached_state); > if (ret) > break; > Sure. Checked the function's stack will propagate the error here back to the parent functions. Thanks, Anand > otherwise this patch looks fine, you can add > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > > Thanks, > > Josef
On 3/20/24 05:25, Qu Wenruo wrote: > > > 在 2024/3/20 01:25, Anand Jain 写道: >> Rename the function's local variable werr to ret and err to ret2, then >> move ret2 to the local variable of the while loop. Drop the >> initialization >> there since it's immediately assigned below. > > I love the local return value inside the loop, but I still think we can > do further improvement. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/transaction.c | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c >> index feffff91c6fe..167893457b58 100644 >> --- a/fs/btrfs/transaction.c >> +++ b/fs/btrfs/transaction.c >> @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct >> btrfs_trans_handle *trans) >> int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, >> struct extent_io_tree *dirty_pages, int mark) >> { >> - int err = 0; >> - int werr = 0; >> + int ret = 0; > > Can we rename it to something like global_ret or whatever can indicate > that variable is our final result? @ret is the actual function return variable and is consistent with doc: https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#code Thanks, Anand > > >> struct address_space *mapping = fs_info->btree_inode->i_mapping; >> struct extent_state *cached_state = NULL; >> u64 start = 0; >> @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct >> btrfs_fs_info *fs_info, >> >> while (find_first_extent_bit(dirty_pages, start, &start, &end, >> mark, &cached_state)) { >> + int ret2; > > And @ret inside the loop. > >> bool wait_writeback = false; >> >> - err = convert_extent_bit(dirty_pages, start, end, >> + ret2 = convert_extent_bit(dirty_pages, start, end, >> EXTENT_NEED_WAIT, >> mark, &cached_state); >> /* >> @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct >> btrfs_fs_info *fs_info, >> * We cleanup any entries left in the io tree when committing >> * the transaction (through extent_io_tree_release()). >> */ >> - if (err == -ENOMEM) { >> - err = 0; >> + if (ret2 == -ENOMEM) { >> + ret2 = 0; >> wait_writeback = true; >> } >> - if (!err) >> - err = filemap_fdatawrite_range(mapping, start, end); >> - if (err) >> - werr = err; >> + if (!ret2) >> + ret2 = filemap_fdatawrite_range(mapping, start, end); >> + if (ret2) >> + ret = ret2; >> else if (wait_writeback) >> - werr = filemap_fdatawait_range(mapping, start, end); >> + ret = filemap_fdatawait_range(mapping, start, end); > > This does not follow the regular update sequence, we always update the > local one @ret2/@err, but here we directly update the @ret/@werr. > > Just for the sake of consistency, can we only use @ret2 for all the > functions calls? > >> free_extent_state(cached_state); >> cached_state = NULL; >> cond_resched(); >> start = end + 1; > > Another thing is, we can move most of the writeback code (aka code > inside the loop) into a helper function, and make things much simpler at > least for the caller. > > { > int global_ret = 0; > > while (find_first_extent_bit(dirty_pages, start, &start, &end, > mark, &cached_state)) { > int ret; > > ret = writeback_range(); > if (ret < 0) > global_ret = ret; > cond_resched(); > start = end + 1 > } > return global_ret; > } > > Thanks, > Qu >> } >> - return werr; >> + return ret; >> } >> >> /*
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index feffff91c6fe..167893457b58 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans) int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, struct extent_io_tree *dirty_pages, int mark) { - int err = 0; - int werr = 0; + int ret = 0; struct address_space *mapping = fs_info->btree_inode->i_mapping; struct extent_state *cached_state = NULL; u64 start = 0; @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, while (find_first_extent_bit(dirty_pages, start, &start, &end, mark, &cached_state)) { + int ret2; bool wait_writeback = false; - err = convert_extent_bit(dirty_pages, start, end, + ret2 = convert_extent_bit(dirty_pages, start, end, EXTENT_NEED_WAIT, mark, &cached_state); /* @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, * We cleanup any entries left in the io tree when committing * the transaction (through extent_io_tree_release()). */ - if (err == -ENOMEM) { - err = 0; + if (ret2 == -ENOMEM) { + ret2 = 0; wait_writeback = true; } - if (!err) - err = filemap_fdatawrite_range(mapping, start, end); - if (err) - werr = err; + if (!ret2) + ret2 = filemap_fdatawrite_range(mapping, start, end); + if (ret2) + ret = ret2; else if (wait_writeback) - werr = filemap_fdatawait_range(mapping, start, end); + ret = filemap_fdatawait_range(mapping, start, end); free_extent_state(cached_state); cached_state = NULL; cond_resched(); start = end + 1; } - return werr; + return ret; } /*
Rename the function's local variable werr to ret and err to ret2, then move ret2 to the local variable of the while loop. Drop the initialization there since it's immediately assigned below. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/transaction.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)