Re: [PATCH 01/15] btrfs: Honour FITRIM range constraints during free space trim

David Sterba
2019-01-31T15:22:13Z

On Wed, Jan 30, 2019 at 04:50:48PM +0200, Nikolay Borisov wrote:
> Up until know trimming the freespace was done irrespective of what the
> arguments of the FITRIM ioctl were. For example fstrim's -o/-l arguments
> will be entirely ignored. Fix it by correctly handling those paramter.
> This requires breaking if the found freespace extent is after the end
> of the passed range as well as completing trim after trimming
> fstrim_range::len bytes.

How does this work with with multipe-device filesystems? The fstrim
range would apply to all of them. Which does make some sense, though
might be unexpected as this does not happen for other filesystems.

The FITRIM range is in the physical coordinates, so eg. the taking the
maximum size of all devices and iterating over that by 1GiB steps would
go though the whole filesystem. Something to put to the changelog and
documentation.

> Fixes: 499f377f49f0 ("btrfs: iterate over unused chunk space in FITRIM")
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/extent-tree.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 1fdba38761f7..fc3f6acc3c9b 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -11190,9 +11190,9 @@ int btrfs_error_unpin_extent_range(struct btrfs_fs_info *fs_info,
> * held back allocations.
> */
> static int btrfs_trim_free_extents(struct btrfs_device *device,
> - u64 minlen, u64 *trimmed)
> + struct fstrim_range *range, u64 *trimmed)
> {
> - u64 start = 0, len = 0;
> + u64 start = range->start, len = 0;
> int ret;
>
> *trimmed = 0;
> @@ -11235,8 +11235,8 @@ static int btrfs_trim_free_extents(struct btrfs_device *device,
> if (!trans)
> up_read(&fs_info->commit_root_sem);
>
> - ret = find_free_dev_extent_start(trans, device, minlen, start,
> - &start, &len);
> + ret = find_free_dev_extent_start(trans, device, range->minlen,
> + start, &start, &len);
> if (trans) {
> up_read(&fs_info->commit_root_sem);
> btrfs_put_transaction(trans);
> @@ -11249,6 +11249,16 @@ static int btrfs_trim_free_extents(struct btrfs_device *device,
> break;
> }
>
> + /* If we are out of the passed range break */
> + if (start > range->start + range->len - 1) {
> + mutex_unlock(&fs_info->chunk_mutex);
> + ret = 0;
> + break;
> + }
> +
> + start = max(range->start, start);
> + len = min(range->len, len);
> +
> ret = btrfs_issue_discard(device->bdev, start, len, &bytes);
> mutex_unlock(&fs_info->chunk_mutex);
>
> @@ -11258,6 +11268,10 @@ static int btrfs_trim_free_extents(struct btrfs_device *device,
> start += len;
> *trimmed += bytes;
>
> + /* We've trimmed enough */
> + if (*trimmed >= range->len)
> + break;
> +
> if (fatal_signal_pending(current)) {
> ret = -ERESTARTSYS;
> break;
> @@ -11341,8 +11355,7 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
> mutex_lock(&fs_info->fs_devices->device_list_mutex);
> devices = &fs_info->fs_devices->devices;
> list_for_each_entry(device, devices, dev_list) {
> - ret = btrfs_trim_free_extents(device, range->minlen,
> - &group_trimmed);
> + ret = btrfs_trim_free_extents(device, range, &group_trimmed);
> if (ret) {
> dev_failed++;
> dev_ret = ret;
> --
> 2.17.1