| Message ID | 8f1298da5496557ca89592916cd4a445b6048b8f.1685363099.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: some delayed refs optimizations and cleanups | expand |
On Mon, May 29, 2023 at 04:17:07PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > btrfs_destroy_delayed_refs() always returns 0 and its single caller does > not even check its return value, so make it return void. Function can return void if none of its callees return an error, directly or indirectly, there are no BUG_ONs left to be turned to proper error handling or there's no missing error handling. There's still: 4610 cache = btrfs_lookup_block_group(fs_info, head->bytenr); 4611 BUG_ON(!cache); and calling btrfs_error_unpin_extent_range unpin_extent_range cache = btrfs_lookup_block_group() BUG_ON(!cache) If a function like btrfs_cleanup_one_transaction has limited options how to handle errors then we can ignore them there but at least a comment would be good that we're doing that intentionally. This case is a bit special because there's only one caller so we know the context and btrfs_destroy_delayed_refs() should eventually return void but I'd rather do that as the last step after the call graph is audited for proper error handling.
On Tue, May 30, 2023 at 4:10 PM David Sterba <dsterba@suse.cz> wrote: > > On Mon, May 29, 2023 at 04:17:07PM +0100, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > btrfs_destroy_delayed_refs() always returns 0 and its single caller does > > not even check its return value, so make it return void. > > Function can return void if none of its callees return an error, > directly or indirectly, there are no BUG_ONs left to be turned to > proper error handling or there's no missing error handling. > > There's still: > > 4610 cache = btrfs_lookup_block_group(fs_info, head->bytenr); > 4611 BUG_ON(!cache); > > and calling > > btrfs_error_unpin_extent_range > unpin_extent_range > cache = btrfs_lookup_block_group() > BUG_ON(!cache) > > If a function like btrfs_cleanup_one_transaction has limited options how > to handle errors then we can ignore them there but at least a comment > would be good that we're doing that intentionally. > > This case is a bit special because there's only one caller so we know > the context and btrfs_destroy_delayed_refs() should eventually return > void but I'd rather do that as the last step after the call graph is > audited for proper error handling. What possible error handling are you expecting? This is the transaction abort path, we have no way of dealing with errors - every cleanup of resources is best effort. Thanks.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index fb7ec47f21f1..02e9004f79dc 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -4809,13 +4809,12 @@ static void btrfs_destroy_all_ordered_extents(struct btrfs_fs_info *fs_info) btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1); } -static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, - struct btrfs_fs_info *fs_info) +static void btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, + struct btrfs_fs_info *fs_info) { struct rb_node *node; struct btrfs_delayed_ref_root *delayed_refs; struct btrfs_delayed_ref_node *ref; - int ret = 0; delayed_refs = &trans->delayed_refs; @@ -4823,7 +4822,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, if (atomic_read(&delayed_refs->num_entries) == 0) { spin_unlock(&delayed_refs->lock); btrfs_debug(fs_info, "delayed_refs has NO entry"); - return ret; + return; } while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) { @@ -4884,8 +4883,6 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, btrfs_qgroup_destroy_extent_records(trans); spin_unlock(&delayed_refs->lock); - - return ret; } static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root)