On 29.08.2018 08:15, Qu Wenruo wrote:
> [BUG]
> fstrim on some btrfs only trims the unallocated space, not trimming any
> space in existing block groups.
>
> [CAUSE]
> Before fstrim_range passed to btrfs_trim_fs(), it get truncated to
> range [0, super->total_bytes).
> So later btrfs_trim_fs() will only be able to trim block groups in range
> [0, super->total_bytes).
>
> While for btrfs, any bytenr aligned to sector size is valid, since btrfs use
> its logical address space, there is nothing limiting the location where
> we put block groups.
>
> For btrfs with routine balance, it's quite easy to relocate all
> block groups and bytenr of block groups will start beyond super->total_bytes.
>
> In that case, btrfs will not trim existing block groups.
>
> [FIX]
> Just remove the truncation in btrfs_ioctl_fitrim(), so btrfs_trim_fs()
> can get the unmodified range, which is normally set to [0, U64_MAX].
>
> Reported-by: Chris Murphy <lists@xxxxxxxxxxxxxxxxx>
> Fixes: f4c697e6406d ("btrfs: return EINVAL if start > total_bytes in fitrim ioctl")
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.0+
> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
Seems legit,
Reviewed-by: Nikolay Borisov <nborisov@xxxxxxxx>
> ---
> fs/btrfs/extent-tree.c | 10 +---------
> fs/btrfs/ioctl.c | 11 +++++++----
> 2 files changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 7768f206196a..d1478d66c7a5 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -10851,21 +10851,13 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
> u64 start;
> u64 end;
> u64 trimmed = 0;
> - u64 total_bytes = btrfs_super_total_bytes(fs_info->super_copy);
> u64 bg_failed = 0;
> u64 dev_failed = 0;
> int bg_ret = 0;
> int dev_ret = 0;
> int ret = 0;
>
> - /*
> - * try to trim all FS space, our block group may start from non-zero.
> - */
> - if (range->len == total_bytes)
> - cache = btrfs_lookup_first_block_group(fs_info, range->start);
> - else
> - cache = btrfs_lookup_block_group(fs_info, range->start);
> -
> + cache = btrfs_lookup_first_block_group(fs_info, range->start);
> for (; cache; cache = next_block_group(fs_info, cache)) {
> if (cache->key.objectid >= (range->start + range->len)) {
> btrfs_put_block_group(cache);
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 63600dc2ac4c..8165a4bfa579 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -491,7 +491,6 @@ static noinline int btrfs_ioctl_fitrim(struct file *file, void __user *arg)
> struct fstrim_range range;
> u64 minlen = ULLONG_MAX;
> u64 num_devices = 0;
> - u64 total_bytes = btrfs_super_total_bytes(fs_info->super_copy);
> int ret;
>
> if (!capable(CAP_SYS_ADMIN))
> @@ -515,11 +514,15 @@ static noinline int btrfs_ioctl_fitrim(struct file *file, void __user *arg)
> return -EOPNOTSUPP;
> if (copy_from_user(&range, arg, sizeof(range)))
> return -EFAULT;
> - if (range.start > total_bytes ||
> - range.len < fs_info->sb->s_blocksize)
> +
> + /*
> + * NOTE: Don't truncate the range using super->total_bytes.
> + * Bytenr of btrfs block group is in btrfs logical address space,
> + * which can be any sector size aligned bytenr in [0, U64_MAX].
> + */
> + if (range.len < fs_info->sb->s_blocksize)
> return -EINVAL;
>
> - range.len = min(range.len, total_bytes - range.start);
> range.minlen = max(range.minlen, minlen);
> ret = btrfs_trim_fs(fs_info, &range);
> if (ret < 0)
>
