On Tue, Dec 04, 2018 at 08:34:15AM +0200, Nikolay Borisov wrote: > > > On 3.12.18 г. 19:25 ч., David Sterba wrote: > > On Sat, Nov 17, 2018 at 09:29:27AM +0800, Anand Jain wrote: > >>> - ret = find_free_dev_extent(trans, device, min_free, > >>> - &dev_offset, NULL); > >>> - if (!ret) > >>> + if (!find_free_dev_extent(trans, device, min_free, > >>> + &dev_offset, NULL)) > >> > >> This can return -ENOMEM; > >> > >>> @@ -2856,8 +2856,7 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) > >>> */ > >>> lockdep_assert_held(&fs_info->delete_unused_bgs_mutex); > >>> > >>> - ret = btrfs_can_relocate(fs_info, chunk_offset); > >>> - if (ret) > >>> + if (!btrfs_can_relocate(fs_info, chunk_offset)) > >>> return -ENOSPC; > >> > >> And ends up converting -ENOMEM to -ENOSPC. > >> > >> Its better to propagate the accurate error. > > > > Right, converting to bool is obscuring the reason why the functions > > fail. Making the code simpler at this cost does not look like a good > > idea to me. I'll remove the patch from misc-next for now. > > The patch itself does not make the code more obscure than currently is, > because even if ENOMEM is returned it's still converted to ENOSPC in > btrfs_relocate_chunk. Sorry, but this can be hardly used as an excuse to keep the code obscure. btrfs_can_relocate & co are not very often changed so there's cruft accumulated. The error value propagation was probably not a hot topic in 2009, btrfs_can_relocate needs the cleanups but please let's do that properly.
