| Message ID | 44301b9e5e365a7b0f5bc57b72811aa4467427c2.1685704678.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: make btrfs_destroy_delayed_refs() return void | expand |
On 2023/6/2 19:19, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > btrfs_destroy_delayed_refs() always returns 0 and its single caller does > not check its return value, as it also returns void, and so does the > callers' caller and so on. This is because we are in the transaction abort > path, where we have no way to deal with errors (we are in a critical > situation) and all cleanup of resources works in a best effort fashion. > So make btrfs_destroy_delayed_refs() return void. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > > V2: Make it explicit in the changelog that we are in the transaction > abort path and therefore have no way to deal with errors. > > V1 was part of a patchset that was merged except for this patch: > https://lore.kernel.org/linux-btrfs/cover.1685363099.git.fdmanana@suse.com/ > > fs/btrfs/disk-io.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 90f998ac68f0..0bd437fbe07d 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -4555,13 +4555,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; > > @@ -4569,7 +4568,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) { > @@ -4630,8 +4629,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)
On Fri, Jun 02, 2023 at 12:19:42PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > btrfs_destroy_delayed_refs() always returns 0 and its single caller does > not check its return value, as it also returns void, and so does the > callers' caller and so on. This is because we are in the transaction abort > path, where we have no way to deal with errors (we are in a critical > situation) and all cleanup of resources works in a best effort fashion. > So make btrfs_destroy_delayed_refs() return void. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > > V2: Make it explicit in the changelog that we are in the transaction > abort path and therefore have no way to deal with errors. > > V1 was part of a patchset that was merged except for this patch: > https://lore.kernel.org/linux-btrfs/cover.1685363099.git.fdmanana@suse.com/ Added to misc-next, my previous objections were meant for clarity and the core of the error handling and return value passing lies in unpin_extent_range(), not so significant for transaction cleanup.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 90f998ac68f0..0bd437fbe07d 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -4555,13 +4555,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; @@ -4569,7 +4568,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) { @@ -4630,8 +4629,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)